Bug 84351 - [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list
Summary: [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks: 27247
  Show dependency treegraph
 
Reported: 2012-04-19 10:29 PDT by Kent Tamura
Modified: 2012-04-30 19:53 PDT (History)
3 users (show)

See Also:


Attachments
Patch (9.01 KB, patch)
2012-04-19 17:30 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (11.17 KB, patch)
2012-04-19 18:36 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (21.67 KB, patch)
2012-04-24 19:06 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (24.22 KB, patch)
2012-04-24 21:07 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (24.24 KB, patch)
2012-04-26 00:59 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cq-03 (6.42 MB, application/zip)
2012-04-26 23:35 PDT, WebKit Review Bot
no flags Details
Patch (24.61 KB, patch)
2012-04-27 00:50 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cq-02 (6.28 MB, application/zip)
2012-04-27 05:59 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-04-19 10:29:12 PDT
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.
Comment 1 Keishi Hattori 2012-04-19 17:30:35 PDT
Created attachment 138012 [details]
Patch
Comment 2 Kent Tamura 2012-04-19 17:45:28 PDT
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?
Comment 3 Keishi Hattori 2012-04-19 18:36:52 PDT
Created attachment 138024 [details]
Patch
Comment 4 Build Bot 2012-04-19 18:46:14 PDT
Comment on attachment 138024 [details]
Patch

Attachment 138024 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12483040
Comment 5 Kent Tamura 2012-04-19 18:55:56 PDT
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):
> +
> +2012-04-19  Keishi Hattori  <keishi@webkit.org>
> +
> +        [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
Comment 6 Kent Tamura 2012-04-20 07:36:31 PDT
We should add test cases to fast/forms/datalist/input-list.html.
Comment 7 Kent Tamura 2012-04-24 00:03:47 PDT
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.
Comment 8 Keishi Hattori 2012-04-24 19:06:02 PDT
Created attachment 138717 [details]
Patch
Comment 9 Kent Tamura 2012-04-24 20:18:04 PDT
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.
Comment 10 Kent Tamura 2012-04-24 20:19:59 PDT
(In reply to comment #9)
> This test must not be flay.

flay -> flaky.
"PASS FAIL" means the test is flaky.
Comment 11 Keishi Hattori 2012-04-24 21:07:49 PDT
Created attachment 138733 [details]
Patch
Comment 12 Kent Tamura 2012-04-24 21:37:24 PDT
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].
Comment 13 WebKit Review Bot 2012-04-25 05:50:38 PDT
Comment on attachment 138733 [details]
Patch

Attachment 138733 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12525542
Comment 14 Kent Tamura 2012-04-25 18:07:04 PDT
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::
Comment 15 Keishi Hattori 2012-04-26 00:59:32 PDT
Created attachment 138950 [details]
Patch
Comment 16 WebKit Review Bot 2012-04-26 23:35:00 PDT
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
Comment 17 WebKit Review Bot 2012-04-26 23:35:06 PDT
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
Comment 18 Keishi Hattori 2012-04-27 00:50:58 PDT
Created attachment 139143 [details]
Patch
Comment 19 WebKit Review Bot 2012-04-27 05:59:01 PDT
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
Comment 20 WebKit Review Bot 2012-04-27 05:59:07 PDT
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
Comment 21 WebKit Review Bot 2012-04-30 19:52:38 PDT
Comment on attachment 139143 [details]
Patch

Clearing flags on attachment: 139143

Committed r115704: <http://trac.webkit.org/changeset/115704>
Comment 22 WebKit Review Bot 2012-04-30 19:53:10 PDT
All reviewed patches have been landed.  Closing bug.