Implement selectedOptions attribute of HTMLSelectElement

• Component: DOM
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

Implement selectedOptions attribute of <select>.

[Alexis Menard (darktears)]

Created attachment 130884 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 130885 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 130888 [details]
Patch

[Alexis Menard (darktears)]

(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 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)]

Created attachment 131024 [details]
Patch

[Alexis Menard (darktears)]

(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 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 on attachment 131024 [details]
Patch

Clearing flags on attachment: 131024

Committed r110340: <http://trac.webkit.org/changeset/110340>

[Kent Tamura]

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)]

(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]

Reopen because http://trac.webkit.org/changeset/110470 was rolled out.

[Kent Tamura]

(In reply to comment #12)
> Reopen because http://trac.webkit.org/changeset/110470 was rolled out.

Rollout changes:
http://trac.webkit.org/changeset/120413
http://trac.webkit.org/changeset/120425

[Alexis Menard (darktears)]

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 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]

Adding rniwa since he's been mucking around with related things lately.

[Ryosuke Niwa]

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)]

(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)]

(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]

(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)]

Created attachment 149807 [details]
Patch

[Build Bot]

Comment on attachment 149807 [details]
Patch

Attachment 149807 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13093897

[Alexis Menard (darktears)]

Created attachment 149814 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 149815 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 149815 [details]
Patch for landing

Clearing flags on attachment: 149815

Committed r121392: <http://trac.webkit.org/changeset/121392>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.