Closed Bug 1599627 Opened 3 months ago Closed 1 month ago

Huge Jerks scrolling down in Facebook Messenger

Categories

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

Desktop
macOS
defect

Tracking

()

RESOLVED INVALID

People

(Reporter: mgaudet, Unassigned)

References

(Blocks 1 open bug)

Details

Basic information

Steps to Reproduce: Login to facebook messenger, and find a conversation with history. Scroll up a little bit (there may be hicups loading archived content, this isn't what I'm worried about), then begin to scoll down.

Expected Results:

Scrolling down is smooth, as if all content is already rendered and ready.

Actual Results:

Scrolling down is a herky-jerky disaster as the page hiccups, stopping and starting scolling all the time.

While I can say this didn't used to be like this in Firefox, I have used mozregression to test Firefox 60, and it shows very similar behaviour, so it's not a behaviour change in Firefox methinks.


More information

(Sorry, I wasn't able to produce a video without personal info that shows the issue, and subset videos aren't great either; ni? me and I can provide privately a video of the situation)

Profile URL: https://perfht.ml/2OT63WO

Basic systems configuration:

OS version: OS/X 10.15.1

GPU model:
Radeon Pro 560X 4 GB
Intel UHD Graphics 630 1536 MB

Number of cores: 6

Amount of memory (RAM): 64GB

Looking at the profile, it's easy to see the jerks by moving the mouse along the screenshot thumbs to get a playback. Interestingly, I don't see much in the profile that could account for the jerks - the main threads seem reasonably healthy, and the compositor is receiving vsyncs.

So this might not be a performance issue, but it might be an APZ bug (perhaps the mouse events are being processed incorrectly?) or, this might also be a Facebook bug. It's unclear. But for now, I'm going to put this in the APZ component.

Component: Performance → Panning and Zooming

Here's the profile zoomed in to one of the large jerks: https://perfht.ml/2DsfSFQ
I identified the jerk by moving my mouse across the screenshot track and seeing where scrolling got "stuck".

In the parent process main thread, we still see regular "wheel" DOMEvent markers.
But on the compositor thread, we don't see any Composite markers during that time, other than no-op composites which also emit a marker that says "NoCompositorScreenshot because nothing changed".
I think this means that APZ determined that nothing needs to be scrolled in response to these wheel events (PanGestureInput events). Maybe the hit testing tree is wrong somehow? Or maybe Facebook is messing with hit testing by setting pointer-events:none in some places? Or maybe we're applying a confused transient APZ transform to the event location and are hit testing in the wrong place?

Flags: needinfo?(botond)
See Also: → 1568098

When I reproduce the scrolling sometimes completely stops even though I'm flinging pretty fast. If I try scrolling again right after it works fine. It feels like an apz scroll is getting clobbered by a main thread update (could be caused by other things, it just feels like that).

Blocks: apz-mac
Flags: needinfo?(botond)
Priority: -- → P2

I'm working on getting access to a Mac machine on which I can debug this.

I got access to a Mac machine, reproduced the problem, and started debugging it. The issue appears to be related to the website prevent-defaulting some but not all of the wheel events that we dispatch in response to the touchpad panning.

The Facebook code that's prevent-defaulting the events comes from this file and looks something like this:

:b("UserAgent_DEPRECATED").firefox()&&
e._wrap.addEventListener("DOMMouseScroll",function(a)
  {a.axis===a.HORIZONTAL_AXIS&&a.preventDefault()},
  !1);

Looks like it's specific to Firefox?

If I comment out this call to DispatchLegacyMouseScrollEvents(), the issue does not occur.

I'm not sure yet what conclusions to draw from this. Should the Facebook website code not be prevent-defaulting DOMMouseScroll events? Should we not be propagating the prevent-default to the originating wheel event? Or is all that fine, and the prevent-default just shouldn't have the consequence of interrupting momentum scrolling for the remainder of the gesture?

So that should only call preventDefault() when the axis is HORIZONTAL_AXIS, right? Are we setting that even when there's no scroll range?

(In reply to Botond Ballo [:botond] from comment #6)

The Facebook code that's prevent-defaulting the events comes from this file and looks something like this:

:b("UserAgent_DEPRECATED").firefox()&&
e._wrap.addEventListener("DOMMouseScroll",function(a)
  {a.axis===a.HORIZONTAL_AXIS&&a.preventDefault()},
  !1);

Looks like it's specific to Firefox?

Yes, it is. The event is fired only on Gecko.

I'm not sure yet what conclusions to draw from this. Should the Facebook website code not be prevent-defaulting DOMMouseScroll events? Should we not be propagating the prevent-default to the originating wheel event? Or is all that fine, and the prevent-default just shouldn't have the consequence of interrupting momentum scrolling for the remainder of the gesture?

I guess that even if it switched to wheel event, you'll see same result. I guess that it's caused by our smooth scroll or macOS's momentum scroll is prevented only when horizontal DOMMouseScroll event is fired. How would the symptom be changed if you disable our smooth scroll ("Use smooth scrolling" in Preferences/Options)?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

So that should only call preventDefault() when the axis is HORIZONTAL_AXIS, right? Are we setting that even when there's no scroll range?

Yes, it looks like EventStateManager::DispatchLegacyMouseScrollEvents() dispatches a distinct DOMMouseScroll event for each axis which has a nonzero delta in the originating event, and does not take the scroll range into consideration.

Should it perhaps only be firing events in directions with a scroll range?

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 12/28-1/4) from comment #8)

How would the symptom be changed if you disable our smooth scroll ("Use smooth scrolling" in Preferences/Options)?

There is no difference in behaviour if I disable smooth scrolling.

(In reply to Botond Ballo [:botond] [back Jan 3] from comment #9)

Should it perhaps only be firing events in directions with a scroll range?

Don't we still need to fire the scroll events in case the page wants to do something with them?

(In reply to Timothy Nikkel (:tnikkel) from comment #11)

(In reply to Botond Ballo [:botond] [back Jan 3] from comment #9)

Should it perhaps only be firing events in directions with a scroll range?

Don't we still need to fire the scroll events in case the page wants to do something with them?

I agree that we should fire wheel events regardless of scroll range. For DOMMouseScroll events, maybe we can find a different solution. DOMMouseScroll events are legacy events that I wish we could drop entirely. (I don't know what the webcompat impact of dropping DOMMouseScroll is.) But as long as we support them, let's try to minimize their potential for negative impact.
So there are three aspects to this problem, I think:

  • Facebook messenger is processing DOMMouseScroll events rather than wheel events. It would be great to get them to switch.
  • DOMMouseScroll events can only express one axis at a time. wheel can express diagonal deltas.
  • Calling preventDefault() on a DOMMouseScroll event needs to prevent scrolling, even for pixel-delta-based input methods.

We have this code that looks at the preventDefault status of the last DOMMouseScroll event when determining whether to default-handle pixel-delta-based scrolling. Would it make sense to split this flag into two flags, "prevented in X direction" and "prevented in Y direction"?

Flags: needinfo?(masayuki)

(In reply to Markus Stange [:mstange] from comment #12)

  • Facebook messenger is processing DOMMouseScroll events rather than wheel events. It would be great to get them to switch.

Based on a brief offline discussion with :BenWa, it seems that what Facebook really wants here is something like overscroll-behavior-x: contain, and the Messenger code just hasn't been ported over to use it. I'll try to prod them about this on the partner mailing list.

We have this code that looks at the preventDefault status of the last DOMMouseScroll event when determining whether to default-handle pixel-delta-based scrolling. Would it make sense to split this flag into two flags, "prevented in X direction" and "prevented in Y direction"?

What would be the semantics of these flags? Currently, preventDefault() interrupts the input block in APZ. Would we need to implement a new type of behaviour where the input block is retained, but one component of the events' deltas is dropped?

(In reply to Timothy Nikkel (:tnikkel) from comment #11)

Should it perhaps only be firing events in directions with a scroll range?

Don't we still need to fire the scroll events in case the page wants to do something with them?

What if we still fired the event, but ignored a preventDefault() response in a direction that has no scroll range?

(In reply to Markus Stange [:mstange] from comment #12)

We have this code that looks at the preventDefault status of the last DOMMouseScroll event when determining whether to default-handle pixel-delta-based scrolling. Would it make sense to split this flag into two flags, "prevented in X direction" and "prevented in Y direction"?

Well, that's sounds reasonable to me.

WidgetWheelEvent should have two bool flags for it, and it should be set in `EventStateManager::DispatchLagacyMouseScrollEvents().

Then, EventStateManager::PostHandleEvent() should clear delta value which is prevented temporarily around here:
https://searchfox.org/mozilla-central/rev/4537228c0a18bc0ebba2eb7f5cbebb6ea9ab211c/dom/events/EventStateManager.cpp#3434

Flags: needinfo?(masayuki)

I did some investigation on the Facebook side. This seems to apply to the majority of our nested scrollable areas. It's Firefox specific targeting the legacy 'DOMMouseScroll' as you know. I'm not able to find a good answer why yet but I'm still digging. This code all pre-dates 'overscroll-behavior'. So we'll likely want to modernize this on our side so don't feel the need to work around this issue on Facebook's account. I'll follow up once I know more.

Flags: needinfo?(b56girard)

A fix that removes this code will be in production tomorrow. It may take a few days longer to propagate, clearing your FB cookies will give you the fix sooner to test.

Flags: needinfo?(b56girard)

Hmm, I wonder, this must be unexpected requirement for wheel event. I mean that if web apps listens to only standardized wheel events, they cannot prevent only one direction scroll.

Filed spec issue: https://github.com/w3c/uievents/issues/257

Update: I was running Firefox 71.0 release, no problem with messenger, I upgraded to 72.0.1 today, issues with messenger.com scrolling is back. This issue appears to come and go every few release version.

Interesting; I just logged into messenger and found that it's definitely fixed here (there was some mention above from :BenWa about a propagating deploy; may be worth logging out and back in?)

The issue looks fixed for me as well, by the change on the site side. Thanks BenWa for pursuing this!

Given the site change, I don't think we need a browser-side mitigation of the sort discussed in comments 12-14. I think we can close the bug.

(I think the appropriate resolution here is INVALID, since the issue was fixed by a site change. However, from a user's point of view, the issue is fixed.)

Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → INVALID

(In reply to Chris More [:cmore] from comment #19)

Update: I was running Firefox 71.0 release, no problem with messenger, I upgraded to 72.0.1 today, issues with messenger.com scrolling is back. This issue appears to come and go every few release version.

Chris, if you still see this issue after waiting for a few days for the Facebook site changes to propagate (or after clearing your Facebook cookies as BenWa suggested in comment 17), it's possible you're experiencing a different issue with similar symptoms. Let's follow up in bug 1568098 in that case.

This has been all fine for a number of days now. Thanks!

You need to log in before you can comment on or make changes to this bug.