Closed Bug 1286953 Opened 8 years ago Closed 5 years ago

Add new zoomSiteSpecific and zoomFullPage settings in browserSettings API

Categories

(WebExtensions :: General, enhancement, P5)

71 Branch
enhancement

Tracking

(firefox75 fixed)

RESOLVED FIXED
mozilla75
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)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID: 20160623154057 Steps to reproduce: I am migrating my Zoom Page add-on to the new WebExtensions API's. I have found that some of the Zoom API's in chrome.tabs do not work correctly and there are incompatibilities with Chrome. Actual results: What Works ----------- 1. chrome.tabs.setZoom(tabId, zoomFactor, callback) - correctly sets the zoom to the zoom factor. 2. chrome.tabs.getZoom(tabId, callback) - correctly returns the zoom factor. 3. chrome.tabs.getZoomSettings(tabId, callback) - correctly returns mode, scope and default zoom factor. What Doesn't Work ------------------ 4. chrome.tabs.setZoomSettings(tabId, { mode: "automatic", scope: "per-origin" }, callback) - always produces an error message in the console: Unsupported zoom settings: {"mode":"automatic","scope":"per-origin","defaultZoomFactor":null} ExtensionUtils.jsm:283 If these settings are unsupported, then why isn't it mentioned in the documentation? Why is "defaultZoomFactor" shown in this error message as a passed-in parameter, when in Chrome it is only available as a returned value in getZoomSettings()? 5. chrome.tabs.setZoomSettings(tabId, { mode: "automatic", scope: "per-tab" }, callback) - always produces an error message in the console: Unsupported zoom settings: {"mode":"automatic","scope":"per-tab","defaultZoomFactor":null} ExtensionUtils.jsm:283 Same comments as in 4. 6. Per-Origin zoom is forgotten when a page is reloaded. It is remembered in Chrome. 7. Per-Origin zoom is forgotten when a page from the same site is loaded into another tab. It is remembered in Chrome. 8. Per-Origin zoom is forgotten between browser sessions. It is remembered in Chrome. 9. Per-Tab zoom applies zoom changes to other tabs from same site. It is not applied in Chrome. 10. Per-Tab zoom does not apply Per-Origin zoom when page navigation occurs. It is applied in Chrome. Expected results: 4 & 5. If it is not supported, then it should be documented. If it is supported, then there should not be an error message in the console. "defaultZoomFactor" should not be treated as a parameter. 6 & 7 & 8. Per-Origin zoom should be remembered in all these cases. 9. Per-Tab zoom should not affect the zoom in other tabs. 10. Per-Tab zoom should apply Per-Origin zoom when page navigation occurs.
Component: Untriaged → WebExtensions
OS: Unspecified → Windows 10
Product: Firefox → Toolkit
Hardware: Unspecified → x86_64
Version: 47 Branch → 50 Branch
Per-tab zoom is not supported. setZoomSettings is only a stub which returns an error if any of the passed values don't match the current settings. I guess it should ignore settings that weren't passed, though. Per-origin zoom should work as expected. There are tests for all of the cases that you mention in comment 0.
Updating my original comments ... 4. In the Chrome API documentation, although the ZoomSettings object can have three properties, only two (mode and scope) are used as parameters to setZoomSettings. It is noted that defaultZoomFactor is used to return the default zoom level for the current tab in calls to getZoomSettings. I do not understand why passing mode: "automatic" and scope: "per-origin" produces an error. As far as I can see, this is the only valid combination! 5. Since "per-tab" zoom is not supported, producing an error message is okay. However, I cannot find the lack of support documented anywhere and the current error message is misleading. 6,7,8. I thought I had tested these cases with a version of Zoom Page that had the failed calls to setZoomSettings and some experimental event-driven logic both commented out, but it seems I was wrong. I have redone my testing and can now confirm that these cases all work. Apologies! 9,10. Since "per-tab" zoom is not supported, my previous comments are not relevant. So, points 4 & 5 are the only ones that need addressing. It is a pity that "per-tab" zoom is not supported. It means that I will have to implement that functionality (and override "per-origin") directly in Zoom Page. My aim is to support per-origin, per-tab and all-tabs zooming in both Firefox and Chrome with a single source code.
Kris, I have done some more investigation of the WebExtensions "zoom" API's. It appears that "per-tab" zoom is supported if 'FullZoom.siteSpecific' (effectively 'browser.zoom.siteSpecific') is set to 'false' - see line 987 in browser/components/extensions/ext-tabs.js. setZoomSettings() returns an error message if the parameters are not the same as the current settings returned by _getZoomSettings(), which will be mode: "automatic", scope: "per-tab" or "per-origin" depending on FullZoom.siteSpecific, and defaultZoomFactor: 1. So, I have two issues with setZoomSEttings(): 1. To be compatible with Chrome, setZoomSettings() should ignore any 'defaultZoomFactor' parameter (not compare it to the current settings). 2. Why doesn't setZoomSettings() change the setting of 'browser.zoom.siteSpecific' to reflect the value of the 'scope' parameter?
Priority: -- → P2
Whiteboard: [zoom] triaged
Some more observations regarding setZoomSettings() and compatibility with Chrome: - To be compatible with the way Chrome works, it must be possible to set the zoom scope (per-origin or per-tab) for each tab independently, rather than for all tabs using the existing 'browser.zoom.siteSpecific' preference. - With Chrome, the zoom scope is automatically reset to 'per-origin' every time a page is loaded or reloaded. Setting the zoom scope to 'per-tab' for a particulat tab using setZoomSettings() only persists until that tab is loaded or reloaded.
Component: WebExtensions: Untriaged → WebExtensions: General
Priority: P2 → P5
Summary: WebExtensions: Zoom API's in chrome.tabs don't work correctly → Zoom API's in chrome.tabs don't work correctly
Product: Toolkit → WebExtensions

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.

The compatibility data for tabs.setZoomSettings was changed in December 2017 to flag this is unsupported. Is this therefore dev-doc-done?

Flags: needinfo?(dw-dev)

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.

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.

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?

Flags: needinfo?(tomica)

In light of comment 8 and the subsequent discussion, I'm removing dev-doc-needed. Please reapply if this bug results in a change.

Keywords: dev-doc-needed

Yes, that's fine.

Also, just noticed, in Comment 11, the title of Point 2 should be 2. browser.browserSettings.siteSpecific.

Flags: needinfo?(dw-dev)

(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).

Flags: needinfo?(tomica)

(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.

Type: defect → enhancement
Summary: Zoom API's in chrome.tabs don't work correctly → Add new zoomSiteSpecific and zoomFullPage settings in browserSettings API
Version: 50 Branch → 71 Branch

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.

Flags: needinfo?(mixedpuppy)

Jim, please can you make me the assignee for this bug.

Flags: needinfo?(jmathies)

(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: nobody → dw-dev
Flags: needinfo?(jmathies)

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.

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?

https://searchfox.org/mozilla-central/rev/029d9d2477ef0232bb08db94696badddec4d5bda/toolkit/components/telemetry/docs/data/environment.rst#385

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.

Flags: needinfo?(mixedpuppy) → needinfo?(philipp)

(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.

Add new zoomSiteSpecific and zoomFullPage settings in browserSettings API.

Add new zoomSiteSpecific and zoomFullPage settings in browserSettings API

Updated description of zoomSiteSpecific in brower_settings.json

Attachment #9119921 - Attachment description: Bug 1286953 - Patch 2 - Add new browserSettings APIs; r?mixedpuppy → Bug 1286953 - Add new browserSettings APIs; r?mixedpuppy
Attachment #9119921 - Attachment description: Bug 1286953 - Add new browserSettings APIs; r?mixedpuppy → Bug 1286953 - Add zoom settings to browserSettings APIs
Attachment #9119921 - Attachment description: Bug 1286953 - Add zoom settings to browserSettings APIs → Bug 1286953 - Patch 2 - Add new browserSettings APIs; r?mixedpuppy
Attachment #9119921 - Attachment description: Bug 1286953 - Patch 2 - Add new browserSettings APIs; r?mixedpuppy → Bug 1286953 - Add new browserSettings APIs; r?mixedpuppy

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'.

Attachment #9119356 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 10 → All
Hardware: x86_64 → All

@dw-dev can you look at bug 1590485 zombie just linked, should there be additional work here for exposing any settings?

Flags: needinfo?(dw-dev)

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.

Shane, Is there any way to move this forwards? It looks like Fallen is unavailable or too busy to review.

Flags: needinfo?(dw-dev) → needinfo?(mixedpuppy)

Looks like the patch can land now.

Flags: needinfo?(mixedpuppy)

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?

Flags: needinfo?(mixedpuppy)

(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.

Flags: needinfo?(philipp)
Flags: needinfo?(mixedpuppy)
Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6c405454620d Add new browserSettings APIs; r=mixedpuppy,Fallen
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75

(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.

Keywords: dev-doc-needed

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?

Flags: needinfo?(dw-dev)

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.

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.

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.

Thanks, I've set up a separate ticket on GitHub to check the other documentation

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?

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.

Is any more information required?

Flags: needinfo?(dw-dev)
Blocks: 1363856
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: