Closed
Bug 1214164
Opened 9 years ago
Closed 9 years ago
Only <options> that are children of <select> or children of <optgroup> children of <select> should be honored
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla44
| Tracking | Status | |
|---|---|---|
| firefox44 | --- | fixed |
People
(Reporter: ayg, Assigned: ayg)
References
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(1 file, 2 obsolete files)
|
(deleted),
patch
|
Details | Diff | Splinter Review |
Spec:
"""
The list of options for a select element consists of all the option element children of the select element, and all the option element children of all the optgroup element children of the select element, in tree order.
"""
https://html.spec.whatwg.org/#concept-select-option-list
Test-case:
data:text/html,<!doctype html>
<script>
var select = document.createElement("select");
select.appendChild(document.createElement("optgroup"));
select.firstChild.appendChild(document.createElement("optgroup"));
select.firstChild.firstChild.appendChild(document.createElement("option"));
document.documentElement.textContent = select.options.length;
</script>
IE11 outputs 0, per spec. Gecko and Chrome output 1. But tkent says Blink now follows the spec <https://github.com/whatwg/html/issues/249#issuecomment-147635203>. The DOMs in question cannot be produced by the parser, so behavior is essentially inconsequential, so I think we should follow the path of least resistance and change to match the spec. annevk is inclined to agree <https://github.com/whatwg/html/issues/249#issuecomment-147638810>.
|
||
Comment 1•9 years ago
|
||
So there are no nested optgroups? At one point I thought there were... but discussion in bug 79431 suggests they were just a proposal that never actually materialized,
If there aren't (and the special handling of option/optgroup in nsCSSFrameConstructor::AddFrameConstructionItemsInternal suggests there aren't in display terms), then we probably need to fix (simplify, likely) at least the following:
HTMLSelectElement::InsertOptionsIntoListRecurse
HTMLSelectElement::RemoveOptionsFromListRecurse
AddOptionsRecurse in HTMLSelectElement.cpp
VerifyOptionsRecurse in same
HTMLOptGroupElement::GetSelect
|
Assignee | |
Comment 3•9 years ago
|
||
Lightly tested, and I don't understand the code, so beware. It's likely that further simplifications are possible somewhere here.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af2e1e7c53ab
Attachment #8673103 -
Flags: review?(bzbarsky)
|
|
||
Comment 4•9 years ago
|
||
Comment on attachment 8673103 [details] [diff] [review]
Patch
> HTMLSelectElement::RemoveOptionsFromList(nsIContent* aOptions,
>+ nsCOMPtr<nsIDOMHTMLOptionElement> optElement(do_QueryInterface(aOptions));
I know that's what it used to do, but this can just be HTMLOptionElement::FromContent, right?
>+ optElement = do_QueryInterface(child);
And here.
>+HTMLSelectElement::VerifyOptionsArray()
>+ nsCOMPtr<nsIDOMHTMLOptionElement> opt = do_QueryInterface(child);
And here.
>+ nsCOMPtr<nsIDOMHTMLOptionElement> opt = do_QueryInterface(grandchild);
And here.
r=me with those nits fixed.
Attachment #8673103 -
Flags: review?(bzbarsky) → review+
|
|
Assignee | |
Comment 5•9 years ago
|
||
Attachment #8673103 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
|
||
Comment 6•9 years ago
|
||
That still has the "nsCOMPtr<nsIDOMHTMLOptionElement> opt = do_QueryInterface(grandchild);" bit...
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
|
||
Comment 10•9 years ago
|
||
Posted the site compatibility document just in case.
https://www.fxsitecompat.com/en-US/docs/2015/nested-optgroups-are-no-longer-allowed/
Keywords: dev-doc-complete,
site-compat
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•