Closed Bug 1440112 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

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).
Depends on: 1425573
Priority: -- → P3
Whiteboard: [gfx-noted]
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.
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".
(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?
Flags: needinfo?(mikokm)
Comment on attachment 8953654 [details]
Bug 1440112 - Support CompositorHitTestInfo::eRequiresTargetConfirmation with EventRegions-based hit testing. r=kats

Kartikaya Gupta (email:kats@mozilla.com) has approved the revision.

https://phabricator.services.mozilla.com/D642
Attachment #8953654 - Flags: review+
Comment on attachment 8953656 [details]
Bug 1440112 - Enable helper_scroll_overscroll_behavior.html for non-WebRender as well. r=kats

Kartikaya Gupta (email:kats@mozilla.com) has approved the revision.

https://phabricator.services.mozilla.com/D643
Attachment #8953656 - Flags: review+
You'd think at some point Phabricator would at least cc me on the bug...
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
Depends on: 1440118
(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.
Flags: needinfo?(mikokm)
Pushed by bballo@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/adb71752bca8
https://hg.mozilla.org/mozilla-central/rev/94526b17561f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.