Bug 163296 – select.options may return too many option elements

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

select.options may return too many option elements. We're only supposed to return the option element children of the select element, and the option element children of all the optgroup element children of the select element, in tree order:
- https://html.spec.whatwg.org/#dom-select-options
- https://html.spec.whatwg.org/#concept-select-option-list

Firefox and Chrome agrees with the specification. However, WebKit returns all the option elements that are descendants of the select element.

[Chris Dumez]

Created attachment 291305 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 291305 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291305&action=review

> Source/WebCore/html/HTMLOptionsCollection.cpp:85
> +        if (is<HTMLOptGroupElement>(*child)) {
> +            if (auto* firstOptionChild = childrenOfType<HTMLOptionElement>(*child).first())

I don't think this is quite correct. opgroup needs to be a child of select:
https://html.spec.whatwg.org/#concept-select-option-list

> Source/WebCore/html/HTMLOptionsCollection.cpp:101
> +HTMLOptionElement* HTMLOptionsCollection::customElementAfter(Element* element) const

I think it's better to just check the parent being either the root HTMLSelectElement or HTMLOptGroupElement,
and if it's HTMLOptGroupElement, then its parent is the root HTMLSelectElement.

[Chris Dumez]

Created attachment 291313 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 291313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291313&action=review

> Source/WebCore/html/HTMLOptionsCollection.h:76
> +    if (element.parentNode() == &selectElement())

Can we store parentNode in a local variable instead of calling it thrice?

> Source/WebCore/html/HTMLOptionsCollection.h:79
> +    return element.parentNode()->hasTagName(optgroupTag) && element.parentNode()->parentNode() == &selectElement();

Should we null check parentNode()? I guess we're guaranteed to be traversing descendent which is not equal to the root so it can never be null?
But we should at least add ASSERT?

[Chris Dumez]

Created attachment 291316 [details]
Patch

[Chris Dumez]

Created attachment 291317 [details]
Patch

[Chris Dumez]

(In reply to comment #4)
> Comment on attachment 291313 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291313&action=review
> 
> > Source/WebCore/html/HTMLOptionsCollection.h:76
> > +    if (element.parentNode() == &selectElement())
> 
> Can we store parentNode in a local variable instead of calling it thrice?
> 
> > Source/WebCore/html/HTMLOptionsCollection.h:79
> > +    return element.parentNode()->hasTagName(optgroupTag) && element.parentNode()->parentNode() == &selectElement();
> 
> Should we null check parentNode()? I guess we're guaranteed to be traversing
> descendent which is not equal to the root so it can never be null?
> But we should at least add ASSERT?

I added an assertion even though it would crash anyway if it were null when referencing it. If we encounter an option element while traversing the descendants of a select element, and if its parent is not a select element then the descendant necessarily has a grand-parent.

[WebKit Commit Bot]

Comment on attachment 291317 [details]
Patch

Clearing flags on attachment: 291317

Committed r207181: <http://trac.webkit.org/changeset/207181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

HTMLSelectElement::recalcListItems should be updated too.  It picks up OPTION children of a nested OPTGROUP.

[Chris Dumez]

(In reply to comment #10)
> HTMLSelectElement::recalcListItems should be updated too.  It picks up
> OPTION children of a nested OPTGROUP.

Thanks for letting me know Tamura. I'll fix in a follow-up.