Summary: | Implement touch-action: manipulation; for iOS | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||||
Component: | CSS | Assignee: | 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
Wenson Hsieh
2015-10-06 13:26:20 PDT
Created attachment 262604 [details]
Work in progress on touch-action: manipulation;
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. Created attachment 262612 [details]
Basic touch-action: manipulation test using buttons
Refer to test page for details.
Created attachment 262614 [details]
Work in progress v2 on touch-action: manipulation;
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. Created attachment 263203 [details]
Patch
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 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 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. > 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). Created attachment 263365 [details]
Patch
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.
Created attachment 263486 [details]
Patch
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.
Created attachment 263502 [details]
Patch
Created attachment 263506 [details]
Patch
Created attachment 263514 [details]
Patch
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 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. Committed r191452: <http://trac.webkit.org/changeset/191452> Relevant blog post: https://webkit.org/blog/5610/more-responsive-tapping-on-ios/ Only in iOS? Why not support other platforms? 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! 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. 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! |