1914149 - Site-specific browser zoom may be applied after first paint on bfcached-navigation

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[rchristian]

Attachment: Video showcasing the effect

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:130.0) Gecko/20100101 Firefox/130.0

Steps to reproduce:

I'm using https://docs.astro.build for this example -- I don't think this is an issue limited to their site at all, but it's a multi-page app with pretty small file sizes and so shows off what I'm guessing is some sort of race condition between restoring the zoom level & first paint.

1. Navigate to https://docs.astro.build/en/getting-started/
2. Set browser zoom to anything beyond 100% -- it doesn't matter if the preferred value is 100% or something else. This effect occurs whenever the zoom is set to anything but 100%.
3. Using the left side bar, click some links to switch pages
4. It might take a few clicks, but you'll see that the page sometimes has the zoom level applied after first paint, resulting in a really bad "pop" effect

It doesn't always occur, hence the guess that there is some race condition in applying the zoom before the page is restored from cache perhaps? Not sure.

Actual results:

Zoom is applied post page load, resulting in a flash of content being resized.

Expected results:

Zoom level should be applied before anything is drawn to the screen, ensuring this doesn't occur.

Comment 1

[Alice0775 White]

I cxan reproduce the issue on Nightly131.0a1 Windows11.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=331879c52f189b25568dadb7d93320a331eb02fb&tochange=40dc04fa64a26e3b4148153af1470ce55a2f184d

Suspect:Bug 1846141

Comment 2

[Alice0775 White]

Links of Full table of contents on https://html.spec.whatwg.org/multipage/ is also affected. And got same regression window.

Comment 3

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

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

For more information, please visit BugBot documentation.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yeah that seems somewhat expected. So... with site-specific zoom we kinda rely on the front-end to set the zoom. But that is async so it may cause issues like this.

I guess we could make the check in here more subtle... Nika, do you know if we happen to have an easy way of telling whether the navigation is cross-site from CanonicalBrowsingContext::ReplacedBy()?

Comment 5

[rchristian]

I'm not sure "Site-specific browser zoom" is correct, unless I'm misunderstanding or the distinction doesn't matter much.

I have my Firefox-wide zoom setting (Settings -> General -> Zoom) set to 110%, and that's how I came across the issue. I think any level of zoom besides 100% can trigger this, regardless of the setting being site-specific or browser-wide.

Sorry, my issue was a bit unclear.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Firefox has two modes of zoom, controlled by browser.zoom.siteSpecific:

• Site-specific zoom (the default), which tracks zoom on a per-site basis.
• Per-tab zoom, which doesn't have that issue.

That's the "site-specific" bit here.

Comment 7

[rchristian]

Ah, makes sense!

Thanks for answering that, I'm (obviously) quite unfamiliar.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

I guess we could make the check in here more subtle... Nika, do you know if we happen to have an easy way of telling whether the navigation is cross-site from CanonicalBrowsingContext::ReplacedBy()?

I'm not sure if we can, but perhaps we could do something a bit less clever which might reduce the frequency of flashing here.

There are 2 major situations where we end up wanting to call ReplacedBy to set up a new browser - the first is when we're restoring a cached page from the BFCache which has already been loaded (and could already have a zoom level set on it). This is the one which led to the check being restricted based on my understanding of bug 1846141. The second case is when we're loading a fresh new BrowsingContext, rather than restoring one which has previously loaded content, which I believe is the main case causing an issue here (due to the fresh document loading very quickly).

It might make sense to tweak the check to, instead of (or in addition to) checking the siteSpecific pref, also check whether this BC is EverAttached, and always propagate the zoom level if it is a brand new BrowsingContext. This won't fix flashing if you had navigated through a site, changed the zoom level, then navigated back to the previously BFCache'd page, but it might help in this edge-case.

The check might be something like this:

  // When using site-specific zoom, we let the frontend manage the zoom level
  // of BFCache'd contexts. Overriding those zoom levels can cause weirdness
  // like bug 1846141. We always copy to new contexts to avoid bug 1914149.
  if (!StaticPrefs::browser_zoom_siteSpecific() || !aNewContext->EverAttached()) {
    txn.SetFullZoom(GetFullZoom());
    txn.SetTextZoom(GetTextZoom());
  }

Comment 9

[Nika Layzell [:nika] (ni? for response)]

I should also note that I expect long-term the best option here is perhaps to move the site-specific zoom handling into platform, such that we can determine the correct zoom levels internally and ensure that the new tab doesn't paint until the zoom level is correct.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1914149 - Propagate zoom to new browsing contexts unconditionally. r=nika!

Your patch looks good to me. Not sure if there's a non-racy way of
testing the internal full-zoom value here...

But in general, assuming consistent zoom lacking other sources of
information seems like the right thing to do.

I agree that eventually we want to probably move the site-specific zoom
impl into Gecko (if only because Android is also going to want that, see
bug 1547181, where I added a comment).

Co-authored-by: Nika Layzell <[e-mail]>

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Yeah, I think that looks good to me.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0570ac001c71
Propagate zoom to new browsing contexts unconditionally. r=nika

Comment 13

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/0570ac001c71

Comment 14

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

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

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

For more information, please visit BugBot documentation.

Comment 15

[Monica Chiorean]

I was able to reproduce the issue on Win11x64 using FF build 131.0a1.
Verified as fixed on Win11x64/Ubuntu 24.04 using FF builds 133.0a1 and 132.0b2.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Do we want this on ESR128?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9421033 [details]
Bug 1914149 - Propagate zoom to new browsing contexts unconditionally. r=nika!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Easy fix.
• User impact if declined: comment 0
• Fix Landed on Version: 133
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial fix.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9421033 [details]
Bug 1914149 - Propagate zoom to new browsing contexts unconditionally. r=nika!

Approved for 128.4esr.

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/88987a202bb2

Comment 20

[Monica Chiorean]

Verified as fixed on Win11x64/Ubuntu 24.04 using FF build 128.4.0esr(20241021193311).