Bug 196536 - Compute accurate regions for touch-action
Summary: Compute accurate regions for touch-action
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 194814
  Show dependency treegraph
 
Reported: 2019-04-03 06:09 PDT by Antti Koivisto
Modified: 2019-04-10 14:45 PDT (History)
6 users (show)

See Also:


Attachments
wip (20.52 KB, patch)
2019-04-03 06:10 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
wip (27.41 KB, patch)
2019-04-03 06:14 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (37.99 KB, patch)
2019-04-03 08:27 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (38.11 KB, patch)
2019-04-03 08:48 PDT, Antti Koivisto
simon.fraser: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews549 for win-future (13.24 MB, application/zip)
2019-04-03 11:05 PDT, Build Bot
no flags Details
patch (45.18 KB, patch)
2019-04-04 08:59 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2019-04-03 06:09:06 PDT
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.
Comment 1 Antti Koivisto 2019-04-03 06:09:24 PDT
<rdar://problem/49516022>
Comment 2 Antti Koivisto 2019-04-03 06:10:56 PDT
Created attachment 366594 [details]
wip
Comment 3 Antti Koivisto 2019-04-03 06:14:12 PDT
Created attachment 366595 [details]
wip
Comment 4 Antti Koivisto 2019-04-03 08:27:44 PDT
Created attachment 366605 [details]
patch
Comment 5 Antti Koivisto 2019-04-03 08:48:14 PDT
Created attachment 366611 [details]
patch
Comment 6 Antoine Quint 2019-04-03 08:54:46 PDT
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"?
Comment 7 Antti Koivisto 2019-04-03 09:09:10 PDT
> 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.
Comment 8 Build Bot 2019-04-03 11:05:39 PDT
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
Comment 9 Build Bot 2019-04-03 11:05:45 PDT
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
Comment 10 Simon Fraser (smfr) 2019-04-03 11:13:38 PDT
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?
Comment 11 Simon Fraser (smfr) 2019-04-03 11:35:47 PDT
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.
Comment 12 Antoine Quint 2019-04-03 12:22:40 PDT
(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.
Comment 13 Antti Koivisto 2019-04-04 08:45:52 PDT
> Do we want this to affect operator==? Will that trigger unwanted
> layouts/repaints?

No, it is needed for correct operation of these data structures.
Comment 14 Antti Koivisto 2019-04-04 08:59:27 PDT
Created attachment 366719 [details]
patch
Comment 15 Antti Koivisto 2019-04-04 09:01:59 PDT
> 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
Comment 16 WebKit Commit Bot 2019-04-04 10:50:00 PDT
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 commit-queue@webkit.org...
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
Comment 17 Ryan Haddad 2019-04-04 11:59:11 PDT
(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.
Comment 18 WebKit Commit Bot 2019-04-04 13:28:52 PDT
Comment on attachment 366719 [details]
patch

Clearing flags on attachment: 366719

Committed r243893: <https://trac.webkit.org/changeset/243893>
Comment 19 WebKit Commit Bot 2019-04-04 13:28:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Timothy Hatcher 2019-04-08 14:33:41 PDT
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.