Only <options> that are children of <select> or children of <optgroup> children of <select> should be honored

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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>.
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
Duplicate of this bug: 79431
Posted patch Patch (obsolete) — Splinter Review
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 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+
Posted patch Patch for checkin (obsolete) — Splinter Review
Attachment #8673103 - Attachment is obsolete: true
That still has the "nsCOMPtr<nsIDOMHTMLOptionElement> opt = do_QueryInterface(grandchild);" bit...
Oops, sorry.
Attachment #8673226 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/10739e1e3d8c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1250401
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.