Bug 80631 - Implement selectedOptions attribute of HTMLSelectElement
Summary: Implement selectedOptions attribute of HTMLSelectElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2012-03-08 13:04 PST by Alexis Menard (darktears)
Modified: 2012-06-27 18:05 PDT (History)
13 users (show)

See Also:


Attachments
Patch (10.13 KB, patch)
2012-03-08 13:07 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (10.26 KB, patch)
2012-03-08 13:08 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (10.98 KB, patch)
2012-03-08 13:25 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (12.09 KB, patch)
2012-03-09 03:34 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch (30.98 KB, patch)
2012-06-27 15:45 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (18.06 KB, patch)
2012-06-27 16:26 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (18.07 KB, patch)
2012-06-27 16:31 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-03-08 13:04:36 PST
Implement selectedOptions attribute of <select>.
Comment 1 Alexis Menard (darktears) 2012-03-08 13:07:31 PST
Created attachment 130884 [details]
Patch
Comment 2 Alexis Menard (darktears) 2012-03-08 13:08:41 PST
Created attachment 130885 [details]
Patch
Comment 3 Alexis Menard (darktears) 2012-03-08 13:25:41 PST
Created attachment 130888 [details]
Patch
Comment 4 Alexis Menard (darktears) 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.
Comment 5 Benjamin Poulain 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"
Comment 6 Alexis Menard (darktears) 2012-03-09 03:34:54 PST
Created attachment 131024 [details]
Patch
Comment 7 Alexis Menard (darktears) 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.
Comment 8 Benjamin Poulain 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
Comment 9 WebKit Review Bot 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>
Comment 10 Kent Tamura 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.
Comment 11 Alexis Menard (darktears) 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.
Comment 12 Kent Tamura 2012-06-15 00:15:19 PDT
Reopen because http://trac.webkit.org/changeset/110470 was rolled out.
Comment 13 Kent Tamura 2012-06-15 02:03:12 PDT
(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
Comment 14 Alexis Menard (darktears) 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.
Comment 15 Andreas Kling 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.
Comment 16 Andreas Kling 2012-06-27 05:30:17 PDT
Adding rniwa since he's been mucking around with related things lately.
Comment 17 Ryosuke Niwa 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.
Comment 18 Alexis Menard (darktears) 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
Comment 19 Alexis Menard (darktears) 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.
Comment 20 Ryosuke Niwa 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.
Comment 21 Alexis Menard (darktears) 2012-06-27 15:45:21 PDT
Created attachment 149807 [details]
Patch
Comment 22 Build Bot 2012-06-27 16:10:36 PDT
Comment on attachment 149807 [details]
Patch

Attachment 149807 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13093897
Comment 23 Alexis Menard (darktears) 2012-06-27 16:26:42 PDT
Created attachment 149814 [details]
Patch
Comment 24 Alexis Menard (darktears) 2012-06-27 16:31:55 PDT
Created attachment 149815 [details]
Patch for landing
Comment 25 WebKit Review Bot 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>
Comment 26 WebKit Review Bot 2012-06-27 18:05:59 PDT
All reviewed patches have been landed.  Closing bug.