RESOLVED FIXED 84351
[Chromium] datalist: Inconsistent behavior of HTMLInputElement::list
https://bugs.webkit.org/show_bug.cgi?id=84351
Summary [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list
Kent Tamura
Reported Thursday, April 19, 2012 6:29:12 PM UTC
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.
Attachments
Patch (9.01 KB, patch)
2012-04-19 17:30 PDT, Keishi Hattori
no flags
Patch (11.17 KB, patch)
2012-04-19 18:36 PDT, Keishi Hattori
no flags
Patch (21.67 KB, patch)
2012-04-24 19:06 PDT, Keishi Hattori
no flags
Patch (24.22 KB, patch)
2012-04-24 21:07 PDT, Keishi Hattori
no flags
Patch (24.24 KB, patch)
2012-04-26 00:59 PDT, Keishi Hattori
no flags
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
Patch (24.61 KB, patch)
2012-04-27 00:50 PDT, Keishi Hattori
no flags
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
Keishi Hattori
Comment 1 Friday, April 20, 2012 1:30:35 AM UTC
Kent Tamura
Comment 2 Friday, April 20, 2012 1:45:28 AM UTC
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
Comment 3 Friday, April 20, 2012 2:36:52 AM UTC
Build Bot
Comment 4 Friday, April 20, 2012 2:46:14 AM UTC
Kent Tamura
Comment 5 Friday, April 20, 2012 2:55:56 AM UTC
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
Kent Tamura
Comment 6 Friday, April 20, 2012 3:36:31 PM UTC
We should add test cases to fast/forms/datalist/input-list.html.
Kent Tamura
Comment 7 Tuesday, April 24, 2012 8:03:47 AM UTC
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
Comment 8 Wednesday, April 25, 2012 3:06:02 AM UTC
Kent Tamura
Comment 9 Wednesday, April 25, 2012 4:18:04 AM UTC
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
Comment 10 Wednesday, April 25, 2012 4:19:59 AM UTC
(In reply to comment #9) > This test must not be flay. flay -> flaky. "PASS FAIL" means the test is flaky.
Keishi Hattori
Comment 11 Wednesday, April 25, 2012 5:07:49 AM UTC
Kent Tamura
Comment 12 Wednesday, April 25, 2012 5:37:24 AM UTC
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 13 Wednesday, April 25, 2012 1:50:38 PM UTC
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 14 Thursday, April 26, 2012 2:07:04 AM UTC
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
Comment 15 Thursday, April 26, 2012 8:59:32 AM UTC
WebKit Review Bot
Comment 16 Friday, April 27, 2012 7:35:00 AM UTC
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
Comment 17 Friday, April 27, 2012 7:35:06 AM UTC
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
Comment 18 Friday, April 27, 2012 8:50:58 AM UTC
WebKit Review Bot
Comment 19 Friday, April 27, 2012 1:59:01 PM UTC
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
Comment 20 Friday, April 27, 2012 1:59:07 PM UTC
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 21 Tuesday, May 1, 2012 3:52:38 AM UTC
Comment on attachment 139143 [details] Patch Clearing flags on attachment: 139143 Committed r115704: <http://trac.webkit.org/changeset/115704>
WebKit Review Bot
Comment 22 Tuesday, May 1, 2012 3:53:10 AM UTC
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.