1611903 - Zoom level is not applied correctly without a page refresh after changing "Zoom text only" option in about:preferences.

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Alin Ilea, Desktop Test Engineering]

Attachment: 2020-01-28_11h27_15.mp4

Affected versions:

Nightly 74.0a1 (2020-01-28), Beta 73.0b10

Affected platforms:

ALL

Steps:

1. Launch Firefox with a new profile and go to about:preferences.
2. Under General, go to Zoom section and select 133% value (or any other) from the "Default zoom" dropdown.
3. Perform a zoom in or zoom out using ctrl + mousewheel and then click on the zoom level badge from the address bar in order to return to the default zoom level.
4. Check the "Zoom text only" checkbox and perform another zoom in or out like in step 3.

Actual result:

After checking the Zoom text only box and perform a zoom change using ctrl + mousewheel, the zoom level is applied for text only but starting from 100% instead from the default zoom level.

Expected result:

The zoom level should be applied starting from the default zoom level value set in step 2 (133% in our case).

NOTE: After checking the "Zoom text only" option from step 4, if the user refresh the page it works ok, the zoom changes are applied starting from the default zoom value. Note that other checkbox options like "Use smooth scrolling" from Browsing section is applied immediately without refreshing the page. Also if I change the Zoom text only option from menu bar->view->zoom->zoom text only, it works ok, the zoom level is changed starting from the default zoom level set in step 2, not from the 100%.
Please see the attachment.

Comment 1

[Morgan Reschenberg [:morgan]]

Hmm my guess here is that the property name we're using to get the global value changes when it becomes text instead of full zoom? https://searchfox.org/mozilla-central/rev/cbd110d718bc89a499d3f996af24532abbf6f8ea/browser/modules/ZoomUI.jsm#59

Comment 2

[:Gijs (out for now; he/him)]

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

Hmm my guess here is that the property name we're using to get the global value changes when it becomes text instead of full zoom? https://searchfox.org/mozilla-central/rev/cbd110d718bc89a499d3f996af24532abbf6f8ea/browser/modules/ZoomUI.jsm#59

I don't think so? _applyZoomToPref always uses FullZoom.name which is constant... https://searchfox.org/mozilla-central/rev/ad6234148892bc29bf08d736604ac71925138040/browser/base/content/browser-fullZoom.js#475 . So I suspect it's something else, just not sure what off-hand...

BTW, for triage I need to pick a priority - are you working on this? :-)

Comment 3

[Morgan Reschenberg [:morgan]]

Hihi sorry, yes I'm working on this :) work has been slow with all hands but its on my radar

Comment 4

[:Gijs (out for now; he/him)]

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

Hihi sorry, yes I'm working on this :) work has been slow with all hands but its on my radar

No worries, the all-hands thing makes complete sense, just gonna slap a P1 on this as a result. :-)

Comment 5

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1611903: Change 'Text zoom only' checkbox behaviour to call ZoomManager.toggleZoom instead of changing preference directly.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2eff74f53aaa
Change 'Text zoom only' checkbox behaviour to call ZoomManager.toggleZoom instead of changing preference directly. r=Gijs

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2eff74f53aaa

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider as an RC/dot release ride-along or can this fix ride Fx74 to release?

Comment 9

[Alin Ilea, Desktop Test Engineering]

Verified - Fixed in latest Nightly 74.0a1 (Build id: 20200205215017) using Windows 10, Mac OS 10.15 and Ubuntu 18.04.

Comment 10

[Morgan Reschenberg [:morgan]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)

Is this something we should consider as an RC/dot release ride-along or can this fix ride Fx74 to release?

It'd be awesome to get this into a dot release; especially since this is exposed through preferences UI and users are more likely to engage with it, even if they weren't trying out text zoom before.

What's the procedure to request this patch be included?

Comment 11

[Morgan Reschenberg [:morgan]]

gijs messaged me lol I know what to do now

Comment 12

[Morgan Reschenberg [:morgan]]

Comment on attachment 9124297 [details]
Bug 1611903: Change 'Text zoom only' checkbox behaviour to call ZoomManager.toggleZoom instead of changing preference directly.

Beta/Release Uplift Approval Request

• User impact if declined: Users will see inaccurate zoom levels when switching from full zoom to text zoom (or vice versa) without a refresh on about:preferences
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch involves changing the checkbox functionality from flipping browser.zoom.full directly to calling ZoomManager.toggleZoom(). This functionality already exists within the browser (View > Zoom > Zoom Text Only); this patch serves only to mirror this logic in preferences.
• String changes made/needed: N/A

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9124297 [details]
Bug 1611903: Change 'Text zoom only' checkbox behaviour to call ZoomManager.toggleZoom instead of changing preference directly.

Improvement to the new global zoom feature shipping in Fx73. Approved for 73.0 RC2.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/0f1cd67c7f7e

Comment 15

[Alin Ilea, Desktop Test Engineering]

Verified - Fixed in Fx 73.0 (Build id: 20200207001703) using Windows 10, Mac OS 10.15 and Ubuntu 18.04.