Bug 84351 – [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

The specification says:

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#concept-input-list
> If the list attribute does not apply, there is no suggestions source element.


For <input type=date list=foo>, it returns the <datalist id=foo> though the date type doesn't support datalist in Chromium yet.
For <input type=color list=foo>, it returns null though the color type doesn't support datalist in Chromium yet.

They should return null for consistency.

[Keishi Hattori]

Created attachment 138012 [details]
Patch

[Kent Tamura]

Comment on attachment 138012 [details]
Patch

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

> Source/WebCore/html/InputType.cpp:624
> +    return theme->supportsDataListUI(formControlType());

We should ask RenderTheme about it only for supported types defined by the standard.

> Source/WebCore/rendering/RenderTheme.h:135
> +    virtual bool supportsDataListUI(const AtomicString& type) const { printf("XX\n"); return false; }

printf

> Source/WebCore/rendering/RenderThemeChromiumMac.mm:77
> +    return type == InputTypeNames::text();

We supports email, tel, search, url, and number, right?

[Keishi Hattori]

Created attachment 138024 [details]
Patch

[Build Bot]

Comment on attachment 138024 [details]
Patch

Attachment 138024 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12483040

[Kent Tamura]

Comment on attachment 138024 [details]
Patch

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

> Source/WebCore/ChangeLog:38
> +        * rendering/RenderThemeChromiumSkia.h:
> +        (RenderThemeChromiumSkia):
> +
> [phone]  Keishi Hattori  <[e-mail]>
> +
> +        [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list
> +        https://bugs.webkit.org/show_bug.cgi?id=84351

Duplicated ChangeLog entries

> Source/WebCore/html/ColorInputType.cpp:164
> +    Document* document = element()->document();
> +    RefPtr<RenderTheme> theme = document->page() ? document->page()->theme() : RenderTheme::defaultTheme();
> +    return theme->supportsDataListUI(formControlType());

Repeating this code is not good.
How about adding a static function to InputType and call it from FooInputType::shouldRespectListAttribute, or implementing this in InputType and overriding in non-supported types?

> Source/WebCore/rendering/RenderTheme.h:134
> +    // A method asking if the theme is able to show datalist suggestions.

In this case, "theme is able to show ..." is not correct; "The platform is able to show ..."?
You should explain what is "type" argument.
We should mention what types should be supported as per the standard.

> Source/WebCore/rendering/RenderThemeChromiumMac.h:35
> +    virtual bool supportsDataListUI(const AtomicString& type) const;

should append OVERRIDE.

> Source/WebCore/rendering/RenderThemeChromiumMac.mm:78
> +    return type == InputTypeNames::text() || type == InputTypeNames::search() || type == InputTypeNames::url()
> +        || type == InputTypeNames::telephone() || type == InputTypeNames::email() || type == InputTypeNames::number();

We should add a FIXME comment about unsupported types.

> Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:133
> +bool RenderThemeChromiumSkia::supportsDataListUI(const AtomicString& type) const
> +{
> +    return type == InputTypeNames::text() || type == InputTypeNames::search() || type == InputTypeNames::url()
> +        || type == InputTypeNames::telephone() || type == InputTypeNames::email() || type == InputTypeNames::number();
> +}

We shouldn't repeat the same code.
We might want a file for such chromium-common code.  RenderThemeChromiumCommon.h?

> Source/WebCore/rendering/RenderThemeChromiumSkia.h:54
> +        virtual bool supportsDataListUI(const AtomicString& type) const;

should append OVERRIDE

[Kent Tamura]

We should add test cases to fast/forms/datalist/input-list.html.

[Kent Tamura]

Comment on attachment 138024 [details]
Patch

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

>> Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:133
>> +}
> 
> We shouldn't repeat the same code.
> We might want a file for such chromium-common code.  RenderThemeChromiumCommon.h?

Also, we have a problem with type=email (Bug 84346). We might need to unsupport it.

[Keishi Hattori]

Created attachment 138717 [details]
Patch

[Kent Tamura]

Comment on attachment 138717 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeChromiumMac.h:35
> +    bool supportsDataListUI(const AtomicString& type) const OVERRIDE;

Don't you add virtual?

> Source/WebCore/rendering/RenderThemeChromiumSkia.h:55
> +        bool supportsDataListUI(const AtomicString& type) const OVERRIDE;

ditto.

> LayoutTests/fast/forms/datalist/input-list.html:40
>  <input type="text" id="i4" list="dl1">
> +<input type="search" id="i5" list="dl1">
> +<input type="url" id="i6" list="dl1">
> +<input type="telephone" id="i7" list="dl1">
> +<input type="email" id="i8" list="dl1">
> +<input type="datetime" id="i9" list="dl1">
> +<input type="date" id="i10" list="dl1">
> +<input type="month" id="i11" list="dl1">
> +<input type="week" id="i12" list="dl1">
> +<input type="time" id="i13" list="dl1">
> +<input type="datetime-local" id="i14" list="dl1">
> +<input type="number" id="i15" list="dl1">
> +<input type="range" id="i16" list="dl1">
> +<input type="color" id="i17" list="dl1">
>  <!-- Unsupported type -->
> -<input type="password" id="i5" list="dl1">
> +<input type="hidden" id="i18" list="dl1">
> +<input type="password" id="i19" list="dl1">
> +<input type="checkbox" id="i20" list="dl1">
> +<input type="radio" id="i21" list="dl1">
> +<input type="file" id="i22" list="dl1">
> +<input type="submit" id="i23" list="dl1">
> +<input type="image" id="i24" list="dl1">
> +<input type="reset" id="i25" list="dl1">
> +<input type="button" id="i26" list="dl1">

We had better rename their ID attribute values like
   id="text"
   id="search"
and test them with
    var datalistElement = document.getElementById('dl1');
    shouldBe('document.getElementById("text").list', 'datalistElement');
    shouldBe('document.getElementById("search").list', 'datalistElement');
to improve readability of the test result.

> LayoutTests/platform/chromium/test_expectations.txt:1101
> +// Implementation of datalist is incomplete.
> +BUGWK27247 : fast/forms/datalist/input-list.html = PASS FAIL
> +

This test must not be flay.
We should commit the ideal result as fast/forms/datalist/input-list-expected.txt, and commit the current Chromium result as platform/chromium/fast/forms/datalist/input-list-expected.txt. We shouldn't update test_expectations.txt.

[Kent Tamura]

(In reply to comment #9)
> This test must not be flay.

flay -> flaky.
"PASS FAIL" means the test is flaky.

[Keishi Hattori]

Created attachment 138733 [details]
Patch

[Kent Tamura]

Comment on attachment 138733 [details]
Patch

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

Looks ok

> Source/WebCore/ChangeLog:3
> +        [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list

This patch will resolve a Chromium issue, but touching non-Chromium files.  So we should remove [Chromium].

[WebKit Review Bot]

Comment on attachment 138733 [details]
Patch

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

[Kent Tamura]

Comment on attachment 138733 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:128
> +bool RenderThemeChromiumMac::supportsDataListUI(const AtomicString& type) const

should be RenderThemeChromiumSkia::

[Keishi Hattori]

Created attachment 138950 [details]
Patch

[WebKit Review Bot]

Comment on attachment 138950 [details]
Patch

Rejecting attachment 138950 [details] from commit-queue.

New failing tests:
fast/forms/datalist/input-list.html
fast/images/gif-large-checkerboard.html
Full output: http://queues.webkit.org/results/12552123

[WebKit Review Bot]

Created attachment 139138 [details]
Archive of layout-test-results from ec2-cq-03

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Keishi Hattori]

Created attachment 139143 [details]
Patch

[WebKit Review Bot]

Comment on attachment 139143 [details]
Patch

Rejecting attachment 139143 [details] from commit-queue.

New failing tests:
fast/images/gif-large-checkerboard.html
Full output: http://queues.webkit.org/results/12567033

[WebKit Review Bot]

Created attachment 139179 [details]
Archive of layout-test-results from ec2-cq-02

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 139143 [details]
Patch

Clearing flags on attachment: 139143

Committed r115704: <http://trac.webkit.org/changeset/115704>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.