Closed Bug 1818967 Opened 2 years ago Closed 2 years ago

https://bugzilla.mozilla.org/ is broken after pinch zooming then resetting

Categories

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

Firefox 112
x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
113 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox111 --- wontfix
firefox112 --- wontfix
firefox113 --- verified
firefox114 --- verified

People

(Reporter: gregp, Assigned: botond)

References

(Regression, )

Details

(Keywords: regression)

Attachments

(4 files)

Attached video broken.webm (deleted) —

Steps to reproduce:

  1. Navigate to https://bugzilla.mozilla.org/buglist.cgi?bug_status=__open__
  2. Scroll down a bit
  3. Pinch zoom the webpage
  4. Reset pinch zoom with Ctrl+0 keyboard shortcut

Actual results:
Page becomes blank

Expected result:
Page doesn't become blank

Set release status flags based on info from the regressing bug 1519339

:botond, since you are the author of the regressor, bug 1519339, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Flags: needinfo?(botond)

Sorry for the delay in triaging this. I'm able to reproduce this even without scrolling in some cases, so this does appear to break the pinch zoom reset feature quite severely.

Severity: -- → S3
Flags: needinfo?(botond)
Priority: -- → P2

Set release status flags based on info from the regressing bug 1519339

Whoops, didn't mean to change status flags.

This time, the blank rendering is not the result of a scroll offset that's temporarily stuck out-of-bounds, it's "just" checkerboarding (another way to think about it is a displayport rect that's temporarily stuck in a position that's away from the viewport).

I can actually reproduce this in builds that predate the landing of bug 1519339 as well. I think it's a pre-existing issue.

(Sorry, I don't know why Bugzilla keeps pre-populating the page with stale flag changes...)

I ran mozregression and found bug 1745969 to be the regressor.

That said, I suspect the role of bug 1745969 is kind of incidental, i.e. it caused us to avoid an extra paint which was hiding the underlying problem by getting us out of the bad temporary state.

Regressed by: 1745969
No longer regressed by: 1519339
Assignee: nobody → botond

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

I ran mozregression and found bug 1745969 to be the regressor.

That said, I suspect the role of bug 1745969 is kind of incidental

Indeed, if I test with a page whose scrollable height is not fractional, for example https://theres-waldo.github.io/mozilla-testcases/grid.html?rows=100, then the bug is reproducible all the way back to the introduction of the ability to reset the pinch-zoom level with Ctrl-0 in bug 1660054.

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

That said, I suspect the role of bug 1745969 is kind of incidental, i.e. it caused us to avoid an extra paint which was hiding the underlying problem by getting us out of the bad temporary state.

Out of curiosity, I did investigate a bit the reason why bug 1745969 affects the behaviour on pages with a fractional scrollable height.

It turns out that bug 1745969 does not affect the total number of paints that happen -- in both cases, we get two paints after the Ctrl+0, and in both cases the first paint has an incorrect displayport that checkerboards.

The difference is that, before bug 1745969, on pages with a fractional scrollable height, when the first paint is processed in APZ, this condition is true (the stored scrollable rect has a fractional height, e.g. 20064.3008, and the incoming scrollable rect from the main thread does not, e.g. 20064), triggering a repaint request. The processing of the repaint request on the main-thread corrects the bad displayport, leading to the second paint having a correct displayport.

With bug 1745969, we still get a second paint (I haven't checked what triggers it, but clearly it must have a trigger other than just the APZ repaint request), but since we haven't processed an APZ repaint request the second paint is still stuck with the bad displayport.

Anyways, all this is mostly an aside to better understand the behaviour of bug 1745969. The proper fix in this bug is to address the underlying causes of the bad displayport.

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

The proper fix in this bug is to address the underlying causes of the bad displayport.

The underlying cause is that, when translating the displayport margins from coordinates relative to the visual viewport to coordinates relative to the layout viewport here, we use an incorrect scale. mScale there is the scale at the time the display port margins were set, but the resulting ScreenMargin is interpreted in the context of the scale at which we're about to paint. In this bug, mScale is the scale before the Ctrl+0 (e.g. 10x) but the resulting ScreenMargin is the scale after the Ctrl+0 (1x), leading to the displayport margins being off by a potentially very large amount.

Note that the margins themselves do not have this problem. We want the magnitude of the margins to be constant in screen pixels, i.e. a constant fraction of width/height around the visual viewport. But the offset between the visual and layout viewports by which we'd like to adjust the margins is constant in CSS pixels.

mScale is the zoom at the time the displayport margins were set, but
the resulting ScreenMargin will be interpreted in the context of the
scale at which we're about to paint, which may be different.

Note that mMargins themselves are not affected by these two scales
being potentially different, since we want the magnitude of the
margins to the constant in Screen pixels (i.e. a constant fraction
of the viewport size).

Attached file Bug 1818967 - Add a mochitest. r=hiro (deleted) —
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/051938067379 Use the correct scale in DisplayPortMargins::GetRelativeToLayoutViewport(). r=hiro https://hg.mozilla.org/integration/autoland/rev/0f8e733ac2ad Remove DisplayPortMargins::mScale which is no longer used. r=hiro https://hg.mozilla.org/integration/autoland/rev/cb95af4b230e Add a mochitest. r=hiro

The patch landed in nightly and beta is affected.
:botond, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(botond)

Given that the issue has been around for a while, my preference is to let the fix ride the trains.

Flags: needinfo?(botond)
Flags: qe-verify+

Reproducible on a 2023-02-26 Nightly build on Ubuntu 22.
Verified as fixed on Firefox 113.0b6(build ID: 20230420180037) and Nightly 114.0a1(build ID: 20230420212414) on Ubuntu 22, Windows 10, macOS 12.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: