Closed Bug 596681 Opened 14 years ago Closed 11 years ago

Implement HTMLSelectElement selectedOptions IDL attribute

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: mounir, Assigned: reuben)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, html5)

Attachments

(2 files, 5 obsolete files)

It should return all selected options inside the select element.
Summary: Implement HTMLSelectElement selcetedOptions IDL attribute → Implement HTMLSelectElement selectedOptions IDL attribute
Blocks: 344614
Attached patch WIP Patch (obsolete) — Splinter Review
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...
Status: ASSIGNED → NEW
Attached patch Patch, rebased to tip (obsolete) — Splinter Review
(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)
Attached patch Patch, rebased to tip (obsolete) — Splinter Review
Oops, forgot to qref.
Attachment #722498 - Attachment is obsolete: true
Attachment #722498 - Flags: feedback?(mounir)
Attachment #722499 - Flags: feedback?(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).
Attachment #722499 - Flags: feedback?(mounir)
Flags: needinfo?(reuben.bmo)
(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)
Flags: needinfo?(mounir)
Flags: needinfo?(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.
Attachment #722499 - Flags: feedback?(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).
Attachment #722499 - Flags: feedback?(mounir) → feedback+
Reuben, do you still plan to work on this?
Assignee: mounir → reuben.bmo
Flags: needinfo?(reuben.bmo)
Oh wow, your feedback reply fell through the cracks. Yea, I can work on this.
Flags: needinfo?(reuben.bmo)
Attachment #722499 - Attachment is obsolete: true
Attachment #795109 - Flags: review?(bugs)
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-
Attachment #795110 - Attachment is obsolete: true
Attachment #795156 - Flags: review?(bugs)
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+
Blocks: 911808
https://hg.mozilla.org/mozilla-central/rev/499f3c4fbd98
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(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.
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.

Attachment

General

Created:
Updated:
Size: