We should apply -webkit-tap-highlight-color to allow the web developers to change the tap highlight color to fit their website.
Created attachment 72217 [details] add -webkit-tap-highlight-color feature with virtual function allow different platform to choose default color
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.
Attachment 72217 [details] did not build on qt: Build output: http://queues.webkit.org/results/4866016
I am interested in this bug. @Tiancheng, will you continue working on this?
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 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.
(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
(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?)
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.
Created attachment 106856 [details] patch v1
Can someone take a look and give comments? Thanks.
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.
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.
Created attachment 107469 [details] patch V2
Created attachment 107474 [details] patch v2 upload latest patch v2 to add missing layout test files.
(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.
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 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.
Zalan as you did a similar patch for us, can you have a look at this?
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
(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.
(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.
(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.
Created attachment 109020 [details] patch v3
Comment on attachment 109020 [details] patch v3 Attachment 109020 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9889043
Comment on attachment 109020 [details] patch v3 Attachment 109020 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9888172
Created attachment 109113 [details] patch v3 to make ews happy
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 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
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 on attachment 109337 [details] patch v3 to fix reported broken tests Clearing flags on attachment: 109337 Committed r96433: <http://trac.webkit.org/changeset/96433>
All reviewed patches have been landed. Closing bug.
This test times out on Leopard: fast/events/touch/tap-highlight-color.html Is that expected?
I've noted the test as timing out: http://trac.webkit.org/changeset/96457 If that is in error, please correct the issue.
> 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.
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 on attachment 110297 [details] patch to clean test expectation Clearing flags on attachment: 110297 Committed r97027: <http://trac.webkit.org/changeset/97027>
(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?
(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?