596681 - Implement HTMLSelectElement selectedOptions IDL attribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

It should return all selected options inside the select element.

Comment 1

[Mounir Lamouri (:mounir)]

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

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

Attachment: Patch, rebased to tip

Oops, forgot to qref.

Comment 4

[Mounir Lamouri (:mounir)]

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

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

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

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

Reuben, do you still plan to work on this?

Comment 9

[Reuben Morais [:reuben]]

Oh wow, your feedback reply fell through the cracks. Yea, I can work on this.

Comment 10

[Reuben Morais [:reuben]]

Attachment: Implement HTMLSelectElement.selectedOptions

Comment 11

[Reuben Morais [:reuben]]

Attachment: 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][[e-mail]]]

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

Attachment: Implement HTMLSelectElement.selectedOptions, v3

Comment 14

[Reuben Morais [:reuben]]

Attachment: interdiff v2 => v3

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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][[e-mail]]]

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 17

[Reuben Morais [:reuben]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/499f3c4fbd98

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/499f3c4fbd98

Comment 19

[Simon Charette]

Do we need a doc update: https://developer.mozilla.org/fr/docs/Web/API/HTMLSelectElement?

Comment 20

[David Bruant]

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

I gave it a try but I can't seems to find the right macro. I tried {{ FirefoxVersionIndicator(26) }} and got an exception.