Open Bug 1683631 Opened 3 years ago Updated 3 years ago

When using meta/ctrl + mousewheel to zoom, the zoom indicator updates without actually reflowing/zooming the page

Categories

(Firefox :: Toolbars and Customization, defect, P3)

defect

Tracking

()

Tracking Status
firefox85 --- affected
firefox86 --- affected

People

(Reporter: csasca, Unassigned, NeedInfo)

References

Details

Affected versions

  • Firefox 85.0b4
  • Firefox 86.0a1

Affected platforms

  • macOS 10.15.7
  • Ubuntu 18.04
  • Windows 7 & 10

Steps to reproduce

  1. Launch Firefox
  2. Access for example Wikipedia
  3. Use CTRL+Mouse scroll on the page

Expected result

  • Zoom changes are visible above 200% for every zoom values

Actual result

  • Zoom changes are visible at random values above 200%

Regression range

  • Implementation of Bug 1681213, makes it more noticeable. I will see for a regression for this

Additional notes

  • The issue can be seen in the following attachment

Suggested severity

  • S3

Moving to Toolbars & Customization (*) as this is an issue with reflowing zoom, and based on comment 0 a possible regression from bug 1681213.

(*) Would it perhaps be helpful to create a separate component for isssues with reflowing zoom?

Component: Panning and Zooming → Toolbars and Customization
Product: Core → Firefox

This does not appear to be a regression from bug 1681213. I can reproduce this in earlier versions when going from the zoom levels 270% --> 280% --> 290% --> 300%. When going from 280% to 290%, the page does not update visually. This issue goes at least as far back as Firefox 28.

What bug 1681213 did is increase the maximum zoom level from 300% to 500%, and the underlying issue occurs more frequently at intervals between 300% and 500%, so it makes this more noticeable.

I do not understand the steps. I tried to reproduce on macOS and Windows. Scrolling with cmd + trackpad changes the zoom display. This happens on a nightly from Dec. 1st too (well before bug 1681213 landed). On macOS, I see nothing like what is reported here. On Windows, I can see that for some mousewheel scrolls, the zoom indicator updates but the viewport does not change (which seems to be the opposite of what this bug is complaining about?). This happens e.g. between 280% and 290% for me (on a 150% scaling Windows display).

It doesn't help that the video does not show what the user is doing, and doesn't show expected results. I also do not understand how/why you determined that bug 1681213 was at fault (there's no regression window here and you haven't marked a regression relationship, so I guess you didn't actually try to use mozregression?), as the previous maximum was 300%, not 200%, so I don't see what it has to do with this bug. I tried a nightly from Dec. 1st on Windows, and that shows the issue from the previous paragraph.

Please clarify and verify the regression window (if any). Perhaps bug 1681213 made the existing problem more noticeable, or something?

Flags: needinfo?(catalin.sasca)
Flags: needinfo?(catalin.sasca)
Summary: Page zoom changes are visible at random values above 200% while using mouse scroll only → When using meta/ctrl + mousewheel to zoom, the zoom indicator updates without actually reflowing/zooming the page
See Also: → 1655223

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

When going from 280% to 290%, the page does not update visually.

If I had to guess, that's because 60 / 2.8 = 21.4 and 60 / 2.9 = 20.6, both of which round to 21. 60 is the number of app units per device pixel at unit zoom, and we always render the page at an integer number of app units per device pixel.

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

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

When going from 280% to 290%, the page does not update visually.

If I had to guess, that's because 60 / 2.8 = 21.4 and 60 / 2.9 = 20.6, both of which round to 21. 60 is the number of app units per device pixel at unit zoom, and we always render the page at an integer number of app units per device pixel.

If only my guessing was as well-educated as yours - I just spent some quality time with MSVS to find exactly this. The numbers were a little different for me because my device scale factor is 1.5dppi, but the same issue applies: somewhere in layout code in the child process, we realize we can't meaningfully change this number. This is well after we've processed all of this in the parent, of course.

So what's a good solution here? The "meaningful" values will differ based on device scale factors as well. Re-implementing the same computations in the parent process in JS is a recipe for disaster (e.g. bug 1474783), so I'd really prefer to avoid that. Is there a good way of exposing "meaningful" zoom levels from layout code, perhaps on the browsing context (which is what we use to zoom right now)?

Flags: needinfo?(botond)

(In reply to :Gijs (he/him) from comment #5)

Is there a good way of exposing "meaningful" zoom levels from layout code, perhaps on the browsing context (which is what we use to zoom right now)?

In principle we could do either of the following:

  1. Expose a list of all percentage zoom levels (within a min/max range) that have distinct corresponding AppUnitPerDevPixel scales. Note, at lower zoom levels, such as between 100% and 110%, this will have entries that are closer to each other than every 10%, so the consumer of this may need to do further pruning of it.
  2. Send a list of desired zoom levels (e.g. ones at 10% intervals), and have layout code return a subset that have distinct corresponding AppUnitPerDevPixel scales (so e.g. it would prune out 290%).

It depends on what makes more sense in terms of how the API would be used.

Flags: needinfo?(botond)

Ok, so as Botond said in Comment 2, it is happening before the implementation of Bug 1681213. And now that it was implemented, it was easier to notice this issue. I will see then for a regression here, if there is one.

Thanks Gijs!

Has Regression Range: --- → no
Has STR: --- → yes
QA Whiteboard: [qa-regression-triage]

(In reply to Botond Ballo [:botond] [away until Jan 4] from comment #6)

(In reply to :Gijs (he/him) from comment #5)

Is there a good way of exposing "meaningful" zoom levels from layout code, perhaps on the browsing context (which is what we use to zoom right now)?

In principle we could do either of the following:

  1. Expose a list of all percentage zoom levels (within a min/max range) that have distinct corresponding AppUnitPerDevPixel scales. Note, at lower zoom levels, such as between 100% and 110%, this will have entries that are closer to each other than every 10%, so the consumer of this may need to do further pruning of it.
  2. Send a list of desired zoom levels (e.g. ones at 10% intervals), and have layout code return a subset that have distinct corresponding AppUnitPerDevPixel scales (so e.g. it would prune out 290%).

It depends on what makes more sense in terms of how the API would be used.

I'm not sure. Right now the code uses frontend-side logic to take the current zoom level. Then for wheel/scroll zooming, we add/subtract 0.1. For the other UI, we use a list of levels we have in a pref (so is user-customizable...) and pick the next/previous item in that list, and then assign to .fullZoom on the browser custom element, which talks to the browsing context.

I guess this means the simplest solution would be (2). This means we can do the filtering for the shortcuts and non-wheel/scroll UI in the same place where we find the next item in the list. For the wheel/scroll zooming, we'd need to adjust the implementation so we generate a list and then use similar logic, instead of "just" incrementing/decrementing by 0.1 .

What would be the right place to add such an API, and when should frontend discard the list and refresh it, e.g. if the OS DPI changes or the window is moved between screens? (Right now we cache the list based on the pref.)

Flags: needinfo?(botond)

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

This issue goes at least as far back as Firefox 28.

I've removed the flag for regressionwindow-wanted since Botond said this issue has been around for a long while.

Regards, Flor.

QA Whiteboard: [qa-regression-triage]
Severity: -- → S4
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.