Bug 163296

Summary: select.options may return too many option elements
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, esprehn+autocc, gyuyoung.kim, rniwa, sam, tkent
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://html.spec.whatwg.org/#concept-select-option-list
Bug Depends on:    
Bug Blocks: 163358    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2016-10-11 14:05:11 PDT
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.
Comment 1 Chris Dumez 2016-10-11 15:34:40 PDT
Created attachment 291305 [details]
Patch
Comment 2 Ryosuke Niwa 2016-10-11 16:22:01 PDT
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.
Comment 3 Chris Dumez 2016-10-11 16:26:38 PDT
Created attachment 291313 [details]
Patch
Comment 4 Ryosuke Niwa 2016-10-11 16:29:03 PDT
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?
Comment 5 Chris Dumez 2016-10-11 16:35:37 PDT
Created attachment 291316 [details]
Patch
Comment 6 Chris Dumez 2016-10-11 16:39:09 PDT
Created attachment 291317 [details]
Patch
Comment 7 Chris Dumez 2016-10-11 16:41:19 PDT
(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.
Comment 8 WebKit Commit Bot 2016-10-11 17:28:31 PDT
Comment on attachment 291317 [details]
Patch

Clearing flags on attachment: 291317

Committed r207181: <http://trac.webkit.org/changeset/207181>
Comment 9 WebKit Commit Bot 2016-10-11 17:28:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Kent Tamura 2016-10-12 13:59:16 PDT
HTMLSelectElement::recalcListItems should be updated too.  It picks up OPTION children of a nested OPTGROUP.
Comment 11 Chris Dumez 2016-10-12 14:00:10 PDT
(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.