Bug 235237 – When inserting a selected <option> in a <select> element, its selected state should remain

• Component: Forms
• Status: RESOLVED FIXED

[Chris Dumez]

When inserting a selected <option> in a <select> element, its selected state should remain and other selected options should be de-selected.

This is as per the specification [1] that says:
"""
If the multiple attribute is absent, whenever an option element in the select element's list of options has its selectedness set to true, and whenever an option element with its selectedness set to true is added to the select element's list of options, the user agent must set the selectedness of all the other option elements in its list of options to false.
"""

Firefox and Chrome correctly implement this. However, in WebKit, after insertion of a new <option>, if there is more than one selected <option> it is the last selected <option> in the list that retains the selected state (not necessarily the newly inserted one).

[1] https://html.spec.whatwg.org/multipage/form-elements.html#the-select-element

[Chris Dumez]

Created attachment 449181 [details]
Patch

[Chris Dumez]

Created attachment 449232 [details]
Patch

[Chris Dumez]

Created attachment 449394 [details]
Patch

[Darin Adler]

Comment on attachment 449394 [details]
Patch

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

> Source/WebCore/html/HTMLOptGroupElement.cpp:80
> +    for (auto* option = Traversal<HTMLOptionElement>::lastChild(*this); option; option = Traversal<HTMLOptionElement>::previousSibling(*option)) {

Does this work?

    for (auto& option : makeReversedRange(childrenOfType<HTMLOptionElement>(*this))) {

I think it should work and I like it better. But maybe makeReversedRange won’t work until we add rbegin and rend to ElementChildRange? I’d want to do this soon.

> Source/WebCore/html/HTMLSelectElement.cpp:374
>  void HTMLSelectElement::childrenChanged(const ChildChange& change)

Can we find a way to share more of the code between HTMLSelectElement and HTMLOptGroupElement? Helper function or class?

[Chris Dumez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 449394 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449394&action=review
> 
> > Source/WebCore/html/HTMLOptGroupElement.cpp:80
> > +    for (auto* option = Traversal<HTMLOptionElement>::lastChild(*this); option; option = Traversal<HTMLOptionElement>::previousSibling(*option)) {
> 
> Does this work?
> 
>     for (auto& option :
> makeReversedRange(childrenOfType<HTMLOptionElement>(*this))) {
> 
> I think it should work and I like it better. But maybe makeReversedRange
> won’t work until we add rbegin and rend to ElementChildRange? I’d want to do
> this soon.

No, this doesn't work currently:
In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource173.cpp:4:
./html/HTMLOptGroupElement.cpp:80:25: error: no matching function for call to 'makeReversedRange'
    for (auto& option : makeReversedRange(childrenOfType<HTMLOptionElement>(*this))) {
In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource173.cpp:1:
In file included from /Volumes/Data/WebKit/OpenSource/Source/WebCore/WebCorePrefix.h:171:
In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/HashMap.h:26:
/Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/IteratorRange.h:61:53: note: candidate template ignored: substitution failure [with Container = WebCore::ElementChildRange<WebCore::HTMLOptionElement>]: no type named 'reverse_iterator' in 'WebCore::ElementChildRange<WebCore::HTMLOptionElement>'
IteratorRange<typename Container::reverse_iterator> makeReversedRange(Container& container)
/Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/IteratorRange.h:67:59: note: candidate template ignored: substitution failure [with Container = WebCore::ElementChildRange<WebCore::HTMLOptionElement>]: no type named 'const_reverse_iterator' in 'WebCore::ElementChildRange<WebCore::HTMLOptionElement>'
IteratorRange<typename Container::const_reverse_iterator> makeReversedRange(const Container& container)
1 error generated.


> 
> > Source/WebCore/html/HTMLSelectElement.cpp:374
> >  void HTMLSelectElement::childrenChanged(const ChildChange& change)
> 
> Can we find a way to share more of the code between HTMLSelectElement and
> HTMLOptGroupElement? Helper function or class?

I already tried to share as much as I could. I'll do another pass and see if I can do better.

[Darin Adler]

Comment on attachment 449394 [details]
Patch

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

>>> Source/WebCore/html/HTMLOptGroupElement.cpp:80
>>> +    for (auto* option = Traversal<HTMLOptionElement>::lastChild(*this); option; option = Traversal<HTMLOptionElement>::previousSibling(*option)) {
>> 
>> Does this work?
>> 
>>     for (auto& option : makeReversedRange(childrenOfType<HTMLOptionElement>(*this))) {
>> 
>> I think it should work and I like it better. But maybe makeReversedRange won’t work until we add rbegin and rend to ElementChildRange? I’d want to do this soon.
> 
> No, this doesn't work currently:
> In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource173.cpp:4:
> ./html/HTMLOptGroupElement.cpp:80:25: error: no matching function for call to 'makeReversedRange'
>     for (auto& option : makeReversedRange(childrenOfType<HTMLOptionElement>(*this))) {
> In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource173.cpp:1:
> In file included from /Volumes/Data/WebKit/OpenSource/Source/WebCore/WebCorePrefix.h:171:
> In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/HashMap.h:26:
> /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/IteratorRange.h:61:53: note: candidate template ignored: substitution failure [with Container = WebCore::ElementChildRange<WebCore::HTMLOptionElement>]: no type named 'reverse_iterator' in 'WebCore::ElementChildRange<WebCore::HTMLOptionElement>'
> IteratorRange<typename Container::reverse_iterator> makeReversedRange(Container& container)
> /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/IteratorRange.h:67:59: note: candidate template ignored: substitution failure [with Container = WebCore::ElementChildRange<WebCore::HTMLOptionElement>]: no type named 'const_reverse_iterator' in 'WebCore::ElementChildRange<WebCore::HTMLOptionElement>'
> IteratorRange<typename Container::const_reverse_iterator> makeReversedRange(const Container& container)
> 1 error generated.

I will fix this and make it usable here after you land.

[Chris Dumez]

Created attachment 449436 [details]
Patch

[EWS]

Committed r288174 (246158@main): <https://commits.webkit.org/246158@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449436 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87746816>