1612068 - Delay-loaded OOP iframes do not inherit correct zoom value

Status: VERIFIED FIXED

(Toolkit :: General, defect, P3)

Description

[Morgan Reschenberg [:morgan]]

STR:

1. In about:config, ensure fission.autostart is set to true. If it is not, set to true and restart the browser to ensure the pref takes affect.

2. In about:preferences, scroll to "Language and Appearance" and set a default zoom value from the dropdown.

3. Paste the following test case in the URL bar: data:text/html,hello <iframe src=""></iframe> world <script>setTimeout(function() { document.querySelector("iframe").src = "https://nvaccess.org/"; }, 3000);</script>

Expected: The iframe loaded after the delay reflects the default zoom value of the whole page (value set in step 2)

Actual: The iframe loaded after the delay remains zoomed at 100% despite the value set in step 2.

Comment 1

[:Gijs (he/him)]

We talked about this today, and I think the plan for fixing this is to sync the zoom value via "synced" values on browsingcontexts. As an aside, we may want to nuke the docshell zoom property from orbit...

Jamie, did you find out more here?

(Also setting ::General component priority and flagging this for fission triage.)

Comment 2

[James Teh [:Jamie]]

Morgan and I spoke with the Fission team about this. Here's what we think needs to be done here:

1. Add FullZoom and TextZoom as synced properties in WindowContext:
   https://searchfox.org/mozilla-central/rev/ad6234148892bc29bf08d736604ac71925138040/docshell/base/WindowContext.h#15
   We use WindowContext instead of BrowsingContext because BrowsingContext doesn't change when the document is navigated, where WindowContext does. Practically, it probably doesn't matter because we're going to traverse to a DocShell anyway, but it's probably more correct this way.
2. Add FullZoom and TextZoom to the WindowContext webidl so we can set it from JS:
   https://searchfox.org/mozilla-central/rev/ad6234148892bc29bf08d736604ac71925138040/dom/chrome-webidl/WindowGlobalActors.webidl#13
   Setting it from JS will then call SetFull/PageZoom, which will sync it across processes.
3. Change the browser zoom code to set full/textZoom on the WindowContext for the tab document:
   https://searchfox.org/mozilla-central/rev/ad6234148892bc29bf08d736604ac71925138040/toolkit/content/widgets/browser-custom-element.js#806
4. Implement WindowContext::DidSetFullZoom and WindowContext::DidSetTextZoom:
   1. Get the browsing context using GetBrowsingContext().
   2. Walk the BrowsingContext tree using BrowsingContext::PreOrderWalk().
   3. For each BrowsingContext, if it has no DocShell, skip it. Otherwise, get the DocShell, get its nsIContentViewer using nsDocShell::GetContentViewer(), and call nsIContentViewer::SetFullZoom/nsIContentViewer::SetTextZoom.

Comment 3

[Morgan Reschenberg [:morgan]]

(In reply to James Teh [:Jamie] from comment #2)

Morgan and I spoke with the Fission team about this. Here's what we think needs to be done here:

1. Add FullZoom and TextZoom as synced properties in WindowContext:
   https://searchfox.org/mozilla-central/rev/ad6234148892bc29bf08d736604ac71925138040/docshell/base/WindowContext.h#15
   We use WindowContext instead of BrowsingContext because BrowsingContext doesn't change when the document is navigated, where WindowContext does. Practically, it probably doesn't matter because we're going to traverse to a DocShell anyway, but it's probably more correct this way.
2. Add FullZoom and TextZoom to the WindowContext webidl so we can set it from JS:
   https://searchfox.org/mozilla-central/rev/ad6234148892bc29bf08d736604ac71925138040/dom/chrome-webidl/WindowGlobalActors.webidl#13
   Setting it from JS will then call SetFull/PageZoom, which will sync it across processes.
3. Change the browser zoom code to set full/textZoom on the WindowContext for the tab document:
   https://searchfox.org/mozilla-central/rev/ad6234148892bc29bf08d736604ac71925138040/toolkit/content/widgets/browser-custom-element.js#806
4. Implement WindowContext::DidSetFullZoom and WindowContext::DidSetTextZoom:
   1. Get the browsing context using GetBrowsingContext().
   2. Walk the BrowsingContext tree using BrowsingContext::PreOrderWalk().
   3. For each BrowsingContext, if it has no DocShell, skip it. Otherwise, get the DocShell, get its nsIContentViewer using nsDocShell::GetContentViewer(), and call nsIContentViewer::SetFullZoom/nsIContentViewer::SetTextZoom.

if I were hypothetically doing this between meetings, how would we do step 3; I don't see an easy getter or example for working with window contexts in the js

Comment 4

[:Gijs (he/him)]

(In reply to Morgan Reschenberg [:morgan] from comment #3)

3. Change the browser zoom code to set full/textZoom on the WindowContext for the tab document:
   https://searchfox.org/mozilla-central/rev/ad6234148892bc29bf08d736604ac71925138040/toolkit/content/widgets/browser-custom-element.js#806

if I were hypothetically doing this between meetings, how would we do step 3; I don't see an easy getter or example for working with window contexts in the js

For a given browser element, you can ask for browser.browsingContext.currentWindowGlobal, which gets you a WindowGlobalParent, which inherits from WindowContext, which I think would work here?

Comment 6

[James Teh [:Jamie]]

I forgot one critical piece:
When a new content document loads, we need to get the top level WindowContext and apply its fullZoom/pageZoom property. (For a delay-loaded OOP iframe, we're not going to get a DidSetFull/PageZoom callback, since it's already been set.) I'm not quite sure where we'll do that, though. Maybe somewhere near here?
https://searchfox.org/mozilla-central/rev/2e355fa82aaa87e8424a9927c8136be184eeb6c7/layout/base/nsDocumentViewer.cpp#807
To get to the top WindowContext from there, I guess we call nsDocumentViewer::GetContainer to get the DocShell, nsDocShell::GetBrowsingContext, BrowsingContext::Top, BrowsingContext::GetCurrentWindowContext?

Comment 7

[James Teh [:Jamie]]

Carrying Fission milestone m5 from duplicate bug 1603282, which was already triaged by cpeterson.

Comment 8

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1612068: Add Text/FullZoom as synced properties on WindowContext.

Comment 9

[Chris Peterson [:cpeterson]]

Morgan, do you have any updates on this zoom patch? Looks like you made some change to your patch in Phabricator back on February 11, but didn't ask Nika for re-review yet.

Deferring to Fission Nightly (M6)

unlinking session restore zoom bug 1566559 because Nika says it will have a different fix.

Comment 10

[Morgan Reschenberg [:morgan]]

Hello! I wasn't working on this because it looked connected to the session restore thing, but if it isn't I can carve out some time over the next week to take a look at it again :)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

I told sean I'd poke a bit as this a bit.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1612068 - Remove nsIContentViewer.effectiveTextZoom. r=nika

It's unused.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1612068 - Rename and simplify nsIContentViewer.deviceFullZoom. r=nika

It's only needed for a single test, and definitely not used in print
preview.

Depends on D71967

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1612068 - Move zoom from the content viewer to the browsing context. r=nika

We need it to live in BrowsingContext instead of WindowContext, because
we need to preserve the zoom level across same-origin navigation.

It'd be nice if it only lived in the top BC, but that's not possible at
the moment because a lot of tests rely on zooming only iframes. Some of
them can be adjusted for scaling the top instead, but not sure it's
worth it's worth fixing them and moving the zoom to be top-only, as it'd
be a bunch of effort, and the complexity and overhead of propagating the
zoom is not so big.

The print-preview-specific code in nsContentViewer is from before we did
the document cloning setup, and it seems useless. I've tested print
preview scaling before and after my patch and both behave the same.

The rest is just various test changes to use the SpecialPowers APIs or
BrowsingContext as needed instead of directly poking at the content
viewer.

I named the pres context hook RecomputeBrowsingContextDependentData, as
more stuff should move there like overrideDPPX and other media emulation
shenanigans.

I also have some ideas to simplify or even remove ZoomChild and such,
but that's followup work.

Depends on D71968

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1612068 - Fix top-level document zoom when restoring from bfcache. r=nika

This still doesn't really fix subframes, but those are broken in trunk
already.

Fixing subframes could be done by making the zoom Top()-only, or by
propagating to browsing contexts in the bfcache as well from DidSet(..).

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11caddf760fd
Remove nsIContentViewer.effectiveTextZoom. r=nika
https://hg.mozilla.org/integration/autoland/rev/af6964d3761b
Rename and simplify nsIContentViewer.deviceFullZoom. r=nika

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1612068 - Fix an RDM test utility function. r=bradwerth

requestAnimationFrame runs before reflow, so you actually need too to
guarantee that a reflow has happened.

I was getting intermittents in devtools/client/responsive/test/browser/browser_window_sizing.js
without this with the rest of the patches in this bug.

Also while at it fix the comparison to be less prone to floating point
errors, like a lot of the other zoom code in browser/.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6468e0c30bb8
Fix an RDM test utility function. r=bradwerth

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4dcf0c18bbe
Appease eslint.

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/11caddf760fd
https://hg.mozilla.org/mozilla-central/rev/af6964d3761b
https://hg.mozilla.org/mozilla-central/rev/6468e0c30bb8
https://hg.mozilla.org/mozilla-central/rev/b4dcf0c18bbe

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a26615a65a8c
Move zoom from the content viewer to the browsing context. r=nika

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5db725324c27
Fix top-level document zoom when restoring from bfcache. r=nika

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1612068 - followup: Fix some bits which I forgot to update before landing.

Missed two tests and clang-formatting.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/660cf4863eab
followup: Fix some bits which I forgot to update before landing.

Comment 25

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a26615a65a8c
https://hg.mozilla.org/mozilla-central/rev/5db725324c27
https://hg.mozilla.org/mozilla-central/rev/660cf4863eab

Comment 27

[Anca Soncutean, Desktop QA]

I’ve set the qe+ as part of Beta 77 bug triage, but pref fission.autostart is unlocked only on the Nightly versions. Nevertheless, I confirm the issue as verified fixed with 78.0a1 (2020-05-08) on Windows 10 and Ubuntu 18.04.