Bug 195845 - UI-process hit-testing needs to know about containing block relationships
Summary: UI-process hit-testing needs to know about containing block relationships
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:
 
Reported: 2019-03-15 22:13 PDT by Simon Fraser (smfr)
Modified: 2019-03-21 12:54 PDT (History)
11 users (show)

See Also:


Attachments
Simple testcase (6.32 KB, text/html)
2019-03-15 22:13 PDT, Simon Fraser (smfr)
no flags Details
wip (9.84 KB, patch)
2019-03-20 07:59 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (16.70 KB, patch)
2019-03-21 02:55 PDT, Antti Koivisto
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.74 MB, application/zip)
2019-03-21 04:13 PDT, Build Bot
no flags Details
patch (16.71 KB, patch)
2019-03-21 05:23 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (18.30 KB, patch)
2019-03-21 05:50 PDT, Antti Koivisto
simon.fraser: review+
Details | Formatted Diff | Diff
patch (18.33 KB, patch)
2019-03-21 12:15 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 Simon Fraser (smfr) 2019-03-15 22:13:34 PDT
Created attachment 364917 [details]
Simple testcase

When we hit-test in the UI process, hit-testing via the UIView hierarchy isn't always correct, because that reflects painting order, not hit-testing order.

This matters when overflow:scroll contains a composited layer (say, position:relative) whose stacking context is outside the overflow. You can't scroll the overflow:scroll if you happen to hit this box.

This breaks scrolling at https://palace-games.com:

1. Load https://palace-games.com/ on iPad
2. Tap “Reserve a room today”
3. Try to scroll the list of rooms; try touching in different places. Sometimes the overflow doesn't scroll, and you rubber-band the body instead.
Comment 1 Radar WebKit Bug Importer 2019-03-15 22:23:43 PDT
<rdar://problem/48949633>
Comment 2 Simon Fraser (smfr) 2019-03-18 22:27:28 PDT
On macOS, what happens is that hit-testing finds the position:relative box via a reverse z-order tree walk, then via event propagation up the DOM, the element with overflow:scroll handles the event.
Comment 3 Antti Koivisto 2019-03-19 00:12:02 PDT
Scrolling tree needs to know scrolling relationships so we can probably just ask it.
Comment 4 Antti Koivisto 2019-03-20 07:59:16 PDT
Created attachment 365346 [details]
wip
Comment 5 Build Bot 2019-03-20 11:02:58 PDT
Attachment 365346 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:85:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:86:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Antti Koivisto 2019-03-21 02:55:59 PDT
Created attachment 365526 [details]
patch
Comment 7 Build Bot 2019-03-21 02:59:15 PDT
Attachment 365526 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:85:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:86:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Build Bot 2019-03-21 04:12:58 PDT
Comment on attachment 365526 [details]
patch

Attachment 365526 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11596959

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html
Comment 9 Build Bot 2019-03-21 04:13:00 PDT
Created attachment 365532 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 10 Antti Koivisto 2019-03-21 05:23:58 PDT
Created attachment 365536 [details]
patch
Comment 11 Build Bot 2019-03-21 05:26:36 PDT
Attachment 365536 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:85:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:86:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Antti Koivisto 2019-03-21 05:50:53 PDT
Created attachment 365540 [details]
patch
Comment 13 Build Bot 2019-03-21 05:53:09 PDT
Attachment 365540 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:85:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:86:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Simon Fraser (smfr) 2019-03-21 11:26:37 PDT
Comment on attachment 365540 [details]
patch

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

> Source/WebKit/ChangeLog:11
> +        When an overflow scroller contains a positioned element it may not be a descendant layer of the scroller,

The "it" is ambiguous.

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:83
> -    std::unique_ptr<ScrollingStateTree> stateTree(const_cast<RemoteScrollingCoordinatorTransaction&>(transaction).scrollingStateTree().release());
> +    auto stateTree = WTFMove(const_cast<RemoteScrollingCoordinatorTransaction&>(transaction).scrollingStateTree());

Haha

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingCoordinatorProxyIOS.mm:133
> +    // FIXME: This doesn't contain ScrollPositioningBehavior::Stationary nodes. They may need to be handled too.

File bug for this, reference it here.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingCoordinatorProxyIOS.mm:139
> +            auto* positionedLayerNode = RemoteLayerTreeNode::forCALayer(positionedNode->layer());

Maybe null-check positionedNode, since we've had other bugs where stale nodeIDs get left around.
Comment 15 Antti Koivisto 2019-03-21 12:15:46 PDT
Created attachment 365588 [details]
patch
Comment 16 Antti Koivisto 2019-03-21 12:18:56 PDT
> File bug for this, reference it here.

https://bugs.webkit.org/show_bug.cgi?id=196100
Comment 17 Build Bot 2019-03-21 12:19:33 PDT
Attachment 365588 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:85:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:86:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 WebKit Commit Bot 2019-03-21 12:54:31 PDT
Comment on attachment 365588 [details]
patch

Clearing flags on attachment: 365588

Committed r243316: <https://trac.webkit.org/changeset/243316>
Comment 19 WebKit Commit Bot 2019-03-21 12:54:33 PDT
All reviewed patches have been landed.  Closing bug.