Bug 85937

Summary: [Chromium][Forms] HTMLOptionsCollection doesn't have indexed properties on property enumeration
Product: WebKit Reporter: yosin
Component: FormsAssignee: yosin
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, bweinstein, dglazkov, haraken, japhet, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://jsfiddle.net/MbtFP/5/
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch 3
none
Patch 4 none

Description yosin 2012-05-08 17:57:06 PDT
ConsoleMessage._printArray takes elements of array object of inspector's internal representation from numeric property name. However, HTMLOptionsCollection object don't have such properties.

Even though, getting a element works in scripts in original HTML and JavaScript console.

Below is sample steps:

1. Visit data:text/html,<form><select id="s"><option value="1">one</option><option value="2">two</option></select></form>
2. Invoke JavaScript Console
3. Enter "keys(documents.forms)"
  ["0", "length"]
4. Enter "keys(s.options)"
  ["selectedIndex", "length"]
5. Enter: "s.options
  []

Note: Safari's JavaScript console works fine.
Comment 1 yosin 2012-05-08 20:54:15 PDT
Object.keys(select.options) should return an array of property keys containing numeric index value, e.g. ["0", "1", "selectedIndex", "length"].

Here is test result of browsers with simple test http://jsfiddle.net/MbtFP/5/
* FF12 OK
* Chrome18 NOT -- no "0" and "1"
* IE9 OK
* Opera11 OK
Comment 2 yosin 2012-05-08 21:28:44 PDT
Created attachment 140863 [details]
Patch
Comment 3 yosin 2012-05-08 21:34:14 PDT
This bug blocks http://crbug.com/121890 - DOM Select Object always returns empty list for options attribute
Comment 4 Kentaro Hara 2012-05-08 21:34:55 PDT
Comment on attachment 140863 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140863&action=review

What is happening for other interfaces that have indexed properties? I mean, should this change be for HTMLOptionsCollection only?

> Source/WebCore/ChangeLog:10
> +

Please describe other browsers' behavior in ChangeLog, to justify your change.
Comment 5 yosin 2012-05-08 21:42:35 PDT
Created attachment 140864 [details]
Patch
Comment 6 WebKit Review Bot 2012-05-09 00:27:24 PDT
Comment on attachment 140864 [details]
Patch

Attachment 140864 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12662002

New failing tests:
inspector/console/console-format-collections.html
Comment 7 WebKit Review Bot 2012-05-09 00:27:46 PDT
Created attachment 140872 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 8 yosin 2012-05-09 00:43:53 PDT
Created attachment 140875 [details]
Patch 3
Comment 9 Kentaro Hara 2012-05-09 00:45:54 PDT
(In reply to comment #4)
> What is happening for other interfaces that have indexed properties? I mean, should this change be for HTMLOptionsCollection only?

Would you clarify the above point? I am not sure if we should fix HTMLOptionsCollection specific code or more general IndexedProperty code.(In reply to comment #0)
> ConsoleMessage._printArray takes elements of array object of inspector's internal representation from numeric property name. However, HTMLOptionsCollection object don't have such properties.
> 
> Even though, getting a element works in scripts in original HTML and JavaScript console.
> 
> Below is sample steps:
> 
> 1. Visit data:text/html,<form><select id="s"><option value="1">one</option><option value="2">two</option></select></form>
> 2. Invoke JavaScript Console
> 3. Enter "keys(documents.forms)"
>   ["0", "length"]
> 4. Enter "keys(s.options)"
>   ["selectedIndex", "length"]
> 5. Enter: "s.options
>   []
> 
> Note: Safari's JavaScript console works fine.
Comment 10 yosin 2012-05-09 02:14:00 PDT
(In reply to comment #9)
> (In reply to comment #4)
> > What is happening for other interfaces that have indexed properties? I mean, should this change be for HTMLOptionsCollection only?
> 
> Would you clarify the above point? I am not sure if we should fix HTMLOptionsCollection specific code or more general IndexedProperty code.(In reply to comment #0)

This change should affect HTMLOptionCollection only. 

Enumerator should be attached for indexed properties. In IDL which is specified by IndexedGetter(HTMLCollection, HTMLSelect), NumericIndexer({Float,Int,UInt}{16,32,64,8}Array), or CustomGetOwnPropertySlot (no use case, HTMLAppletElement, HTMLEmbedElement, HTMLObjectElement have JSCustomGetOwnPropertySlotAndDescriptor). 

Code generator doesn't generate code for NumericIndexer, CustomGetOwnPropertySlot is handlbed by (%indexerSpecialCases hash map, we should add this attribute to these interfaces and remove %indexerSpecialCases).

For interfaces which have IndexedGetter, GenerateImplementationIndexer generates code of calling setCollectionXXX<T> which setting enumerator if interfaces inherited from Node or no CustomSetter, e.g. HTMLSelectElement interface has IndexedGetter and CustomSetter attribute, HTMLCollection has IndexedGetter attribute but no CustomSetter.
Comment 11 Kentaro Hara 2012-05-09 02:21:52 PDT
Comment on attachment 140875 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=140875&action=review

The patch looks good to me.

> LayoutTests/fast/forms/select/options-indexed-properties.html:11
> +</select>

Nit: Remove this.

> LayoutTests/fast/forms/select/options-indexed-properties.html:23
> +for (var propertyKey in options) {
> +    properties[propertyKey] = options[propertyKey];
> +}
> +shouldBe('properties[0].value', '"a"');
> +shouldBe('properties[1].value', '"b"');

You might want to write this as follows:

shouldBeEqualToString('options[0].value', "a");
shouldBeEqualToString('options[1].value', "b");

> LayoutTests/fast/forms/select/options-indexed-properties.html:29
> +

Nit: Remove the redundant new line.
Comment 12 yosin 2012-05-09 02:34:21 PDT
Created attachment 140893 [details]
Patch 4
Comment 13 WebKit Review Bot 2012-05-09 04:10:10 PDT
Comment on attachment 140893 [details]
Patch 4

Clearing flags on attachment: 140893

Committed r116513: <http://trac.webkit.org/changeset/116513>
Comment 14 WebKit Review Bot 2012-05-09 04:10:42 PDT
All reviewed patches have been landed.  Closing bug.