Add new zoomSiteSpecific and zoomFullPage settings in browserSettings API
Categories
(WebExtensions :: General, enhancement, P5)
Tracking
(firefox75 fixed)
Tracking | Status | |
---|---|---|
firefox75 | --- | fixed |
People
(Reporter: dw-dev, Assigned: dw-dev)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [zoom] triaged)
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details |
Comment 1•8 years ago
|
||
Updated•8 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•6 years ago
|
Now that my Zoom Page WE extension has become a Recommended Extension, I want to raise the priority of this bug.
The key issue is that tabs.setZoomSettings({ mode: ... , scope: ... }) is NOT supported.
In effect, this means that mode is always 'automatic' and scope is determined by the 'browser.zoom.siteSpecific' preference.
It would be a BIG improvement if the 'scope' property just set the state of the 'browser.zoom.siteSpecific' preference.
The zooming behaviour would still not be exactly the same as in Chrome, BUT it would avoid the user having to manually set the 'browser.zoom.siteSpecific' preference. Most of the other differences in zooming behaviour are easy to work around.
Please consider implementing this simple fix.
Comment 8•5 years ago
|
||
The compatibility data for tabs.setZoomSettings was changed in December 2017 to flag this is unsupported. Is this therefore dev-doc-done?
It is fair to say that the documentation reflects the current implementation of tabs.setZoomSettings(). It is not supported, or at least doesn't do anything useful.
However, my point is that it would be very simple to make tabs.setZoomSettings() do something useful (i.e. just set the state of the 'browser.zoom.siteSpecific' preference from the 'scope' property) without doing a full implementation.
This would allow zoom extensions (such as my Zoom Page WE) to switch between site-specific and non-site-specific zoom very easily, WITHOUT having to ask the user to do that manually by setting the 'browser.zoom.siteSpecific' preference.
Comment 10•5 years ago
|
||
Without looking into all the details here, I think we would likely take a patch for that, or at least alternatively to add a browserSetting for browser.zoom.siteSpecific
if it turns out your suggestion has some negative side effects.
Assignee | ||
Comment 11•5 years ago
|
||
I would be happy with either solution, since either would allow Zoom Page WE to switch between site-specific and non-site-specific zoom.
1. browser.tabs.setZoomSettings()
If we go with this solution, the simplest implementation would be to ignore the 'mode' parameter and always default to 'automatic', and to set browser.zoom.siteSpecific based on the value of the 'scope' parameter (true if 'per-origin' or false if 'per-tab').
There are minor differences in how 'per-tab' zoom works in Firefox and Chrome, for example, Chrome resets the zoom level when a new page is loaded, whereas Firefox doesn't, but I don't think this really matters.
2. browser.browserSettings.newTabPosition
If we go with this solution, we just need to support 'set' and 'get' operations.
Which would be your preference?
Is anyone likely to get assigned to implement this?
Comment 12•5 years ago
|
||
In light of comment 8 and the subsequent discussion, I'm removing dev-doc-needed. Please reapply if this bug results in a change.
Updated•5 years ago
|
Assignee | ||
Comment 13•5 years ago
|
||
Yes, that's fine.
Also, just noticed, in Comment 11, the title of Point 2 should be 2. browser.browserSettings.siteSpecific.
Comment 14•5 years ago
|
||
(In reply to dw-dev from comment #11)
1. browser.tabs.setZoomSettings()
If we go with this solution, the simplest implementation would be to ignore the 'mode' parameter and always default to 'automatic', and to set browser.zoom.siteSpecific based on the value of the 'scope' parameter (true if 'per-origin' or false if 'per-tab').
I didn't test in Chrome, but it seems this api would be used to select specific origins for which the zoom should be a certain value, leaving all other origins in the default management state of "per tab". Not sure if that makes a difference in practice, or if that would work differently in Firefox.
2. browser.browserSettings.newTabPosition
I'm guessing wrong copy/paste?
Anyway, is the browser.zoom.siteSpecific
pref exposed anywhere in firefox UI as a setting? Was it ever? How about a similar setting in Chrome?
Is anyone likely to get assigned to implement this?
Not very, it was triaged as P5 (low priority - contributions welcome).
Assignee | ||
Comment 15•5 years ago
|
||
(In reply to dw-dev from comment #14)
1. browser.tabs.setZoomSettings()
In Chrome, the zoom scope (per-tab or per-site) can be set independently for each tab. So there can be some tabs using per-tab zoom and other tabs using per-site (site-specific) zoom. Whereas in Firefox, either all tabs will be using per-tab zoom, or all tabs will be using per-site (site-specific) zoom, depending on the value of the global browser.zoom.siteSpecific preference.
I have been developing my Zoom Page WE extension for so long that I had forgotten this difference between Firefox and Chrome. Zoom Page WE always enforces the Firefox zoom scope model, whether installed in Firefox or Chrome.
Modifying the Firefox browser.tabs.setZoomSettings() API as previously suggested would not be compatible with the Chrome implementation and would be confusing for extension developers.
2. browser.browserSettings.zoomSiteSpecific
My preference is to implement two new boolean settings in browserSettings:
- browser.browserSettings.zoomSiteSpecific [set or get]
- browser.browserSettings.zoomFullPage [set or get]
This would allow zoom extensions (such as Zoom Page WE) to switch between per-tab and per-site (site-specific) zoom, and to switch between full-page and text-only zoom, on a global basis (in all tabs).
I have changed the title of this bug to reflect this way forward - and I am currently working on a patch to implement these new two new settings.
Assignee | ||
Comment 16•5 years ago
|
||
Shane, I have added you to the CC list for this bug, since you appear in two of the other three browserSettings bugs, and you mentored me when I was developing the tabs.print, tabs.printPreview and tabs.saveAsPDF API's.
I hope you will be able to review my patch that implements the browserSettings.zoomSiteSpecific and browserSettings.zoomFullPage API's. The changes in the patch are very small. I already have the new API's and automated tests working in my environment.
Assignee | ||
Comment 17•5 years ago
|
||
Jim, please can you make me the assignee for this bug.
Comment 18•5 years ago
|
||
(In reply to dw-dev from comment #17)
Jim, please can you make me the assignee for this bug.
I though you should be able to assign yourself if you're able to edit bug titles. Anyway, I assigned you, and if want the so called editbugs
privileges, I'd be happy to vouch (or you can file something like bug 1580360 directly, and ni? me there/point to this comment).
Anyway, is the
browser.zoom.siteSpecific
pref exposed anywhere in firefox UI as a setting? Was it ever? How about a similar setting in Chrome?
Can you please provide some info about this?
Assignee | ||
Comment 19•5 years ago
|
||
Although I could change the bug title, I couldn't change the Assignee because theres was no input box after the "Assignee:" label. Now that I have been assigned, there is an input box after the "Assignee:" label, and if I wanted to I could change that field. So maybe it was just a glitch.
The 'browser.zoom.siteSpecific' pref is not exposed anywhere in the Firefox UI (other than about:config). If it was exposed, I'm pretty sure that I would know about it. The 'browser.zoom.full' pref is exposed in the Firefox main menu: View > Zoom > Zoom Text Only.
Without an extension, Chrome always applies per-site (site-specific) zoom to every tab. There is nothing in the Chrome UI to switch between per-site and per-tab zoom. If there was, then it would almost certainly only apply to the selected (active) tab, not globally like the Firefox prefs.
The reasons for Zoom Page WE needing to control the 'browser.zoom.siteSpecific' and 'browser.zoom.full' preferences are as follows:
- Most features in Zoom Page WE will only work correctly when 'browser.zoom.full' is false.
- Some features in Zoom Page WE will only work (and are only allowed) when 'browser.zoom.siteSpecific' is false.
- Less technical users are deterred from using Zoom Page WE by the need to change "hidden" preferences.
Comment 20•5 years ago
|
||
I would lean towards browserSettings also. There is likely some changes that need to happen in the underlying zoom code, such as dealing with the caching in FullZoom.siteSpecific.
I'm curious about the "deprecated" text here, is it the telemetry flag, or the pref that is deprecated?
Lets be sure about the longevity of both (I'm sure they are not going away) prior to landing anything.
Feel free to r? me on the patch.
ni?Fallen as an FYI on new API.
Assignee | ||
Comment 21•5 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #20)
The legacy version of my Zoom Page add-on integrated closely with the browser-fullZoom.js code. I have looked at this code again and I don't think it needs changing. In particular, the _applyZoomToPref() and _applyPrefToZoom() functions bail out if browser.zoom.siteSpecific is false.
The comments in Bug 1282881 indicate that it is the telemetry flag that is deprecated.
I will submit the patch for review.
Assignee | ||
Comment 22•5 years ago
|
||
Add new zoomSiteSpecific and zoomFullPage settings in browserSettings API.
Assignee | ||
Comment 23•5 years ago
|
||
Add new zoomSiteSpecific and zoomFullPage settings in browserSettings API
Updated description of zoomSiteSpecific in brower_settings.json
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 24•5 years ago
|
||
Please ignore the two Phabricator Automation messages immediately after Comment 23.
I am working on patches for two bugs and the changes for the other bug were accidentally included in an 'hg amend' for this bug.
I have fixed this in the latest 'hg amend'.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 25•5 years ago
|
||
@dw-dev can you look at bug 1590485 zombie just linked, should there be additional work here for exposing any settings?
Assignee | ||
Comment 26•5 years ago
|
||
I've taken a look at Bug 1590485 - the discussion, the code diff, some of the source files - and done some testing.
Bug 1590485 is described as implementing global zoom functionality, but in practice appears only to implement a way of setting the global default zoom value. Prior to this, the global default zoom value has always been 100%. The global default zoom value is managed by the nsIContentPrefService2 using cps2.getGlobal() and cps2.setGlobal().
My testing indicates the following:
-
The operation of the browser.zoom.siteSpecific preference essentially remains unchanged:
- When true, site-specfic zoom is applied. Changes to the zoom value apply to pages from the same site in all tabs. If no site-specific zoom value has been set, then the global default zoom value is applied.
- When false, site-specfic zoom is not applied. Changes to the zoom value apply to each tab individually. 100% zoom is applied when a new tab is created. The zoom value in a tab persists across page loads.
I am a little surprised that the global default zoom value is not applied when a new tab is created.
-
The operation of the browser.zoom.full preference remains unchanged:
- When true, full-page zoom is applied to pages in all tabs.
- When false, text-only zoom is applied to pages in all tabs.
It would be possible to have a new browserSettings.zoomGlobalDefault() API to manage the the global default zoom value. However, I am not convinced about the need to have such an API.
Most zoom WebExtensions would manage the zoom levels themselves using the tabs.setZoom() API, which they can do at present. What they can't do at present, is control the application of site-specific zoom or switch between full-page zoom and text-only zoom, hence the two new APIs proposed in this bug.
Assignee | ||
Comment 27•5 years ago
|
||
Shane, Is there any way to move this forwards? It looks like Fallen is unavailable or too busy to review.
Assignee | ||
Comment 29•5 years ago
|
||
Shane,
Sorry for the delay, but I have been busy with bug fixes and updates to some of my extensions.
I have tried landing this patch, but after clicking the "Preview Landing" button in Lando, I am receiving the following message:
"You have insufficient permissions to land. Level 3 Commit Access is required."
My understanding is that I would need two people to vouch for me. These people need to be module owners or peers of code stored at this level, or owners or peers of the "Tree Sheriffs" module. I'm not sure who these people would be. Would you and Tomislav meet these criteria?
Alternatively, are you able to land this patch?
Comment 30•5 years ago
|
||
(In reply to dw-dev from comment #29)
Shane,
Sorry for the delay, but I have been busy with bug fixes and updates to some of my extensions.
I have tried landing this patch, but after clicking the "Preview Landing" button in Lando, I am receiving the following message:
"You have insufficient permissions to land. Level 3 Commit Access is required."
My understanding is that I would need two people to vouch for me. These people need to be module owners or peers of code stored at this level, or owners or peers of the "Tree Sheriffs" module. I'm not sure who these people would be. Would you and Tomislav meet these criteria?
Alternatively, are you able to land this patch?
"Non-Level 3 committers" can land the patches already signed off with the help of the Sheriffs, but the phabricator revision should be marked with the "Check-in needed" tag (which can be added to the phabricator revision by clicking on "Edit Revision" in the sidebar, and then look for the "Check-in needed" tag in the "Tags" field).
I just added the "Check-in needed" tag on the phabricator revision https://phabricator.services.mozilla.com/D59433, and so the patch will be soon pushed to autoland by a Sheriff.
Comment 31•5 years ago
|
||
Comment 32•5 years ago
|
||
bugherder |
Comment 33•5 years ago
|
||
(In reply to dw-dev from comment #29)
My understanding is that I would need two people to vouch for me. These people need to be module owners or peers of code stored at this level, or owners or peers of the "Tree Sheriffs" module. I'm not sure who these people would be. Would you and Tomislav meet these criteria?
I believe landing directly (even via phabricator/auto land) requires Level 3, which is a high mark, but you should be able to use the "checkin-needed" tag as Luca explained above.
If you intend to continue contributing, Level 1 might be useful to you, as it enables you to submit patches to Try for CI testing, and I'd be happy to vouch for your (as I'm sure will Shane). See bug 1610805 for example if you want to do that.
Comment 34•5 years ago
|
||
In the browser_settings.json it states that zoomSiteSpecific
is "This boolean setting controls whether zoom is applied on a per-site basis or to the current tab only." However, can I confirm that this setting controls the configuration setting browser.zoom.siteSpecific
and therefore whether zoom is on a per-site or per-tab basis globally, not just the zoom behavior in the current/active tab when it set?
Assignee | ||
Comment 35•5 years ago
|
||
browserSettings.zoomSiteSpecific directly controls the browser.zoom.siteSpecific preference, switching between site-specfic and non-site-specific zoom operation for all tabs.
If browser.zoom.siteSpecific is set true:
- when a page is loaded, if there is already a site-specific zoom level for that site, it will be applied to the page.
- when the zoom level for a page is changed, the zoom levels of other pages from the same site in other tabs will also be changed.
- so a zoom operation applies to all pages from the same site.
If browser.zoom.siteSpecific is set false:
- when a new tab is opened, the global default zoom level will be applied.
- when the zoom level in a tab is changed, it will persist across page loads and will not affect the zoom level in any other tab.
- so a zoom operation applies to the current tab only.
As far as I can see, this is consistent with the description in browser_settings.json.
Comment 36•5 years ago
|
||
Thanks, I have now updated the information on MDN:
In addition updated compatibility data has been merged.
Please let me know if I need to make any changes.
Does this have any effect on tabs.ZoomSettingsScope or any of the other zoom properties of tabs. For example, I would assume that:
"per-tab"
Zoom changes only take effect in this tab, and zoom changes in other tabs will not affect the zooming of this tab. Also, per-tab zoom changes are reset on navigation; navigating a tab will always load pages with their per-origin zoom factors.
should read
"per-tab"
Zoom changes only take effect in this tab, and zoom changes in other tabs will not affect the zooming of this tab. Also, per-tab zoom changes are reset on navigation; navigating a tab will always load pages according to the browser default defined in the browser.zoom.siteSpecific setting.
Assignee | ||
Comment 37•5 years ago
|
||
I think this is an accurate summary of the current situation.
There appear to be some errors in both behaviour and documentation that need correcting.
tabs.setZoomSettings() is not supported.
tabs.getZoomSettings() returns:
- mode: "automatic"
- scope: !privacy.resistFingerprinting && browser.zoom.siteSpecific
- defaultZoomFactor: 1 - This is not the correct behaviour. It should return the (global default zoom level)/100.
tabs.onZoomChange() returns:
- mode: "automatic"
- scope: !privacy.resistFingerprinting && browser.zoom.siteSpecific
- defaultZoomFactor: 1 - This is not the correct behaviour. It should return the (global default zoom level)/100.
"per-tab" description:
- The existing wording appears to be copied from Chrome.
- The sentence "Also, per-tab zoom changes are reset on navigation; navigating a tab will always load pages with their per-origin zoom factors." is not true for Firefox. The zoom level persists across page loads and navigation.
Updated•5 years ago
|
Comment 38•5 years ago
|
||
Thanks, I've set up a separate ticket on GitHub to check the other documentation
Comment 39•5 years ago
|
||
I have updated the "per-tab" description on tabs.ZoomSettingsScope and also added a note to Chrome incompatibilities. However, I'm unclear what issues are being highlighted by the comments:
tabs.getZoomSettings() returns:
mode: "automatic"
scope: !privacy.resistFingerprinting && browser.zoom.siteSpecific
defaultZoomFactor: 1 - This is not the correct behaviour. It should return the (global default zoom level)/100.
tabs.onZoomChange() returns:
mode: "automatic"
scope: !privacy.resistFingerprinting && browser.zoom.siteSpecific
defaultZoomFactor: 1 - This is not the correct behaviour. It should return the (global default zoom level)/100.
When I execute these I get a response that seems consistent with the documentation: mode: "automatic", scope: "per-origin", defaultZoomFactor: 1
Can you clarify?
Assignee | ||
Comment 40•5 years ago
|
||
To clarify:
-
mode: "automatic" is always returned, which is fine.
-
scope: if (!privacy.resistFingerprinting && browser.zoom.siteSpecific == true) "per-origin" is returned, otherwise "per-tab" is returned, which is fine.
-
defaultZoomFactor: "1" is always returned. This is no longer the correct behaviour, because Bug 1590485 (fixed in Firefox 73.0) now allows the user to change the Default Zoom level in Tools > Options.
Assignee | ||
Comment 41•4 years ago
|
||
Is any more information required?
Description
•