414636 - make global page zoom preference multiplicative

Status: RESOLVED WONTFIX

(Firefox :: Settings UI, defect)

Description

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

There are two ways to implement a global page zoom pref: apply it to those sites that don't have a site-specific pref, or apply it to all sites and make it a multiplier to the site-specific pref, if any.

For example, if the global pref is 120%, and the site-specific pref for a given site is 130%, then the former approach sets the zoom for the site to 130%, while the latter approach sets it to 156% (130% * 120%).

The PageZoom controller implements the former approach, but there currently isn't any UI for setting the global page zoom, so the functionality isn't used.  After some consideration, I think it would be better to implement the latter approach, because that would serve two identified use cases from users I've conversed with:

    "I want to temporarily increase the size of all sites when sitting
     farther away from my monitor."

    "I want to temporarily increase the size of all sites in the morning,
     when my eyesight is worse."

For the former approach, on the other hand, I haven't identified any concrete use cases.

Note: I'm not suggesting we implement UI for this in Firefox.  I'm only suggesting that we make the current functionality work in a more useful way so that extensions can implement UI to support it.

Requesting wanted-firefox3 for this extensibility improvement.  Also adding uiwanted so that the UX folks are aware of this and can weigh in if they want.

Comment 1

[Elmar Ludwig]

I really like this suggestion. Is there any way to set the global page zoom pref at the moment, apart from using sqlite tools?

Comment 2

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

(In reply to comment #1)
> I really like this suggestion. Is there any way to set the global page zoom
> pref at the moment, apart from using sqlite tools?

No, unfortunately, at the moment there's no way to set the global pref.

Comment 3

[Elmar Ludwig]

Attachment: first attempt

This basically works, but I need to think about it some more, so not requesting review yet.

Should the ZoomManager settings (MIN, MAX, fullZoomValues) be applied to the combined value or to the site-specific value alone? Should the global zoom value always set full page zoom or should it follow thef pref introduced in bug 401322 (once that patch lands)?

Comment 4

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

> Should the ZoomManager settings (MIN, MAX, fullZoomValues) be applied to the
> combined value or to the site-specific value alone?

If we think it doesn't make sense to zoom pages beyond MIN and MAX, then I imagine they should be applied to the combined value.


> Should the global zoom
> value always set full page zoom or should it follow thef pref introduced in bug
> 401322 (once that patch lands)?

I imagine it should respect the pref introduced in bug 401322.  I think the use cases are similar for text zoom, and a multiplicative global text zoom is more useful than one that only applies to sites without their own zoom prefs.

Comment 5

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

Comment on attachment 301181 [details] [diff] [review]
first attempt

Overall this patch seems right to me, except that it no longer applies and should respect the pref in bug 401322.  Elmar, can you update the patch now that you've landed that other bug?

Comment 6

[Elmar Ludwig]

Attachment: patch v1.1 (updated to trunk)

This is the patch updated to trunk. I also tried to simplify the tests in _applyPrefToSetting() a bit.

There is one remaining problem though: If I set the global value to e.g. "1.1", then zoom one step in and one step out again using the keyboard shortcuts, I get a zoom value of "1.0". This is because the ZoomManager always jumps to the next zoom step in the list of fixed zoom values (.5, .75, 1, 1.25, 1.5, 2, 3).

Comment 7

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

(In reply to comment #6)
> Created an attachment (id=305453) [details]
> patch v1.1 (updated to trunk)
> 
> This is the patch updated to trunk. I also tried to simplify the tests in
> _applyPrefToSetting() a bit.

Yup, this code looks great.


> There is one remaining problem though: If I set the global value to e.g. "1.1",
> then zoom one step in and one step out again using the keyboard shortcuts, I
> get a zoom value of "1.0". This is because the ZoomManager always jumps to the
> next zoom step in the list of fixed zoom values (.5, .75, 1, 1.25, 1.5, 2, 3).

Hmm, that is a problem.  Seems like the solution would be for FullZoom::enlarge and ::reduce to calculate the zoom value itself, taking into account the global value, and then set ZoomManager::zoom directly instead of having ZoomManager::enlarge and ::reduce calculate the zoom value.

Do you think you could create a patch that does that?

Comment 8

[Elmar Ludwig]

(In reply to comment #7)
> Seems like the solution would be for FullZoom::enlarge and ::reduce to
> calculate the zoom value itself, taking into account the global value, and
> then set ZoomManager::zoom directly instead of having ZoomManager::enlarge
> and ::reduce calculate the zoom value.

But doing so would duplicate a lot of the ZoomManager code inside the FullZoom implementation, so I'm not sure I like that. One alternative would be to automatically include the global value in the list of zoom steps inside the ZoomManager (but only if is not already in the list), if this is possible.

I can create a patch that does either of this, but I'll have to experiment a bit to see which one looks better to me.

Comment 9

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

(In reply to comment #8)
> But doing so would duplicate a lot of the ZoomManager code inside the FullZoom
> implementation, so I'm not sure I like that. One alternative would be to
> automatically include the global value in the list of zoom steps inside the
> ZoomManager (but only if is not already in the list), if this is possible.

Hmm, yeah, it's not entirely clear what the right thing to do is.  Does the auto-include approach work for all multiples of the zoom steps?


> I can create a patch that does either of this, but I'll have to experiment a
> bit to see which one looks better to me.

Ok, sounds good.  Thanks for looking into this.  I stand ready to review a patch with whichever approach works best in your testing.

Comment 10

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

Elmar: ping; any update on this?

Comment 11

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.