378549 - remember the value of the text zoom setting on a site-specific basis

Status: VERIFIED FIXED

(Firefox :: General, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

Most of the time, when users use the "text zoom" setting to change the size of text on a web page, they'd like to see the same size text the next time they visit that page, and in many cases they'd like that size to apply to all pages on the site they're browsing, not just that particular page.

But the setting currently only applies on a tab-specific basis.  Once a user closes a tab, their preference is lost, and when they return to that site they have to reset the setting to their preferred value.

The text zoom setting would be more useful Firefox remembered its value on a site-specific basis and would restore the value when users revisit sites on which they'd previously set a text zoom.

Comment 1

[Ria Klaassen (not reading all bugmail)]

See Bug 108391 and Bug 218302.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #1)
> See Bug 108391 and Bug 218302.

Err, right.  I guess this is a duplicate of bug 108391, although that bug is so heavy with the weight of years of accumulated comments and duplicates that I'd rather work over here.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: implements site-specific text zoom

In conjunction with patch v2 (attachment 264346 [details] [diff] [review]) on bug 378547, which implements a service for storing site-specific prefs, this patch implements a site-specific text zoom setting.

Note: as opposed to earlier versions of this implementation in the Content Preferences extension, in this patch the per-window object that used to be called the "controller" is now called the "event sink", while the per-setting object that used to be called the "handler" is now called the "controller".

The new names are intended to more accurately reflect what those objects actually do.

Outstanding issues:

1. This patch takes over the View > Text Size menu items, but it doesn't change that menu to reflect the fact that the items in it are now site-specific.  It might make sense to identify this detail, perhaps by adding the name of the site to the menu somehow.

2. There is at least one identified use case for the existing functionality: users who want to jack up the text size temporarily while sitting farther back from their monitor.  The current behavior doesn't fully satisfy this use case, since it increases text size on a tab-specific basis rather than for the browser as a whole.  But the new behavior undermines it, since it is no longer possible to set an overall text size.

To address this, we might add menu items for setting the general text size in addition to the site-specific size.  Or we could let users go into a "zoom everything" mode in which the text size changes they make would apply to the browser as a whole.  Or we might offer a text zoom toolbar widget with sliders for both site-specific and global zoom.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2.1: fix for bug in patch v2

This patch fixes a bug in the previous one that threw exceptions on certain pages (I think those with iframes).

Comment 5

[Myk Melez [:myk] [@mykmelez]]

> 2. There is at least one identified use case for the existing functionality:
> users who want to jack up the text size temporarily while sitting farther back
> from their monitor.  The current behavior doesn't fully satisfy this use case,
> since it increases text size on a tab-specific basis rather than for the
> browser as a whole.  But the new behavior undermines it, since it is no longer
> possible to set an overall text size.
> 
> To address this, we might add menu items for setting the general text size in
> addition to the site-specific size.  Or we could let users go into a "zoom
> everything" mode in which the text size changes they make would apply to the
> browser as a whole.  Or we might offer a text zoom toolbar widget with sliders
> for both site-specific and global zoom.

Or... or... or ask the UI guys what they think. cc:ing beltzner and faaborg.

Note: sspitzer has suggested we detect when fonts are smaller than users usually like them and display a notification with UI for increasing font size.  That'd improve discoverability without requiring UI to be always present, but faaborg has warned of the high user-annoyance cost of false positives (cf. the friendly paper clip).

Comment 6

[John P Baker]

I could end up looking foolish here...

I can't see where TextZoom gets its onContentPrefSet and onContentPrefRemoved observer calls from! It doesn't have an observer on the pref service, and neither does the ContentPrefSink which used to pass them on to its observers.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2.2: makes TextZoom controller observe pref changes

(In reply to comment #6)
> I could end up looking foolish here...

No, rather I'm the foolish one.  I stopped making ContentPrefSink observe and pass on these notifications but forgot to add the two lines of code that would make TextZoom start observing them.  Here's a patch that includes those two lines.

Re: the change, ContentPrefSink exists for a couple of reasons.  One is to reduce the amount of redundant code we have to write in pref controllers (particularly for location changes), but a more important one is to improve performance by only retrieving preferences once for each location change or DOMContentLoaded event.

But for pref changed notifications, there's no performance win to routing them through the sink; in fact, there might be a bit of a performance hit.  And there's also very little reduction in code size.  So it seemed to make sense to have controllers observe pref changes directly, although on the downside it increases controller complexity.

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

Talked with Beltzner about this, I agreed with his suggestion to create a global preference for text size in Preferences/Options, and that the commands in the View menu (control+/-/0) should change the site specific size, relative to the global preference.

We didn't talk about what happens to site specific sizes when you go back and change the global preference, but I think it makes sense to readjust them as well.  Beltzner?

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Hey Myk,

Alex and I chatted briefly about this in IRC, and came up with what we think is an ideal solution:

* add a browser-wide pref in the preferences dialog, possibly replacing the existing hard to understand "Default Text Size" option

* save page-specific settings as well, relative to the browser-wide preference

* the menuItems would act as a mechanism of changing the page-specific setting

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #8)
> We didn't talk about what happens to site specific sizes when you go back and
> change the global preference, but I think it makes sense to readjust them as
> well.  Beltzner?

Well, if they're always relative to the browser-wide pref, that'll happen automatically, I think.

Having said that, though, I think we might want to do the other thing. That is, save the site-specific settings as absolute values, so that if a user visits a bunch of pages & increases the font size, if they then learn of the browser-wide preference and globally increase the font size, they don't end up with a now-double-huge font size on those pages. That sounds more right to me.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: addresses UI review

Mike Beltzner wrote:

> * add a browser-wide pref in the preferences dialog, possibly replacing the
> existing hard to understand "Default Text Size" option

Ok, I've added this pref.  It consists of a text field for entering a numeric value plus a slider for selecting a value via click and drag.  The pref is labeled "Text Size", which seems suboptimal to me, although I'm having a hard time coming up with a better label ("Text Zoom" and "Magnify text" were two others).

Feedback welcome on the label and on whether it makes sense to have both a text field and a slider for this preference.

Technical note: I use the "global pref" feature of the content prefs service to store the pref, which means I have to do my own storage to and retrieval from that service instead of using the prefpane plumbing to do that work for me.

Although it's more code, I think it's better to store the pref with the content prefs service because that'll make it easier in the future to build interfaces that provide a consolidated view of both global and site-specific content prefs (f.e. the sidebar in the Content Preferences extension).


> * the menuItems would act as a mechanism of changing the page-specific setting

Yup, that's how they work.

> Having said that, though, I think we might want to do the other thing. That 
> is, save the site-specific settings as absolute values, so that if a user
> visits a bunch of pages & increases the font size, if they then learn of the
> browser-wide preference and globally increase the font size, they don't end up
> with a now-double-huge font size on those pages. That sounds more right to me.

Yeah, seems more right to me too, so this is the way I've implemented it.

Note: this implementation now offers site-specific as well as global persistence of text zoom preferences, but it still doesn't provide session-specific text zoom, which appears to be what John Lilly described when he said he sometimes wants to increase the text size temporarily so that he can lean back from his monitor and read the screen from several further feet away.

Other changes include removal of the dump statements that were scattered throughout the code and a fix for a bug that caused the wrong value to be applied when a user reset the site-specific pref to the global value.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

(Per PRD, not blocking on P2 items, but really-really want!)

Comment 13

[Mike Connor [:mconnor]]

ok, so, I talked with beltzner, seems like there's some wires crossed here.

We want to split the prefpane stuff off, the global zoom + page zoom + font size thing isn't quite right, I think what we want is something that uses the slider to adjust the base text size, and have that show a useful preview, i.e. something like the below:

- Fonts and Colors ---------------------

Font: [Lucida Grande                 \/]
Size: 16pt -----|----------------------
_______________________________________
| Preview of default font+size         |
|______________________________________|


That shouldn't, however, gate this bug (beltzner's going to spin this off).  I think we want to just have this bug be able the per-site zoom (we also discussed page vs. site zoom here, I think we want to try just site zoom for now, and add the per-page layer based on feedback)

Myk, does patch 2.2 cover the per-site only stuff?

Comment 14

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #13)
> Myk, does patch 2.2 cover the per-site only stuff?

Patch 2.2 covers both per-site stuff and global stuff, but the global stuff is well-encapsulated in the last three files of the patch, so excluding the global stuff is simply a matter of removing those files from the patch.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #13)
> ok, so, I talked with beltzner, seems like there's some wires crossed here.
> 
> We want to split the prefpane stuff off, the global zoom + page zoom + font
> size thing isn't quite right, I think what we want is something that uses the
> slider to adjust the base text size, and have that show a useful preview, i.e.
> something like the below:
> 
> - Fonts and Colors ---------------------
> 
> Font: [Lucida Grande                 \/]
> Size: 16pt -----|----------------------
> _______________________________________
> | Preview of default font+size         |
> |______________________________________|

Given that most sites reduce the default size, I don't think such a preview will be super useful. Why not just update the content immediately?
As for the font family, I don't think it should be part of the site-specific UI at all. It won't have an effect in most cases.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: same as patch v3 minus global stuff

Per mconnor's recent comments, here's a version of the patch without any UI for setting a global text zoom pref.

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 268139 [details] [diff] [review]
patch v4: same as patch v3 minus global stuff

Index: browser/base/content/browser-contentPrefSink.js


+  // Note: we can't use the Ci shortcut here because it isn't defined yet.

its not? seems like it should be, but I might be missing something.

general note: can we preprocess the large header comments away, its sort of odd to not do that for files being included in browser.js...


+  //**************************************************************************//
+  // Event Handlers
+
+  // nsIDOMEventListener
+
+  handleEvent: function TextZoom_handleEvent(event) {
+    // The only events we handle are DOMMouseScroll events.
+    this._handleMouseScrolled(event);
+  },
+
+  _handleMouseScrolled: function TextZoom__handleMouseScrolled(event) {
+    // Construct the "mousewheel action" pref key corresponding to this event.
+    // Based on nsEventStateManager::GetBasePrefKeyForMouseWheel.
+    var pref = "mousewheel";

+    if (event.scrollFlags & MOUSE_SCROLL_IS_HORIZONTAL)
+      pref += ".horizscroll";

+    if (event.shiftKey)
+      pref += ".withshiftkey";
+    else if (event.ctrlKey)
+      pref += ".withcontrolkey";
+    else if (event.altKey)
+      pref += ".withaltkey";
+    else if (event.metaKey)
+      pref += ".withmetakey";
+    else
+      pref += ".withnokey";

+    pref += ".action";

I assume this is the order in which the backend code checks the modifiers? can you put newlines around the big block for readability?

+    // XXX Lazily cache all the possible action prefs so we don't have to get
+    // them anew from the pref service for every scroll event?  We'd have to
+    // make sure to observe them so we can update the cache when they change.

the overhead of a bunch of observers might not be worth it, hard to say

+  //**************************************************************************//
+  // Utilities
+
+  _ensureValid: function TextZoom__ensureValid(aValue) {
+    if (isNaN(aValue))
+      return this.defaultValue;
+    else if (aValue < this.minValue)
+      return this.minValue;
+    else if (aValue > this.maxValue)
+      return this.maxValue;
+    else
+      return aValue;
+  },

else after return to the nth degree!  if (foo) return; if (bar) return; please

+  /**
+   * Get a value from a pref or a default value if the pref doesn't exist.
+   *
+   * @param   aPrefName
+   * @param   aDefaultValue
+   * @returns the pref's value or the default (if it is missing)
+   */
+  _getAppPref: function TextZoom__getAppPref(aPrefName, aDefaultValue) {
+    try {
+      switch (this._prefBranch.getPrefType(aPrefName)) {
+      case this._prefBranch.PREF_STRING:
+          return this._prefBranch.getCharPref(aPrefName);
+      case this._prefBranch.PREF_BOOL:
+        return this._prefBranch.getBoolPref(aPrefName);
+      case this._prefBranch.PREF_INT:
+        return this._prefBranch.getIntPref(aPrefName);
+      }
+    }

the indenting is kinda weird here, we tend to indent cases, though Ben wrote some switches that didn't

I'd probably throw in a newline after each, its a little dense with all of the this._prefBranch prefixes

r=me though, great work!

Comment 18

[John P Baker]

Drive-by nits:

>+  _getObservers: function ContentPrefService__getObservers(name) {

ContentPrefSink__getObservers

>+    var observers = [];


TextZoom_onContentPrefSet(aGroup, aName, aValue) {
>+    if (aGroup == this._cps.grouper.group(gBrowser.currentURI))
>+        this._applyPrefToSetting(aValue);

nit: too large indent

>+    else if (aGroup == null) {
>+      this.globalValue = this._ensureValid(aValue);


>+  onContentPrefRemoved: function TextZoom_onContentPrefRemoved(aGroup, aName) {
>+    if (aGroup == this._cps.grouper.group(gBrowser.currentURI))
>+        this._applyPrefToSetting();

nit: ditto

>+    else if (aGroup == null) {
>+      this.globalValue = undefined;


>+  _getAppPref: function TextZoom__getAppPref(aPrefName, aDefaultValue) {
>+    try {
>+      switch (this._prefBranch.getPrefType(aPrefName)) {
>+      case this._prefBranch.PREF_STRING:
>+          return this._prefBranch.getCharPref(aPrefName);

nit: ditto

>+      case this._prefBranch.PREF_BOOL:
>+        return this._prefBranch.getBoolPref(aPrefName);

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4.1: nits fixed

(In reply to comment #17)
> +  // Note: we can't use the Ci shortcut here because it isn't defined yet.
> 
> its not? seems like it should be, but I might be missing something.

For Places builds, per the comment in browser.js on lines 62-64, those shortcuts are defined by the Places controller.  But the controller gets loaded by placesOverlay.xul, which gets overlaid onto browser.xul after the contents of browser.js (and thus browser-contentPrefSink.js) have already been loaded.


> general note: can we preprocess the large header comments away, its sort of 
> odd to not do that for files being included in browser.js...

Sure.  Per our chat, I'm using the following technique so as not to hork syntax checking in my IDE:

/*
#ifdef 0
 * ***** BEGIN LICENSE BLOCK *****
...
 * ***** END LICENSE BLOCK *****
#endif
 */


> I assume this is the order in which the backend code checks the modifiers?

Yes, this is the order that nsEventStateManager::GetBasePrefKeyForMouseWheel uses.


> can you put newlines around the big block for readability?

Sure thing.


> 
> +    // XXX Lazily cache all the possible action prefs so we don't have to get
> +    // them anew from the pref service for every scroll event?  We'd have to
> +    // make sure to observe them so we can update the cache when they change.
> 
> the overhead of a bunch of observers might not be worth it, hard to say

Yeah, I wasn't sure about it, and I didn't want to optimize prematurely, but it's something to think about for the future.


> else after return to the nth degree!  if (foo) return; if (bar) return; 
> please

You got it.


> the indenting is kinda weird here, we tend to indent cases, though Ben wrote
> some switches that didn't

Awesome!  I'd much rather indent cases.  I only didn't do so because I thought not indenting them was the standard (I think I heard this from Ben a while ago).  In any case, I've indented these and will do so for other cases from now on.


> I'd probably throw in a newline after each, its a little dense with all of 
> the this._prefBranch prefixes

Ok, sounds good.


(In reply to comment #18)
> Drive-by nits:
> 
> >+  _getObservers: function ContentPrefService__getObservers(name) {
> 
> ContentPrefSink__getObservers

Good catch.


> TextZoom_onContentPrefSet(aGroup, aName, aValue) {
> >+    if (aGroup == this._cps.grouper.group(gBrowser.currentURI))
> >+        this._applyPrefToSetting(aValue);
> 
> nit: too large indent

Ah, indeed.

> >+  onContentPrefRemoved: function TextZoom_onContentPrefRemoved(aGroup, aName) {
> >+    if (aGroup == this._cps.grouper.group(gBrowser.currentURI))
> >+        this._applyPrefToSetting();
> 
> nit: ditto

Yupski.


> >+    else if (aGroup == null) {
> >+      this.globalValue = undefined;
> 
> 
> >+  _getAppPref: function TextZoom__getAppPref(aPrefName, aDefaultValue) {
> >+    try {
> >+      switch (this._prefBranch.getPrefType(aPrefName)) {
> >+      case this._prefBranch.PREF_STRING:
> >+          return this._prefBranch.getCharPref(aPrefName);
> 
> nit: ditto

Bien sur!


Here's a patch with the nits fixed.  This is the patch I'll check in, presuming Mike's review carries forward.  Cancelling UI review request since the fix no longer makes any UI modifications (I removed them in patch v4).

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #19)
> (In reply to comment #17)
> > +  // Note: we can't use the Ci shortcut here because it isn't defined yet.
> > 
> > its not? seems like it should be, but I might be missing something.
> 
> For Places builds, per the comment in browser.js on lines 62-64, those
> shortcuts are defined by the Places controller.  But the controller gets loaded
> by placesOverlay.xul, which gets overlaid onto browser.xul after the contents
> of browser.js (and thus browser-contentPrefSink.js) have already been loaded.

Mano and I talked about changing this so that we just use vars and unconditionally set them where they're needed, ignoring potential strict warnings if that's what it takes. We need a better solution for this :(

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Checked into the trunk:

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.799; previous revision: 1.798
done
Checking in browser/base/content/browser-sets.inc;
/cvsroot/mozilla/browser/base/content/browser-sets.inc,v  <--  browser-sets.inc
new revision: 1.98; previous revision: 1.97
done
RCS file: /cvsroot/mozilla/browser/base/content/browser-contentPrefSink.js,v
done
Checking in browser/base/content/browser-contentPrefSink.js;
/cvsroot/mozilla/browser/base/content/browser-contentPrefSink.js,v  <--  browser-contentPrefSink.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/base/content/browser-textZoom.js,v
done
Checking in browser/base/content/browser-textZoom.js;
/cvsroot/mozilla/browser/base/content/browser-textZoom.js,v  <--  browser-textZoom.js
initial revision: 1.1
done

Comment 22

[Jeff Walden [:Waldo]]

There should be a test or two in Litmus for this.

Comment 23

[j.j.]

Are bug 108391 and bug 218302 fixed with this?

Comment 24

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #23)
> Are bug 108391 and bug 218302 fixed with this?

Yes, thanks for the reminder.  I have resolved those bugs fixed.

Comment 25

[Adam Guthrie]

Litmus testcases are here: http://litmus.mozilla.org/show_test.cgi?searchType=by_category&product_id=1&branch_id=15&testgroup_id=55&subgroup_id=821.

Comment 27

[Tony Chung [:tchung]]

This bug is verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X; es-ES; rv:1.9a9pre) Gecko/2007101504 Minefield/3.0a9pre.   

Also the litmus cases all passed back on 9/21.  

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Especially now that we have full zoom this makes UI to work strangely.
I really don't want to zoom all pages from same domain.
I was zooming one b.m.o page, and then switched to other b.m.o tab
and noticed that it was zoomed too and I immediately thought that there
must be a bug somewhere. Apparently not a bug but "feature" :/
Is there at least some pref to disable the behavior?

Has there been any user tests regarding this feature?
(Do we have any user tests anyway)
I understand that in some cases this feature is useful; if I have just one page
open from a site and I restart the browser, it is useful to get back the same 
zoom level. But to change the zoom level of pages which I obviously don't
want to zoom in anyway is wrong. Not sure what the best solution could be.
Maybe not changing the zoom level of other tabs, but when closing the browser, storing the zoom level of the previously focused tab/per site.

Comment 29

[Brian Polidoro]

(In reply to comment #28)
> Is there at least some pref to disable the behavior?

http://kb.mozillazine.org/Browser.zoom.siteSpecific