Delay-loaded OOP iframes do not inherit correct zoom value
Categories
(Toolkit :: General, defect, P3)
Tracking
()
People
(Reporter: morgan, Assigned: emilio)
References
Details
(Keywords: access)
Attachments
(6 files, 1 obsolete file)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
STR:
-
In
about:config
, ensurefission.autostart
is set to true. If it is not, set to true and restart the browser to ensure the pref takes affect. -
In
about:preferences
, scroll to "Language and Appearance" and set a default zoom value from the dropdown. -
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•5 years ago
|
||
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•5 years ago
•
|
||
Morgan and I spoke with the Fission team about this. Here's what we think needs to be done here:
- 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. - 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. - 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 - Implement WindowContext::DidSetFullZoom and WindowContext::DidSetTextZoom:
- Get the browsing context using GetBrowsingContext().
- Walk the BrowsingContext tree using BrowsingContext::PreOrderWalk().
- 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.
Reporter | ||
Comment 3•5 years ago
|
||
(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:
- 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.- 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.- 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- Implement WindowContext::DidSetFullZoom and WindowContext::DidSetTextZoom:
- Get the browsing context using GetBrowsingContext().
- Walk the BrowsingContext tree using BrowsingContext::PreOrderWalk().
- 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•5 years ago
|
||
(In reply to Morgan Reschenberg [:morgan] from comment #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#806if 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?
Reporter | ||
Updated•5 years ago
|
Comment 6•5 years ago
|
||
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•5 years ago
|
||
Carrying Fission milestone m5 from duplicate bug 1603282, which was already triaged by cpeterson.
Reporter | ||
Comment 8•5 years ago
|
||
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Comment 9•5 years ago
|
||
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.
Reporter | ||
Comment 10•5 years ago
|
||
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 :)
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
I told sean I'd poke a bit as this a bit.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
It's unused.
Assignee | ||
Comment 13•5 years ago
|
||
It's only needed for a single test, and definitely not used in print
preview.
Depends on D71967
Assignee | ||
Comment 14•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 15•5 years ago
|
||
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(..).
Assignee | ||
Updated•5 years ago
|
Comment 16•5 years ago
|
||
Pushed by ealvarez@mozilla.com: 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
Assignee | ||
Comment 17•5 years ago
|
||
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•5 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6468e0c30bb8 Fix an RDM test utility function. r=bradwerth
Comment 19•5 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/b4dcf0c18bbe Appease eslint.
Comment 20•5 years ago
|
||
bugherder |
Comment 21•5 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a26615a65a8c Move zoom from the content viewer to the browsing context. r=nika
Comment 22•5 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5db725324c27 Fix top-level document zoom when restoring from bfcache. r=nika
Assignee | ||
Comment 23•5 years ago
|
||
Missed two tests and clang-formatting.
Comment 24•5 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/660cf4863eab followup: Fix some bits which I forgot to update before landing.
Comment 25•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 27•4 years ago
|
||
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.
Description
•