Last Comment Bug 596681 - Implement HTMLSelectElement selectedOptions IDL attribute
: Implement HTMLSelectElement selectedOptions IDL attribute
Status: RESOLVED FIXED
: dev-doc-needed, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: mozilla26
Assigned To: Reuben Morais [:reuben]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: html5forms 911808
  Show dependency treegraph
 
Reported: 2010-09-15 11:45 PDT by Mounir Lamouri (:mounir)
Modified: 2013-12-22 16:19 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP Patch (9.85 KB, patch)
2010-10-13 03:43 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch, rebased to tip (12.39 KB, patch)
2013-03-07 14:31 PST, Reuben Morais [:reuben]
no flags Details | Diff | Splinter Review
Patch, rebased to tip (12.63 KB, patch)
2013-03-07 14:34 PST, Reuben Morais [:reuben]
mounir: feedback+
Details | Diff | Splinter Review
Implement HTMLSelectElement.selectedOptions (12.99 KB, patch)
2013-08-24 18:08 PDT, Reuben Morais [:reuben]
no flags Details | Diff | Splinter Review
Implement HTMLSelectElement.selectedOptions, v2 (14.33 KB, patch)
2013-08-24 18:22 PDT, Reuben Morais [:reuben]
bugs: review-
Details | Diff | Splinter Review
Implement HTMLSelectElement.selectedOptions, v3 (15.30 KB, patch)
2013-08-25 07:35 PDT, Reuben Morais [:reuben]
bugs: review+
Details | Diff | Splinter Review
interdiff v2 => v3 (2.01 KB, patch)
2013-08-25 07:36 PDT, Reuben Morais [:reuben]
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2010-09-15 11:45:42 PDT
It should return all selected options inside the select element.
Comment 1 Mounir Lamouri (:mounir) 2010-10-13 03:43:07 PDT
Created attachment 482810 [details] [diff] [review]
WIP Patch

I don't remember if this patch work (even compiles) but it surely doesn't follow the specifications. I attach it here so if someone want to work on that...
Comment 2 Reuben Morais [:reuben] 2013-03-07 14:31:38 PST
Created attachment 722498 [details] [diff] [review]
Patch, rebased to tip

(In reply to Mounir Lamouri (:mounir) from comment #1)
> I don't remember if this patch work (even compiles) but it surely doesn't
> follow the specifications.

It works if I re-create the nsContentList on every call to GetSelectedOptions. For some reason the list doesn't seem to be updated when the node tree changes. I can't call SetDirty() since it's protected, so I'm not sure how it's supposed to work. See the commented code in GetSelectedOptions.
Comment 3 Reuben Morais [:reuben] 2013-03-07 14:34:05 PST
Created attachment 722499 [details] [diff] [review]
Patch, rebased to tip

Oops, forgot to qref.
Comment 4 Mounir Lamouri (:mounir) 2013-03-08 01:54:07 PST
Comment on attachment 722499 [details] [diff] [review]
Patch, rebased to tip

If I understand it correctly, this patch is my patch rebased to tip, right? According to my comment, this patch wasn't following the specifications. Did you double-check this?

Maybe you could write another patch (to keep authorship correct) on top of mine that fix the specification corectness (if needed).
Comment 5 Reuben Morais [:reuben] 2013-03-08 01:58:55 PST
(In reply to Mounir Lamouri (:mounir) from comment #4)
> If I understand it correctly, this patch is my patch rebased to tip, right?
> According to my comment, this patch wasn't following the specifications. Did
> you double-check this?

Yes. I uncommented the tree order tests, made several tests myself in the JavaScript console, and everything seemed in accordance to the spec. What doesn't work is returning the existing mSelectedOptions attribute, because it never gets updated, even when elements are removed/added. To test it, I made it create and return a new nsContentList every time GetSelectedOptions is called. Is there anything else I have to do to make the nsContentList update when something changes?

> Maybe you could write another patch (to keep authorship correct) on top of
> mine that fix the specification corectness (if needed).

All I really did was rebasing, fixing some PR types, uncommenting the tests, and testing it locally. Here's the interdiff: http://pastebin.mozilla.org/2202681
Comment 6 Mounir Lamouri (:mounir) 2013-03-09 05:38:58 PST
Comment on attachment 722499 [details] [diff] [review]
Patch, rebased to tip

I will have a deeper look at the code and give a feedback but anyway given that I wrote most of that code, someone else will have to review it.
Comment 7 Mounir Lamouri (:mounir) 2013-03-12 17:27:09 PDT
Comment on attachment 722499 [details] [diff] [review]
Patch, rebased to tip

Review of attachment 722499 [details] [diff] [review]:
-----------------------------------------------------------------

I think you are missing some tests. For example, when the <option>s are not direct children of the <select>. Also, add tests for when the <option> is disabled.
You should also check that the returned value is live. That means if you change the selected options, the previously returned selectedOptions should be updated, IOW:
<select>
  <option selected>foo</option>
  <option>bar</option>
</select>
<script>
  var s = document.getElementsByTagName('select')[0];
  var selected = s.selectedOptions;
  is(selected.length, 1);
  document.getElementsByTagName('option')[1].selected = true;
  is(selected.length, 2);
</script>

If those tests are passing, this is great and ready to be reviewed (after the requested changes).

Thanks :)

::: content/html/content/src/nsHTMLSelectElement.cpp
@@ +1928,5 @@
> +                                          void* aData)
> +{
> +  nsCOMPtr<nsIDOMHTMLOptionElement> option = do_QueryInterface(aContent);
> +  bool selected = false;
> +  return option && NS_SUCCEEDED(option->GetSelected(&selected)) && selected;

I wonder if checking if the element is a <html:option> wouldn't be useful. I'm not sure so I guess I will let the reviewer take that decision.

@@ +1936,5 @@
> +nsHTMLSelectElement::GetSelectedOptions(nsIDOMHTMLCollection** aSelectedOptions)
> +{
> +  if (!mSelectedOptions) {
> +    // nsRefPtr<nsContentList> selectedOptions = new nsContentList(this, MatchSelectedOptions,
> +    //                                                             nullptr, nullptr);

nit: remove that comment

@@ +1938,5 @@
> +  if (!mSelectedOptions) {
> +    // nsRefPtr<nsContentList> selectedOptions = new nsContentList(this, MatchSelectedOptions,
> +    //                                                             nullptr, nullptr);
> +    mSelectedOptions = new nsContentList(this, MatchSelectedOptions, nullptr,
> +                                         nullptr);

nit: add 'true' as a fifht argument.

@@ +1941,5 @@
> +    mSelectedOptions = new nsContentList(this, MatchSelectedOptions, nullptr,
> +                                         nullptr);
> +  }
> +
> +  // NS_ADDREF(*aSelectedOptions = selectedOptions);

nit: remove that comment

::: content/html/content/src/nsHTMLSelectElement.h
@@ +684,5 @@
> +  /**
> +   * @return Whether aContent is a selected option element.
> +   */
> +  static bool MatchSelectedOptions(nsIContent* aContent, PRInt32 aNameSpaceID,
> +                                     nsIAtom* aAtom, void* aData);

nit: indentation

::: content/html/content/test/Makefile.in
@@ +343,5 @@
>  		test_style_attributes_reflection.html \
>  		test_bug629801.html \
>  		test_bug839371.html \
>  		test_formData.html \
> +		test_bug596681.html \

Could you add this test to content/html/content/test/forms/ instead?
And name it something like test_select_selectedOptions.html

::: content/html/content/test/test_bug596681.html
@@ +3,5 @@
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=596681
> +-->
> +<head>
> +  <title>Test for Bug 596681</title>

Test for select.selectedOptions

@@ +12,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=596681">Mozilla Bug 596681</a>
> +<p id="display"></p>
> +<pre id="test">
> +<script type="application/javascript;version=1.8">

Why 1.8?

@@ +14,5 @@
> +<p id="display"></p>
> +<pre id="test">
> +<script type="application/javascript;version=1.8">
> +
> +/** Test for Bug 596681 **/

Could you give a short description of the test here? (better than giving the bug number)

@@ +34,5 @@
> +ok("selectedOptions" in select,
> +   "select element should have a selectedOptions IDL attribute");
> +
> +ok(select.selectedOptions instanceof HTMLCollection,
> +   "selectedOptions should be an HTMLCollection instance");

Could you do those checks ('selectedOptions' in select) and the instance in a test_select_attributes_reflection.html test? (like test_button_attributes_reflection.html or test_input_attributes_reflection.html)

::: dom/interfaces/html/nsIDOMHTMLSelectElement.idl
@@ +43,2 @@
>                                  [optional] in nsIVariant before)
> +                                                     raises(DOMException);

Please, don't fix the trailing whitespaces in this commit (feel free to do that in another commit).
Comment 8 Mounir Lamouri (:mounir) 2013-08-13 05:12:13 PDT
Reuben, do you still plan to work on this?
Comment 9 Reuben Morais [:reuben] 2013-08-13 05:15:12 PDT
Oh wow, your feedback reply fell through the cracks. Yea, I can work on this.
Comment 10 Reuben Morais [:reuben] 2013-08-24 18:08:49 PDT
Created attachment 795109 [details] [diff] [review]
Implement HTMLSelectElement.selectedOptions
Comment 11 Reuben Morais [:reuben] 2013-08-24 18:22:31 PDT
Created attachment 795110 [details] [diff] [review]
Implement HTMLSelectElement.selectedOptions, v2

Oops, forgot we still have nsIDOMHTMLSelectElement. Interdiff:

diff -u b/content/html/content/src/HTMLSelectElement.cpp b/content/html/content/src/HTMLSelectElement.cpp
--- b/content/html/content/src/HTMLSelectElement.cpp
+++ b/content/html/content/src/HTMLSelectElement.cpp
@@ -793,6 +793,13 @@
   return mSelectedOptions;
 }
 
+NS_IMETHODIMP
+HTMLSelectElement::GetSelectedOptions(nsIDOMHTMLCollection** aSelectedOptions)
+{
+  *aSelectedOptions = SelectedOptions();
+  return NS_OK;
+}
+
 //NS_IMPL_INT_ATTR(HTMLSelectElement, SelectedIndex, selectedindex)
 
 NS_IMETHODIMP
only in patch2:
unchanged:
--- a/dom/interfaces/html/nsIDOMHTMLSelectElement.idl
+++ b/dom/interfaces/html/nsIDOMHTMLSelectElement.idl
@@ -39,16 +39,17 @@ interface nsIDOMHTMLSelectElement : nsIS
   // since IDL doesn't support overfload.
   //   void add(in nsIDOMHTMLElement, [optional] in nsIDOMHTMLElement)
   //   void add(in nsIDOMHTMLElement, in long)
   void                      add(in nsIDOMHTMLElement element, 
                                 [optional] in nsIVariant before)
                                                      raises(DOMException);   
   void                      remove(in long index);
 
+  readonly attribute nsIDOMHTMLCollection  selectedOptions;
            attribute long                  selectedIndex;
            attribute DOMString             value;
 
   // The following lines are part of the constraint validation API, see:
   // http://www.whatwg.org/specs/web-apps/current-work/#the-constraint-validation-api
   readonly attribute boolean             willValidate;
   readonly attribute nsIDOMValidityState validity;
   readonly attribute DOMString           validationMessage;
Comment 12 Olli Pettay [:smaug] 2013-08-25 04:29:00 PDT
Comment on attachment 795110 [details] [diff] [review]
Implement HTMLSelectElement.selectedOptions, v2


>+HTMLSelectElement::MatchSelectedOptions(nsIContent* aContent,
>+                                        int32_t /* unused */,
>+                                        nsIAtom* /* unused */,
>+                                        void* /* unused*/)
>+{
>+  HTMLOptionElement* option = HTMLOptionElement::FromContent(aContent);
>+  return option && option->Selected();
>+}





>+NS_IMETHODIMP
>+HTMLSelectElement::GetSelectedOptions(nsIDOMHTMLCollection** aSelectedOptions)
>+{
>+  *aSelectedOptions = SelectedOptions();
>+  return NS_OK;
You should addref *aSelectedOptions

>+  static bool MatchSelectedOptions(nsIContent*, int32_t, nsIAtom*, void*);
Could you give parameters names, at least for nsIContent*, since you're using that


>+let select = document.createElement("select");
>+document.body.appendChild(select);
Could you test also the case when select isn't in document


>+select.length = 0;
>+option1.selected = false;
>+option2.selected = false;
>+option3.selected = false;
>+var optgroup1 = document.createElement("optgroup");
>+optgroup1.appendChild(option1);
>+optgroup1.appendChild(option2);
>+select.add(optgroup1)
>+var optgroup2 = document.createElement("optgroup");
>+optgroup2.appendChild(option3);
>+select.add(optgroup2);

Could you add some more test for the case when option is removed.
For example when a selected option is removed from an optgroup


>+++ b/dom/interfaces/html/nsIDOMHTMLSelectElement.idl
>@@ -39,16 +39,17 @@ interface nsIDOMHTMLSelectElement : nsIS
update uuid

Would like to see an updated patch
Comment 13 Reuben Morais [:reuben] 2013-08-25 07:35:21 PDT
Created attachment 795156 [details] [diff] [review]
Implement HTMLSelectElement.selectedOptions, v3
Comment 14 Reuben Morais [:reuben] 2013-08-25 07:36:09 PDT
Created attachment 795158 [details] [diff] [review]
interdiff v2 => v3
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2013-08-27 05:05:41 PDT
Comment on attachment 795156 [details] [diff] [review]
Implement HTMLSelectElement.selectedOptions, v3

Review of attachment 795156 [details] [diff] [review]:
-----------------------------------------------------------------

Does anyone else support this? Do we really want a HTMLCollection rather than a NodeList?

::: content/html/content/src/HTMLSelectElement.cpp
@@ +26,5 @@
>  #include "nsIFormProcessor.h"
>  #include "nsIFrame.h"
>  #include "nsIListControlFrame.h"
>  #include "nsISelectControlFrame.h"
> +#include "nsContentList.h"

Keep the list sorted

@@ +795,5 @@
> +
> +NS_IMETHODIMP
> +HTMLSelectElement::GetSelectedOptions(nsIDOMHTMLCollection** aSelectedOptions)
> +{
> +  NS_IF_ADDREF(*aSelectedOptions = SelectedOptions());

No need for the IF

::: content/html/content/test/forms/test_select_selectedOptions.html
@@ +4,5 @@
> +https://bugzilla.mozilla.org/show_bug.cgi?id=596681
> +-->
> +<head>
> +  <title>Test for HTMLSelectElement.selectedOptions</title>
> +  <script type="application/javascript" src="/MochiKit/packed.js"></script>

Would be better to write this as a testharness.js test and submit it to https://github.com/w3c/web-platform-tests

@@ +19,5 @@
> + *
> + * selectedOptions is a live list of the options that have selectedness of true
> + * (not the selected content attribute).
> + *
> + * See http://www.w3.org/html/wg/drafts/html/master/forms.html#dom-select-selectedoptions

No, don't look at that. See http://www.whatwg.org/html/#dom-select-selectedoptions

@@ +41,5 @@
> +
> +ok("selectedOptions" in select,
> +   "select element should have a selectedOptions IDL attribute");
> +
> +ok(select.selectedOptions instanceof HTMLCollection,

Should probably also test the named getter on HTMLCollection, i.e. select.selectedOptions.foo with <option name=foo>
Comment 16 Olli Pettay [:smaug] 2013-08-27 07:09:30 PDT
Comment on attachment 795156 [details] [diff] [review]
Implement HTMLSelectElement.selectedOptions, v3

r+ with Ms2ger's comments addressed, though if you think it takes time
to convert the tests to use testharness, using normal mochitest is ok for now.
Comment 18 Ed Morley [:emorley] 2013-09-03 04:15:30 PDT
https://hg.mozilla.org/mozilla-central/rev/499f3c4fbd98
Comment 19 Simon Charette 2013-12-22 14:55:58 PST
Do we need a doc update: https://developer.mozilla.org/fr/docs/Web/API/HTMLSelectElement?
Comment 20 David Bruant 2013-12-22 15:43:51 PST
(In reply to Simon Charette from comment #19)
> Do we need a doc update:
> https://developer.mozilla.org/fr/docs/Web/API/HTMLSelectElement?
Yes we do. That's what "dev-doc-needed" means in the keywords field. Do you feel like contributing to the documentation? If so, make a change to the relevant page (usually the English page first) and ping me (by email) for a review if you need to.
Comment 21 Simon Charette 2013-12-22 16:19:46 PST
I gave it a try but I can't seems to find the right macro. I tried {{ FirefoxVersionIndicator(26) }} and got an exception.

Note You need to log in before you can comment on or make changes to this bug.