Bug 149854

Summary: Implement touch-action: manipulation; for iOS
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: CSSAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Enhancement CC: 18runt88, 50167214, benjamin, bruno, commit-queue, daes234, eoconnor, m.goleb+bugzilla, nekr.fabula, rbyers, redux, simon.fraser, thorton, webkit-bug-importer, webkit
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: iPhone / iPad   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 122212, 133112    
Attachments:
Description Flags
Work in progress on touch-action: manipulation;
none
Basic touch-action: manipulation test using buttons
none
Work in progress v2 on touch-action: manipulation;
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch benjamin: review+, benjamin: commit-queue-

Wenson Hsieh
Reported 2015-10-06 13:26:20 PDT
From https://developer.mozilla.org/en-US/docs/Web/CSS/touch-action: touch-action: manipulation; The user agent may consider touches that begin on the element only for the purposes of scrolling and continuous zooming. Any additional behaviors supported by auto are out of scope for this specification.
Attachments
Work in progress on touch-action: manipulation; (21.14 KB, patch)
2015-10-07 07:30 PDT, Wenson Hsieh
no flags
Basic touch-action: manipulation test using buttons (1.97 KB, text/html)
2015-10-07 09:10 PDT, Wenson Hsieh
no flags
Work in progress v2 on touch-action: manipulation; (22.73 KB, patch)
2015-10-07 09:26 PDT, Wenson Hsieh
no flags
Patch (38.43 KB, patch)
2015-10-15 15:27 PDT, Wenson Hsieh
no flags
Patch (38.72 KB, patch)
2015-10-16 23:38 PDT, Wenson Hsieh
no flags
Patch (38.82 KB, patch)
2015-10-19 09:35 PDT, Wenson Hsieh
no flags
Patch (29.10 KB, patch)
2015-10-19 13:09 PDT, Wenson Hsieh
no flags
Patch (27.59 KB, patch)
2015-10-19 13:35 PDT, Wenson Hsieh
no flags
Patch (26.08 KB, patch)
2015-10-19 14:58 PDT, Wenson Hsieh
benjamin: review+
benjamin: commit-queue-
Wenson Hsieh
Comment 1 2015-10-07 07:30:08 PDT
Created attachment 262604 [details] Work in progress on touch-action: manipulation;
Wenson Hsieh
Comment 2 2015-10-07 07:32:32 PDT
Comment on attachment 262604 [details] Work in progress on touch-action: manipulation; View in context: https://bugs.webkit.org/attachment.cgi?id=262604&action=review > Source/WebCore/dom/Element.cpp:2365 > + return parent && parent->touchShouldPreventDoubleTapGesture(); This will have to be smarter than just walking up the DOM.
Wenson Hsieh
Comment 3 2015-10-07 09:10:32 PDT
Created attachment 262612 [details] Basic touch-action: manipulation test using buttons Refer to test page for details.
Wenson Hsieh
Comment 4 2015-10-07 09:26:15 PDT
Created attachment 262614 [details] Work in progress v2 on touch-action: manipulation;
Wenson Hsieh
Comment 5 2015-10-07 13:22:32 PDT
Rick Byers
Comment 6 2015-10-14 16:36:22 PDT
Thanks for working on this! I implemented touch-action in chromium, and also edit the spec for it. Please feel free to reach out to me (here if you like, or directly via e-mail) for any questions on the intended behavior. Note that we have one outstanding issue that's relevant here which I expect we'll fix soon: http://crbug.com/529295.
Wenson Hsieh
Comment 7 2015-10-15 15:27:53 PDT
WebKit Commit Bot
Comment 8 2015-10-15 15:30:07 PDT
Attachment 263203 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:741: The parameter name "!" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:763: The parameter name "!" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Simon Fraser (smfr)
Comment 9 2015-10-16 12:36:12 PDT
Comment on attachment 263203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263203&action=review > Source/WebCore/dom/Element.cpp:1390 > + return !parent || parent->allowsDoubleTapGesture(); Scary that this function hides a full ancestor walk. > Source/WebCore/rendering/style/RenderStyle.h:1122 > +#if PLATFORM(IOS) Seems like all these should be TOUCH_EVENT #ifdefs, not PLATFORM(IOS) #ifdefs. > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:188 > + unsigned m_touchAction : 3; // TouchAction Why 3 bits for 2 values?
Wenson Hsieh
Comment 10 2015-10-16 13:03:26 PDT
Comment on attachment 263203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263203&action=review >> Source/WebCore/dom/Element.cpp:1390 >> + return !parent || parent->allowsDoubleTapGesture(); > > Scary that this function hides a full ancestor walk. True -- perhaps we could cache the result for each element in the chain, though it wouldn't be useful for elements like links that are only tapped once before the page navigates. One other thing is that http://www.w3.org/TR/pointerevents/#the-touch-action-css-property says: "To determine the effect of a touch, find the nearest ancestor (starting from the element itself) that has a default touch behavior. Then examine the touch-action property of each element between the hit tested element and the element with the default touch behavior (including both the hit tested element and the element with the default touch behavior). If the touch-action property of any of those elements disallows the default touch behavior, do nothing. Otherwise allow the element to start considering the touch for the purposes of executing a default touch behavior." I wasn't sure exactly what an "element that has a default touch behavior" refers to (currently, I'm taking this to be the root). Once we've cleared this up, we might have to rework our implementation of this function. >> Source/WebCore/rendering/style/RenderStyle.h:1122 >> +#if PLATFORM(IOS) > > Seems like all these should be TOUCH_EVENT #ifdefs, not PLATFORM(IOS) #ifdefs. Got it -- I'll change them. >> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:188 >> + unsigned m_touchAction : 3; // TouchAction > > Why 3 bits for 2 values? My bad -- I'll fix this. I originally was going to include all 5 values here, but decided to include only auto and manipulation in this patch, since those are the only 2 we support after this patch. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1108 > +- (void)_endPotentialTap On second thought, maybe I should rename this to be something more descriptive, like _endPotentialTapAndEnableDoubleTapGesturesIfNecessary.
Rick Byers
Comment 11 2015-10-16 13:20:31 PDT
> I wasn't sure exactly what an "element that has a default touch behavior" refers to (currently, I'm taking this to be the root). Once we've cleared this up, we might have to rework our implementation of this function. If all you're supporting is 'manipulation', then yes this should simply look up to the root. This is where the bug I described in Chrome is - we currently stop as soon as we hit anything that scrolls, but that only makes sense for the scrolling-related features of touch-action, not the zooming-related ones (spec bug here https://github.com/w3c/pointerevents/issues/19). I.e. a developer should be able to write 'html { touch-action: manipulation; }' and have double-tap zoom disabled for the entire document (assuming it's not re-enabled by some other use of touch-action).
Wenson Hsieh
Comment 12 2015-10-16 23:38:19 PDT
WebKit Commit Bot
Comment 13 2015-10-16 23:40:09 PDT
Attachment 263365 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:834: The parameter name "!" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:856: The parameter name "!" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Wenson Hsieh
Comment 14 2015-10-19 09:35:25 PDT
WebKit Commit Bot
Comment 15 2015-10-19 09:39:00 PDT
Attachment 263486 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:834: The parameter name "!" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:856: The parameter name "!" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Wenson Hsieh
Comment 16 2015-10-19 13:09:25 PDT
Wenson Hsieh
Comment 17 2015-10-19 13:35:13 PDT
Wenson Hsieh
Comment 18 2015-10-19 14:58:36 PDT
Benjamin Poulain
Comment 19 2015-10-21 15:03:19 PDT
Comment on attachment 263514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263514&action=review Looks good. A bunch of minor comments. > Source/WebCore/ChangeLog:1 > +2015-10-19 Wenson Hsieh <wenson_hsieh@apple.com> Don't forget to update the copyright on the files you modify substantially. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3434 > +#if ENABLE(TOUCH_EVENTS) > + case CSSPropertyTouchAction: > + return cssValuePool.createValue(style->touchAction()); > +#endif I suggest moving this code with WebkitTapHighlightColor and TouchCallout. > Source/WebCore/css/CSSPrimitiveValueMappings.h:5256 > +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(TouchAction t) t -> touchAction Avoid abreviating names. > Source/WebCore/dom/Element.cpp:1387 > + if (renderer() && renderer()->style().touchAction() != TouchAction::Auto) This is an unusual way of getting a RenderStyle. You should instead use renderStyle() which can return nullptr. > Source/WebCore/rendering/style/RenderStyle.h:1687 > + void setTouchAction(TouchAction t) { SET_VAR(rareNonInheritedData, m_touchAction, static_cast<unsigned>(t)); } t -> touchAction. > LayoutTests/css3/touch-action/touch-action-computed-style.html:8 > + <p id="description"></p> > + <div id="console"></div> I don't think you need those. The "pre-" script adds the output elements as needed. > LayoutTests/css3/touch-action/touch-action-computed-style.html:12 > + var stylesheet; You can declare this later when initializing the variable. You can use "let" instead of "var" now :) > LayoutTests/css3/touch-action/touch-action-computed-style.html:26 > + Few additional tests needed IMHO: -various CaSe for the property value -!important -CSS cascading (verifying this is not Inherited). > LayoutTests/css3/touch-action/touch-action-computed-style.html:31 > + successfullyParsed = true; ??? What??? > LayoutTests/css3/touch-action/touch-action-manipulation-fast-clicks.html:21 > + if (window.testRunner) { > + testRunner.dumpAsText(); > + testRunner.waitUntilDone(); > + } Why not use the text-test script for this test? > LayoutTests/css3/touch-action/touch-action-parsing.html:31 > + successfullyParsed = true; ???
Wenson Hsieh
Comment 20 2015-10-21 22:27:38 PDT
Comment on attachment 263514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263514&action=review >> Source/WebCore/ChangeLog:1 >> +2015-10-19 Wenson Hsieh <wenson_hsieh@apple.com> > > Don't forget to update the copyright on the files you modify substantially. I wasn't sure exactly which changes are substantial, but I updated the year on the following files: WebPageIOS.mm and Element.cpp since I introduced some non-cookie-cutter logic to them and their years were also outdated. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3434 >> +#endif > > I suggest moving this code with WebkitTapHighlightColor and TouchCallout. Sounds good. Fixed! >> Source/WebCore/css/CSSPrimitiveValueMappings.h:5256 >> +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(TouchAction t) > > t -> touchAction > > Avoid abreviating names. Got it. Changed to touchAction. >> Source/WebCore/dom/Element.cpp:1387 >> + if (renderer() && renderer()->style().touchAction() != TouchAction::Auto) > > This is an unusual way of getting a RenderStyle. > > You should instead use renderStyle() which can return nullptr. Fixed! >> Source/WebCore/rendering/style/RenderStyle.h:1687 >> + void setTouchAction(TouchAction t) { SET_VAR(rareNonInheritedData, m_touchAction, static_cast<unsigned>(t)); } > > t -> touchAction. Changed to touchAction. >> LayoutTests/css3/touch-action/touch-action-computed-style.html:8 >> + <div id="console"></div> > > I don't think you need those. The "pre-" script adds the output elements as needed. Got it. Removed the console divs. >> LayoutTests/css3/touch-action/touch-action-computed-style.html:12 >> + var stylesheet; > > You can declare this later when initializing the variable. > You can use "let" instead of "var" now :) Wooo ES6! Changed. >> LayoutTests/css3/touch-action/touch-action-computed-style.html:26 >> + > > Few additional tests needed IMHO: > -various CaSe for the property value > -!important > -CSS cascading (verifying this is not Inherited). Got it. Updated computed style test to verify these behaviors. >> LayoutTests/css3/touch-action/touch-action-computed-style.html:31 >> + successfullyParsed = true; > > ??? > What??? Sorry about that -- removed. I think I included this when I was trying to follow the footsteps of some other computed style tests. >> LayoutTests/css3/touch-action/touch-action-manipulation-fast-clicks.html:21 >> + } > > Why not use the text-test script for this test? Fixed. >> LayoutTests/css3/touch-action/touch-action-parsing.html:31 >> + successfullyParsed = true; > > ??? Removed.
Wenson Hsieh
Comment 21 2015-10-22 06:55:36 PDT
Chris Rebert
Comment 22 2015-12-15 14:03:51 PST
yisibl
Comment 23 2015-12-16 02:42:14 PST
Only in iOS? Why not support other platforms?
Rick Byers
Comment 24 2016-01-20 17:42:53 PST
I tried out iOS 9.3 beta on http://rbyers.net/touch-action.html. I see CSS parser support for touch-action:auto and touch-action:manipulation, but it doesn't appear to affect the click delay or double-tap to zoom gesture. Are there bits of this that didn't make it into the 9.3 release, or is there a bug? The viewport-based click delay elimination looks awesome though, thanks!
Wenson Hsieh
Comment 25 2016-01-20 17:59:56 PST
Thanks for the catch -- what's happening in that example is that since the touch-action: manipulation div is not clickable, we skip that node when trying to find a clickable element for the touch. To verify this, add an onclick to the div with touch-action manipulation and touches on it should turn fast.
Rick Byers
Comment 26 2016-01-20 18:37:13 PST
Ah, I forgot about having to annotate clickable things explicitly in mobile Safari, thanks! I updated that test, and now it works great! I tried a couple other scenarios (touch-action specified on a container that isn't itself clickable, iframes, overflow:scroll divs) and they all look good to me. Thanks again for implementing this!
Note You need to log in before you can comment on or make changes to this bug.