Bug 193447 – Limit user-agent interactions based on the touch-action property on iOS

• Component: CSS
• Status: RESOLVED FIXED

[Antoine Quint]

Now that we've added parsing support for additional values for the touch-action property (see webkit.org/b/193314), we need to account for them when processing user interaction in WebKit.

[Radar WebKit Bug Importer]

<rdar://problem/47283874>

[Antoine Quint]

Created attachment 359189 [details]
Patch

[Antoine Quint]

Created attachment 359208 [details]
Patch

[Antoine Quint]

Created attachment 359257 [details]
Patch

[Antoine Quint]

Created attachment 359258 [details]
Patch

[Antoine Quint]

Created attachment 359260 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 359260 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:4151
> +    if (auto* page = this->page()) {
> +        if (auto* frameView = view()) {
> +            if (auto* scrollingCoordinator = page->scrollingCoordinator())
> +                scrollingCoordinator->frameViewEventTrackingRegionsChanged(*frameView);
> +        }
> +    }

Should this really be inside ENABLE(TOUCH_EVENTS)?

> Source/WebCore/dom/Document.cpp:8676
> +#if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS_FAMILY)

These are normally the other way around. Also this entire function is inside #if ENABLE(TOUCH_EVENTS).

> Source/WebCore/dom/Element.cpp:4125
> +#if ENABLE(TOUCH_EVENTS)

Why aren't your changes inside ENABLE(POINTER_EVENTS)?

> Source/WebCore/dom/Element.cpp:4126
> +OptionSet<TouchAction> Element::computedTouchActions() const

This function has a tree walk up to the root element. That seems really expensive.

> Source/WebCore/dom/Element.cpp:4129
> +    for (auto* element = this; element; element = parentCrossingFrameBoundaries(element)) {

parentCrossingFrameBoundaries seems wrong here.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2587
> +        if ([_contentView preventsPanningInXAxis])
> +            targetContentOffset->x = scrollView.contentOffset.x;
> +        if ([_contentView preventsPanningInYAxis])
> +            targetContentOffset->y = scrollView.contentOffset.y;

What happens if you don't have this code?

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:215
> +

Blank line.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:76
> +            bool canPanX = true;
> +            bool canPanY = true;

These are unused.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:147
> +    bool canPanX = true;
> +    bool canPanY = true;

These are unused.

> Source/WebKit/UIProcess/WebPageProxy.h:685
> +bool isScrollingOrZooming() const { return m_isScrollingOrZooming; }

Indentation.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1160
> +                [_singleTapGestureRecognizer setEnabled:NO];
> +                [_nonBlockingDoubleTapGestureRecognizer setEnabled:NO];
> +                [_twoFingerDoubleTapGestureRecognizer setEnabled:NO];
> +                [_highlightLongPressGestureRecognizer setEnabled:NO];
> +                [_twoFingerSingleTapGestureRecognizer setEnabled:NO];
> +                [_stylusSingleTapGestureRecognizer setEnabled:NO];
> +                [_doubleTapGestureRecognizer setEnabled:NO];
> +                [_longPressGestureRecognizer setEnabled:NO];

Where do they get re-enabled?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1190
> +- (void)_resetPanningPreventionFlags

You need to call this from -cleanupInteraction too to clean up state when a web process crashes or is swapped.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:838
> +Vector<std::pair<OptionSet<WebCore::TouchAction>, WebCore::ScrollingNodeID>> WebPageProxy::touchActionsForTouchEvent(const WebTouchEvent& event)

Vector<std::pair<OptionSet<WebCore::TouchAction>, WebCore::ScrollingNodeID>> is gross. At least typedef it.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:841
> +    process().sendSync(Messages::WebPage::TouchActionsForTouchEvent(event), Messages::WebPage::TouchActionsForTouchEvent::Reply(touchActions), m_pageID);

This is going to be a massive new source of UI process hangs.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 359260 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359260&action=review
> 
> > Source/WebCore/dom/Element.cpp:4125
> > +#if ENABLE(TOUCH_EVENTS)
> 
> Why aren't your changes inside ENABLE(POINTER_EVENTS)?

Do you think we should add it? I don't think it currently exists and the touch-action property is enabled if ENABLE_TOUCH_EVENTS in CSSProperties.json.
 
> > Source/WebCore/dom/Element.cpp:4126
> > +OptionSet<TouchAction> Element::computedTouchActions() const
> 
> This function has a tree walk up to the root element. That seems really
> expensive.

What do you suggest as an alternative?

> > Source/WebCore/dom/Element.cpp:4129
> > +    for (auto* element = this; element; element = parentCrossingFrameBoundaries(element)) {
> 
> parentCrossingFrameBoundaries seems wrong here.

I think it is correct, setting a touch-action property on an element in a parent frame that is the ancestor of the <iframe> element affects the permitted touch actions for the element within the frame.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2587
> > +        if ([_contentView preventsPanningInXAxis])
> > +            targetContentOffset->x = scrollView.contentOffset.x;
> > +        if ([_contentView preventsPanningInYAxis])
> > +            targetContentOffset->y = scrollView.contentOffset.y;
> 
> What happens if you don't have this code?

There would be a deceleration animation even though we overrode the panning during the interaction.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1160
> > +                [_singleTapGestureRecognizer setEnabled:NO];
> > +                [_nonBlockingDoubleTapGestureRecognizer setEnabled:NO];
> > +                [_twoFingerDoubleTapGestureRecognizer setEnabled:NO];
> > +                [_highlightLongPressGestureRecognizer setEnabled:NO];
> > +                [_twoFingerSingleTapGestureRecognizer setEnabled:NO];
> > +                [_stylusSingleTapGestureRecognizer setEnabled:NO];
> > +                [_doubleTapGestureRecognizer setEnabled:NO];
> > +                [_longPressGestureRecognizer setEnabled:NO];
> 
> Where do they get re-enabled?

They're not. I think this code should be removed actually. We should deal with those other gestures individually in separate patches.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1190
> > +- (void)_resetPanningPreventionFlags
> 
> You need to call this from -cleanupInteraction too to clean up state when a
> web process crashes or is swapped.

Thanks, will add that.

> > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:838
> > +Vector<std::pair<OptionSet<WebCore::TouchAction>, WebCore::ScrollingNodeID>> WebPageProxy::touchActionsForTouchEvent(const WebTouchEvent& event)
> 
> Vector<std::pair<OptionSet<WebCore::TouchAction>, WebCore::ScrollingNodeID>>
> is gross. At least typedef it.

Will do.

> > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:841
> > +    process().sendSync(Messages::WebPage::TouchActionsForTouchEvent(event), Messages::WebPage::TouchActionsForTouchEvent::Reply(touchActions), m_pageID);
> 
> This is going to be a massive new source of UI process hangs.

We have discussed this in person extensively and could not come up with an asynchronous mechanism that would allow interactions to be overridden in time when a fast interaction occurs. If you have another idea, I'm interested.

[Antoine Quint]

Created attachment 359363 [details]
Patch

[Antoine Quint]

Created attachment 359365 [details]
Patch

[Antoine Quint]

About WebPageProxy::touchActionsForTouchEvent() being synchronous, it's important to note that:

1. the sync message will only be dispatched for new touches if we haven't excluded that the touch's location is contained within the bounds of a sub-tree element that may have a touch-action.

2. there is no events dispatched or JS code ran as a result, this is all WebCore code and no user code can run. The sync message is sent prior to dispatching new touch events for the new gesture.

[Antti Koivisto]

Seems to me that there are basically two ways to avoid the sync ipc.

- Push sufficient amount of data about touch action regions to UI process so we can hit test them fully in the UI process side. This could be done for example as part of layer tree transactions.

- Gather similar data in the web process side. Sync IPC to a dedicated thread (somewhat like how scrolling thread work) to access this data and do the hit testing.

[Antoine Quint]

Created attachment 360028 [details]
Patch

[Antoine Quint]

Created attachment 360029 [details]
Patch

[Antoine Quint]

Created attachment 360031 [details]
Patch

[Antoine Quint]

Created attachment 360089 [details]
Patch

[Antoine Quint]

Created attachment 360090 [details]
Patch

[Antoine Quint]

Created attachment 360100 [details]
Patch

[Antti Koivisto]

Comment on attachment 360100 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:7113
> +Document::RegionFixedPair Document::absoluteRegionForNode(Node* node)

This should take a reference as there is no reason to call this with null node

Maybe call this absoluteEventRegionForNode or something similarly informative?

> Source/WebCore/page/scrolling/ScrollingTree.cpp:422
> +void ScrollingTree::setTouchActionsForScrollingNodeID(OptionSet<TouchAction> touchActions, ScrollingNodeID scrollingNodeID)
> +{
> +    if (auto* scrollingTreeNode = nodeForID(scrollingNodeID))
> +        scrollingTreeNode->setTouchActions(touchActions);
> +}

Maybe setActiveTouchActions/setActiveTouchActionsForScrollingNodeID or similar to indicate this is a temporary state?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1156
> +                        scrollingCoordinator->setTouchActionsForScrollingNodeID(touchActionData->touchActions, touchActionData->scrollingNodeID);

Do we ever clear the touch actions we set here? Should we?

[Antti Koivisto]

Comment on attachment 360100 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:8680
> +void Document::updateTouchActionElements(Node& node, RenderStyle* style)

This should take Element&, const RenderStyle&

> Source/WebCore/dom/Document.cpp:8683
> +    if (!m_touchActionElements)
> +        m_touchActionElements = std::make_unique<HashSet<Node*>>();

This could be done when we see the first touch-action element. That would also avoid unnecessary hash lookups on remove for all elements.

> Source/WebCore/dom/Document.h:1280
> +    const HashSet<Node*>* touchActionElements() const { return m_touchActionElements.get(); }

This is called touchActionElements() and also seems only ever contains elements. It should be HashSet<Element*> or maybe HashSet<const Element*>.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:116
> +        for (auto& node : *frame.document()->touchActionElements()) {

Is touchActionElements() always guaranteed to be non-null?

[Antti Koivisto]

Simon should check the ScrollingTree bits

[Antoine Quint]

(In reply to Antti Koivisto from comment #19)
> Comment on attachment 360100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360100&action=review
> 
> > Source/WebCore/dom/Document.cpp:7113
> > +Document::RegionFixedPair Document::absoluteRegionForNode(Node* node)
> 
> This should take a reference as there is no reason to call this with null
> node

Sure, will fix when landing.

> Maybe call this absoluteEventRegionForNode or something similarly
> informative?

Sure, will fix when landing.

> > Source/WebCore/page/scrolling/ScrollingTree.cpp:422
> > +void ScrollingTree::setTouchActionsForScrollingNodeID(OptionSet<TouchAction> touchActions, ScrollingNodeID scrollingNodeID)
> > +{
> > +    if (auto* scrollingTreeNode = nodeForID(scrollingNodeID))
> > +        scrollingTreeNode->setTouchActions(touchActions);
> > +}
> 
> Maybe setActiveTouchActions/setActiveTouchActionsForScrollingNodeID or
> similar to indicate this is a temporary state?

Sure, will fix when landing.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1156
> > +                        scrollingCoordinator->setTouchActionsForScrollingNodeID(touchActionData->touchActions, touchActionData->scrollingNodeID);
> 
> Do we ever clear the touch actions we set here? Should we?

We don't. However, I'm not sure where to do that. Simon might know.

[Antoine Quint]

(In reply to Antti Koivisto from comment #20)
> Comment on attachment 360100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360100&action=review
> 
> > Source/WebCore/dom/Document.cpp:8680
> > +void Document::updateTouchActionElements(Node& node, RenderStyle* style)
> 
> This should take Element&, const RenderStyle&

Will fix when landing.

> > Source/WebCore/dom/Document.cpp:8683
> > +    if (!m_touchActionElements)
> > +        m_touchActionElements = std::make_unique<HashSet<Node*>>();
> 
> This could be done when we see the first touch-action element. That would
> also avoid unnecessary hash lookups on remove for all elements.

Good idea! Will fix when landing.

> > Source/WebCore/dom/Document.h:1280
> > +    const HashSet<Node*>* touchActionElements() const { return m_touchActionElements.get(); }
> 
> This is called touchActionElements() and also seems only ever contains
> elements. It should be HashSet<Element*> or maybe HashSet<const Element*>.

Changing to HashSet<Element*> when landing. However, making it <const Element*> is tricky because the chain of calls made on these elements goes deep and is non-const.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:116
> > +        for (auto& node : *frame.document()->touchActionElements()) {
> 
> Is touchActionElements() always guaranteed to be non-null?

No, I will add a guard when landing.

[Simon Fraser (smfr)]

Comment on attachment 360100 [details]
Patch

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

It seems like this patch assume that all iOS scrolling is going to happen in the scrolling tree. That might not be true if I fail at my job, or if I have to fall off the fast path because of things like background-attachment:fixed inside overflow on macOS.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:2408
> +		83520C7E1A71BFCC006BD2AA /* CSSFontFamily.h in Headers */ = {isa = PBXBuildFile; fileRef = 83520C7D1A71BFCC006BD2AA /* CSSFontFamily.h */; settings = {ATTRIBUTES = (Private, ); }; };

Does this really need to become a private header?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:2894
> +		9AB1F38018E2489A00534743 /* CSSToLengthConversionData.h in Headers */ = {isa = PBXBuildFile; fileRef = 9AB1F37E18E2489A00534743 /* CSSToLengthConversionData.h */; settings = {ATTRIBUTES = (Private, ); }; };

Ditto

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:4772
> +		E1ED8AC30CC49BE000BFC557 /* CSSPrimitiveValueMappings.h in Headers */ = {isa = PBXBuildFile; fileRef = E1ED8AC20CC49BE000BFC557 /* CSSPrimitiveValueMappings.h */; settings = {ATTRIBUTES = (Private, ); }; };

Ditto.

> Source/WebCore/dom/Document.cpp:4155
> +#if ENABLE(POINTER_EVENTS)

Blank line above please.

> Source/WebCore/dom/Document.cpp:7124
> +        else if (Element* element = document->ownerElement())
> +            rootRelativeBounds = element->absoluteEventHandlerBounds(insideFixedPosition);

The bounds of the ownerElement aren't necessarily the same as those of its contained document, because of borders and padding on the <iframe>.

Also this code is confusing; can absoluteRegionForNode() be called with a node from a different document?

I see that you copied this code, so I should ask those questions of my self :|

> Source/WebCore/dom/Document.cpp:7126
> +        Element* element = downcast<Element>(node);

Slightly nicer as auto& element = downcast<Element>(*node);

> Source/WebCore/dom/Document.cpp:8690
> +        changed |= m_touchActionElements->add(&node).isNewEntry;
> +    else
> +        changed |= m_touchActionElements->remove(&node);

I was worried that |= can short-circuit but Tim looked it up and it doesn't. Be careful though!

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:115
> +        auto& touchActionData = eventTrackingRegions.touchActionData;

Please make a system trace point for this code, since it's expensive. It would look something like TraceScope tracingScope(TouchActionRegionComputationStart, TouchActionRegionComputationEnd) with relevant changes to Tools/Tracing/SystemTracePoints.plist (you can do this in a followup).

> Source/WebCore/page/scrolling/ScrollingTree.h:109
> +    WEBCORE_EXPORT Optional<TouchActionData> touchActionDataAtPoint(const IntPoint) const;

Comment should say what coordinate system the point is in. No need for "const IntPoint".

> Source/WebCore/platform/EventTrackingRegions.h:48
> +typedef uint64_t ScrollingNodeID;

I want to put ScrollingNodeID in ScrollTypes.h.

> Source/WebCore/platform/EventTrackingRegions.h:51
> +    ScrollingNodeID scrollingNodeID;

Can this be { 0 }

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2589
> +        if ([_contentView preventsPanningInXAxis])
> +            targetContentOffset->x = scrollView.contentOffset.x;
> +        if ([_contentView preventsPanningInYAxis])
> +            targetContentOffset->y = scrollView.contentOffset.y;

Doesn't this cause snapping at the end of the gesture?

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:85
> +        auto touchActions = _scrollingTreeNodeDelegate->scrollingNode().touchActions();
> +        if (touchActions != WebCore::TouchAction::Auto && touchActions != WebCore::TouchAction::Manipulation) {
> +            bool canPanX = true;
> +            bool canPanY = true;
> +            if (!touchActions.contains(WebCore::TouchAction::PanX)) {
> +                canPanX = false;
> +                targetContentOffset->x = scrollView.contentOffset.x;
> +            }
> +            if (!touchActions.contains(WebCore::TouchAction::PanY)) {
> +                canPanY = false;
> +                targetContentOffset->y = scrollView.contentOffset.y;
> +            }

This is in -scrollViewWillEndDragging. Doesn't this mean that the user can pan on both axes but then we'll snap at the end?

> Source/WebKit/UIProcess/WebPageProxy.h:689
> +#if ENABLE(POINTER_EVENTS)
> +    bool isScrollingOrZooming() const { return m_isScrollingOrZooming; }
> +#endif

Don't #ifdef stuff like this.

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1156
>>> +                        scrollingCoordinator->setTouchActionsForScrollingNodeID(touchActionData->touchActions, touchActionData->scrollingNodeID);
>> 
>> Do we ever clear the touch actions we set here? Should we?
> 
> We don't. However, I'm not sure where to do that. Simon might know.

We don't have a good place. Can we do this in a way that doesn't involve shoving state onto what is supposed to be a read-only tree?

[Antti Koivisto]

> We don't have a good place. Can we do this in a way that doesn't involve
> shoving state onto what is supposed to be a read-only tree?

Maybe keep the state in RemoteScrollingCoordinatorProxy or thereabouts, just read it from the scrolling tree code? You can reach it via scrollingTree().

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #24)
> Comment on attachment 360100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360100&action=review
> 
> It seems like this patch assume that all iOS scrolling is going to happen in
> the scrolling tree. That might not be true if I fail at my job, or if I have
> to fall off the fast path because of things like background-attachment:fixed
> inside overflow on macOS.

OK, I was not aware of that. Are there known scenarios where scrolling does not happen in the scrolling tree on iOS? If so, I think we can address those as followups bugs.

We'll be tackling macOS separately from this patch anyway. I'm renaming this bug to state that, it was the intent all along but the bug title doesn't say so.

> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:2408
> > +		83520C7E1A71BFCC006BD2AA /* CSSFontFamily.h in Headers */ = {isa = PBXBuildFile; fileRef = 83520C7D1A71BFCC006BD2AA /* CSSFontFamily.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> Does this really need to become a private header?

Yes, if we want to be able to include CSSPrimitiveValueMappings.h from WebKit code, which we do to be able to have OptionSet<TouchAction>.

> > Source/WebCore/dom/Document.cpp:4155
> > +#if ENABLE(POINTER_EVENTS)
> 
> Blank line above please.

Will add.

> > Source/WebCore/dom/Document.cpp:7124
> > +        else if (Element* element = document->ownerElement())
> > +            rootRelativeBounds = element->absoluteEventHandlerBounds(insideFixedPosition);
> 
> The bounds of the ownerElement aren't necessarily the same as those of its
> contained document, because of borders and padding on the <iframe>.
> 
> Also this code is confusing; can absoluteRegionForNode() be called with a
> node from a different document?
> 
> I see that you copied this code, so I should ask those questions of my self
> :|

That is correct :)

> > Source/WebCore/dom/Document.cpp:7126
> > +        Element* element = downcast<Element>(node);
> 
> Slightly nicer as auto& element = downcast<Element>(*node);

Will change.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:115
> > +        auto& touchActionData = eventTrackingRegions.touchActionData;
> 
> Please make a system trace point for this code, since it's expensive. It
> would look something like TraceScope
> tracingScope(TouchActionRegionComputationStart,
> TouchActionRegionComputationEnd) with relevant changes to
> Tools/Tracing/SystemTracePoints.plist (you can do this in a followup).

Will do in a followup.

> > Source/WebCore/page/scrolling/ScrollingTree.h:109
> > +    WEBCORE_EXPORT Optional<TouchActionData> touchActionDataAtPoint(const IntPoint) const;
> 
> Comment should say what coordinate system the point is in. No need for
> "const IntPoint".

Will change.

> > Source/WebCore/platform/EventTrackingRegions.h:48
> > +typedef uint64_t ScrollingNodeID;
> 
> I want to put ScrollingNodeID in ScrollTypes.h.

Please do :)

> > Source/WebCore/platform/EventTrackingRegions.h:51
> > +    ScrollingNodeID scrollingNodeID;
> 
> Can this be { 0 }

Yes, will change.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2589
> > +        if ([_contentView preventsPanningInXAxis])
> > +            targetContentOffset->x = scrollView.contentOffset.x;
> > +        if ([_contentView preventsPanningInYAxis])
> > +            targetContentOffset->y = scrollView.contentOffset.y;
> 
> Doesn't this cause snapping at the end of the gesture?

I did not see that but will double-check.

> > Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:85
> > +        auto touchActions = _scrollingTreeNodeDelegate->scrollingNode().touchActions();
> > +        if (touchActions != WebCore::TouchAction::Auto && touchActions != WebCore::TouchAction::Manipulation) {
> > +            bool canPanX = true;
> > +            bool canPanY = true;
> > +            if (!touchActions.contains(WebCore::TouchAction::PanX)) {
> > +                canPanX = false;
> > +                targetContentOffset->x = scrollView.contentOffset.x;
> > +            }
> > +            if (!touchActions.contains(WebCore::TouchAction::PanY)) {
> > +                canPanY = false;
> > +                targetContentOffset->y = scrollView.contentOffset.y;
> > +            }
> 
> This is in -scrollViewWillEndDragging. Doesn't this mean that the user can
> pan on both axes but then we'll snap at the end?

Will double-check.

> > Source/WebKit/UIProcess/WebPageProxy.h:689
> > +#if ENABLE(POINTER_EVENTS)
> > +    bool isScrollingOrZooming() const { return m_isScrollingOrZooming; }
> > +#endif
> 
> Don't #ifdef stuff like this.

OK.

> >>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1156
> >>> +                        scrollingCoordinator->setTouchActionsForScrollingNodeID(touchActionData->touchActions, touchActionData->scrollingNodeID);
> >> 
> >> Do we ever clear the touch actions we set here? Should we?
> > 
> > We don't. However, I'm not sure where to do that. Simon might know.
> 
> We don't have a good place. Can we do this in a way that doesn't involve
> shoving state onto what is supposed to be a read-only tree?

Antti suggests putting the state in RemoteScrollingCoordinatorProxy. I will try that.

I will make an updated patch to be reviewed once more before landing since there are a few substantial changes between yours and Antti's comments.

[Antoine Quint]

(In reply to Antti Koivisto from comment #25)
> > We don't have a good place. Can we do this in a way that doesn't involve
> > shoving state onto what is supposed to be a read-only tree?
> 
> Maybe keep the state in RemoteScrollingCoordinatorProxy or thereabouts, just
> read it from the scrolling tree code? You can reach it via scrollingTree().

Actually, I'm a little confused. How do I get back to the RemoteScrollingCoordinatorProxy where the state is held from ScrollingTreeScrollingNodeDelegateIOS where I need this information?

[Antti Koivisto]

> Actually, I'm a little confused. How do I get back to the
> RemoteScrollingCoordinatorProxy where the state is held from
> ScrollingTreeScrollingNodeDelegateIOS where I need this information?

downcast<RemoteScrollingTree>(scrollingTree()).scrollingCoordinatorProxy()

[Antoine Quint]

Created attachment 360338 [details]
Patch

[Antti Koivisto]

Comment on attachment 360338 [details]
Patch

r=me

[Antti Koivisto]

Comment on attachment 360338 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1190
> +    for (const auto& touchPoint : touchEvent.touchPoints()) {
> +        auto phase = touchPoint.phase();
> +        if (phase != WebKit::WebPlatformTouchPoint::TouchPressed) {
> +            if (auto touchActionData = scrollingCoordinator->touchActionDataAtPoint(touchPoint.location())) {
> +                if (touchActionData->touchActions == WebCore::TouchAction::None)
> +                    [_touchEventGestureRecognizer setDefaultPrevented:YES];
> +                else if (!touchActionData->touchActions.contains(WebCore::TouchAction::Manipulation)) {
> +                    if (auto scrollingNodeID = touchActionData->scrollingNodeID)
> +                        scrollingCoordinator->setTouchDataForTouchIdentifier(*touchActionData, touchPoint.identifier());
> +                    else {
> +                        if (!touchActionData->touchActions.contains(WebCore::TouchAction::PinchZoom))
> +                            _webView.scrollView.pinchGestureRecognizer.enabled = NO;
> +                        _preventsPanningInXAxis = !touchActionData->touchActions.contains(WebCore::TouchAction::PanX);
> +                        _preventsPanningInYAxis = !touchActionData->touchActions.contains(WebCore::TouchAction::PanY);
> +                    }
> +                }
> +            }
> +        } else if (phase == WebKit::WebPlatformTouchPoint::TouchReleased || phase == WebKit::WebPlatformTouchPoint::TouchCancelled)
> +            scrollingCoordinator->clearTouchDataForTouchIdentifier(touchPoint.identifier());

This sort of code usually reads better (and will be less tower-like) if you use more 'continue' and fewer/no else branches.

[Antoine Quint]

Committed r240579: <https://trac.webkit.org/changeset/240579>

[Antoine Quint]

Committed r240587: <https://trac.webkit.org/changeset/240587>

[Antoine Quint]

Committed r240589: <https://trac.webkit.org/changeset/240589>

[Antoine Quint]

Committed r240596: <https://trac.webkit.org/changeset/240596>