Summary: | When inserting a selected <option> in a <select> element, its selected state should remain | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | Forms | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, cdumez, changseok, darin, esprehn+autocc, ews-watchlist, ggaren, gyuyoung.kim, kangil.han, mifenton, sam, webkit-bug-importer, wenson_hsieh, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 235247 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Chris Dumez
2022-01-14 10:16:46 PST
Created attachment 449181 [details]
Patch
Created attachment 449232 [details]
Patch
Created attachment 449394 [details]
Patch
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? (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. 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. Created attachment 449436 [details]
Patch
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]. |