Bug 196536 – Compute accurate regions for touch-action

• Component: Scrolling
• Status: RESOLVED FIXED

[Antti Koivisto]

We currently use a naive approach when computing regions for touch-action since we use the bounds of the element regardless of whether content might overlap or paint outside of its bounds.

[Antti Koivisto]

<rdar://problem/49516022>

[Antti Koivisto]

Created attachment 366594 [details]
wip

[Antti Koivisto]

Created attachment 366595 [details]
wip

[Antti Koivisto]

Created attachment 366605 [details]
patch

[Antti Koivisto]

Created attachment 366611 [details]
patch

[Antoine Quint]

Comment on attachment 366611 [details]
patch

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

> Source/WebCore/ChangeLog:15
> +        This patch doesn't yes use the computed region for anything except test output.

Typo: yet.

> Source/WebCore/css/StyleResolver.cpp:853
> +    if (effectiveTouchActions.containsAll(elementTouchActions))
> +        return elementTouchActions;

I don't understand the intent of this code. What is an "effective touch-action"?

[Antti Koivisto]

> I don't understand the intent of this code. What is an "effective
> touch-action"?

"Effective touch-action" is what Element::computedTouchActions() used to compute, except now resolved by the style system.

[EWS Watchlist]

Comment on attachment 366611 [details]
patch

Attachment 366611 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11753416

New failing tests:
fast/loader/stateobjects/popstate-after-load-complete-addeventlistener.html
fast/loader/stateobjects/no-popstate-when-back-to-stateless-entry.html
fast/loader/stateobjects/popstate-after-load-complete-body-attribute.html
fast/loader/stateobjects/popstate-after-load-complete-body-inline-attribute.html
fast/loader/stateobjects/popstate-after-load-complete-window-attribute.html

[EWS Watchlist]

Created attachment 366624 [details]
Archive of layout-test-results from ews549 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews549  Port: win-future  Platform: CYGWIN_NT-10.0-2.11.1-0.329-5-3-x86_64-64bit

[Simon Fraser (smfr)]

Comment on attachment 366611 [details]
patch

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

> Source/WebCore/css/StyleResolver.cpp:849
> +    if (effectiveTouchActions.contains(TouchAction::None))
> +        return { TouchAction::None };

So "none" on an ancestor clobbers any value on descendants? That's weird.

>> Source/WebCore/css/StyleResolver.cpp:853
>> +        return elementTouchActions;
> 
> I don't understand the intent of this code. What is an "effective touch-action"?

I'm a bit confused by this code too. I think it needs a link to spec text.

> Source/WebCore/css/StyleResolver.cpp:1107
> +        style.setEffectiveTouchActions(computeEffectiveTouchActions(style.touchActions(), style.effectiveTouchActions()));

This might be clearer if the second param used parentStyle.effectiveTouchActions()

> Source/WebCore/rendering/PaintInfo.h:46
> +class TouchActionRegion;

Needs if ENABLE(POINTER_EVENTS) guard

> Source/WebCore/rendering/PaintInfo.h:135
> +    TouchActionRegion* touchActionRegion { nullptr };

Needs if ENABLE(POINTER_EVENTS) guard

> Source/WebCore/rendering/RenderLayer.cpp:4873
> +        paintInfo.touchActionRegion = localPaintingInfo.touchActionRegion;

if ENABLE(POINTER_EVENTS)

> Source/WebCore/rendering/RenderLayer.h:81
> +class TouchActionRegion;

if ENABLE(POINTER_EVENTS)

> Source/WebCore/rendering/RenderLayer.h:918
> +        TouchActionRegion* touchActionRegion { nullptr };

if ENABLE(POINTER_EVENTS)

> Source/WebCore/rendering/TouchActionRegion.cpp:73
> +constexpr const char* toString(TouchAction touchAction)

Or you could just implement TextStream& operator<<(TextStream&, TouchAction)

> Source/WebCore/rendering/style/RenderStyle.h:709
> +    OptionSet<TouchAction> effectiveTouchActions() const { return OptionSet<TouchAction>::fromRaw(m_rareInheritedData->effectiveTouchActions); }

I think this justifies a comment saying that "touch-action is not an inherited property, but we fake one with "effectiveTouchActions" so we can blah blah".

> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:373
> +        && effectiveTouchActions == o.effectiveTouchActions

Do we want this to affect operator==? Will that trigger unwanted layouts/repaints?

[Simon Fraser (smfr)]

I would like to see 2 additional tests:
1. A test for dynamic changes of touch-action (including with nesting), to test that we correctly trigger compositing updates and re-compute the regions.
2. A test for touch-action on descendants of a stacking-context composited layer with negative z-order descendants; in that case we have a more complex layer hierarchy with a "foreground" layer, and we need to ensure that we get the event regions on the right layers.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 366611 [details]
> 
> > Source/WebCore/css/StyleResolver.cpp:849
> > +    if (effectiveTouchActions.contains(TouchAction::None))
> > +        return { TouchAction::None };
> 
> So "none" on an ancestor clobbers any value on descendants? That's weird.

That is correct though.

[Antti Koivisto]

> Do we want this to affect operator==? Will that trigger unwanted
> layouts/repaints?

No, it is needed for correct operation of these data structures.

[Antti Koivisto]

Created attachment 366719 [details]
patch

[Antti Koivisto]

> 1. A test for dynamic changes of touch-action (including with nesting), to
> test that we correctly trigger compositing updates and re-compute the
> regions.

Filed https://bugs.webkit.org/show_bug.cgi?id=196608 for this

[WebKit Commit Bot]

Comment on attachment 366719 [details]
patch

Rejecting attachment 366719 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 366719, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=366719&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=196536&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 366719 from bug 196536.
Fetching: https://bugs.webkit.org/attachment.cgi?id=366719
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	LayoutTests/pointerevents/ios/touch-action-region-basic-expected.txt
	A	LayoutTests/pointerevents/ios/touch-action-region-basic.html
	A	LayoutTests/pointerevents/ios/touch-action-region-layers-expected.txt
	A	LayoutTests/pointerevents/ios/touch-action-region-layers.html
	A	LayoutTests/pointerevents/ios/touch-action-region-pan-x-y-expected.txt
	A	LayoutTests/pointerevents/ios/touch-action-region-pan-x-y.html
	A	Source/WebCore/rendering/TouchActionRegion.cpp
	A	Source/WebCore/rendering/TouchActionRegion.h
	M	LayoutTests/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/LayoutTests/ChangeLog' is out of date
W: bef3df8af37866e14ded5da7f380b344e56e2939 and refs/remotes/origin/master differ, using rebase:
:040000 040000 5e7e1b25249eee802961774c39d96156590e3d19 c05e205d0152764832577a6ca506268af3682aa2 M	LayoutTests
:040000 040000 edf1efeb0534f1659771780e0f6e189c2e629f06 b9cf54b0bba23459a89091da8dfe62e5bdb8c48c M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	LayoutTests/pointerevents/ios/touch-action-region-basic-expected.txt
	A	LayoutTests/pointerevents/ios/touch-action-region-basic.html
	A	LayoutTests/pointerevents/ios/touch-action-region-layers-expected.txt
	A	LayoutTests/pointerevents/ios/touch-action-region-layers.html
	A	LayoutTests/pointerevents/ios/touch-action-region-pan-x-y-expected.txt
	A	LayoutTests/pointerevents/ios/touch-action-region-pan-x-y.html
	A	Source/WebCore/rendering/TouchActionRegion.cpp
	A	Source/WebCore/rendering/TouchActionRegion.h
	M	LayoutTests/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/LayoutTests/ChangeLog' is out of date
W: bef3df8af37866e14ded5da7f380b344e56e2939 and refs/remotes/origin/master differ, using rebase:
:040000 040000 5e7e1b25249eee802961774c39d96156590e3d19 c05e205d0152764832577a6ca506268af3682aa2 M	LayoutTests
:040000 040000 edf1efeb0534f1659771780e0f6e189c2e629f06 b9cf54b0bba23459a89091da8dfe62e5bdb8c48c M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   aedf7fdcc88..580b123afc0  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 243879 = aedf7fdcc8890304ec4e2f3fbd2b2444c6c6deb6
r243880 = 580b123afc0ce83c990bee461f3eb6af2d639d68
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: https://webkit-queues.webkit.org/results/11765928

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #16)
> ERROR from SVN:
> Item is out of date: File '/trunk/LayoutTests/ChangeLog' is out of date
> W: bef3df8af37866e14ded5da7f380b344e56e2939 and refs/remotes/origin/master

I think this happened because https://trac.webkit.org/changeset/243881/webkit landed and modified LayoutTests/ChangeLog while this CQ action was in flight. It should work if retried.

[WebKit Commit Bot]

Comment on attachment 366719 [details]
patch

Clearing flags on attachment: 366719

Committed r243893: <https://trac.webkit.org/changeset/243893>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

Comment on attachment 366719 [details]
patch

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

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

Removing the usage of parentCrossingFrameBoundaries() broke the iOSMac build. See https://trac.webkit.org/changeset/244046/webkit.