WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Alexis Menard (darktears)
Comment 1
2012-03-08 13:07:31 PST
Created
attachment 130884
[details]
Patch
Alexis Menard (darktears)
Comment 2
2012-03-08 13:08:41 PST
Created
attachment 130885
[details]
Patch
Alexis Menard (darktears)
Comment 3
2012-03-08 13:25:41 PST
Created
attachment 130888
[details]
Patch
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
Created
attachment 131024
[details]
Patch
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
(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)
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
Created
attachment 149807
[details]
Patch
Build Bot
Comment 22
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
Alexis Menard (darktears)
Comment 23
2012-06-27 16:26:42 PDT
Created
attachment 149814
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug