452557 - Consider adding a 'zoom text only' menu-item entry

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, enhancement)

Description

[philippe (part-time)]

Something like this:
Zoom In
Actual Size
Zoom Out
--
Zoom Text Only
(adjust text-label as needed)

When checked, only text will be resized, using the default shortcuts [Cmd(+shift)+, Cmd-. See the equivalent Firefox menu entry under View > Zoom
Optionally, when the menu-item is checked, holding the option key would temporarily override (that is, scale whole page).

From bug 389795 comment #23
> ...
> 
> I'm pretty sure I'm not the only one who vastly prefers 'text-zoom'. The
> keyboard shortcut for 'Zoom In' already requires the Shift key on my keyboard
> (apple-jis kbd). Adding the Option key makes text-zoom less conformable. 

And bug 389795 comment #25
> (In reply to comment #23)
> > Regarding the text-zoom issue. Has it been considered to have something like
> > what Fx3+ does: adding 1 (one) menu item: 'zoom text only' ?
> 
> Stuart, Sam, and I discussed that (or something close to that) the other day,
> and the decision, after some back and forth, was to land things as described in
> comment 18/19 and evaluate feedback from nightlies and alphas.
> 

---

PS - I know it is easy to change keyboard settings; I've already swapped the kbd-shortcuts on my side. :-)

Comment 1

[philippe (part-time)]

fwiw, the just release version of Safari 4 beta now defaults to page zoom, but includes a menu item 'Zoom text only'.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

If we want to do this for 2.0, we need to decide so that it can be done for b3; it's now or never.

Comment 3

[Ilya Sherman]

Note also that the current Camino shortcuts for text zoom (opt cmd = and opt cmd -) conflict with the OS Universal Access shortcuts for zoom.

Comment 4

[Christopher Henderson]

I have a working implementation of this for the menu items. I will polish it before the next meeting.

A related issue that needs to be resolved is the behaviour of the toolbar items. Should we just keep the current sets, ie: separate text and content zoom buttons? Maybe we could just have one set that does zooming, and change the icons depending on whether 'Zoom Text Only' is set (if such icon changing is possible). Safari 4 keeps the same icons no matter what mode it's in.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I think we want to allow people to mix and match buttons and menu defaults.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

[12:53pm] pinkerton: sure
[12:54pm] hendy: righto, i'll get a patch together.

Comment 7

[Christopher Henderson]

Attachment: Fix v1.0

Adds Zoom In, Zoom Out, Actual Size, and Zoom Text Only items to the View menu. Adds corresponding methods to MainController and BWC, as well as validation methods. Updates BrowserWindow's pKE method. Uses the browser.zoom.full Gecko pref (http://kb.mozillazine.org/Browser.zoom.full) to keep track of the Zoom Text Only item.

Comment 8

[Christopher Henderson]

Attachment: Updated MainMenu.nib

Corresponding MainMenu.nib with the new View menu items. Will need to be resaved in 10.4 once the patch has passed sr.

Comment 9

[Stuart Morgan]

Comment on attachment 376012 [details] [diff] [review]
Fix v1.0

Sorry for the slow review here. Two issues:
1) This removes the ability to use Option to change the behavior temporarily; that wasn't my understanding of what we wanted (although I'm not sure it was ever explicitly discussed, so maybe we need to hammer that out).
2) I don't like MainController being a repository of pref state for other classes. How about we change the target of the menu, rather than having BWC consult with MC about expected behavior?

Comment 10

[Stuart Morgan]

... and by "target" I mean "action" :P

Comment 11

[Christopher Henderson]

Attachment: Fix v1.1

Now includes opt key alternatives. Items are swapped when Zoom Text Only is clicked on, and when browser.zoom.full is changed. The swapping is done by looping through the six (three zoom actions, with alts) menu items above the Zoom Text Only item. This is rather inelegant, but works. Another method would be to add three IBOutlets for the zoom actions and loop through them instead.

Comment 12

[Christopher Henderson]

Attachment: MainMenu.nib, Version 1.1

New MainMenu.nib, with an alternate item for Zoom Text Only, so holding down the option key gives correct-looking items. Saved with 10.5; will resave on 10.4 once patch has passed sr.

Comment 13

[Ilya Sherman]

Comment on attachment 385268 [details] [diff] [review]
Fix v1.1

Looks pretty good; below are mostly nits/requests for clearer documentation (and largely a summary of tonight's IRC discussion)


>+- (void)fullContentZoomPrefChanged:(NSNotification*)aNotification

This should have a more generic name.  For now, we can leave the method body the same, but there should be a comment noting that if we want to be notified of other pref changes also, this method will need to check which pref was changed and decide accordingly what the response should be.

>+- (IBAction)toggleTextZoom:(id)aSender

Include a comment indicating that aSender is expected to be nil, the Text Zoom Only menu item, or its alternate.  Also, might be helpful to mention that for any given menu item, the index of the corresponding alternate menu item is +1 (perhaps this is a common NIB convention, in which case I guess you don't need to mention it).
And, don't forget other method-level comments ;)

>+  // Return straight away if the sender is the alternate menu item,
>+  // since toggling the alternate state is equivalent to doing nothing.

Expand this comment a bit, to the effect of "the regular state of zoom should become the inverse of whatever is on the screen at that moment"

>+  if(aSender && (aSender != mTextZoomOnlyMenuItem))

For greater clarity, I would write this as below, (the code would need to be rearranged a bit for swapIndex to already be defined at this point)

NSMenuItem *alternateTextZoomOnlyMenuItem = [mViewMenu itemAtIndex:swapIndex+1]
if (aSender == alternateTextZoomOnlyMenuItem)


>+  int swapIndex = [mViewMenu indexOfItem:mTextZoomOnlyMenuItem];

I would call this something like |toggleMenuItemIndex|, since you don't actually swap another menu item into this index

>+  [[mViewMenu itemAtIndex:swapIndex+1] setState:textZoomOnly];

If you make the change above, you can write this as [alternateTextZoomOnlyMenuItem setState:textZoomOnly];

>+  for (int i = swapIndex-2; i >= swapIndex-6; i=i-2) {

Comment that you are looping over the relevant 3 menu items and their alternates

>+    NSMenuItem* topItem = [mViewMenu itemAtIndex:i];
>+    NSMenuItem* bottomItem = [mViewMenu itemAtIndex:i+1];

Perhaps call these |defaultItem| and |alternateItem|

>+    NSString* tempTitle = [[topItem title] copy];

I think smorgan generally recommends writing this as [[[topItem title] copy] autorelease];

>+    [tempTitle release];

and then removing this line.


>+  // Only flip the preference if we have a sender (the menu item).
>+  if (aSender) {
>+    PreferenceManager* prefManager = [PreferenceManager sharedInstance];
>+    BOOL fullContentZoomPref = [prefManager getBooleanPref:kGeckoPrefFullContentZoom
>+                                               withSuccess:NULL];
>+    [prefManager setPref:kGeckoPrefFullContentZoom toBoolean:!fullContentZoomPref];
>+  }

Why not just have the body be [prefManager setPref:kGeckoPrefFullContentZoom toBoolean:!textZoomOnly]; ?

Comment 14

[Christopher Henderson]

Attachment: Fix v1.2

Addresses the review comments.

>For greater clarity, I would write this as below, (the code would need to be
>rearranged a bit for swapIndex to already be defined at this point)

The intent was to do this check before having to do anything else, like defining local variables.

Comment 15

[Ilya Sherman]

Comment on attachment 387619 [details] [diff] [review]
Fix v1.2

(In reply to comment #14)

r=me, with the nit below

>+  [prefManager setPref:kGeckoPrefFullContentZoom toBoolean:![self isTextZoomOnly]];

I think you can use the cached |textZoomOnly| here


> >For greater clarity, I would write this as below, (the code would need to be
> >rearranged a bit for swapIndex to already be defined at this point)
> 
> The intent was to do this check before having to do anything else, like
> defining local variables.

Perhaps I'm missing something, but I'm not sure what the advantage is -- AFAICT, the code is less fragile if you pick out the alternate menu item first, and there isn't really a noticeable performance cost to doing it.  But, if you want to do the check first, I guess that's o.k. too.

Comment 16

[Stuart Morgan]

Comment on attachment 387619 [details] [diff] [review]
Fix v1.2

>+  if (textZoomOnly) // Full content zoom is the default

One more space before the //.

>+  // We need to register a Gecko pref observer and then register an
>+  // NSNotificationCenter observer for the pref change notification.
>+  [prefManager addObserver:self forPref:kGeckoPrefFullContentZoom];
>+  [[NSNotificationCenter defaultCenter] addObserver:self

The comment should go; it reads almost exactly like the code.

I don't see a corresponding removeObserver for either of these in dealloc (it looks like there should have already been one for NSNotificationCenter, but apparently it's missing).

>+  if (fullContentZoomPref == textZoomOnly)
>+    [self toggleTextZoom:nil];

I had to read this a couple of times before I understood why it wasn't wrong. Maybe change fullContentZoomPref to textZoomOnlyPref (which textZoomOnly becoming textZoomOnlyUISet or similar) and negate the pref as you read it, so you can do a more natural != here.

>+  // The regular state of zoom should become the inverse of whatever is on the
>+  // screen at that moment. When the sender is the alternate menu item, the
>+  // inverse is equal to the regular state, and thus should not be changed.
>+  if (aSender && (aSender != mTextZoomOnlyMenuItem))
>+    return;

I'm confused. Why on earth does mTextZoomOnlyMenuItem have an alternate?

>+  // Loop over the zoom (in, actual, out) items and their alternates,
>+  // swapping their respective titles and actions.
>+  for (int i = toggleMenuItemIndex-2; i >= toggleMenuItemIndex-6; i=i-2) {

Doing this by index seems a bit fragile; our usual approach when we need to operate on a block of menu items (e.g., the context menu stuff in BWC) is to use a menu item tag. I would say give all 3 primary items the same tag, and call it something like kZoomActionsTag in the code, and find them based on the tag. The alternates you can operate on by relative index to the primary of course.

>+    // Alternate menu items are directly below the regular item
>+    NSMenuItem* alternateItem = [mViewMenu itemAtIndex:i+1];

Again, the comment here is not really saying anything that's not clear from reading the line of code.

>+  PreferenceManager* prefManager = [PreferenceManager sharedInstance];
>+  [prefManager setPref:kGeckoPrefFullContentZoom toBoolean:![self isTextZoomOnly]];

This should only be done if sender is nil, since a Gecko pref change callback should not trigger call to set the Gecko pref.

>+- (BOOL)isTextZoomOnly

"is" is an odd verb here since you would say "The MainController is textZoomOnly"; it's also not clear that this refers to the UI rather than the pref. How about textZoomMenuItemEnabled? Although I wonder if that's really enough of a readability win over just inlining the code to make the method worthwhile.

>+      BOOL textZoomOnly = ![[PreferenceManager sharedInstance] getBooleanPref:kGeckoPrefFullContentZoom
>+                                                                  withSuccess:NULL];

Maybe s/textZoomOnly/zoomTextOnly/ ?

>+      if ((textZoomOnly && altModifier) || (!textZoomOnly && !altModifier)) {

This would be clearer if you just do
  if (altModifier)
    textZoomOnly = !textZoomOnly
then switch on whether or not we are doing text zoom.

Comment 17

[Christopher Henderson]

Attachment: Fix v1.3

> I don't see a corresponding removeObserver for either of these in dealloc.

Done.

> Maybe change fullContentZoomPref to textZoomOnlyPref…

Done.

> I'm confused. Why on earth does mTextZoomOnlyMenuItem have an alternate?

When the user holds down Opt, the zoom items and the state of the text zoom only item would be at odds. Eg: if text zoom is true, the Opt key state would be Zoom In/Zoom Out and Text Zoom Only being checked. The alternate is there so they match.

> Doing this by index seems a bit fragile…use a menu item tag.

Done, with an NSEnumerator and tag.

> This should only be done if sender is nil, since a Gecko pref change callback should not trigger call to set the Gecko pref.

I assume you meant "if aSender is not nil". Done.

> Although I wonder if that's really
enough of a readability win over just inlining the code to make the method
worthwhile.

isTextZoom has been excised.

> This would be clearer if you just do…

Done.

Comment 18

[Christopher Henderson]

Attachment: MainMenu.nib, Version 1.3

Corresponding MainMenu.nib, with added zoom item tags. IB 2.5 on Leopard.

Comment 19

[Stuart Morgan]

Comment on attachment 392600 [details] [diff] [review]
Fix v1.3

sr=smorgan with nits.

>+  [[mViewMenu itemAtIndex:toggleMenuItemIndex+1] setState:textZoomOnly];

Here and below, change :foo+1 to :(foo + 1)

>+               toBoolean:!([mTextZoomOnlyMenuItem state] == NSOnState)];

toBoolean:([mTextZoomOnlyMenuItem state] != NSOnState)];

>+      BOOL altModifier = ([theEvent modifierFlags] & NSAlternateKeyMask) != 0;
>+
>+      if (altModifier)
>+        zoomTextOnly = !zoomTextOnly;

This is a logic block, so get rid of the blank line (or just inline the condition; I'm not sure it makes much of a readability difference, but I'm fine with a temp variable if you feel that it does).

Comment 20

[Christopher Henderson]

Attachment: Fix, for checkin

Fix ready for checkin, with sr comments addressed.

Comment 21

[Christopher Henderson]

Attachment: MainMenu.nib, for checkin

New MainMenu.nib, with zoom menu items. Saved with IB 2.5.4 on Tiger.

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Checked in on the CAMINO_2_0_BRANCH and cvs trunk.  My internet made me feel a bit like http://quotes.burntelectrons.org/1788 in the process, though :P