Summary: | select.options may return too many option elements | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | DOM | Assignee: | 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
Chris Dumez
2016-10-11 14:05:11 PDT
Created attachment 291305 [details]
Patch
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. Created attachment 291313 [details]
Patch
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? Created attachment 291316 [details]
Patch
Created attachment 291317 [details]
Patch
(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 on attachment 291317 [details] Patch Clearing flags on attachment: 291317 Committed r207181: <http://trac.webkit.org/changeset/207181> All reviewed patches have been landed. Closing bug. HTMLSelectElement::recalcListItems should be updated too. It picks up OPTION children of a nested OPTGROUP. (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. |