Bug 195360 - [iOS] Basic hit testing for content overlapping fast-scrollable overflow
Summary: [iOS] Basic hit testing for content overlapping fast-scrollable overflow
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-06 07:07 PST by Antti Koivisto
Modified: 2019-03-06 13:06 PST (History)
7 users (show)

See Also:


Attachments
patch (14.48 KB, patch)
2019-03-06 07:19 PST, Antti Koivisto
simon.fraser: review-
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.53 MB, application/zip)
2019-03-06 09:29 PST, EWS Watchlist
no flags Details
patch (14.52 KB, patch)
2019-03-06 10:01 PST, Antti Koivisto
simon.fraser: review+
Details | Formatted Diff | Diff
patch (14.53 KB, patch)
2019-03-06 11:19 PST, 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-03-06 07:07:01 PST
Better UI side hit testing.
Comment 1 Antti Koivisto 2019-03-06 07:19:09 PST
Created attachment 363739 [details]
patch
Comment 2 Sam Weinig 2019-03-06 09:29:01 PST
Comment on attachment 363739 [details]
patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:77
> +    for (auto i = viewsAtPoint.size(); i--;) {

I know this doesn't really have to do with this bug, this makes me sad I still haven't added the reversed adaptor :(.  I think all it would end up being is something like:

template<typename Container>
IteratorRange<typename Container::reverse_iterator> makeReversedIteratorRange(Container& container)
{
    return makeIteratorRange(std::rbegin(container), std::rend(container));
}

template<typename Container>
IteratorRange<typename Container::const_reverse_iterator> makeReversedIteratorRange(const Container& container)
{
    return makeIteratorRange(std::crbegin(container), std::crend(container));
}


and then this loop would become:

for (auto *view : makeReversedIteratorRange(viewsAtPoint)) {

Do you think that's worth adding?
Comment 3 EWS Watchlist 2019-03-06 09:29:05 PST
Comment on attachment 363739 [details]
patch

Attachment 363739 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11397079

New failing tests:
fast/scrolling/ios/overflow-scroll-overlap.html
Comment 4 EWS Watchlist 2019-03-06 09:29:06 PST
Created attachment 363748 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 5 Antti Koivisto 2019-03-06 09:29:44 PST
> for (auto *view : makeReversedIteratorRange(viewsAtPoint)) {
> 
> Do you think that's worth adding?

Yes!
Comment 6 Simon Fraser (smfr) 2019-03-06 09:57:13 PST
Comment on attachment 363739 [details]
patch

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

I think the first version of this patch needs at least to have a a bit that comes from the web process that says "does this layer contain some content that should participate in hit testing". It should lay the groundwork for the region-based approach (I don't think we can ship without that).

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:52
> +            if (view.layer.contents)
> +                return false;

We can make layer with no contents for empty divs (see isSimpleContainerCompositingLayer()), yet they should still intercept hit testing. In addition, there may be other views in the hierarchy which are there for structural purposes and should not participate in hit testing (e.g. a GraphicsLayer's m_structuralLayer).

I think you need to actually send data from the web process about which layers are hit-testable (and, in future, that data would include a region).

Also, a GraphicsLayer can paint one or more renderers which don't cover its entire area. Trivially, you can do this with box-shadow: 0 0 20px black to add layer padding that should not hit test. It can also happen with nested positioned elements.

Also, elements with pointer-events: none need to be ignored.
Comment 7 Antti Koivisto 2019-03-06 10:01:16 PST
Created attachment 363752 [details]
patch
Comment 8 Antti Koivisto 2019-03-06 10:02:51 PST
> We can make layer with no contents for empty divs (see
> isSimpleContainerCompositingLayer()), yet they should still intercept hit
> testing. In addition, there may be other views in the hierarchy which are
> there for structural purposes and should not participate in hit testing
> (e.g. a GraphicsLayer's m_structuralLayer).
> 
> I think you need to actually send data from the web process about which
> layers are hit-testable (and, in future, that data would include a region).
> 
> Also, a GraphicsLayer can paint one or more renderers which don't cover its
> entire area. Trivially, you can do this with box-shadow: 0 0 20px black to
> add layer padding that should not hit test. It can also happen with nested
> positioned elements.
> 
> Also, elements with pointer-events: none need to be ignored.

That'S why there is a FIXME for more coverage.
Comment 9 Simon Fraser (smfr) 2019-03-06 10:42:13 PST
Comment on attachment 363752 [details]
patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:45
> +        // FIXME: This probably doesn't cover all possible cases.

We know this doesn't cover all cases.
Comment 10 Antti Koivisto 2019-03-06 11:19:01 PST
Created attachment 363762 [details]
patch
Comment 11 WebKit Commit Bot 2019-03-06 13:05:12 PST
Comment on attachment 363762 [details]
patch

Clearing flags on attachment: 363762

Committed r242564: <https://trac.webkit.org/changeset/242564>
Comment 12 WebKit Commit Bot 2019-03-06 13:05:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-03-06 13:06:20 PST
<rdar://problem/48649132>