Bug 48544 – support -webkit-tap-highlight-color

• Component: CSS
• Status: RESOLVED FIXED

[Tiancheng Jiang]

We should apply -webkit-tap-highlight-color to allow the web developers to change the tap highlight color to fit their website.

[Tiancheng Jiang]

Created attachment 72217 [details]
add -webkit-tap-highlight-color feature with virtual function allow different platform to choose default color

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 72217 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4866016

[Johnny(Jianning) Ding]

I am interested in this bug.

@Tiancheng, will you continue working on this?

[Tiancheng Jiang]

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?

[Simon Fraser (smfr)]

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.

[Johnny(Jianning) Ding]

(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

[Johnny(Jianning) Ding]

(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?)

[Johnny(Jianning) Ding]

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.

[Johnny(Jianning) Ding]

Created attachment 106856 [details]
patch v1

[Johnny(Jianning) Ding]

Can someone take a look and give comments? Thanks.

[Sam Weinig]

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.

[Johnny(Jianning) Ding]

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.

[Johnny(Jianning) Ding]

Created attachment 107469 [details]
patch V2

[Johnny(Jianning) Ding]

Created attachment 107474 [details]
patch v2

upload latest patch v2 to add missing layout test files.

[Antonio Gomes]

(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.

[Johnny(Jianning) Ding]

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.

[Antonio Gomes]

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.

[Kenneth Rohde Christiansen]

Zalan as you did a similar patch for us, can you have a look at this?

[Kenneth Rohde Christiansen]

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 [e-mail]'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

[Johnny(Jianning) Ding]

(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.

[Johnny(Jianning) Ding]

(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.

[Antonio Gomes]

(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.

[Johnny(Jianning) Ding]

Created attachment 109020 [details]
patch v3

[Early Warning System Bot]

Comment on attachment 109020 [details]
patch v3

Attachment 109020 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9889043

[WebKit Review Bot]

Comment on attachment 109020 [details]
patch v3

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

[Johnny(Jianning) Ding]

Created attachment 109113 [details]
patch v3 to make ews happy

[WebKit Review Bot]

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

[Kenneth Rohde Christiansen]

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

[Johnny(Jianning) Ding]

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!

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

This test times out on Leopard:

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

Is that expected?

[Adam Barth]

I've noted the test as timing out:
http://trac.webkit.org/changeset/96457

If that is in error, please correct the issue.

[Johnny(Jianning) Ding]

> 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.

[Johnny(Jianning) Ding]

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

[WebKit Review Bot]

Comment on attachment 110297 [details]
patch to clean test expectation

Clearing flags on attachment: 110297

Committed r97027: <http://trac.webkit.org/changeset/97027>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

(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?

[Johnny(Jianning) Ding]

(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?