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-

Description Wenson Hsieh 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.
Comment 1 Wenson Hsieh 2015-10-07 07:30:08 PDT
Created attachment 262604 [details]
Work in progress on touch-action: manipulation;
Comment 2 Wenson Hsieh 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.
Comment 3 Wenson Hsieh 2015-10-07 09:10:32 PDT
Created attachment 262612 [details]
Basic touch-action: manipulation test using buttons

Refer to test page for details.
Comment 4 Wenson Hsieh 2015-10-07 09:26:15 PDT
Created attachment 262614 [details]
Work in progress v2 on touch-action: manipulation;
Comment 5 Wenson Hsieh 2015-10-07 13:22:32 PDT
<rdar://problem/23017145>
Comment 6 Rick Byers 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.
Comment 7 Wenson Hsieh 2015-10-15 15:27:53 PDT
Created attachment 263203 [details]
Patch
Comment 8 WebKit Commit Bot 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.
Comment 9 Simon Fraser (smfr) 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?
Comment 10 Wenson Hsieh 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.
Comment 11 Rick Byers 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).
Comment 12 Wenson Hsieh 2015-10-16 23:38:19 PDT
Created attachment 263365 [details]
Patch
Comment 13 WebKit Commit Bot 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.
Comment 14 Wenson Hsieh 2015-10-19 09:35:25 PDT
Created attachment 263486 [details]
Patch
Comment 15 WebKit Commit Bot 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.
Comment 16 Wenson Hsieh 2015-10-19 13:09:25 PDT
Created attachment 263502 [details]
Patch
Comment 17 Wenson Hsieh 2015-10-19 13:35:13 PDT
Created attachment 263506 [details]
Patch
Comment 18 Wenson Hsieh 2015-10-19 14:58:36 PDT
Created attachment 263514 [details]
Patch
Comment 19 Benjamin Poulain 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;

???
Comment 20 Wenson Hsieh 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.
Comment 21 Wenson Hsieh 2015-10-22 06:55:36 PDT
Committed r191452: <http://trac.webkit.org/changeset/191452>
Comment 22 Chris Rebert 2015-12-15 14:03:51 PST
Relevant blog post: https://webkit.org/blog/5610/more-responsive-tapping-on-ios/
Comment 23 yisibl 2015-12-16 02:42:14 PST
Only in iOS? Why not support other platforms?
Comment 24 Rick Byers 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!
Comment 25 Wenson Hsieh 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.
Comment 26 Rick Byers 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!