720385 - Implement index property on <option> in <datalist>

Status: RESOLVED FIXED

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

Description

[Codacoder]

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

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 (he/him; ⌚ UTC+1/+2)]

"list of options" is only defined for select elements, not for datalists, so we should return 0.

Comment 3

[Matthew N. [:MattN]]

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

IE 10 PP also returns 0 for the index property of an <option> in a <datalist>.

Comment 5

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

Code is here:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLOptionElement.cpp#196

and needs to be updated to match the specification at

http://www.whatwg.org/html/#concept-option-index

Comment 6

[Codacoder]

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 (he/him; ⌚ UTC+1/+2)]

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

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

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]

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

Attachment: Patch v1

Ms2ger, what about that patch? :)

Comment 12

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

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 (he/him; ⌚ UTC+1/+2)]

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

(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 (he/him; ⌚ UTC+1/+2)]

That sounds a little scary, but I can live with it if you add those givens to the documentation for GetOptionIndex.

Comment 16

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

Oh, and you can invent a better commit message ;)

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/b1ad94b656eb

Comment 18

[Jean-Yves Perrier [:teoli]]

I updated https://developer.mozilla.org/en/Firefox_13_for_developers
and
https://developer.mozilla.org/en/DOM/HTMLOptionElement