1440112 - overscroll-behavior not obeyed for unlayerized scroll frame when main thread is busy (EventRegions edition)

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Botond Ballo [:botond]]

This is a follow-up to bug 1425573 to handle to fix the problem for the EventRegions codepaths (see bug 1425573 comment 7; bug 1425573 is only fixing the issue for the WebRender codepath).

Comment 1

[Phabricator Automation]

Attachment: Bug 1440112 - Support CompositorHitTestInfo::eRequiresTargetConfirmation with EventRegions-based hit testing. r=kats

Comment 2

[Phabricator Automation]

Attachment: Bug 1440112 - Enable helper_scroll_overscroll_behavior.html for non-WebRender as well. r=kats

Comment 3

[Botond Ballo [:botond]]

Trying out something new and submitting patches via Phabricator.

Note: apparently Phabricator doesn't set r? flags in Bugzilla. So, just to clarify, I'm requesting review on these patches in spite of them not having r? flags.

Comment 4

[Botond Ballo [:botond]]

A general comment on the approach I'm taking: I think it's overkill to create a new region representing "the part of the dispatch-to-content region where target confirmation is required" (and especially so as EventRegions are eventually going to be deprecated). Instead, I just added a flag to EventRegions that means "the entire DTC region requires target confirmation".

The only downside of this is that for a layer that has a DTC region containing some portions that conceptually require target confirmation and some portions that don't, we treat the entire DTC region as requiring target confirmation. Doesn't seem like a common case, or a terrible behaviour even when it does arise.

I also only implemented this for codepath #2 from bug 1425573 comment 7, "nsDisplayCompositorHitTestInfo -> EventRegions". It looks like this is now the default, and if this rides the trains in Firefox 60, then I won't bother implementing this for codepath #3, "nsDisplayLayerEventRegions -> EventRegions".

Comment 5

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #4)
> I also only implemented this for codepath #2 from bug 1425573 comment 7,
> "nsDisplayCompositorHitTestInfo -> EventRegions". It looks like this is now
> the default, and if this rides the trains in Firefox 60, then I won't bother
> implementing this for codepath #3, "nsDisplayLayerEventRegions ->
> EventRegions".

Miko, could you confirm whether you intend for layout.simple-event-region-items=true (which, if I understand correctly, causes us to build event regions from nsDisplayCompositorHitTestInfo items instead of  nsDisplayLayerEventRegions items) to ride the trains in Firefox 60?

Comment 6

[Phabricator Automation]

Comment on attachment 8953654 [details]
Bug 1440112 - Support CompositorHitTestInfo::eRequiresTargetConfirmation with EventRegions-based hit testing. r=kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

https://phabricator.services.mozilla.com/D642

Comment 7

[Phabricator Automation]

Comment on attachment 8953656 [details]
Bug 1440112 - Enable helper_scroll_overscroll_behavior.html for non-WebRender as well. r=kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

https://phabricator.services.mozilla.com/D643

Comment 8

[Kartikaya Gupta (email:[e-mail])]

You'd think at some point Phabricator would at least cc me on the bug...

Comment 9

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> You'd think at some point Phabricator would at least cc me on the bug...

You'd think :) Filed bug 1440864 for this.

Comment 10

[Botond Ballo [:botond]]

I guess I need to wait for bug 1440118 to merge around before landing this. Should've been a bit more patient and waited to land the test *after* this bug.

Comment 11

[Miko Mynttinen]

(In reply to Botond Ballo [:botond] from comment #5)
> (In reply to Botond Ballo [:botond] from comment #4)
> > I also only implemented this for codepath #2 from bug 1425573 comment 7,
> > "nsDisplayCompositorHitTestInfo -> EventRegions". It looks like this is now
> > the default, and if this rides the trains in Firefox 60, then I won't bother
> > implementing this for codepath #3, "nsDisplayLayerEventRegions ->
> > EventRegions".
> 
> Miko, could you confirm whether you intend for
> layout.simple-event-region-items=true (which, if I understand correctly,
> causes us to build event regions from nsDisplayCompositorHitTestInfo items
> instead of  nsDisplayLayerEventRegions items) to ride the trains in Firefox
> 60?

As per bug 1436409, the goal is to eventually get rid of nsDisplayLayerEventRegions. The pref flag in question is just to have a mature codepath as a fallback, in case there are serious regressions from using nsDisplayCompositorHitTestInfo.
So far things seem to be working as expected, and I don't think it makes sense to work on new nsDisplayLayerEventRegions related code.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adb71752bca8
Support CompositorHitTestInfo::eRequiresTargetConfirmation with EventRegions-based hit testing. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/94526b17561f
Enable helper_scroll_overscroll_behavior.html for non-WebRender as well. r=kats

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/adb71752bca8
https://hg.mozilla.org/mozilla-central/rev/94526b17561f