Closed Bug 1603494 Opened 4 years ago Closed 4 years ago

Global zoom functions incorrectly with disabled site-specific zoom

Tracking

()

Status:

RESOLVED FIXED

Milestone:

Firefox 75

Tracking Flags:

Tracking

Status

firefox75

---

fixed

People

(Reporter: morgan, Assigned: morgan)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

sample.js 4 years ago Morgan Reschenberg [:morgan] 1.70 KB, application/x-javascript		Details
Bug 1603494: Allow global zoom values to be read from CPS when site specific zoom is disabled. r=gijs 4 years ago Morgan Reschenberg [:morgan] 47 bytes, text/x-phabricator-request		Details \| Review

Morgan Reschenberg [:morgan]

Assignee

Description

•

4 years ago

•

Edited

See attached test case.

Currently, we only allow our zoom preference (getGlobalValue) to be read in the case that site specific zoom is false, or we're in print preview mode.

In light of the new global zoom patch, this logic should be re-evaluated. If we turn site-specific zoom off (note: there's no user-facing pref for this, it exists in about:config only), set a global zoom value (ie. 67%), and test the zoom value of our active tab, we'll see the global zoom value is not reflected. Instead, the default value of 100% is maintained.

I don't think this is expected behaviour, and I think the best option would be to remove this !siteSpecific casing, but this bug is a follow-up for further testing and also possible removal of this pref if appropriate.
https://searchfox.org/mozilla-central/rev/923eec8d2fb8078ebc7a33a9e1ce73eac01f7446/browser/base/content/browser-fullZoom.js#427

Morgan Reschenberg [:morgan]

Assignee

Updated

•

4 years ago

Summary: Disabled site-specific zoom functions incorrectly with global zoom → Global zoom functions incorrectly with disabled site-specific zoom

Morgan Reschenberg [:morgan]

Assignee

Comment 1

•

4 years ago

Attached file sample.js — Details

BugBot [:suhaib / :marco/ :calixte]

Comment 2

•

4 years ago

The priority flag is not set for this bug.
:asa, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(asa)

Alice Wyman

Updated

•

4 years ago

Comment 4

•

4 years ago

Transplanting this from the other bug:

(In reply to Morgan Reschenberg [:morgan] from bug 1609880 comment #11)

And the reason global zoom doesn't work when site specific zoom is disabled is because global zoom requires a call to _applyPrefToZoom, which, for some reason, doesn't fire when site specific is disabled. Looks like fixing [this bug] is as straightforward as removing this if conditional [https://searchfox.org/mozilla-central/rev/1db5ef59eba65d32d6a29a494e87b6078453e559/browser/base/content/browser-fullZoom.js#456] but I don't have enough information on why that conditional is there in the first place to feel comfortable deleting it.

OK, so I dug into this a bit using the "annotate"/"blame" information in searchfox to see how the condition ended up there. This has changed a few times over the years but was actually first implemented in bug 419609 (back before Firefox 3, before rapid releases...) - as soon as we added site-specific zoom preferences, and was a way to turn that "back" off and get per-tab zoom. There's no rationale in the bug or the patch ( https://hg.mozilla.org/mozilla-central/rev/929a4bb77ddbc11c81a0ad5b92d3bd4d12b7f1bf ) as to why that should not use the default/global zoom as opposed to 1, and in fact, the summary of bug 430870 (duped to the current metabug) identified this exact problem fairly early on.

Inbetween today and that change from 12 years ago, there were a few edgecases that impacted the code that it may be worth paying attention to:

image documents (ie what happens if you load an image file directly as a tab, cf. opening https://www.mozilla.org/media/contentcards/img/home-2019/card_2/guide-high-res.a12bff1c92ab.jpg directly as a tab)
private browsing

I think the latter should probably just use the default zoom level as-is.

For the former, it would be useful to check what happens. It looks like even with site-specific zoom turned on, current behaviour is maybe not optional (if I set a default zoom level, and then load an image, the URL bar shows the "zoom" bubble set to 100%, and using both that and the click-based zoom-to-fit vs. zoom-to-actual-size toggle on the image makes for a confusing experience).
Anyway, considering that also affects the current state with site-specific zoom turned on, I don't think it's a reason not to proceed here and remove the site-specific zoom condition (keeping the printpreview one) - we can use a different follow-up bug for the image document confusion.

Morgan, do you have cycles / are you happy to drive this bug home? If not, let me know and I can give it a shot.

Flags: needinfo?(mreschenberg)

Morgan Reschenberg [:morgan]

Assignee

Updated

•

4 years ago

Flags: needinfo?(mreschenberg)

Priority: -- → P3

Morgan Reschenberg [:morgan]

Assignee

Updated

•

4 years ago

Flags: needinfo?(asa)

Morgan Reschenberg [:morgan]

Assignee

Comment 5

•

4 years ago

•

Edited

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

Transplanting this from the other bug:

(In reply to Morgan Reschenberg [:morgan] from bug 1609880 comment #11)

And the reason global zoom doesn't work when site specific zoom is disabled is because global zoom requires a call to _applyPrefToZoom, which, for some reason, doesn't fire when site specific is disabled. Looks like fixing [this bug] is as straightforward as removing this if conditional [https://searchfox.org/mozilla-central/rev/1db5ef59eba65d32d6a29a494e87b6078453e559/browser/base/content/browser-fullZoom.js#456] but I don't have enough information on why that conditional is there in the first place to feel comfortable deleting it.

OK, so I dug into this a bit using the "annotate"/"blame" information in searchfox to see how the condition ended up there. This has changed a few times over the years but was actually first implemented in bug 419609 (back before Firefox 3, before rapid releases...) - as soon as we added site-specific zoom preferences, and was a way to turn that "back" off and get per-tab zoom. There's no rationale in the bug or the patch ( https://hg.mozilla.org/mozilla-central/rev/929a4bb77ddbc11c81a0ad5b92d3bd4d12b7f1bf ) as to why that should not use the default/global zoom as opposed to 1, and in fact, the summary of bug 430870 (duped to the current metabug) identified this exact problem fairly early on.

Inbetween today and that change from 12 years ago, there were a few edgecases that impacted the code that it may be worth paying attention to:

image documents (ie what happens if you load an image file directly as a tab, cf. opening https://www.mozilla.org/media/contentcards/img/home-2019/card_2/guide-high-res.a12bff1c92ab.jpg directly as a tab)

private browsing

I think the latter should probably just use the default zoom level as-is.

For the former, it would be useful to check what happens. It looks like even with site-specific zoom turned on, current behaviour is maybe not optional (if I set a default zoom level, and then load an image, the URL bar shows the "zoom" bubble set to 100%, and using both that and the click-based zoom-to-fit vs. zoom-to-actual-size toggle on the image makes for a confusing experience).
Anyway, considering that also affects the current state with site-specific zoom turned on, I don't think it's a reason not to proceed here and remove the site-specific zoom condition (keeping the printpreview one) - we can use a different follow-up bug for the image document confusion.

Morgan, do you have cycles / are you happy to drive this bug home? If not, let me know and I can give it a shot.

wow thanks for digging! yeah I can take this.I probably don't have time to dig into the image document stuff, but I can file a follow-up on it after landing this patch :)

Morgan Reschenberg [:morgan]

Assignee

Updated

•

4 years ago

Assignee: nobody → mreschenberg

Morgan Reschenberg [:morgan]

Assignee

Comment 6

•

4 years ago

Attached file Bug 1603494: Allow global zoom values to be read from CPS when site specific zoom is disabled. r=gijs — Details

Pulsebot

Comment 7

•

4 years ago

Pushed by mreschenberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65dd9e70071a
Allow global zoom values to be read from CPS when site specific zoom is disabled. r=Gijs

Morgan Reschenberg [:morgan]

Assignee

Updated

•

4 years ago

Blocks: 1616305

Noemi Erli[:noemi_erli]

Comment 8

•

4 years ago

Backed out changeset 65dd9e70071a (Bug 1603494) for browser chrome failures in browser_bug1369357_site_specific_zoom_level.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&revision=65dd9e70071acf3ef844ec09f93579282a5a5451&searchStr=linux%2Cx64%2Casan%2Cmochitests%2Ctest-linux64-asan%2Fopt-mochitest-browser-chrome-e10s-13%2Cm%28bc13%29&selectedJob=289370329

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=289365767&repo=autoland&lineNumber=2584

Backout: https://hg.mozilla.org/integration/autoland/rev/e3aadf80c0997bbaf491f74b31089c40adec7f0c

Flags: needinfo?(mreschenberg)

Morgan Reschenberg [:morgan]

Assignee

Comment 9

•

4 years ago

pinged gijs to see if removing the test is the right solution

Flags: needinfo?(mreschenberg)

Pulsebot

Comment 10

•

4 years ago

Pushed by mreschenberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5ff6ca9e011
Allow global zoom values to be read from CPS when site specific zoom is disabled. r=Gijs

Alexandru Michis [:malexandru]

Comment 11

•

4 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 4 years ago

status-firefox75: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 75

Andreea Pavel [:apavel]

Updated

•

4 years ago

Regressions: 1616714

Gingerbread Man

Updated

•

4 years ago

Regressions: 1617544

You need to log in before you can comment on or make changes to this bug.