Bug 197836

Summary: Event region computation should respect transforms
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: ScrollingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=197818
https://bugs.webkit.org/show_bug.cgi?id=197892
Attachments:
Description Flags
patch
darin: review+
patch none

Description Antti Koivisto 2019-05-13 05:54:42 PDT
If transformed layers share a compositing layer we miscompute the event region.
Comment 1 Radar WebKit Bug Importer 2019-05-14 05:05:54 PDT
<rdar://problem/50762971>
Comment 2 Antti Koivisto 2019-05-14 05:18:39 PDT
Created attachment 369836 [details]
patch
Comment 3 Darin Adler 2019-05-14 09:03:47 PDT
Comment on attachment 369836 [details]
patch

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

> Source/WebCore/ChangeLog:12
> +        Add support for transforming regions. Non-rectlinear results use enclosing rects.

Is it acceptable to just use enclosing rects?

> Source/WebCore/rendering/EventRegion.h:41
> +    EventRegionContext(EventRegion&);

We want "explicit" here since we wouldn’t want this to be done as a conversion.

> Source/WebCore/rendering/EventRegion.h:58
> +    EventRegionContext makeContext() { return { *this }; }

Not sure why we have this since EventRegionContext also has a public constructor. And it’s only used in one place. I think we could do without it. Not sure, it does seem a little more elegant than invoking a constructor.

> Source/WebCore/rendering/RenderLayer.cpp:4603
> +    auto oldTransfrom = context.getCTM();

Spelling error here: "transfrom".
Comment 4 Antti Koivisto 2019-05-14 10:27:40 PDT
> Is it acceptable to just use enclosing rects?

Event region is used to synchronously determine on UI process side whether swipes should scroll or not. In this use approximation is often unnoticeable and definitely better than getting it completely wrong.

If needed this can be improved later, either by approximating with more rects or by using different data structures (bitmap or multiple Regions with attached transforms).

> Not sure why we have this since EventRegionContext also has a public
> constructor. And it’s only used in one place. I think we could do without
> it. Not sure, it does seem a little more elegant than invoking a constructor.

I like construction function like this because then the client site does not need to spell out uninteresting types:

auto eventRegionContext = eventRegion.makeContext();
Comment 5 Antti Koivisto 2019-05-14 10:45:49 PDT
Created attachment 369867 [details]
patch
Comment 6 WebKit Commit Bot 2019-05-14 11:28:18 PDT
Comment on attachment 369867 [details]
patch

Clearing flags on attachment: 369867

Committed r245293: <https://trac.webkit.org/changeset/245293>