Bug 48544 - support -webkit-tap-highlight-color
Summary: support -webkit-tap-highlight-color
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-28 11:56 PDT by Tiancheng Jiang
Modified: 2011-10-16 23:50 PDT (History)
17 users (show)

See Also:


Attachments
add -webkit-tap-highlight-color feature with virtual function allow different platform to choose default color (14.18 KB, patch)
2010-10-28 11:57 PDT, Tiancheng Jiang
no flags Details | Formatted Diff | Diff
patch candidate (13.81 KB, patch)
2011-09-05 06:36 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v1 (13.81 KB, patch)
2011-09-09 03:50 PDT, Johnny(Jianning) Ding
sam: review-
Details | Formatted Diff | Diff
patch V2 (13.43 KB, patch)
2011-09-15 00:14 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch V2 (13.47 KB, patch)
2011-09-15 00:28 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v2 (17.17 KB, patch)
2011-09-15 01:02 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
latest patch v2 (17.65 KB, patch)
2011-09-27 08:03 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v3 (17.60 KB, patch)
2011-09-28 08:06 PDT, Johnny(Jianning) Ding
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch v3 to make ews happy (17.60 KB, patch)
2011-09-28 19:43 PDT, Johnny(Jianning) Ding
kenneth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch v3 to fix reported broken tests (20.19 KB, patch)
2011-09-30 13:21 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch to clean test expectation (1.63 KB, patch)
2011-10-09 04:34 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tiancheng Jiang 2010-10-28 11:56:01 PDT
We should apply -webkit-tap-highlight-color to allow the web developers to change the tap highlight color to fit their website.
Comment 1 Tiancheng Jiang 2010-10-28 11:57:09 PDT
Created attachment 72217 [details]
add -webkit-tap-highlight-color feature with virtual function allow different platform to choose default color
Comment 2 WebKit Review Bot 2010-10-28 12:02:16 PDT
Attachment 72217 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSParser.cpp:1617:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/css/CSSParser.cpp:1619:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/rendering/RenderTheme.h:149:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/rendering/style/RenderStyle.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Early Warning System Bot 2010-10-28 12:59:26 PDT
Attachment 72217 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4866016
Comment 4 Johnny(Jianning) Ding 2011-09-01 05:12:36 PDT
I am interested in this bug.

@Tiancheng, will you continue working on this?
Comment 5 Tiancheng Jiang 2011-09-01 07:38:05 PDT
Maybe not recently. Feel free to work on it. And I think someone is working on this as well, especially for iOS. There is a bug number but I forget the number, sorry.

(In reply to comment #4)
> I am interested in this bug.
> 
> @Tiancheng, will you continue working on this?
Comment 6 Simon Fraser (smfr) 2011-09-01 11:21:31 PDT
Comment on attachment 72217 [details]
add -webkit-tap-highlight-color feature with virtual function allow different platform to choose default color

I think this should be #ifdeffed to avoid using extra memory on platforms for which the notion of tap highlight does not apply.
Comment 7 Johnny(Jianning) Ding 2011-09-01 16:59:31 PDT
(In reply to comment #5)
> Maybe not recently. Feel free to work on it. And I think someone is working on this as well, especially for iOS. There is a bug number but I forget the number, sorry.
> 
Thanks Tiancheng! After searching existing webkit bugs, I haven't found related on going bug. But the "-webkit-tap-highlight-color" is already supported by Safari only on IOS as a extension,  but it's good to make it supported on other platforms
Comment 8 Johnny(Jianning) Ding 2011-09-01 17:22:31 PDT
(In reply to comment #6)
> (From update of attachment 72217 [details])
> I think this should be #ifdeffed to avoid using extra memory on platforms for which the notion of tap highlight does not apply.

Good point, Thanks Simon!. For using ifdef,  The first thing came to my mind is to use OS() macros since it can distinguish whether the device platforms is for desktop or portable if we only want to support this feature on portable devices which support touch panel.

But since there may be more OS marcos for the portable platforms w/ touch panel. I guess it may be better to use ENABLE(TOUCH_EVENTS) to control it. Sounds good? (Or another ENABLE macro?)
Comment 9 Johnny(Jianning) Ding 2011-09-05 06:36:14 PDT
Created attachment 106332 [details]
patch candidate

This patch is based on tiancheng's patch, only enables the tap-highlight property when enabling touch event.  Also adds a layout test draft.  I want to get some early feedbacks before sending for review.
Thanks.
Comment 10 Johnny(Jianning) Ding 2011-09-09 03:50:05 PDT
Created attachment 106856 [details]
patch v1
Comment 11 Johnny(Jianning) Ding 2011-09-13 20:50:01 PDT
Can someone take a look and give comments? Thanks.
Comment 12 Sam Weinig 2011-09-14 12:12:15 PDT
Comment on attachment 106856 [details]
patch v1

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

> Source/WebCore/platform/graphics/Color.h:166
> +    static const RGBA32 tapHighlight = 0x33000000;

This doesn't seem like a great place for this to go.  It seems like it should just be in RenderTheme.
Comment 13 Johnny(Jianning) Ding 2011-09-15 00:14:26 PDT
Created attachment 107468 [details]
patch V2

(In reply to comment #12)
> This doesn't seem like a great place for this to go.  It seems like it should just be in RenderTheme.

done.
Comment 14 Johnny(Jianning) Ding 2011-09-15 00:28:28 PDT
Created attachment 107469 [details]
patch V2
Comment 15 Johnny(Jianning) Ding 2011-09-15 01:02:34 PDT
Created attachment 107474 [details]
patch v2

upload latest patch v2 to add missing layout test files.
Comment 16 Antonio Gomes 2011-09-26 13:08:48 PDT
(In reply to comment #15)
> Created an attachment (id=107474) [details]
> patch v2
> 
> upload latest patch v2 to add missing layout test files.

Could you re upload a patch that applies to trunk? Also Tiancheng's work was based on some code in iphone's release tarball. Maybe it should be mentioned as well.
Comment 17 Johnny(Jianning) Ding 2011-09-27 08:03:27 PDT
Created attachment 108849 [details]
latest patch v2

(In reply to comment #16)
> Could you re upload a patch that applies to trunk? Also Tiancheng's work was based on some code in iphone's release tarball. Maybe it should be mentioned as well.
done.
Comment 18 Antonio Gomes 2011-09-27 08:37:52 PDT
Comment on attachment 108849 [details]
latest patch v2

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

Looks good to me. Would you mind to send an email to webkit-dev announcing it, since tap-highlight is not yet standard.

Ps: non of the EWS bots manage to apply this patch it seems.

> Source/WebCore/css/CSSParser.cpp:1941
> +             validPrimitive = true; // notice: apple call this valid_primitive 

this comment sounds a bit weird.
Comment 19 Kenneth Rohde Christiansen 2011-09-27 08:43:47 PDT
Zalan as you did a similar patch for us, can you have a look at this?
Comment 20 Kenneth Rohde Christiansen 2011-09-27 14:38:17 PDT
Comment on attachment 108849 [details]
latest patch v2

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

> Source/WebCore/ChangeLog:8
> +        Based on tijiang@rim.com's patch which was based on some code in Apple IPhone's release tarball.

I would write:

Original code from the iOS WebCore code dump, extracted and modified by ....

> Source/WebCore/ChangeLog:12
> +        used as WebKit default tap highlight color.

I would leave out WebKit here.

> Source/WebCore/rendering/style/StyleRareInheritedData.h:31
> -#include <wtf/RefCounted.h>
>  #include <wtf/PassRefPtr.h>
> +#include <wtf/RefCounted.h>

Unrelated change... Should be in another patch

> Source/WebCore/rendering/style/StyleRareInheritedData.h:81
>      unsigned resize : 2; // EResize
> -    unsigned userSelect : 1;  // EUserSelect
> +    unsigned userSelect : 1; // EUserSelect
>      unsigned colorSpace : 1; // ColorSpace

This change seems unreleated

> LayoutTests/fast/events/touch/tap-highlight-color-expected.txt:7
> +Stage: touch start

Stage? Sounds pretty strange. Event: touch start, or so would be more clear

> LayoutTests/fast/events/touch/script-tests/tap-highlight-color.js:56
> +
> +

No need for these newlines
Comment 21 Johnny(Jianning) Ding 2011-09-27 18:16:20 PDT
(In reply to comment #18)
> (From update of attachment 108849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108849&action=review
> 
> Looks good to me. Would you mind to send an email to webkit-dev announcing it, since tap-highlight is not yet standard.

Should I send it before landing it or after? 

> 
> Ps: non of the EWS bots manage to apply this patch it seems.
> 
> > Source/WebCore/css/CSSParser.cpp:1941
> > +             validPrimitive = true; // notice: apple call this valid_primitive 
> 
> this comment sounds a bit weird.
It's originally written by Tiancheng. I will remove it in next patch.
Comment 22 Johnny(Jianning) Ding 2011-09-27 18:22:03 PDT
(In reply to comment #20)
> I would write:
> 
> Original code from the iOS WebCore code dump, extracted and modified by ....
> 
> I would leave out WebKit here.
Good suggestions, will change them in next update.
> 
> > Source/WebCore/rendering/style/StyleRareInheritedData.h:31
> > -#include <wtf/RefCounted.h>
> >  #include <wtf/PassRefPtr.h>
> > +#include <wtf/RefCounted.h>
> 
> Unrelated change... Should be in another patch
It's just to make style-check happy. (resolved include order issue)
> 
> > Source/WebCore/rendering/style/StyleRareInheritedData.h:81
> >      unsigned resize : 2; // EResize
> > -    unsigned userSelect : 1;  // EUserSelect
> > +    unsigned userSelect : 1; // EUserSelect
> >      unsigned colorSpace : 1; // ColorSpace
> 
> This change seems unreleated
ditto. (removed redundant space)

> 
> Stage? Sounds pretty strange. Event: touch start, or so would be more clear
> 
> 
> No need for these newlines
Will change them in next update.
Comment 23 Antonio Gomes 2011-09-27 19:01:11 PDT
(In reply to comment #21)
> (In reply to comment #18)
> > (From update of attachment 108849 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=108849&action=review
> > 
> > Looks good to me. Would you mind to send an email to webkit-dev announcing it, since tap-highlight is not yet standard.
> 
> Should I send it before landing it or after? 

Before, please.
Comment 24 Johnny(Jianning) Ding 2011-09-28 08:06:38 PDT
Created attachment 109020 [details]
patch v3
Comment 25 Early Warning System Bot 2011-09-28 08:16:08 PDT
Comment on attachment 109020 [details]
patch v3

Attachment 109020 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9889043
Comment 26 WebKit Review Bot 2011-09-28 08:24:22 PDT
Comment on attachment 109020 [details]
patch v3

Attachment 109020 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9888172
Comment 27 Johnny(Jianning) Ding 2011-09-28 19:43:52 PDT
Created attachment 109113 [details]
patch v3 to make ews happy
Comment 28 WebKit Review Bot 2011-09-28 23:32:01 PDT
Comment on attachment 109113 [details]
patch v3 to make ews happy

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

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
Comment 29 Kenneth Rohde Christiansen 2011-09-29 12:05:27 PDT
Comment on attachment 109113 [details]
patch v3 to make ews happy

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

r=me, as noone complained about the feature on the ML and it is used by iOS, Playbook, Nokia N9 series and possible others as well. It can thus be considered a widely used WebKit mobile/touch feature.

> Source/WebCore/rendering/style/StyleRareInheritedData.h:80
> -    unsigned userSelect : 1;  // EUserSelect
> +    unsigned userSelect : 1; // EUserSelect

Let's remove unrelated changes
Comment 30 Johnny(Jianning) Ding 2011-09-30 13:21:50 PDT
Created attachment 109337 [details]
patch v3 to fix reported broken tests

This patch fixed the tests broken on try bot reported in comment #28. After considering the features support difference among all webkit ports (they may Change in the future) and "-webkit-tap-highlight-color" only supported on ports which support TOUCH_EVENTS. I disable the "-webkit-tap-highlight-color" output on related get computed style tests and only test it by using the new layout test fast/events/touch/tap-highlight-color.html.

Kenneth, Antonio, would you like to reexamine it? Thanks!
Comment 31 WebKit Review Bot 2011-09-30 15:30:09 PDT
Comment on attachment 109337 [details]
patch v3 to fix reported broken tests

Clearing flags on attachment: 109337

Committed r96433: <http://trac.webkit.org/changeset/96433>
Comment 32 WebKit Review Bot 2011-09-30 15:30:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Adam Barth 2011-10-01 10:27:20 PDT
This test times out on Leopard:

fast/events/touch/tap-highlight-color.html

Is that expected?
Comment 34 Adam Barth 2011-10-01 10:32:03 PDT
I've noted the test as timing out:
http://trac.webkit.org/changeset/96457

If that is in error, please correct the issue.
Comment 35 Johnny(Jianning) Ding 2011-10-02 02:33:41 PDT
> If that is in error, please correct the issue.

Thanks for fix, Adam. I don't have Leopard on vacation now. Will fix it when I am back.
Comment 36 Johnny(Jianning) Ding 2011-10-09 04:34:24 PDT
Created attachment 110297 [details]
patch to clean test expectation

The timeout on LEOPARD is caused bug 66577, change the bug no and close this bug
Comment 37 WebKit Review Bot 2011-10-09 06:08:49 PDT
Comment on attachment 110297 [details]
patch to clean test expectation

Clearing flags on attachment: 110297

Committed r97027: <http://trac.webkit.org/changeset/97027>
Comment 38 WebKit Review Bot 2011-10-09 06:08:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Antonio Gomes 2011-10-12 08:38:34 PDT
(In reply to comment #32)
> All reviewed patches have been landed.  Closing bug.

The patch adds a new field in RareInheritedData, but in RenderStyle::diff() method, it does not take this new property into account when comparing two RenderStyle's.

I would say it is a missing part on the patch. Could you confirm?
Comment 40 Johnny(Jianning) Ding 2011-10-16 23:50:09 PDT
(In reply to comment #39)
> The patch adds a new field in RareInheritedData, but in RenderStyle::diff() method, it does not take this new property into account when comparing two RenderStyle's.
> 
> I would say it is a missing part on the patch. Could you confirm?

Hi Antonio,  I am not sure we need to compare tap-highlight-color in RenderStyle::diff  so far because.
1) tap-highlight-color don't affect the layout. Currently we don't use it in any rendering actions.
2) the render engine embedder (browser) may use tap-highlight-color to draw the highlight rectangle(s) in draw surface when users tap an element, but the render engine should do nothing.
The similar data field like tap-highlight-color in RareInheritedData is cursorData. We currently don't compare it in RenderStyle::diff.

But if we really need to return a diff for tap-highlight-color change,  a diff which can trigger repainting action to let embedder be aware of the change may be right choice, like StyleDifferenceRepaint.

What do you think?