Last Comment Bug 720385 - Implement index property on <option> in <datalist>
: Implement index property on <option> in <datalist>
Status: RESOLVED FIXED
[parity-webkit][parity-opera][parity-...
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
http://www.whatwg.org/html/#concept-o...
Depends on:
Blocks: 555840
  Show dependency treegraph
 
Reported: 2012-01-23 09:07 PST by Codacoder
Modified: 2012-04-24 15:39 PDT (History)
7 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase comparing <select> and <datalist> behaviour (1.75 KB, text/html)
2012-02-15 15:17 PST, Matthew N. [:MattN] (behind on reviews)
no flags Details
Patch v1 (5.31 KB, patch)
2012-02-16 10:53 PST, Mounir Lamouri (:mounir)
Ms2ger: review+
mounir: checkin+
Details | Diff | Review

Description Codacoder 2012-01-23 09:07:19 PST
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

Created a datalist with simple set of options.  Each option has an id.


Actual results:

opt = opts["id-of-opt"]  
opt.index <-- returns -1
opt = opts[0] 
opt.index <-- returns -1


Expected results:

option.index should work as specified - return index of option in collection.
Comment 1 Matthew N. [:MattN] (behind on reviews) 2012-02-15 00:14:12 PST
I can confirm that the index property only returns -1 when the <option>s are in a <datalist> although it works fine when they are instead in a <select>.

"An option element's index is the number of option element that are in the same list of options but that come before it in tree order. If the option element is not in a list of options, then the option element's index is zero." [1]
[1] http://dev.w3.org/html5/spec/the-option-element.html#concept-option-index
Comment 2 :Ms2ger 2012-02-15 04:32:28 PST
"list of options" is only defined for select elements, not for datalists, so we should return 0.
Comment 3 Matthew N. [:MattN] (behind on reviews) 2012-02-15 15:17:18 PST
Created attachment 597578 [details]
Testcase comparing <select> and <datalist> behaviour

It may be worth proposing a spec change to have "list of options" also defined for <datalist> but currently Opera, & Webkit align with the spec.
Comment 4 Matthew N. [:MattN] (behind on reviews) 2012-02-15 15:22:37 PST
IE 10 PP also returns 0 for the index property of an <option> in a <datalist>.
Comment 6 Codacoder 2012-02-16 04:50:50 PST
Yep you guys a correct - my bad.  

I just wonder what the whatwg were smoking when they came up with a datalist element (note "list") that contains a "list of options" that somehow does not qualify as a list-of-options as per the spec:

"If the option element is not in a list of options, then the option element's index is zero."

And I'm told by one of them "It's too late to change it, in any case; it's already shipped in browsers."  I'm further told it's only intended for use by the list="" attribute on other controls.  I guess it must be my fault for trying to use a datalist as a, uh, "list" of, you know, "data".

Anyway, yes, zero it should be.
Comment 7 :Ms2ger 2012-02-16 05:03:06 PST
(In reply to Russ from comment #6)

Unhelpful. if you want the spec to be changed, there are numerous ways to submit feedback (see whatwg.org/html). This bug, however, is not one of those ways.
Comment 8 Codacoder 2012-02-16 05:22:22 PST
Unhelpful? To whom?  What???  The original report is wrong, agreed.  It's still a bug (as per spec - should return zero, mentioned in my comment, at the top and the bottom).  Rest of my comment was sharing "where I'd been".

So, let's be helpful:

When an option is a member of a list of options that make up the list of options within a datalist element the option is not considered to be a member of a [list of options] as defined by the spec.
Comment 9 Mounir Lamouri (:mounir) 2012-02-16 07:26:13 PST
What would that change if you had .index returning a value other than 0? Unless there is a real use case, I think we should just comply with the specs: it will be hard to have all browsers to move to something else for no big enough reason.
Comment 10 Codacoder 2012-02-16 07:31:50 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #9)
"List" implies "order".  Order implies position.  Position (index of) should be discoverable.  An index of zero is meaningless.  What we have is a bag, or a set, not a list.

Yes, we should comply with the spec.  No one is suggesting otherwise.
Comment 11 Mounir Lamouri (:mounir) 2012-02-16 10:53:28 PST
Created attachment 597895 [details] [diff] [review]
Patch v1

Ms2ger, what about that patch? :)
Comment 12 :Ms2ger 2012-02-16 15:13:43 PST
Looks fine on first sight, but I want to cross-check more carefully with the spec. I hope to get to it tomorrow.
Comment 13 :Ms2ger 2012-02-17 06:02:01 PST
Comment on attachment 597895 [details] [diff] [review]
Patch v1

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

r=me with those changes

::: content/html/content/src/nsHTMLOptionElement.cpp
@@ +208,4 @@
>  
> +  // Get the options from the select object.
> +  nsCOMPtr<nsIDOMHTMLOptionsCollection> options;
> +  selectElement->GetOptions(getter_AddRefs(options));

nsHTMLOptionCollection* options = selectElement->GetOptions();

@@ +229,2 @@
>      }
>    }

How about

PRInt32 index = 0;
if (NS_SUCCEEDED(options->GetOptionIndex(this, 0, true, &index)) {
  *aIndex = index;
}
Comment 14 Mounir Lamouri (:mounir) 2012-02-19 15:38:38 PST
(In reply to Ms2ger from comment #13)
> PRInt32 index = 0;
> if (NS_SUCCEEDED(options->GetOptionIndex(this, 0, true, &index)) {
>   *aIndex = index;
> }

Given that index shouldn't be set when there is a failure and a failure shouldn't happen, I think we can do:
return options->GetOptionIndex(this, 0, true, aIndex);

Ok with that Ms2ger?
Comment 15 :Ms2ger 2012-02-20 08:36:50 PST
That sounds a little scary, but I can live with it if you add those givens to the documentation for GetOptionIndex.
Comment 16 :Ms2ger 2012-02-24 13:05:58 PST
Oh, and you can invent a better commit message ;)
Comment 17 Marco Bonardo [::mak] 2012-02-25 02:22:40 PST
https://hg.mozilla.org/mozilla-central/rev/b1ad94b656eb

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