Bug 211512 - [Wheel event region] Add basic support for generating accurate wheel event listener region
Summary: [Wheel event region] Add basic support for generating accurate wheel event li...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-06 07:24 PDT by Antti Koivisto
Modified: 2020-05-07 07:41 PDT (History)
13 users (show)

See Also:


Attachments
patch (14.99 KB, patch)
2020-05-06 07:35 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (15.30 KB, patch)
2020-05-06 07:48 PDT, Antti Koivisto
simon.fraser: review+
Details | Formatted Diff | Diff
patch (17.85 KB, patch)
2020-05-06 09:15 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (17.90 KB, patch)
2020-05-06 09:46 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (18.43 KB, patch)
2020-05-07 02:52 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 2020-05-06 07:24:53 PDT
To EventRegion
Comment 1 Antti Koivisto 2020-05-06 07:35:05 PDT
Created attachment 398611 [details]
patch
Comment 2 Antti Koivisto 2020-05-06 07:48:11 PDT
Created attachment 398614 [details]
patch
Comment 3 Simon Fraser (smfr) 2020-05-06 08:11:18 PDT
Comment on attachment 398614 [details]
patch

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

> Source/WebCore/rendering/style/StyleRareInheritedData.h:158
> +    unsigned eventListenerRegionTypes : 2; // OptionSet<EventListenerRegionType>

It's going to be really easy to forgot to change this when we add more EventListenerRegionType bits. I think maybe just eat the 8 bits right now, or add a compile-time assert that things will fit.

> Source/WebCore/style/StyleAdjuster.cpp:219
> +    auto findListeners = [&](auto eventName, auto type, auto nonPassiveType) {
> +        auto* eventListenerVector = element.eventTargetData()->eventListenerMap.find(eventName);
> +        if (!eventListenerVector)
> +            return;
> +
> +        types.add(type);
> +
> +        bool passiveOnly = true;
> +        for (auto& listener : *eventListenerVector) {
> +            if (!listener->isPassive())
> +                passiveOnly = false;
> +        }
> +
> +        if (!passiveOnly)
> +            types.add(nonPassiveType);
> +    };

Maybe this should be a function on Element
Comment 4 Daniel Bates 2020-05-06 08:28:21 PDT
Comment on attachment 398614 [details]
patch

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

>> Source/WebCore/rendering/style/StyleRareInheritedData.h:158
>> +    unsigned eventListenerRegionTypes : 2; // OptionSet<EventListenerRegionType>
> 
> It's going to be really easy to forgot to change this when we add more EventListenerRegionType bits. I think maybe just eat the 8 bits right now, or add a compile-time assert that things will fit.

Patch is ok as is. No change needed.

Just for me: Could this storage also be used to track the editable region? Usually only elements with userModify != ReadOnly are in that region, but there's one except for HTMLTextFormControl elements that is currently handled by plumbing an overrideUserModifyIsEditable book through EventRegionContext::unite(). This storage could be used to eliminate that bool. I didn't want to add storage to every element's style before, but now that this storage exists it could be used. Just a thought....
Comment 5 Antti Koivisto 2020-05-06 09:15:21 PDT
Created attachment 398625 [details]
patch
Comment 6 Antti Koivisto 2020-05-06 09:19:24 PDT
> Maybe this should be a function on Element

Nah, element is the same for all callers.
Comment 7 Antti Koivisto 2020-05-06 09:46:59 PDT
Created attachment 398629 [details]
patch
Comment 8 Antti Koivisto 2020-05-06 09:58:07 PDT
> Just for me: Could this storage also be used to track the editable region?

Probably.
Comment 9 EWS 2020-05-06 11:00:37 PDT
Committed r261239: <https://trac.webkit.org/changeset/261239>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398629 [details].
Comment 10 Radar WebKit Bug Importer 2020-05-06 11:01:20 PDT
<rdar://problem/62937481>
Comment 11 Ryan Haddad 2020-05-06 15:34:42 PDT
As indicated by mac-debug-wk1 EWS, this change caused fast/events/wheel-event-outside-body.html to assert 

SHOULD NEVER BE REACHED
./rendering/RenderLayerBacking.cpp(2948) : void WebCore::RenderLayerBacking::paintIntoLayer(const WebCore::GraphicsLayer *, WebCore::GraphicsContext &, const WebCore::IntRect &, OptionSet<WebCore::PaintBehavior>, WebCore::EventRegionContext *)

https://bugs.webkit.org/show_bug.cgi?id=211536
Comment 12 Ryan Haddad 2020-05-06 15:43:08 PDT
Reverted r261239 for reason:

Caused fast/events/wheel-event-outside-body.html to assert on macOS WK1

Committed r261253: <https://trac.webkit.org/changeset/261253>
Comment 13 Antti Koivisto 2020-05-07 02:52:12 PDT
Created attachment 398717 [details]
patch
Comment 14 EWS 2020-05-07 04:16:34 PDT
Committed r261279: <https://trac.webkit.org/changeset/261279>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398717 [details].