Bug 197836 - Event region computation should respect transforms
Summary: Event region computation should respect transforms
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-05-13 05:54 PDT by Antti Koivisto
Modified: 2019-05-14 13:14 PDT (History)
4 users (show)

See Also:


Attachments
patch (15.67 KB, patch)
2019-05-14 05:18 PDT, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff
patch (15.88 KB, patch)
2019-05-14 10:45 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-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>