Bug 149854 – Implement touch-action: manipulation; for iOS

• Component: CSS
• Status: RESOLVED FIXED

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 262604 [details]
Work in progress on touch-action: manipulation;

[Wenson Hsieh]

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]

Created attachment 262612 [details]
Basic touch-action: manipulation test using buttons

Refer to test page for details.

[Wenson Hsieh]

Created attachment 262614 [details]
Work in progress v2 on touch-action: manipulation;

[Wenson Hsieh]

<rdar://problem/23017145>

[Rick Byers]

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]

Created attachment 263203 [details]
Patch

[WebKit Commit Bot]

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

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

Created attachment 263365 [details]
Patch

[WebKit Commit Bot]

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]

Created attachment 263486 [details]
Patch

[WebKit Commit Bot]

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]

Created attachment 263502 [details]
Patch

[Wenson Hsieh]

Created attachment 263506 [details]
Patch

[Wenson Hsieh]

Created attachment 263514 [details]
Patch

[Benjamin Poulain]

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
> [phone]  Wenson Hsieh  <[e-mail]>

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 on attachment 263514 [details]
Patch

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

>> Source/WebCore/ChangeLog:1
>> [phone]  Wenson Hsieh  <[e-mail]>
> 
> 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]

Committed r191452: <http://trac.webkit.org/changeset/191452>

[Chris Rebert]

Relevant blog post: https://webkit.org/blog/5610/more-responsive-tapping-on-ios/

[yisibl]

Only in iOS? Why not support other platforms?

[Rick Byers]

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]

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]

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!