1617544 - With browser.zoom.siteSpecific=false, the tab zoom level is reset to the default zoom level when page is reloaded

Status: VERIFIED FIXED

(Firefox :: Disability Access, defect)

Description

[dw-dev]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:73.0) Gecko/20100101 Firefox/73.0

Steps to reproduce:

I am the developer of the Zoom Page WE extension. One of my users has reported a zoom problem when using Firefox 75. I have investigated and found that the problem is caused by Firefox, not by Zoom Page WE.

This problem happens with Firefox 75, but not with Firefox 73 or 74 or previous versions.

Using Firefox 75.0a1 (2020-02-23) (64-bit):

1. In about:config, set browser.zoom.siteSpecific to false.
2. In Tools > Options, set the Default Zoom level to 100%.
3. Load Wikipedia (or any web page) into a tab.
4. Set the page's zoom level to 150%.
5. Reload the page.
6. The result is that the page's zoom level is reset to the Default Zoom level.

Actual results:

The page's zoom level is reset to the Default Zoom level (this is not the expected behaviour).

Expected results:

The page's zoom level should have remained at 150% (as it does in all previous versions of Firefox).

Comment 1

[Gingerbread Man]

Only two bug reports with zoom in the summary and the tracking flag firefox75: fixed/verified: bug 1610960 and bug 1603494. The latter looks like it introduced this behavior. :morgan, please have a look.

Comment 2

[Morgan Reschenberg [:morgan]]

Hmm so best I can tell this is happening because on refresh we call onLocationChange [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#216] which doesn't hit this case [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#235] but instead trickles down to here [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#290] where we get a cached result for the current domain and an undefined value instead of the current domain and its current value. We then try to apply this undefined value and skip the case I added in my previous patch [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#469] and go directly to "apply global zoom".

So a few questions maybe :gijs can provide thoughts on:

(a) should we avoid calling apply on location changes for pages that are being refreshed? is that a possible solution here? or

(b) should we adjust that caching case in onLocationChange to check if the pref has a valid value before attempting to apply it? Not sure if that'd change anything, because it seems like maybe we're just not saving the adjusted zoom values anywhere, so:

(c) is there a way to write to CPS per tab (like, with a PID or something?) instead of per-domain so we can "save" the adjusted value of non-site-specific tabs and apply that on refresh consistently? then we'd apply global zoom to new tabs only? [wondering if https://bugzilla.mozilla.org/show_bug.cgi?id=1616305 is actually expected behaviour]

Comment 3

[dw-dev]

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

So a few questions maybe :gijs can provide thoughts on:

(a) should we avoid calling apply on location changes for pages that are being refreshed? is that a possible solution here? or

(b) should we adjust that caching case in onLocationChange to check if the pref has a valid value before attempting to apply it? Not sure if that'd change anything, because it seems like maybe we're just not saving the adjusted zoom values anywhere, so:

(c) is there a way to write to CPS per tab (like, with a PID or something?) instead of per-domain so we can "save" the adjusted value of non-site-specific tabs and apply that on refresh consistently? then we'd apply global zoom to new tabs only? [wondering if https://bugzilla.mozilla.org/show_bug.cgi?id=1616305 is actually expected behaviour]

(a) The steps I gave to reproduce the problem were just one example. The solution needs to work for any page load, not just page reloads. The zoom level in the tab needs to persist across page loads/reloads.

(b) / (c) Why can't the method that was used for persisting the zoom level in a non-sites-specific tab, prior to implementing a configurable Default Zoom level, still be used?

Comment 4

[:Gijs (he/him)]

(In reply to dw-dev from comment #3)

(b) / (c) Why can't the method that was used for persisting the zoom level in a non-sites-specific tab, prior to implementing a configurable Default Zoom level, still be used?

The problem is that there wasn't a "method" for persisting it before, it was just never changed - if you disabled site-specific zoom, we just avoided ever telling the docshell any zoom level from the preferences.
Now we do need to do that, because by default it starts out at 1, and if the default zoom is not 1, it needs setting. I guess we could avoid calling the setter if the default zoom is 1 (ie 100%) like in the steps in comment #0, but as you already implied, that is not going to help most people, just the narrow steps in comment #0.

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

Hmm so best I can tell this is happening because on refresh we call onLocationChange [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#216] which doesn't hit this case [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#235] but instead trickles down to here [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#290] where we get a cached result for the current domain and an undefined value instead of the current domain and its current value. We then try to apply this undefined value and skip the case I added in my previous patch [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#469] and go directly to "apply global zoom".

So a few questions maybe :gijs can provide thoughts on:

(a) should we avoid calling apply on location changes for pages that are being refreshed? is that a possible solution here? or

That wouldn't be sufficient, unfortunately.

(b) should we adjust that caching case in onLocationChange to check if the pref has a valid value before attempting to apply it? Not sure if that'd change anything, because it seems like maybe we're just not saving the adjusted zoom values anywhere, so:

Right. That last part ("not saving the adjusted zoom values anywhere") is the issue. Fundamentally, right now we have three (in our case, just 2) sources of information:

• the CPS per origin (disabled if sitespecific zoom is off)
• the global zoom
• the docshell / browser's zoom level int property

The app settings/preferences only control the global zoom. Each browser starts with zoom level 1 (ie 100%). The zoom controls and shortcuts control the docshell/browser (and, if site-specific zoom is on, the per-origin prefs). But there's (currently) no way after the fact to figure out if the zoom level known to the docshell/browser was the result of user actions or is just there because it's a new tab and it never got set. There's no other bit of information, just the two zoom levels, and no way to know which is "right".

When reviewing the change in bug 1603494 I don't think I realized that the codepath that was modified would also be hit on every location change, not just when the tab is opened / gets created - Sorry!

(c) is there a way to write to CPS per tab (like, with a PID or something?) instead of per-domain so we can "save" the adjusted value of non-site-specific tabs and apply that on refresh consistently?

No, there isn't.

Fundamentally, to fix this I think we're going to want to avoid setting the zoom level on the browser when sitespecific zoom is turned on. However, we'll still need to set it when the browser is created to ensure that it gets the proper zoom level.

then we'd apply global zoom to new tabs only? [wondering if https://bugzilla.mozilla.org/show_bug.cgi?id=1616305 is actually expected behaviour]

I don't think that's expected, no. In an ideal world, there'd be a way of only applying the zoom when the tab gets created and then leave things alone, but (i) I can't quickly find anything that we could use to distinguish that case and (ii) in any case I bet that works poorly with tab duplication / tearoff, where presumably users would expect the original and duplicate / torn-off tabs to have matching zoom levels, not to have them overridden with the global zoom.

So I'm tempted to add another bit of information... if we set some kind of bool marker (say, _tabHasCustomZoom) on the tab to indicate that the zoom has changed on a per-tab basis, and check that before overriding it with the global zoom, I think that'd work? We could do that in _applyZoomToPref, which currently bails out early if siteSpecific is false, but could be made to simply set that bool before returning. Then we can check for _tabHasCustomZoom in the place where the siteSpecific check used to be before we removed it in bug 1603494. Then we can fix bug 1616305 by checking for the case where siteSpecific is false, and _tabHasCustomZoom is not yet set, and in that case, overwrite the zoom level anyway (even if the location change is caused by a tabswitch, which currently bails out quite early). Does that make some amount of sense? It's possible I'm still missing something...

Comment 5

[dw-dev]

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

(b) / (c) Thanks for the clarfication.

My original (legacy) Zoom Page extension integrated very closely with browser-fullZoom.js, but I hadn't appreciated the impact of other recent changes related to the configurable DEfault Zoom level.

So I'm tempted to add another bit of information... if we set some kind of bool marker (say, _tabHasCustomZoom) on the tab to indicate that the zoom has changed on a per-tab basis, and check that before overriding it with the global zoom, I think that'd work? We could do that in _applyZoomToPref, which currently bails out early if siteSpecific is false, but could be made to simply set that bool before returning. Then we can check for _tabHasCustomZoom in the place where the siteSpecific check used to be before we removed it in bug 1603494. Then we can fix bug 1616305 by checking for the case where siteSpecific is false, and _tabHasCustomZoom is not yet set, and in that case, overwrite the zoom level anyway (even if the location change is caused by a tabswitch, which currently bails out quite early). Does that make some amount of sense? It's possible I'm still missing something...

This logic looks right to me, but that's without walking through the affected code areas..

Comment 6

[Morgan Reschenberg [:morgan]]

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

Fundamentally, to fix this I think we're going to want to avoid setting the zoom level on the browser when sitespecific zoom is turned on. However, we'll still need to set it when the browser is created to ensure that it gets the proper zoom level.

So, we'd want to avoid setting the zoom level when site specific is true because if a user switches it to false, we'd still pick up the values stored in CPS from when site specific was on? I'm a little confused at the difference between CPS values and browser/docshell values. (also: why does the browser start at 1? I guess it doesn't matter if it gets adjusted from preferences, but... that's odd to me?)

So I'm tempted to add another bit of information... if we set some kind of bool marker (say, _tabHasCustomZoom) on the tab to indicate that the zoom has changed on a per-tab basis, and check that before overriding it with the global zoom, I think that'd work? We could do that in _applyZoomToPref, which currently bails out early if siteSpecific is false, but could be made to simply set that bool before returning. Then we can check for _tabHasCustomZoom in the place where the siteSpecific check used to be before we removed it in bug 1603494. Then we can fix bug 1616305 by checking for the case where siteSpecific is false, and _tabHasCustomZoom is not yet set, and in that case, overwrite the zoom level anyway (even if the location change is caused by a tabswitch, which currently bails out quite early). Does that make some amount of sense? It's possible I'm still missing something...

I think this makes sense, this feels similar to how the "ignore internal change" bool (or whatever its called) works. NI'ing jamie here to get his opinion. I brought this up in our meeting yesterday and wanna make sure this solution addresses his concerns.

Comment 7

[:Gijs (he/him)]

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

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

Fundamentally, to fix this I think we're going to want to avoid setting the zoom level on the browser when sitespecific zoom is turned on. However, we'll still need to set it when the browser is created to ensure that it gets the proper zoom level.

So, we'd want to avoid setting the zoom level when site specific is true because if a user switches it to false, we'd still pick up the values stored in CPS from when site specific was on? I'm a little confused at the difference between CPS values and browser/docshell values.

Sorry, I think I just got my terminology mixed up and said on when I meant off. I think we want to avoid setting the zoom level on the browser when origin-specific zoom is off (ie when every tab gets its own zoom and it stays the same across navigations) -- except when the browser is created (because it needs to get the default zoom then).

(also: why does the browser start at 1? I guess it doesn't matter if it gets adjusted from preferences, but... that's odd to me?)

This is because this whole thing is implemented only in frontend code, and there was at some point some resistance to moving the entire implementation to docshell / gecko code. docshell has no concept of preferences for zoom levels, it's just dumb and always starts at 1, that's it. If consumers want something, they get to deal with it themselves.

This is also an issue in terms of e.g. bug 1548313 - given that CPS calls are asynchronous, if we're navigating from one domain to another, and we're responding to this async in the frontend, there's no way to guarantee we don't accidentally paint at the "old" level.

Comment 8

[Morgan Reschenberg [:morgan]]

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

Sorry, I think I just got my terminology mixed up and said on when I meant off. I think we want to avoid setting the zoom level on the browser when origin-specific zoom is off (ie when every tab gets its own zoom and it stays the same across navigations) -- except when the browser is created (because it needs to get the default zoom then).

Gotcha, this makes sense :)

This is also an issue in terms of e.g. bug 1548313 - given that CPS calls are asynchronous, if we're navigating from one domain to another, and we're responding to this async in the frontend, there's no way to guarantee we don't accidentally paint at the "old" level.

Lol just saw this in my own use and was like "hmm do we have a bug on that..?"

Comment 9

[:Gijs (he/him)]

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

This is also an issue in terms of e.g. bug 1548313 - given that CPS calls are asynchronous, if we're navigating from one domain to another, and we're responding to this async in the frontend, there's no way to guarantee we don't accidentally paint at the "old" level.

Lol just saw this in my own use and was like "hmm do we have a bug on that..?"

FWIW, we could do a better job without porting all this into gecko, by moving the code responding to origin changes into the child process (as right now it happens in the parent) and fetching the preferred zoom proactively when the navigation starts, and then using the result sync as soon as the new document gets created, which will definitely be before we paint. Still, because it effectively becomes a race of the network against the CPS, there's no guarantee we win that race, unless we can block painting on getting the answer from the CPS, which I don't believe we can do from the frontend (and in any case is best implemented inside gecko).

Comment 10

[James Teh [:Jamie]]

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

I think this makes sense, this feels similar to how the "ignore internal change" bool (or whatever its called) works. NI'ing jamie here to get his opinion. I brought this up in our meeting yesterday and wanna make sure this solution addresses his concerns.

I think it does. More below.

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

This is because this whole thing is implemented only in frontend code, and there was at some point some resistance to moving the entire implementation to docshell / gecko code. docshell has no concept of preferences for zoom levels, it's just dumb and always starts at 1, that's it. If consumers want something, they get to deal with it themselves.

If we did do this all in Gecko, I don't know how we'd handle the case where site specific is disabled (so tab specific zoom), since tab specific zoom isn't something that's even controlled with a pref. It's entirely session-based.

This is also an issue in terms of e.g. bug 1548313 - given that CPS calls are asynchronous, if we're navigating from one domain to another, and we're responding to this async in the frontend, there's no way to guarantee we don't accidentally paint at the "old" level.

But because CPS is async (and can only be accessed from the parent process), we'd have the same problem if we handled it in Gecko.

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

FWIW, we could do a better job without porting all this into gecko, by moving the code responding to origin changes into the child process (as right now it happens in the parent) and fetching the preferred zoom proactively when the navigation starts,

How would that work in a Fission world? We need to retrieve the origin for the top level tab document, even for OOP iframes. Can we access the origin for an OOP Window/BrowsingContext? Also, adjusting this in response to the user changing it from the UI would be pretty painful.

The solution for Fission I proposed in bug 1612068 uses synced properties on WindowContext to set it up and keep it synced across any OOP iframes. If we wanted to have content query for the zoom value when the origin changes, I guess we could do that, but then we'd be updating the synced WindowContext from two places, which would potentially make the front-end code even messier (since now it would have to respond to changes from content).

Comment 11

[Rares Doghi, Desktop QA]

Hi, I was able to reproduce this issue in our latest Nightly build and I also got a regression range for it:

First Known BAD build:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0551e1798d616e152e13f36db0fb9b3aa36c7f1a&tochange=d5ff6ca9e011edf6d730d125d2366115a2133cc4

Last Known GOOD build:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0551e1798d616e152e13f36db0fb9b3aa36c7f1a&tochange=0f4fc4e582d369244c46b28ee9f86d0b02dc9e39

I will update the flags for this issue.

Comment 12

[:Gijs (he/him)]

Although I think it's valuable to continue discussing what the best model for fullzoom/textzoom implementation is, fixing that is a really big project and there's an immediate regression for 75 to address, so I feel like I'm derailing the bug with it. So I'm going to deliberately not respond to Jamie here. I'll start an email thread or set up a call or something.

I think there's agreement on the solution to the immediate problem, ie using something like _tabHasCustomZoom to distinguish the case where we adjust to the default zoom, vs. the case where the user has changed the zoom and we shouldn't override it.

Comment 13

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1617544: Add tabHasCustomZoom property to tabbrowser to correctly store state when siteSpecific zoom is disabled.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ba497506480
Add tabHasCustomZoom property to tabbrowser to correctly store state when siteSpecific zoom is disabled. r=Gijs

Comment 15

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/5ba497506480

Comment 16

[:Gijs (he/him)]

Hello, could you verify that this is all behaving as expected on current nightlies? I'm aware that this code has more edgecases than I can count so it'd be useful to have confirmation that Things Work now. :-)

Comment 17

[dw-dev]

I've tested Firefox 75.0a1 (2020-03-08) (64-bit) with the latest version of my Zoom Page WE extension.

I have tested:

• The Steps to Reproduce in my original problem report.
• Site-Specific and non-Site-Specific zoom operations.
• The test cases reported to me by extension users, some of which showed transient glitches.

All of the above now work perfectly. Thanks for fixing this bug so quickly.