Closed Bug 1590485 Opened 5 years ago Closed 4 years ago

global/default zoom

Categories

(Firefox :: Disability Access, enhancement, P2)

Product:
Firefox
Component:
Disability Access
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 73
Tracking Flags:
Tracking Status
relnote-firefox --- 73+
firefox73 --- fixed

People

(Reporter: morgan, Assigned: morgan)

References

(Depends on 2 open bugs)

Details

Attachments

(1 file)

Bug 1590485: Add UI/UX and global zoom functionality.
47 bytes, text/x-phabricator-request
Details | Review
No description provided.
Assignee: nobody → mreschenberg
Depends on: 1588895
Component: Disability Access APIs → Disability Access
Priority: -- → P2
Product: Core → Firefox

Did you see https://bugzilla.mozilla.org/show_bug.cgi?id=332275 and the discussion there, in particular Dão suggesting in bug 332275 comment 51 that the default should be implemented at a lower level than browser-fullZoom.js ?

Flags: needinfo?(mreschenberg)

(In reply to :Gijs (he/him) from comment #5)

Did you see https://bugzilla.mozilla.org/show_bug.cgi?id=332275 and the discussion there, in particular Dão suggesting in bug 332275 comment 51 that the default should be implemented at a lower level than browser-fullZoom.js ?

Yep, thanks!

Flags: needinfo?(mreschenberg)
See Also: → 332275

Gijs, it seems we have a global content pref (cps2.getGlobal) for zoom, but nothing ever sets it. 😕 It also seems there's no UI for explicitly disabling site specific zoom. If you disable site specific zoom via about:config, changing the zoom level doesn't get saved anywhere; it just vanishes when you close the tab.

Given this, do you think we should:

  1. Have the Preferences UI adjust the global content pref? Nothing else in preferences UI seems to mess with content prefs, so I'm not sure if this is acceptable.
  2. Stop using the global content pref at all and switch to using a browser (about:config) pref.
  3. Set the global content pref based on a new browser pref. This is what the WIP patch currently does, but I'm not a fan of having a seemingly pointless intermediary preference.
  4. Something else?

As an aside, I can't seem to find anything else in the code base that uses global content prefs!

Flags: needinfo?(gijskruitbosch+bugs)

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

Gijs, it seems we have a global content pref (cps2.getGlobal) for zoom, but nothing ever sets it. 😕 It also seems there's no UI for explicitly disabling site specific zoom. If you disable site specific zoom via about:config, changing the zoom level doesn't get saved anywhere; it just vanishes when you close the tab.

My understanding is that if you disable site-specific zoom, we do tab-specific zoom. This is different from a single global zoom, in that if you load mozilla.org in a tab, zoom to 150%, and then load example.com in the same tab, example.com will also be zoomed to 150% - but other tabs are not affected.

I don't know if we think that behaviour is worth preserving separately from having a single, global default zoom that gets altered if you disable site-specific zoom. I can see arguments either way. Worth discussing with UX. For simplicity's sake, I'd lean towards having a maximum of 2 different operating modes for zoom, as things are messy enough without introducing a third (see below).

Given this, do you think we should:

  1. Have the Preferences UI adjust the global content pref? Nothing else in preferences UI seems to mess with content prefs, so I'm not sure if this is acceptable.

On the general question of whether we can do this in the main preferences UI, I think the answer is "yes" - we do similar things for other options related to permissions (where there is a global option and a list of per-site permissions) so I think that's fine. In fact, I'm not super sure why permissions are stored separately from content prefs as they're effectively the same data structure in the abstract sense... but that's a different issue.

  1. Stop using the global content pref at all and switch to using a browser (about:config) pref.

I think it's weird all of this is browser-specific, tbh. Ultimately, the zoom code writes to browserElement.fullZoom / browserElement.textZoom, which the browser custom element then dispatches to the child ( https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/toolkit/content/widgets/browser-custom-element.js#810-813 ) and then just writes into a ContentViewer setter ( https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/docshell/base/nsIContentViewer.idl#242-249 ), which in turn changes the zoom level for all the pres shells ( https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/layout/base/nsDocumentViewer.cpp#2837,2839-2846 ).

I think it'd be preferable for docshell/layout to do this itself, for several reasons:

  • I don't think all the layers of abstraction are helpful - there's too much indirection compared to what's happening
  • the browser full zoom code doesn't apply zoom until it gets an onLocationChange notification, plus has IPC overhead for that as it needs to talk to the content process - meaning we'll end up potentially doing layout twice (which will be visible to the user, too).
  • with fission, iterating over the pres shells doesn't work with cross-origin iframes anyway. Using browsing contexts, the nested frames can themselves know the origin of the toplevel frame and thus ask for the appropriate zoom level themselves.
  • the onlocationchange handling in frontend is a perf drain, and moving some of this into core C++ would help with that.

I suspect the "right" way to do this is for layout code to be aware of this stuff in the first place and for toplevel contentviewers that are about to load a page (which they will know long before onLocationChange) to proactively look up the zoom level and set it once it hears back and a presshell is created (generally, hopefully it'll hear back in time). But equally, that might require implementing the CPS in C++, I dunno when we can run JS in this process, etc. etc.

Anyway, obviously the perfect should not be the enemy of the good here, so I don't think all of that should necessarily block moving forward here (though if performance is too terrible, it might become an issue...). It does mean that I think a separate pref, outside of the CPS, moves us further away from where we want to be.

  1. Set the global content pref based on a new browser pref. This is what the WIP patch currently does, but I'm not a fan of having a seemingly pointless intermediary preference.

Yeah, I'm not a fan either I'm afraid.

  1. Something else?

No, though there are a few gotchas here:

  • we should check carefully on whether this impacts on how many notifications frontend code gets about zoom changes. If in this setup, with no per-site zoom settings, we have a default zoom of X, and for every page load we notify for a zoom "change" from 1 to X, that would be unhelpful and a perf drain.
  • we need a decision as to whether/how we show and control this in other zoom UI (the in-locationbar zoom indicator, which doesn't show up at default / 100% zoom - should that show up permanently if you change the default? Probably not? What zoom level do we show there and in the zoom controls in the hamburger menu and the custom zoom controls? Do they modify the default zoom? Or only if siteSpecific is turned off? Or always origin/tab-specific?)
  • the multiplicativeness of this thing could be messy (cf bug 414636) - if the default is changed to 300%, do we still allow zooming an additional 300% on top of that? That seems... odd.
  • earlier comments in bug 332275 suggests that only setting the global pref doesn't affect extant tabs, so we'll probably need to notify for a change to the default some way so that extant tabs are affected. If so, we'll need to be smart about how to avoid the browser hanging while we're laying out 100 tabs with a different zoom level...
  • it looks like the content pref service has some notion of load contexts / private browsing, so we probably want to make sure whatever we do works correctly with private browsing... I think the global default should also affect PB, and the preferences UI works the same way irrespective of whether it's opened inside/outside PB, so should still modify the "main" pref (which also affects PB) - that is, I don't think we should support a "special" additional global default for PB, because I don't think it'll make sense to users nor do we have a good way of providing UI for it (and having it persist). But worth bearing in mind when writing the code, because we cannot assume the load context in which the preferences UI loads is non-PB.

We should make sure we make clear decisions on how to address those challenges and/or bear them in mind when implementing.

As an aside, I can't seem to find anything else in the code base that uses global content prefs!

Mm, but as noted, they're pretty similar to permissions and we definitely have "global" permissions and use those, so I'm not too worried about this.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to :Gijs (he/him) from comment #8)

Gijs, it seems we have a global content pref (cps2.getGlobal) for zoom, but nothing ever sets it. 😕 It also seems there's no UI for explicitly disabling site specific zoom. If you disable site specific zoom via about:config, changing the zoom level doesn't get saved anywhere; it just vanishes when you close the tab.

My understanding is that if you disable site-specific zoom, we do tab-specific zoom.

That matches my observations too, though I haven't figured out how that actually gets done. It certainly doesn't seem to use the global content pref. I can't see anywhere that sets the global content pref, which makes me wonder why we currently support it at all.

I think it'd be preferable for docshell/layout to do this itself, for several reasons:
...
I suspect the "right" way to do this is for layout code to be aware of this stuff in the first place and for toplevel contentviewers that are about to load a page (which they will know long before onLocationChange) to proactively look up the zoom level and set it once it hears back and a presshell is created (generally, hopefully it'll hear back in time). But equally, that might require implementing the CPS in C++, I dunno when we can run JS in this process, etc. etc.

If this were a single static pref that gets applied globally without concern for site specific preferences, I'd agree without question. Unfortunately, the site specific stuff really complicates this. Also, I'm not sure we can even use the CPS out of the parent process? If that's true, fixing that is going to require some hefty IPC work.

  • the multiplicativeness of this thing could be messy (cf bug 414636) - if the default is changed to 300%, do we still allow zooming an additional 300% on top of that?

AFAIK, we aren't doing multiplicative here. We either apply site specific or, if none, apply default. That bug should perhaps be wontfix rather than duplicate.

(In reply to :Gijs (he/him) from comment #8)

No, though there are a few gotchas here:

  • we need a decision as to whether/how we show and control this in other zoom UI (the in-locationbar zoom indicator, which doesn't show up at default / 100% zoom - should that show up permanently if you change the default? Probably not? What zoom level do we show there and in the zoom controls in the hamburger menu and the custom zoom controls? Do they modify the default zoom? Or only if siteSpecific is turned off? Or always origin/tab-specific?)

I believe this was settled in the UI/UX bug

  • earlier comments in bug 332275 suggests that only setting the global pref doesn't affect extant tabs, so we'll probably need to notify for a change to the default some way so that extant tabs are affected. If so, we'll need to be smart about how to avoid the browser hanging while we're laying out 100 tabs with a different zoom level...

Right now, global zoom is saved, and then each tab is updated when it becomes the active tab if its previous zoom level differs from the current zoom level.

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

(In reply to :Gijs (he/him) from comment #8)

No, though there are a few gotchas here:

  • we need a decision as to whether/how we show and control this in other zoom UI (the in-locationbar zoom indicator, which doesn't show up at default / 100% zoom - should that show up permanently if you change the default? Probably not? What zoom level do we show there and in the zoom controls in the hamburger menu and the custom zoom controls? Do they modify the default zoom? Or only if siteSpecific is turned off? Or always origin/tab-specific?)

I believe this was settled in the UI/UX bug

Oh, thanks for pointing that out - I'd missed that!

  • earlier comments in bug 332275 suggests that only setting the global pref doesn't affect extant tabs, so we'll probably need to notify for a change to the default some way so that extant tabs are affected. If so, we'll need to be smart about how to avoid the browser hanging while we're laying out 100 tabs with a different zoom level...

Right now, global zoom is saved, and then each tab is updated when it becomes the active tab if its previous zoom level differs from the current zoom level.

That sounds great. I assume that, when siteSpecific is true (the default), and the site has a site-specific pref, nothing changes?

Florian, this change is running into browser_startup.js, for importing Sqlite.jsm . This is a consequence of the fact that all zoom data is stored in the content preference service, and that uses Sqlite.jsm internally. Stack at https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280373608&repo=try&lineNumber=5359:

[task 2019-12-09T22:01:50.854Z] 22:01:50     INFO - Stack trace:
[task 2019-12-09T22:01:50.855Z] 22:01:50     INFO - 0 _getConnection() ["resource://gre/modules/ContentPrefService2.jsm":1195:6]
[task 2019-12-09T22:01:50.856Z] 22:01:50     INFO - 1 get conn/this._connPromise<() ["resource://gre/modules/ContentPrefService2.jsm":145:26]
[task 2019-12-09T22:01:50.856Z] 22:01:50     INFO - 2 get conn() ["resource://gre/modules/ContentPrefService2.jsm":151:6]
[task 2019-12-09T22:01:50.857Z] 22:01:50     INFO - 3 CPS2__execStmts() ["resource://gre/modules/ContentPrefService2.jsm":909:15]
[task 2019-12-09T22:01:50.857Z] 22:01:50     INFO - 4 CPS2__get() ["resource://gre/modules/ContentPrefService2.jsm":255:9]
[task 2019-12-09T22:01:50.858Z] 22:01:50     INFO - 5 CPS2_getGlobal() ["resource://gre/modules/ContentPrefService2.jsm":233:9]
[task 2019-12-09T22:01:50.858Z] 22:01:50     INFO - 6 getGlobalValue/<() ["resource:///modules/ZoomUI.jsm":59:20]
[task 2019-12-09T22:01:50.859Z] 22:01:50     INFO - 7 getGlobalValue() ["resource:///modules/ZoomUI.jsm":43:11]
[task 2019-12-09T22:01:50.859Z] 22:01:50     INFO - 8 updateZoomUI() ["resource:///modules/ZoomUI.jsm":137:34]
[task 2019-12-09T22:01:50.859Z] 22:01:50     INFO - 9 fullZoomLocationChangeObserver() ["resource:///modules/ZoomUI.jsm":83:14]
[task 2019-12-09T22:01:50.860Z] 22:01:50     INFO - 10 FullZoom__notifyOnLocationChange/<() ["chrome://browser/content/browser-fullZoom.js":616:19]

This code is trying to check if the zoom for a given page is the same as the global default zoom, for which it needs to fetch the latter.

I'm a little unsure how to suggest we proceed, because we backed ourselves into a corner here.

  1. We normally hit sqlite already fetching the site-specific zoom, which is stored in the same place. We avoid doing so for about:blank, which is why we don't hit this in the automated test. That seems unrealistic; we don't normally use about:blank as the startup page. (Full disclosure, I reviewed this in bug 1448944, so have only myself to blame...)
  2. when running my local build with ./mach run, we import Sqlite.jsm first from the bookmarks star page action button, before we need it for the content pref service, presumably because it checks if about:home is a bookmarked page (stack: [a]). I can only assume this also happens before handling user events, as it's before (1). I expect we don't do this in the test because that code probably ignores about:blank somewhere, too...

So we could avoid the getGlobal call if the browser is empty, like the site-specific code does. Such a check would shut up the test but IME wouldn't actually improve anything in practice, because people don't use about:blank as a homepage, and also (2) causes us to load sqlite before content prefs do. Of course, we could fix (2) separately, but even after an early return there, in practice we'd then hit (1) for any non-about:blank loads to even get the site-specific zoom.

I don't think this test is making practical, real-world assurances about Sqlite.jsm usage. Can we move the restriction to a different startup phase? Or would you prefer we keep working around the test? Or do you have a different suggestion?

[a]

get resource://gre/modules/XPCOMUtils.jsm:129
    promiseDBConnection resource://gre/modules/PlacesUtils.jsm:1471
    fetchBookmarksByURL resource://gre/modules/Bookmarks.jsm:2531
    fetch resource://gre/modules/Bookmarks.jsm:1529
    fetch resource://gre/modules/Bookmarks.jsm:1567
    BUI_updateStarState chrome://browser/content/browser-places.js:1632
    BUI_onLocationChange chrome://browser/content/browser-places.js:1619
    onLocationChange chrome://browser/content/browser.js:5395
    callListeners chrome://browser/content/tabbrowser.js:830
    _callProgressListeners chrome://browser/content/tabbrowser.js:844
    _callProgressListeners chrome://browser/content/tabbrowser.js:5558
    onLocationChange chrome://browser/content/tabbrowser.js:5976
    _callProgressListeners resource://gre/modules/RemoteWebProgress.jsm:75
    onLocationChange resource://gre/modules/RemoteWebProgress.jsm:117
Flags: needinfo?(florian)

(In reply to :Gijs (he/him) from comment #12)

I don't think this test is making practical, real-world assurances about Sqlite.jsm usage. Can we move the restriction to a different startup phase?

Blacklisting it "before first paint" should be enough. Let's not spend time adding hacks to workaround the test :-).

Flags: needinfo?(florian)

I'm still running into some problems with global zoom in iframes w/ fission; right now, on Windows 10 x64 opt/QR opt and linux x64 debug/opt, the iframe tests fail with fission enabled (the iframes remain zoomed at 100% when global zoom is set to 67% and increased to 80%).
I can reproduce the issue locally on my windows machine, but don't have great intuition around where to go from here. It feels like a fission issue rather than a global zoom one, so I'm wondering if this is more in line with the work in 1558919? If so, might be worth removing fission tests, landing global zoom, and filing a follow-up to fission-proof it next year.

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

I'm wondering if this is more in line with the work in 1558919? If so, might be worth removing fission tests, landing global zoom, and filing a follow-up to fission-proof it next year.

I'd be fine with doing the fission thing for iframes in a follow-up, especially if it's already broken.

(In reply to :Gijs (he/him) from comment #15)

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

I'm wondering if this is more in line with the work in 1558919? If so, might be worth removing fission tests, landing global zoom, and filing a follow-up to fission-proof it next year.

I'd be fine with doing the fission thing for iframes in a follow-up, especially if it's already broken.

Yeah same. I messaged mconley to see if there were tests for that bug that aren't in the patch (maybe it fixed something different than what I'm doing here). If he has any idea about what's happening I'll update here.

Try run with aforementioned failures:
https://bugzilla.mozilla.org/show_bug.cgi?id=1590485#c14

Note the failures are on runs of the mochitests that have fission enabled, so regardless of the pref-enabling I've done in the test, they're running with fission. This can be reproduced locally with ./mach mochitest --enable-fission browser/base/content/test/zoom

The failing tests are all of the same variant:
We load a plain HTML page with a blank iframe <iframe src=""></iframe>, wait for the page load to finish, and then inject the iframe with src='example.org'. Once this load is finished, we check that the local zoom in the iframe matches the global zoom set/adjusted before the iframe load.

The zoom checking happens in a SpecialPowers call which contains an await on the condition iframe zoom == global zoom. This await is required because there is a delay between the global zoom adjusting and the local fullZoom/ZoomManager.zoom reflecting the updated value. The problem arises when that condition fails to resolve (become true), so a rejection happens, and the SpecialPowers call find an exception because of funny special powers things. Logging in the async function in the SpecialPowers call shows the zoom is always 1.00 in the iframe, not .67 or .8 as the cases require.

Blocks: 1603282
Blocks: 1603494
Pushed by mreschenberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8e371771008
Add UI/UX and global zoom functionality. r=fluent-reviewers,Gijs
Flags: needinfo?(mreschenberg)
Pushed by mreschenberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8040f57cb4a
Add UI/UX and global zoom functionality. r=fluent-reviewers,Gijs
Pushed by mreschenberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9be8f3a3de2
Add UI/UX and global zoom functionality. r=fluent-reviewers,Gijs
Depends on: 1605843

https://hg.mozilla.org/mozilla-central/rev/e9be8f3a3de2

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox73: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 73
Blocks: 1605999
See Also: 332275 →

If this change allows people to stop using extensions like Zoom Page WE, it's pretty significant -- might be worth a blog post or something like that.

I agree that we should relnote this. Morgan, can you please set the relnote-firefox? flag and fill out the form? Thanks!

relnote-firefox: --- → ?
Flags: needinfo?(mreschenberg)

Release Note Request (optional, but appreciated)
[Why is this notable]:
This feature allows users to set a default zoom level for all web content. Previously this was only possible through 3rd-party add-ons (see above!).

[Suggested wording]:
Firefox users will now be able to select a default zoom level for all web content, regardless of site! This option is available in about:preferences under "Language and Appearance" and can be scaled up or down from 100% as needed.

[Links (documentation, blog post, etc)]:

This bug! https://bugzilla.mozilla.org/show_bug.cgi?id=1590485
Possible blog post later once the accessibility blog is up and running

Flags: needinfo?(mreschenberg)

Added to the Beta73 relnotes.

relnote-firefox: ? → 73+
Depends on: 1608348
See Also: → 1286953

This wasn't really a metabug in the end. :)

Keywords: meta
Summary: [meta] global/default zoom → global/default zoom
Depends on: 1609880
Depends on: 1610122
Depends on: 1610250
Depends on: 1610323
Depends on: 1611903
Depends on: 1611954
Depends on: 1612068
Depends on: 1616090
Blocks: 1592963
See Also: → 1634221
Depends on: 1773342
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: