Closed Bug 1554795 Opened 5 years ago Closed 2 years ago

Support desktop scrollbar thumb sizing logic in APZ

Categories

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

Product:
Core
Component:
Panning and Zooming
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
111 Branch
Tracking Flags:
Tracking Status
firefox-esr102 --- wontfix
firefox69 --- wontfix
firefox109 --- wontfix
firefox110 --- wontfix
firefox111 --- wontfix
firefox112 --- wontfix
firefox113 --- fixed

People

(Reporter: botond, Assigned: rzvncj, Mentored)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [lang=c++])

Attachments

(3 files)

Bug 1554795 - Support desktop scrollbar thumb sizing logic in APZ. r?botond,dlrobertson
48 bytes, text/x-phabricator-request
Details | Review
Bug 1554795 - Expose the currently effective visual viewport rect. r=hiro
48 bytes, text/x-phabricator-request
Details | Review
Back out Bug 1554795 and things that depend on it from v112. r=tnikkel
48 bytes, text/x-phabricator-request
diannaS
: approval-mozilla-release+
Details | Review

When pinch-zooming, scrollbar thumbs are scaled asynchronously to approximate what they will look like when we repaint at the new resolution.

Our current scaling logic works on Android, where the scroll thumb length is basically scaled proportionally to the page length. On desktop, however, scroll thumbs appear to have a minimum length, making the scaling logic not exactly proportional.

The resulting inaccuracy is not super noticeable, but we do need to resolve it to get these reftests to pass (or else significantly up the fuzz factor on them).

Blocks: desktop-zoom-release

I'm going to make this an Outreachy intermediate task. Please note that this is one of the more challenging ones; before taking this on, please (1) complete another C++ intermediate task first; and (2) double-check in the #apz channel that there's enough time left in the application period to have a good chance of completing this.

Mentor: botond
Whiteboard: [apz-outreachy]
Mentor: kats
Whiteboard: [apz-outreachy] → [apz-outreachy][lang=c++]
Assignee: nobody → tnikkel
Priority: P3 → P2
Whiteboard: [apz-outreachy][lang=c++] → [lang=c++]
Assignee: tnikkel → nobody
Mentor: kats
Severity: normal → S3

What are the steps to manually reproduce this problem? I'm on (Arch) Linux with Firefox 106.0.4 and I'm pinch-zooming with my laptop touchpad, and the scrollbar thumb size appears to be modified in real time proportionally to the zoom factor - in other words, I don't see anything problematic.

This is easiest to see on a page that's long enough that scrollbar thumb is already at its minimum length on desktop:

During the zoom gesture, when APZ is scaling the thumb, its length shrinks proportionally to the zoom level, and therefore goes below the minimum length. When the main thread redraws the thumb at the new zoom level, it respects the minimum length and therefore we can see a jump back to the minimum length.

If you'd like to work on this, let me know and I can dig up some places in the code to look at.

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

This is easiest to see on a page that's long enough that scrollbar thumb is already at its minimum length on desktop:

During the zoom gesture, when APZ is scaling the thumb, its length shrinks proportionally to the zoom level, and therefore goes below the minimum length. When the main thread redraws the thumb at the new zoom level, it respects the minimum length and therefore we can see a jump back to the minimum length.

Right, I see.

If you'd like to work on this, let me know and I can dig up some places in the code to look at.

Sure, it's an as good bug as any still left on my list (unless, as I was telling Daniel a while back, this messes up your Outreachy thing, in which case I can move on to something else).

I haven't verified this, but based on code reading, I think the main-thread computation of the thumb length happens in nsSliderFrame::DoXULLayout(), with the step to respect a minimum length being this std::max() call.

If that's accurate, that suggests the minimum length is stored in the initial value of thumbLength, which comes from thumbBox->GetXULPrefSize() here. It will likely require some debugging to track down where that minimum length is originally specified.

As for the APZ side, the scaling of the thumb in between main-thread paints happens here.

(In reply to Razvan Cojocaru from comment #4)

unless, as I was telling Daniel a while back, this messes up your Outreachy thing

It does not; the Outreachy project for which this was a candidate mentored task concluded in March 2021.

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

I haven't verified this, but based on code reading, I think the main-thread computation of the thumb length happens in nsSliderFrame::DoXULLayout(), with the step to respect a minimum length being this std::max() call.

If that's accurate, that suggests the minimum length is stored in the initial value of thumbLength, which comes from thumbBox->GetXULPrefSize() here. It will likely require some debugging to track down where that minimum length is originally specified.

Well, nsSliderFrame::DoXULLayout() calls thumbBox->GetXULPrefSize(aState), which in turn calls GetXULMinSize(), which calls nsIFrame::AddXULMinSize(), which takes the display->HasAppearance() branch that, on my Linux box, ends up in ScrollbarDrawingGTK::GetMinimumWidgetSize().

This last function uses StaticPrefs::widget_non_native_theme_gtk_scrollbar_thumb_cross_size() (aka widget.non-native-theme.gtk.scrollbar.thumb-cross-size), which, for me, is 40.

For me, the size returned by ScrollbarDrawingGTK::GetMinimumWidgetSize() is (width, height): [12, 40], which the subsequent calls to DevPixelsToAppUnits(size.{width, height}) turn into [720, 2400] (.width and .height get multiplied by 60). So we end up with things like this happening (all logged from nsSliderFrame::DoXULLayout()):

[0x7f14393a9958] thumbSize: [720, 2400]
[0x7f14393a9958] thumbLength: 2400
[0x7f14393a9958] max(2400, 70.7366)
[0x7f14393a9958] [POST MAX] thumbLength: 2400
[0x7f14393a9958] [POST DevPixelsToAppUnits] thumbLength: 2400

(I think you get the jist of where the logs are in the code from what they say.)

The magic 12 value appears on first look to come from here.

Thanks for tracking that down!

The 12 is the scrollbar's thickness (e.g. the width of a vertical scrollbar). This does not change over the course of zooming, so it's not interesting for our purposes. The interesting part is the minimum length which sounds like it's 40 on Linux.

A question is how to make this minimum length available on the compositor (APZ) side. We can't call nsITheme::GetMinimumWidgetSize() or similar because that requires an nsIFrame* which is only available on the main thread of the content process. Nor does it look like that requirement can easily be avoided (e.g. on some platforms, this function queries style information specified in CSS which is also tied to the content process).

So I suggest the following:

  • Add a new mThumbMinLength field to ScrollbarData. (This is a data structure we use to communicate information about a scrollbar from the content process to the compositor.)
    • This structure is sent over IPC so the serialization code here needs to be updated too.
  • In nsSliderFrame::DoXULLayout(), save the minimum length (i.e. the value of thumbLength before the std::max()) into a new field of nsSliderFrame (we can call that mThumbMinLength too).
  • Populate the field into ScrollbarData here.
  • The ScrollbarData is available on the compositor side in ScrollThumbUtils.cpp, so that code can now make use of the minimum length.
Assignee: nobody → rzvncj
Status: NEW → ASSIGNED

I think I'm either misunderstanding the problem, or its description is not accurate based on the latest review comments.

What I've understood is this: on desktop, a thread is scaling the thumb with no regards to an upper or lower bounds on the thumb size. Then, another thread comes in (usually after the scaling operation ends), and this time it looks at the minimum (and, I believe, at a maximum, as well) size, and snaps the thumb size to it if the current size goes beyond. On Android, there are no upper / lower thumb size bounds checks, and the reftests you've referenced pass there because of this.

Now, the latest comments on my patch say that the Android reftest should just pass (with --setpref widget.non-native-theme.gtk.scrollbar.round-thumb=false) for desktop too with the proper fix, but that doesn't make sense to me. That is because the way I've read the problem description, what we want is to not let the first thread go below the minimum thumb size. So there are two possible ways the reftests are going. Either:

  • the reftests make the thumb go below the minimum desktop size. If this is the case, then the reftests will never pass unmodified for desktop, because for desktop the thumb will never go below the minimum size, regardless of whether the other thread temporarily goes below it or not (that is, with or without the fix). Or:
  • the reftests do not make the thumb go below the minimum desktop size. If this is the case, then the reftests should have passed on the desktop before the patch as well (and the patch should make no difference for that behaviour), so they wouldn't be something to consider WRT this problem at all.

Hacking / debugging the code is fine, but I feel that there's no way around me properly understanding what we're trying to fix / achieve here.

(In reply to Razvan Cojocaru from comment #10)

What I've understood is this: on desktop, a thread is scaling the thumb with no regards to an upper or lower bounds on the thumb size. Then, another thread comes in (usually after the scaling operation ends), and this time it looks at the minimum (and, I believe, at a maximum, as well) size, and snaps the thumb size to it if the current size goes beyond. On Android, there are no upper / lower thumb size bounds checks, and the reftests you've referenced pass there because of this.

This matches my understanding (with the first thread being the "compositor thread" and the second being the "main thread"; note they're also in different processes), with a couple of caveats:

  • The main thread does not know about the thumb length that the compositor thread computed. It just gets notified about the new zoom level, and then independently computes a thumb length and position appropriate to that zoom level, which will be reflected the next time the pge is rendered, replacing the rendering that reflects the length and position chosen by the compositor.
    • I think, based on a cursory read of the code, that the main thread computation of the length is the same as the compositor thread computation but subject to a minimum length; but there may be other subtle differences. Part of the purpose of this bug is to understand such differences.
    • The main thread computation is considered the "authoritative" one, and we'd like to make the compositor computation match it.
  • I don't think there's any sort of fixed maximum thumb length; the thumb can grow up to the full length of the slider track (for example on this page it takes up almost all of the length of the slider track).
  • the reftests make the thumb go below the minimum desktop size. If this is the case, then the reftests will never pass unmodified for desktop, because for desktop the thumb will never go below the minimum size, regardless of whether the other thread temporarily goes below it or not (that is, with or without the fix). Or:

The reftests are written such that the test page (e.g. root-scrollbar-async-zoomed-in.html) shows the rendering that reflects the compositor thread computation, and the reference page (e.g. root-scrollbar-async-zoomed-in-ref.html) shows the rendering that reflects the main thread computation. The pages are long enough that, on platforms where a minimum thumb size is respected, the minimum size should be in effect.

Let me know if this clears things up.

(I haven't forgotten about this comment, I just haven't had a chance to think it through in more detail.)

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

  • I don't think there's any sort of fixed maximum thumb length; the thumb can grow up to the full length of the slider track (for example on this page it takes up almost all of the length of the slider track).

Right, but please load https://theres-waldo.github.io/mozilla-testcases/long-100000px.html, then zoom in quite a bit (more than 2x).
Then, zoom out and look at the thumb: the compositor thread tries to increase it's size, then the main thread snaps it back to a smaller size. This is what I mean about a maximum size: if the page is long enough, the thumb size should, I suppose, remain unchanged for both zooming in and zooming out.

  • the reftests make the thumb go below the minimum desktop size. If this is the case, then the reftests will never pass unmodified for desktop, because for desktop the thumb will never go below the minimum size, regardless of whether the other thread temporarily goes below it or not (that is, with or without the fix). Or:

The reftests are written such that the test page (e.g. root-scrollbar-async-zoomed-in.html) shows the rendering that reflects the compositor thread computation, and the reference page (e.g. root-scrollbar-async-zoomed-in-ref.html) shows the rendering that reflects the main thread computation. The pages are long enough that, on platforms where a minimum thumb size is respected, the minimum size should be in effect.

Let me know if this clears things up.

It does, for some reason I was sure that one of these pages was actually just a static .png, a screenshot from an Android test or something of that sort. Of course it would then have followed that it's impossible to have it match the desktop situation.

(In reply to Razvan Cojocaru from comment #12)

Right, but please load https://theres-waldo.github.io/mozilla-testcases/long-100000px.html, then zoom in quite a bit (more than 2x).
Then, zoom out and look at the thumb: the compositor thread tries to increase it's size, then the main thread snaps it back to a smaller size. This is what I mean about a maximum size: if the page is long enough, the thumb size should, I suppose, remain unchanged for both zooming in and zooming out.

Yes, I see what you mean now.

I believe what's happening is something like this:

  • We start in a state where the minimum size is in effect (for example, based on the zoom level and page length alone, the thumb length would be 30px, but it's clamped to the minimum 40px).
  • We zoom out, say by 2x. The compositor applies the 2x scale to the 40px length, resulting in 80px. But the correct value would be to apply the 2x scale to the 30px, resulting in 60px.

So I think what the compositor needs to do is:

  • Calculate the desired thumb length, which in this case is 60px.
  • Look at the actual thumb length rendered by the main thread, which in this case is 40px (this is stored in ScrollbarData::mThumbLength).
  • Calculate the scale needed to transform the latter into the former -- in this case that's 1.5x (note this is different from the zoom level which is 2x).

Similar to the other GetCurrentAsync*() functions,
GetCurrentAsyncVisualViewport() takes into account the async test
attributes as well as the frame delay.

It supersedes GetCurrentAsyncScrollOffsetInCSSPixels() which can
be obtained as the the TopLeft() of the visual viewport rect.

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/509d5dfea632 Expose the currently effective visual viewport rect. r=hiro https://hg.mozilla.org/integration/autoland/rev/ef1bbae9b06b Support desktop scrollbar thumb sizing logic in APZ. r=botond

https://hg.mozilla.org/mozilla-central/rev/509d5dfea632
https://hg.mozilla.org/mozilla-central/rev/ef1bbae9b06b

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox111: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch
Blocks: 1815371
Blocks: 1815372
Regressions: 1815442
Regressions: 1815652
Regressions: 1818702
Regressions: 1825779

After discussing this with Timothy and Hiro and in light of the newly reported regression, I would like to propose the following courses of action:

  • For v112: back out this bug.
  • For v113: put the changes this bug makes to the scrollbar sizing/position code behind a pref that's nightly-only for now. This will allow us to avoid further backouts since we can just keep the pref nightly-only until the known regressions and resolved and we are more confident in shipping it.

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

  • For v112: back out this bug.

Posted a rollup patch that applies cleanly to mozilla-beta and backs out bug 1554795 and follow-up changes that depend on it.

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

  • For v112: back out this bug.

Donal, does this sound reasonable to you? If so, would you be able to merge the backout patch (https://phabricator.services.mozilla.com/D174550) to beta?

Flags: needinfo?(dmeehan)

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Redirecting needinfo to Dianna (release owner for 112) to take a look at Comment 21
:botond FYI 112 was merged to release and 112.0 RC1 was built yesterday.

Flags: needinfo?(dmeehan) → needinfo?(dsmith)

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

  • For v113: put the changes this bug makes to the scrollbar sizing/position code behind a pref that's nightly-only for now. This will allow us to avoid further backouts since we can just keep the pref nightly-only until the known regressions and resolved and we are more confident in shipping it.

Filed bug 1826499 for this.

Sure. Please submit a nomination and I can uplift this for RC respin for 112

Flags: needinfo?(dsmith)

Comment on attachment 9326761 [details]
Back out Bug 1554795 and things that depend on it from v112. r=tnikkel

Beta/Release Uplift Approval Request

  • User impact if declined: On some sites, including Tweetdeck (bug 1825779) and Google Sheets (bug 1818721), the scrollbar may move erratically during scrolling.

The actual scrolling remains smooth (only the rendering of the scrollbar is affected), but the erratic movement of the scrollbar can create the appearance of something going wrong with the scrolling.

  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch backs out the changes made to the affected code from 112, so that we ship the same thing in 112 that we did in 111.
  • String changes made/needed: none
  • Is Android affected?: Unknown
Attachment #9326761 - Flags: approval-mozilla-release?

Comment on attachment 9326761 [details]
Back out Bug 1554795 and things that depend on it from v112. r=tnikkel

Approved for backout 112.0rc2 (will stay in 113)

Attachment #9326761 - Flags: approval-mozilla-release? → approval-mozilla-release+

(In reply to Dianna Smith [:diannaS] from comment #28)

Setting 112 to wontfix
Backed out of release rc2 for causing Bug 1825779
https://treeherder.mozilla.org/jobs?repo=mozilla-release&revision=566eef64df10aa14d23f383cdd171a031f8e7d73

Thanks!

Regressions: 1826947
Blocks: 1843939
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: