RESOLVED FIXED 80631
Implement selectedOptions attribute of HTMLSelectElement
https://bugs.webkit.org/show_bug.cgi?id=80631
Summary Implement selectedOptions attribute of HTMLSelectElement
Alexis Menard (darktears)
Reported 2012-03-08 13:04:36 PST
Implement selectedOptions attribute of <select>.
Attachments
Patch (10.13 KB, patch)
2012-03-08 13:07 PST, Alexis Menard (darktears)
no flags
Patch (10.26 KB, patch)
2012-03-08 13:08 PST, Alexis Menard (darktears)
no flags
Patch (10.98 KB, patch)
2012-03-08 13:25 PST, Alexis Menard (darktears)
no flags
Patch (12.09 KB, patch)
2012-03-09 03:34 PST, Alexis Menard (darktears)
no flags
work in progress, fixes the bug report of https://bugs.webkit.org/show_bug.cgi?id=88749 but haven't yet found a way to test it. (18.65 KB, patch)
2012-06-26 18:38 PDT, Alexis Menard (darktears)
no flags
Patch (30.98 KB, patch)
2012-06-27 15:45 PDT, Alexis Menard (darktears)
no flags
Patch (18.06 KB, patch)
2012-06-27 16:26 PDT, Alexis Menard (darktears)
no flags
Patch for landing (18.07 KB, patch)
2012-06-27 16:31 PDT, Alexis Menard (darktears)
no flags
Alexis Menard (darktears)
Comment 1 2012-03-08 13:07:31 PST
Alexis Menard (darktears)
Comment 2 2012-03-08 13:08:41 PST
Alexis Menard (darktears)
Comment 3 2012-03-08 13:25:41 PST
Alexis Menard (darktears)
Comment 4 2012-03-08 13:26:04 PST
(In reply to comment #3) > Created an attachment (id=130888) [details] > Patch Improve test coverage by testing the values inside the collection.
Benjamin Poulain
Comment 5 2012-03-08 17:08:38 PST
Comment on attachment 130888 [details] Patch The patch looks good. I would add test coverage for: -disabled option -disabled select -nested <select> :-D ... something like this: <select> <option> <select> <option selected> </select> </option> <option></option> </selection> -multiple selected options for a select without "multiple"
Alexis Menard (darktears)
Comment 6 2012-03-09 03:34:54 PST
Alexis Menard (darktears)
Comment 7 2012-03-09 03:36:57 PST
(In reply to comment #5) > (From update of attachment 130888 [details]) > The patch looks good. > > I would add test coverage for: > -disabled option > -disabled select Done both. > -nested <select> :-D ... something like this: > <select> > <option> > <select> > <option selected> > </select> > </option> > <option></option> > </selection> You can't really do that do you? I tried a quick test and the rendering is not really what you would expect it to be so not sure we should "cover" this. > > -multiple selected options for a select without "multiple" This one was covered already.
Benjamin Poulain
Comment 8 2012-03-09 13:00:37 PST
Comment on attachment 131024 [details] Patch Looks good. At first I was not sure about the case of (multiple == false), but this follows http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#attr-select-multiple
WebKit Review Bot
Comment 9 2012-03-09 15:12:05 PST
Comment on attachment 131024 [details] Patch Clearing flags on attachment: 131024 Committed r110340: <http://trac.webkit.org/changeset/110340>
Kent Tamura
Comment 10 2012-03-10 04:50:31 PST
Comment on attachment 131024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131024&action=review > LayoutTests/ChangeLog:11 > + * fast/dom/select-selectedOptions-expected.txt: Added. > + * fast/dom/select-selectedOptions.html: Added. The test should be in fast/dom/HTMLSelectElement/, or fast/forms/select/. > LayoutTests/fast/dom/select-selectedOptions.html:7 > +function reset(mySelect) { > + mySelect.length = 0; We usually use 4-space indentation in JavaScript.
Alexis Menard (darktears)
Comment 11 2012-03-14 06:24:25 PDT
(In reply to comment #10) > (From update of attachment 131024 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131024&action=review > > > LayoutTests/ChangeLog:11 > > + * fast/dom/select-selectedOptions-expected.txt: Added. > > + * fast/dom/select-selectedOptions.html: Added. > > The test should be in fast/dom/HTMLSelectElement/, or fast/forms/select/. > > > LayoutTests/fast/dom/select-selectedOptions.html:7 > > +function reset(mySelect) { > > + mySelect.length = 0; > > We usually use 4-space indentation in JavaScript. Fixed with http://trac.webkit.org/changeset/110470 I also moved some old ones that should not be there.
Kent Tamura
Comment 12 2012-06-15 00:15:19 PDT
Reopen because http://trac.webkit.org/changeset/110470 was rolled out.
Kent Tamura
Comment 13 2012-06-15 02:03:12 PDT
Alexis Menard (darktears)
Comment 14 2012-06-26 18:38:01 PDT
Created attachment 149661 [details] work in progress, fixes the bug report of https://bugs.webkit.org/show_bug.cgi?id=88749 but haven't yet found a way to test it.
Andreas Kling
Comment 15 2012-06-27 05:29:21 PDT
Comment on attachment 149661 [details] work in progress, fixes the bug report of https://bugs.webkit.org/show_bug.cgi?id=88749 but haven't yet found a way to test it. View in context: https://bugs.webkit.org/attachment.cgi?id=149661&action=review Who else implements this collection? Moz, Opera, IE, etc.? I consider HTMLCollection and other live Node list interfaces bad API, so I'm wondering if it's worth adding new ones just to comply with a spec. > Source/WebCore/html/HTMLOptionElement.cpp:250 > + if (HTMLSelectElement* select = ownerSelectElement()) > + select->invalidateSelectedItems(); It's unfortunate that we have walk up the DOM parent chain to find the <select> for every <option> where the selected state changes. Probably doesn't matter much in real-world content though. > Source/WebCore/html/HTMLOptionsCollection.h:55 > +class HTMLSelectedOptionsCollection : public HTMLCollection { Shouldn't this have its own file(s) instead of piggybacking on HTMLOptionsCollection? > Source/WebCore/html/HTMLOptionsCollection.h:59 > + using HTMLCollection::m_cache; Yuck! > Source/WebCore/html/HTMLSelectElement.cpp:713 > + if (m_selectedOptionsCollection) > + m_selectedOptionsCollection->m_cache.clear(); There should be a way to invalidate the internal cache without accessing a collection member variable from another class.
Andreas Kling
Comment 16 2012-06-27 05:30:17 PDT
Adding rniwa since he's been mucking around with related things lately.
Ryosuke Niwa
Comment 17 2012-06-27 11:33:43 PDT
Comment on attachment 149661 [details] work in progress, fixes the bug report of https://bugs.webkit.org/show_bug.cgi?id=88749 but haven't yet found a way to test it. View in context: https://bugs.webkit.org/attachment.cgi?id=149661&action=review > Source/WebCore/html/HTMLCollection.cpp:128 > + if (element->hasLocalName(optionTag)) { > + HTMLOptionElement* option = static_cast<HTMLOptionElement*>(element); > + if (option->selected()) > + return true; > + } > + return false; I would have done: return element->hasLocalName(optionTag) && toHTMLOptionElement(element)->selected() >> Source/WebCore/html/HTMLOptionsCollection.h:55 >> +class HTMLSelectedOptionsCollection : public HTMLCollection { > > Shouldn't this have its own file(s) instead of piggybacking on HTMLOptionsCollection? We don't need this class at all. You just instantiate HTMLCollection(selection, SelectedOptions). >> Source/WebCore/html/HTMLSelectElement.cpp:713 >> + m_selectedOptionsCollection->m_cache.clear(); > > There should be a way to invalidate the internal cache without accessing a collection member variable from another class. HTMLCollection relies on DOM tree version to invalidate its value so there's no need to explicitly invalidate the cache.
Alexis Menard (darktears)
Comment 18 2012-06-27 11:35:44 PDT
(In reply to comment #17) > (From update of attachment 149661 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149661&action=review > > > Source/WebCore/html/HTMLCollection.cpp:128 > > + if (element->hasLocalName(optionTag)) { > > + HTMLOptionElement* option = static_cast<HTMLOptionElement*>(element); > > + if (option->selected()) > > + return true; > > + } > > + return false; > > I would have done: > return element->hasLocalName(optionTag) && toHTMLOptionElement(element)->selected() ok. > > >> Source/WebCore/html/HTMLOptionsCollection.h:55 > >> +class HTMLSelectedOptionsCollection : public HTMLCollection { > > > > Shouldn't this have its own file(s) instead of piggybacking on HTMLOptionsCollection? > > We don't need this class at all. You just instantiate HTMLCollection(selection, SelectedOptions). > > >> Source/WebCore/html/HTMLSelectElement.cpp:713 > >> + m_selectedOptionsCollection->m_cache.clear(); > > > > There should be a way to invalidate the internal cache without accessing a collection member variable from another class. > > HTMLCollection relies on DOM tree version to invalidate its value so there's no need to explicitly invalidate the cache. Yes but when the select state changed the dom tree version is not incremented. Not sure we want to do that. It triggered a bug btw : https://bugs.webkit.org/show_bug.cgi?id=88749
Alexis Menard (darktears)
Comment 19 2012-06-27 11:36:19 PDT
(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 149661 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=149661&action=review > > > > > Source/WebCore/html/HTMLCollection.cpp:128 > > > + if (element->hasLocalName(optionTag)) { > > > + HTMLOptionElement* option = static_cast<HTMLOptionElement*>(element); > > > + if (option->selected()) > > > + return true; > > > + } > > > + return false; > > > > I would have done: > > return element->hasLocalName(optionTag) && toHTMLOptionElement(element)->selected() > > ok. > > > > > >> Source/WebCore/html/HTMLOptionsCollection.h:55 > > >> +class HTMLSelectedOptionsCollection : public HTMLCollection { > > > > > > Shouldn't this have its own file(s) instead of piggybacking on HTMLOptionsCollection? > > > > We don't need this class at all. You just instantiate HTMLCollection(selection, SelectedOptions). > > > > >> Source/WebCore/html/HTMLSelectElement.cpp:713 > > >> + m_selectedOptionsCollection->m_cache.clear(); > > > > > > There should be a way to invalidate the internal cache without accessing a collection member variable from another class. > > > > HTMLCollection relies on DOM tree version to invalidate its value so there's no need to explicitly invalidate the cache. > > > Yes but when the select state changed the dom tree version is not incremented. Not sure we want to do that. It triggered a bug btw : https://bugs.webkit.org/show_bug.cgi?id=88749 Selected state of the option element I meant.
Ryosuke Niwa
Comment 20 2012-06-27 12:12:04 PDT
(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 149661 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=149661&action=review > > >> Source/WebCore/html/HTMLSelectElement.cpp:713 > > >> + m_selectedOptionsCollection->m_cache.clear(); > > > > > > There should be a way to invalidate the internal cache without accessing a collection member variable from another class. > > > > HTMLCollection relies on DOM tree version to invalidate its value so there's no need to explicitly invalidate the cache. > > Yes but when the select state changed the dom tree version is not incremented. Not sure we want to do that. It triggered a bug btw : https://bugs.webkit.org/show_bug.cgi?id=88749 Ah, I see. Right. Selecting an option doesn't change the DOM. Makes sense.
Alexis Menard (darktears)
Comment 21 2012-06-27 15:45:21 PDT
Build Bot
Comment 22 2012-06-27 16:10:36 PDT
Alexis Menard (darktears)
Comment 23 2012-06-27 16:26:42 PDT
Alexis Menard (darktears)
Comment 24 2012-06-27 16:31:55 PDT
Created attachment 149815 [details] Patch for landing
WebKit Review Bot
Comment 25 2012-06-27 18:05:46 PDT
Comment on attachment 149815 [details] Patch for landing Clearing flags on attachment: 149815 Committed r121392: <http://trac.webkit.org/changeset/121392>
WebKit Review Bot
Comment 26 2012-06-27 18:05:59 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.