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

Status: VERIFIED FIXED

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

Description

[Gregory Pappas [:gregp]]

Attachment: broken.webm

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

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 2

[Dan Robertson (:dlrobertson)]

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.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 4

[Botond Ballo [:botond]]

Whoops, didn't mean to change status flags.

Comment 5

[Botond Ballo [:botond]]

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).

Comment 6

[Botond Ballo [:botond]]

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

Comment 7

[Botond Ballo [:botond]]

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

Comment 8

[Botond Ballo [:botond]]

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.

Comment 9

[Botond Ballo [: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.

Comment 10

[Botond Ballo [:botond]]

(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.

Comment 11

[Botond Ballo [:botond]]

(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.

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1818967 - Use the correct scale in DisplayPortMargins::GetRelativeToLayoutViewport(). r=hiro,tnikkel

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).

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1818967 - Remove DisplayPortMargins::mScale which is no longer used. r=hiro,tnikkel

Depends on D173577

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1818967 - Add a mochitest. r=hiro

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/051938067379
https://hg.mozilla.org/mozilla-central/rev/0f8e733ac2ad
https://hg.mozilla.org/mozilla-central/rev/cb95af4b230e

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 18

[Botond Ballo [:botond]]

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

Comment 19

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

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.