Implement HTMLSelectElement selectedOptions IDL attribute

RESOLVED FIXED in mozilla26

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: mounir, Assigned: reuben)

Tracking

(Blocks: 2 bugs, {dev-doc-needed, html5})

Trunk
mozilla26
dev-doc-needed, html5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
It should return all selected options inside the select element.

Updated

7 years ago
Summary: Implement HTMLSelectElement selcetedOptions IDL attribute → Implement HTMLSelectElement selectedOptions IDL attribute
(Reporter)

Updated

7 years ago
Blocks: 344614
Keywords: dev-doc-needed, html5
(Reporter)

Comment 1

7 years ago
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...
(Reporter)

Updated

7 years ago
Status: ASSIGNED → NEW
(Assignee)

Comment 2

4 years ago
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.
Attachment #482810 - Attachment is obsolete: true
Attachment #722498 - Flags: feedback?(mounir)
(Assignee)

Comment 3

4 years ago
Created attachment 722499 [details] [diff] [review]
Patch, rebased to tip

Oops, forgot to qref.
Attachment #722498 - Attachment is obsolete: true
Attachment #722498 - Flags: feedback?(mounir)
Attachment #722499 - Flags: feedback?(mounir)
(Reporter)

Comment 4

4 years ago
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).
Attachment #722499 - Flags: feedback?(mounir)
(Reporter)

Updated

4 years ago
Flags: needinfo?(reuben.bmo)
(Assignee)

Comment 5

4 years ago
(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
Flags: needinfo?(reuben.bmo)
(Assignee)

Updated

4 years ago
Flags: needinfo?(mounir)
(Reporter)

Updated

4 years ago
Flags: needinfo?(mounir)
(Reporter)

Comment 6

4 years ago
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.
Attachment #722499 - Flags: feedback?(mounir)
(Reporter)

Comment 7

4 years ago
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).
Attachment #722499 - Flags: feedback?(mounir) → feedback+
(Reporter)

Comment 8

4 years ago
Reuben, do you still plan to work on this?
Assignee: mounir → reuben.bmo
(Reporter)

Updated

4 years ago
Flags: needinfo?(reuben.bmo)
(Assignee)

Comment 9

4 years ago
Oh wow, your feedback reply fell through the cracks. Yea, I can work on this.
Flags: needinfo?(reuben.bmo)
(Assignee)

Comment 10

4 years ago
Created attachment 795109 [details] [diff] [review]
Implement HTMLSelectElement.selectedOptions
Attachment #722499 - Attachment is obsolete: true
Attachment #795109 - Flags: review?(bugs)
(Assignee)

Comment 11

4 years ago
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;
Attachment #795109 - Attachment is obsolete: true
Attachment #795109 - Flags: review?(bugs)
Attachment #795110 - Flags: review?(bugs)
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
Attachment #795110 - Flags: review?(bugs) → review-
(Assignee)

Comment 13

4 years ago
Created attachment 795156 [details] [diff] [review]
Implement HTMLSelectElement.selectedOptions, v3
Attachment #795110 - Attachment is obsolete: true
Attachment #795156 - Flags: review?(bugs)
(Assignee)

Comment 14

4 years ago
Created attachment 795158 [details] [diff] [review]
interdiff v2 => v3
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 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.
Attachment #795156 - Flags: review?(bugs) → review+
(Assignee)

Comment 17

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/499f3c4fbd98
(Assignee)

Updated

4 years ago
Blocks: 911808
https://hg.mozilla.org/mozilla-central/rev/499f3c4fbd98
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Comment 19

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

Comment 20

4 years ago
(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

4 years ago
I gave it a try but I can't seems to find the right macro. I tried {{ FirefoxVersionIndicator(26) }} and got an exception.
You need to log in before you can comment on or make changes to this bug.