1649191 - Laggy scrolling when zooming after putting focus in input field

Status: RESOLVED FIXED

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

Description

[haswellready]

User Agent: Mozilla/5.0 (Android 10; Mobile; rv:78.0) Gecko/78.0 Firefox/78.0

Steps to reproduce:

When I zoom bugzilla It shows incorrectly.
See the video below https://youtu.be/k0iHNouGUX0
I dont know what firefox version to specify in this bug.
I use firefox preview 5.2.0 which is not listed.

Actual results:

Some parts of Web Page are not shown
The movement of a Web Page is not smooth

Expected results:

No problems with moving and appearence of a Page that is being zoomed

Comment 1

[Botond Ballo [:botond]]

Possibly bug 1648641 occurring on mobile as well?

Comment 2

[Botond Ballo [:botond]]

I'm not able to reproduce this, although the page in question (bugzilla.mozilla.org/enter_bug.cgi) looks slightly different for me than it does in the video.

haswellready, do you see this on any Bugzilla page other than enter_bug.cgi?

Comment 3

[haswellready]

Sometimes It happens on other pages of bugzilla, for example, when commenting bugs and zooming.
I dont See that issue on other sites But I usually dont neet to zoom any other sites because their view is optimized for smartphone
I also use Gmail to open links in Firefox preview.
But the issue is intermittent for me too.
I cant reproduce It all the time.

Comment 4

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

I can reproduce this on enter_bug.cgi if I switch to the bugzilla helper view (so that it looks like in the attached video) and then put focus in the "find product" text input field. Although it seems similar to the slow/laggy scrolling symptoms from bug 1645665, the Gecko build in the latest Fenix build includes that patch and still has this problem. This also seems to happen with WR disabled, so it's likely a different issue.

Comment 5

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

I'm going to mark this S2 since it seems pretty bad and is likely something stupid happening in Gecko that should be easy to fix once we figure out what it is. It sorta reminds me of bug 1512838.

Comment 6

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

... and now I can't reproduce this anymore. haswellready, do you still see this problem with the latest Firefox Preview?

Comment 7

[haswellready]

Still reproducible on nightly 22050611.

• Open New bug Web page.
• zoom in
• click into any text box for example Find product

Comment 8

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

I've seen this a few more times as well since then, although not reliably. Still, I'll try to track this down.

Comment 9

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

I can repro now. I don't fully understand the problem yet, but basically the page (using https://bugzilla.mozilla.org/enter_bug.cgi?format=guided while logged in) fits on the screen when zoomed out. When zoomed in, the root scrollframe becomes scrollable (visually). But then if the keyboard comes up that squashes the composition bounds, and so the root scrollframe also becomes scrollable layout-ly. However the metrics for the root scrollframe don't seem to make much sense, I see the layout viewport being taller than the scrollable rect. Anyway, in this scenario the inner "main" div also becomes scrollable. And if in this scenario the root scrollframe is scrolled downwards the displayport for the inner div remains near the "top" and so this can result in checkerboarding. But apparently only while my finger is down and scrolling.. strange.

I don't recall how the displayport of nested scrollframes gets positioned to account for the scroll position of containing scrollframes, although I remember working on such code to deal with github checkerboarding. That's likely the code to blame here. But there's a bunch of oddities in the metrics that I need to untangle to make more sense of it all.

Comment 10

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

After staring at this some more I think the fundamental problem is that the displayport base rect that gets computed here doesn't account for the visual viewport offset on the RCD. The rects there are all layout-based and so the displayport base rect is also computed as something that would be appropriate for the layout scroll position, but that may not be what the user is actually looking at.

Comment 11

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

I'm not able to reproduce this anymore on a recent nightly Fenix build. Are you still seeing this problem? Since this bug was filed there have been a bunch of improvements made to the code and although I don't know exactly which one would have fixed this, it seems plausible that one of the changes could have. If you can't reproduce the problem any more I can try to bisect to see what fixed it.

Comment 12

[haswellready]

Still reproducible on my side.
Nightly 201115 17:03 (Build #2015775819)
AC: 66.0.20201109143146, b44b70ee1
GV: 84.0a1-20201109095222
AS: 63.0.0

Comment 13

[haswellready]

I can say that clipping issue fixed but laggy scrolling is not.

Comment 14

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

Thanks. Can you also let me know what device you're using, and what it says under "Compositing" on about:support? If you can attach your entire about:support that would be even better. Thanks again!

Comment 15

[haswellready]

My device is Honor 20 Pro (It has another name in about support).

Comment 16

[haswellready]

Attachment: about support.txt

Comment 17

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

Jamie, do we have any known issues with janky scrolling with WR on Mali-G76?

Comment 18

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

(Updating title since the clipping issue is resolved now)

Comment 19

[Jamie Nicol [:jnicol] out of office until 6th Jan]

I think I can reproduce this: If the field is focused, it sometimes takes a second to respond to a zoom gesture, then suddenly jumps to being zoomed in. It's really quite bad, doesn't reproduce all of the time though.

do we have any known issues with janky scrolling with WR on Mali-G76?

There was bug 1661528, but it was fixed just before the 20201109095222 date in comment 12. I can reproduce this on a Mali-G72 and an Adreno 630, so I don't think it is GPU specific.

Here's a profile: https://share.firefox.dev/38MEVnH

There doesn't appear to be anything particularly slow going on in the renderer thread. Could the problem lie somewhere else?

Comment 20

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

It certainly could be elsewhere. If it doesn't reproduce all the time that might explain why I'm not seeing it on my device. I'll try some other devices too.

Comment 21

[Jamie Nicol [:jnicol] out of office until 6th Jan]

I can reproduce this on both layers and webrender as far back as 2020-01-01. I can't find a pattern for what circumstances it reproduces under, however..

Comment 22

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

Hm, strange. I just tried on both a Sony Z3C and a Pixel 2 and couldn't reproduce on either.

Comment 23

[Markus Stange [:mstange]]

Jamie, could you get another profile that includes screenshots and APZ logging? That might make the problem easier to identify.
To get APZ logging into the profiler, you need to start GVE with an environment variable like MOZ_LOG=apz.inputqueue:5,apz.controller:5,profilermarkers.

Comment 24

[Jamie Nicol [:jnicol] out of office until 6th Jan]

https://share.firefox.dev/3pBdnrx

The screenshots do show where the janks happened. I'm not sure if the APZ logging worked correctly. I followed the instructions here to set the environment variable, and confirmed in logcat that it read the file successfully, but I'm not sure what it's supposed to add to the profile.

Comment 25

[Markus Stange [:mstange]]

Oh right, the logging is emitted on the UI thread and doesn't make it into the profile because of bug 1661172. So this didn't actually do what I wanted. (You can still see some of the logging on the compositor thread, in the "Marker Table" tab, but it's not what we need.)

Comment 26

[Markus Stange [:mstange]]

Ok, I see it happening in this time window: https://share.firefox.dev/3faeGbT
We get plenty of touch events, and trigger plenty of composites, but the composites all end up no-op composites. (There's a "Composite" marker on the renderer thread, but no "NumDrawCalls" marker, which means that the renderer didn't render anything.)

Comment 27

[Markus Stange [:mstange]]

Is this reproducible with WebRender off?

My current suspicion is that there's an invalidation bug on the WebRender side.

Comment 28

[Jamie Nicol [:jnicol] out of office until 6th Jan]

I thought I could, but I might have been mistaken. I can't seem to now. it seems a reasonable suspicion, I'll have a dig in to it

Comment 29

[Jamie Nicol [:jnicol] out of office until 6th Jan]

I wonder if this could be related to bug 1675414?

Comment 30

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

Latest Fenix nightly now has the fix for bug 1675414. Can you still reproduce?

Comment 31

[Jamie Nicol [:jnicol] out of office until 6th Jan]

I can still reproduce.

Markus, you are partially correct in that webrender is bailing out on rendering early. However, the reason is because the transform values it is sampling from APZ do not change, so it believes that there is nothing new to render.

The reason the transform values don't change is because this line of code is being executed when the input field is selected. (Sometimes, I still can't spot a pattern as to when.) This means that when calculating the scroll offset we incorrectly use the last paint's value rather than the sampled state, eg here.

I can also reproduce with webrender disabled.

Botond, any ideas why we are sometimes disabling APZ when the input field is selected?

Comment 32

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

My guess would be the caret becomes visible and thinks it's in a poisition-fixed subtree, here: https://searchfox.org/mozilla-central/source/layout/base/AccessibleCaretManager.cpp#388

In this scenario the caret can't be kept in sync with the content during async scrolling and so sync scrolling is disabled intentionally.

Comment 33

[Jamie Nicol [:jnicol] out of office until 6th Jan]

It appears to be taking the if (mIsScrollStarted) branch above that, and mIsCaretPositionChanged is true so we disable APZ.

Comment 34

[Botond Ballo [:botond]]

cc'ing Ting-Yu

(There's a lot of discussion in this bug, so to focus it a bit: we're talking about the STR in comment 7, and the discussion from comment 26 onward is the most relevant.)

I think the two questions we need to answer are:

1. Is this really a scenario where the caret can't be kept in sync with the content during async scrolling, or are we entering the "force-disable APZ" codepath over-eagerly?
2. If the former is true, is preferring correctness (the caret never being rendered outside the text field where it should be) at the cost of slow responsiveness (pan gestures don't cause scrolling until the main thread repaints, which on mobile can take multiple frames) the tradeoff we want?

Comment 35

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 34:

To answer question 1: No, I don't think this is a scenario where APZ needs to be disable. We just entering the "force-disable APZ" codepath over-eagerly.

What happens is: in the step 3 "click into any text box for example Find product" in comment 7, AccessibleCaretManager updates mIsCaretPositionChanged to true. However, since the page is zoomed in, I observe that AccessibleCaretManager::OnScrollPositionChanged() is never called, so mIsCaretPositionChanged remains true, forcing APZ to be disabled just as Jamie observed in comment 33.

AccessibleCaretManager::OnScrollPositionChanged() is called via AccessibleCaretEventHub registered the nsIScrollObserver::ScrollPositionChanged() interface. If it's expected that we are calling ScrollPositionChanged() only if the page is not zoomed-in, I'll need to design a fix to AccessibleCaret. NI myself as a reminder.

To answer question 2: I believe we should prefer responsiveness over correctness especially when the carets are not even visible.

Comment 36

[Botond Ballo [:botond]]

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #35)

However, since the page is zoomed in, I observe that AccessibleCaretManager::OnScrollPositionChanged() is never called, [...]

AccessibleCaretManager::OnScrollPositionChanged() is called via AccessibleCaretEventHub registered the nsIScrollObserver::ScrollPositionChanged() interface. If it's expected that we are calling ScrollPositionChanged() only if the page is not zoomed-in, I'll need to design a fix to AccessibleCaret.

I suspect what's happening is that when the page is zoomed in, only the visual scroll offset is changing and not the layout scroll offset (see this page for the distinction; note, the page is out of date, Firefox works like Chrome now), and nsIScrollObserver::ScrollPositionChanged() is only being called for layout scroll offset changes.

We may want to consider adding a new method to nsIScrollObserver, such as VisualScrollPositionChanged(). This can be notified in PresShell::SetVisualViewportOffset() (as compared to ScrollPositionChanged() which is notified in ScrollFrameHelper::ScrollToImpl()).

Comment 37

[Botond Ballo [:botond]]

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

We may want to consider adding a new method to nsIScrollObserver, such as VisualScrollPositionChanged(). This can be notified in PresShell::SetVisualViewportOffset() (as compared to ScrollPositionChanged() which is notified in ScrollFrameHelper::ScrollToImpl()).

(Another potenetial option would be to notify the existing nsIScrollObserver::ScrollPositionChanged() method on visual scroll offset changes, but we'd have to check the other users of nsIScrollObserver and determine if that's something they want.)

Comment 38

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

This is a bug in AccessibleCaretManager that we don't reset mIsScrollStarted to false in OnScrollEnd(), making APZ disabled incorrectly with the STR in comment 7. I have a detailed analysis in the commit message in my upcoming patch.

Unfortunately, it doesn't help if we notify AccessibleCaretManager when the visual scroll offset changes. Because on Android, the AcessibleCaret's appearance is Appearance::None (not logically visible) when focusing on an empty <input> [1], the carets won't update in AccessibleCaretManager::OnScrollPositionChanged().

[1] Android turns on this pref so that it requires a long tapping to show the caret on an empty input. https://searchfox.org/mozilla-central/rev/ffdb6a4d934b3f5294f18cf0e1df618109ccb36b/mobile/android/app/mobile.js#624

Comment 39

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1649191 - Remove the guarding statement about mismatching carets mode from AccessibleCaretManager APIs.

Steps to reproduce:

1. Open https://bugzilla.mozilla.org/home
   (Note the cursor is already blinking in the <input>
   "Enter a bug number or some search terms")
2. Pinch-zoom in.
3. Tap on the <input> "Enter a bug number or some search terms."
4. Pan/scroll the page, and the scrolling is very laggy.

Without this patch, AccessibleCaret disables APZ incorrectly via the
above operations. Here's an analysis.

In step 2, OnScrollEnd() called at the end of the pinch-zoom operation
is supposed to reset mIsScrollStarted to false, but GetCaretMode()
returns CaretMode::Cursor because the page already has a focus on
<input>. We are early-returned from OnScrollEnd() because
mLastUpdateCaretMode is still the default value CaretMode::None.

In step 3, tapping the <input> will call UpdateCaretsForCursorMode(),
setting mIsCaretPositionChanged to true. Then
UpdateShouldDisableApz() incorrectly sets mShouldDisableApz to
true because we still have mIsScrollStarted=true.

In step 4, the operation is laggy because APZ is disabled.

This patch fixed this bug by removing the guarding statement
mLastUpdateCaretMode != GetCaretMode() from three callback methods.

The statements were added in the very first patch introducing
AccessibleCaretManager. I don't recall why we needed them. (Perhaps to
avoid unnecessary updates notified from other PresShell?). Anyway, since
then, these callbacks have evolved to update carets only if any caret is
logically visible, so I don't see why we need these guards nowadays. By
doing so, mIsScrollStarted can be reset to false in OnScrollEnd()
in step 2.

Comment 40

[Botond Ballo [:botond]]

Thanks for the patch and the detailed explanation!

Comment 41

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Botond, thanks for the prompt review, and thanks everyone involves in this bug. FWIW, I've tested my patch with comment 39's STR on GeckoView Example on a Pixel 2.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9901aa846b0a
Remove the guarding statement about mismatching carets mode from AccessibleCaretManager APIs. r=botond

Comment 43

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/9901aa846b0a