Bug 61273 - Wrap input color in feature flag
Summary: Wrap input color in feature flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
: 61673 (view as bug list)
Depends on:
Blocks: 29358 37024
  Show dependency treegraph
 
Reported: 2011-05-23 05:38 PDT by Keishi Hattori
Modified: 2011-06-20 19:05 PDT (History)
7 users (show)

See Also:


Attachments
Disable input color. Add INPUT_COLOR feature flag. Implement input color sanitizer. (47.79 KB, patch)
2011-05-23 05:40 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.38 MB, application/zip)
2011-05-23 06:38 PDT, WebKit Review Bot
no flags Details
fixed tests. enabled flag for chromium port (50.31 KB, patch)
2011-05-23 19:04 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed mistake (50.34 KB, patch)
2011-05-23 19:26 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed test & changelog (50.47 KB, patch)
2011-05-23 22:09 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
fixed ValidityState-patternMismatch-unsupported test (50.70 KB, patch)
2011-05-23 22:33 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
rebased for commit (50.65 KB, patch)
2011-05-24 20:42 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2011-05-23 05:38:22 PDT
Because the input color UI needs to be implemented individually for each platform, it should be wrapped in a feature flag.
Comment 1 Keishi Hattori 2011-05-23 05:40:27 PDT
Created attachment 94404 [details]
Disable input color. Add INPUT_COLOR feature flag. Implement input color sanitizer.

This patch does the following
- disables the current input type=color implementation
- adds ENABLE_INPUT_COLOR feature flag
- replace typemismatch with value sanitization
Comment 2 WebKit Review Bot 2011-05-23 06:38:21 PDT
Comment on attachment 94404 [details]
Disable input color. Add INPUT_COLOR feature flag. Implement input color sanitizer.

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

New failing tests:
fast/forms/color/input-value-sanitization-color.html
fast/forms/ValidityState-patternMismatch-unsupported.html
fast/forms/input-type-change3.html
Comment 3 WebKit Review Bot 2011-05-23 06:38:26 PDT
Created attachment 94416 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 4 Kent Tamura 2011-05-23 18:11:15 PDT
Comment on attachment 94404 [details]
Disable input color. Add INPUT_COLOR feature flag. Implement input color sanitizer.

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

r- because of Chromium Linux test failures.

> LayoutTests/fast/forms/color/input-value-sanitization-color.html:10
> +<script src="input-value-sanitization-color.js"></script>

You don't need to separate the JS code to another file.  Please embed it here.

> Source/WebCore/ChangeLog:7
> +        Disable input color. Add INPUT_COLOR feature flag. Implement input color sanitizer.
> +        https://bugs.webkit.org/show_bug.cgi?id=29358
> +

You need more words for "Disable input color".  e.g. Disable textfield implementation of <input type=color>.

nit: you can split the patch into three:
 - Introduce INPUT_COLOR flag
 - Disabled textfield implementation
 - Implement value sanitization
You may proceed this patch without splitting because I'm very familiar with the code around there.  But smaller patch is better in general.
Comment 5 Keishi Hattori 2011-05-23 19:04:56 PDT
Created attachment 94545 [details]
fixed tests. enabled flag for chromium port
Comment 6 Keishi Hattori 2011-05-23 19:26:32 PDT
Created attachment 94547 [details]
fixed mistake
Comment 7 Kent Tamura 2011-05-23 20:50:07 PDT
Comment on attachment 94547 [details]
fixed mistake

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

> LayoutTests/ChangeLog:20
> +        * fast/forms/ValidityState-typeMismatch-color-expected.txt: Removed.
> +        * fast/forms/ValidityState-typeMismatch-color.html: Removed.
> +        * fast/forms/color/input-value-sanitization-color-expected.txt: Added.
> +        * fast/forms/color/input-value-sanitization-color.html: Added. Tests
> +        sanitization algorithm for input type=color.
> +        * fast/forms/color/input-value-sanitization-color.js: Added.
> +        * fast/forms/input-widths-expected.txt: 
> +        * fast/forms/input-widths.html: Removed type=color because it
> +        is no loger a text input type.
> +        * fast/forms/script-tests/ValidityState-typeMismatch-color.js: Removed.
> +        * platform/gtk/Skipped: Skip fast/forms/color.
> +        * platform/qt/Skipped: Skip fast/forms/color.
> +        * platform/win/Skipped: Skip fast/forms/color.

This file list is out-of-sync.

> LayoutTests/fast/forms/ValidityState-patternMismatch-unsupported.html:-13
> -<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> -<html>
> -<head>
> -<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">
> -<script src="../../fast/js/resources/js-test-pre.js"></script>
> -</head>
> -<body>
> -<p id="description"></p>
> -<div id="console"></div>
> -<script src="script-tests/ValidityState-patternMismatch-unsupported.js"></script>
> -<script src="../../fast/js/resources/js-test-post.js"></script>
> -</body>
> -</html>

Please don't remove it.
We should change the type=color usage to another type which doesn't support the pattern attribute.
Comment 8 Keishi Hattori 2011-05-23 22:09:16 PDT
Created attachment 94562 [details]
fixed test & changelog
Comment 9 Kent Tamura 2011-05-23 22:24:19 PDT
Comment on attachment 94562 [details]
fixed test & changelog

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

r- because of a wrong test.

> LayoutTests/fast/forms/script-tests/ValidityState-patternMismatch-unsupported.js:6
> -input.type = 'color';
> +input.type = 'range';
>  input.pattern = '#[0-9A-F]{6}';  // Restrict to capital letters
>  input.value = '#0099ff';

The intention of this test is to try to restrict valid values by pattern attribute.
#0099ff is invalid for type=range, and pattern='#[0-9A-F]{6}' makes no sense for type=range.

The 8th line still has 'type=color' in the comment.
Comment 10 Keishi Hattori 2011-05-23 22:33:43 PDT
Created attachment 94566 [details]
fixed ValidityState-patternMismatch-unsupported test
Comment 11 Keishi Hattori 2011-05-23 22:34:36 PDT
(In reply to comment #9)
> (From update of attachment 94562 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94562&action=review
> 
> r- because of a wrong test.
I'm sorry. I hope this fixes it.
Comment 12 Kent Tamura 2011-05-23 23:24:52 PDT
Comment on attachment 94566 [details]
fixed ValidityState-patternMismatch-unsupported test

Looks good!
Comment 13 Keishi Hattori 2011-05-24 20:42:20 PDT
Created attachment 94736 [details]
rebased for commit
Comment 14 Kent Tamura 2011-05-24 23:03:21 PDT
Comment on attachment 94736 [details]
rebased for commit

Clearing flags on attachment: 94736

Committed r87274: <http://trac.webkit.org/changeset/87274>
Comment 15 Kent Tamura 2011-05-24 23:03:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Kent Tamura 2011-06-20 19:05:33 PDT
*** Bug 61673 has been marked as a duplicate of this bug. ***