181712 - Menu item for "default font size" (also indicate current text/font zoom value/size?)

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, enhancement, P4)

Description

[Stephane Moureau]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021122 Chimera/0.6+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021122 Chimera/0.6+

After using Bigger/Smaller Text it's not possible to find back the Page Text Sizes.
You have to close and reopen it.
Would it be possible to add a View menu something like "View - Page Text Sizes"
or another clearer name ?

Reproducible: Always

Steps to Reproduce:

Comment 1

[S Woodside]

Do you mean that it's difficult to "reset" the size to the original size? Is
this really that big a deal? I've never had this trouble myself (and I often use
the size controls).

Comment 2

[Greg K.]

Hm, I'm going to confirm this. Chimera could give some indication of the current
text zoom value. Two ways I can think of to do this are:

1. A Text Zoom menu showing the current value à la Mozilla
2. Show the value on the Status Bar (and word this text and menu items so their
connection is clear)

Comment 3

[Jasper]

This would be a nice addition, so I confirm. I find my self thinking of this
option everytime I enlarge a pages text size. And than can't figure out what it
is or what is was. 

It's bit strange that you click a button that enlarges or reduces but there
isn't a way to see how much you enlarged or reduced the text, it feel a bit like
gambling!

Comment 4

[Kathleen :Brade]

-->sfraser for triage

Comment 5

[Samuel Sidler (old account; do not CC)]

I agree with this feature request. Seems reasonable enough for 1.0 as it's an
accessibility issue.

Targeting for 1.0.

Comment 6

[Samuel Sidler (old account; do not CC)]

Ping again.

Is this something that's as simple as a nib change or two? How easy would this
be for 09?

Comment 7

[Samuel Sidler (old account; do not CC)]

I seem to recall seeing this bug elsewhere. Is there another bug filed on this?

I propose we do an option in the View menu called "Change Text Size" and have
three options within it: "Bigger Text", "Default Text Size", and "Smaller Text".

Comment 8

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

(In reply to comment #7)
> I seem to recall seeing this bug elsewhere. Is there another bug filed on this?

You're probably thinking of bug 280390.
 
> I propose we do an option in the View menu called "Change Text Size" and have
> three options within it: "Bigger Text", "Default Text Size", and "Smaller Text".

I'm all for a "return to default size" option so it's one keystroke to make the
next page you visit normal size instead of huge, which is what this bug was
originally asking for.  The comments and summary, however, somehow morphed into
showing what the current zoom ratio is (90%, 150%, etc.), which I can't figure
out a compelling reason for doing.

Comment 9

[Samuel Sidler (old account; do not CC)]

*** Bug 314289 has been marked as a duplicate of this bug. ***

Comment 10

[Samuel Sidler (old account; do not CC)]

Attachment: MacIE font zoom menu

I just found that compelling reason... IE did it. ;) And... actually, it doesn't look that bad. It seems reasonable enough, anyway...

Comment 11

[Samuel Sidler (old account; do not CC)]

-> 1.1, part of menus cleanup.

Comment 12

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

I hereby claim Cmd-0 (zero) for the shortcut for "Default Size" :P

I'm still not convinced we need to show the current zoom value, and I think doing so would clutter the menu.  People want to make small text larger, large text smaller, and to quickly return to the default size when following a link to a more reasonably-sized page after having modified the size for a poorly-sized one; they don't care if the page's fonts are at 150% or 200%

Comment 13

[froodian (Ian Leue)]

Attachment: Patch

This does exactly what comment 7 says (and is more or less in accordance with the mockup on the meta bug).  It uses Cmd-0 as the shortcut.

One potential issue is that I route the calls through BrowserWindowController, even though it doesn't use them because
a) That's what the bigger/smaller text menu items do and
b) If we ever want to make a default text toolbar button, we'll need it.
This is somewhat circuitous and strange, and I'm a little tempted not to do it, but I wanted to keep parity with the bigger/smaller items.  Thoughts?

BTW, I'm not attaching the nib yet, since it's dirty from another bug, and by the time this gets reviewed, that probably will have landed.

Comment 14

[froodian (Ian Leue)]

Attachment: Patch

This gets rid of the unnecessary routing through BWC.  So, it:
- Implements defaultFontSize, which sets the font to the default size if the page isn't about:blank or already at the default font size
- Implements textIsDefaultSize in order to accomplish the above (seeing if the page is already at the default font size)

Comment 15

[froodian (Ian Leue)]

Attachment: New MainMenu.nib

This add a "Text Size" submenu directly below Reload (and gets rid of the following separator and text size items).  The submenu contains: Bigger Text, Default Text Size (cmd-0) and Smaller Text.

Comment 16

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

Comment on attachment 228316 [details]
New MainMenu.nib

I don't think it makes any sense to shove 3 items in a submenu in a menu that's this small already.

(I also don't think that most of the additions that Sam has in that menu will ever end up there; if they do and the menu looks bloated, we can revisit this then.  But 3 options don't merit a submen.)

Comment 17

[froodian (Ian Leue)]

Attachment: New MainMenu.nib

No submenu - just adds 'Default Text Size' between the existent bigger/smaller text menu items.

Comment 18

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

Comment on attachment 228580 [details]
New MainMenu.nib

Much more sane, r=me

Comment 19

[Nick Kreeger]

Comment on attachment 228315 [details] [diff] [review]
Patch

 - (void)biggerTextSize;
 - (void)smallerTextSize;
+- (void)defaultTextSize;

Im not a big fan of the names of these methods, perhaps we can rename them to something like:
  |makeTextBiggerSize:|, |makeTextSmallerSize:|, ...

 - (BOOL)canMakeTextBigger;
 - (BOOL)canMakeTextSmaller;
+- (BOOL)textIsDefaultSize;

Also I like to use the "is" prefix on BOOL methods as much as possible, this is a good time to do that:
  |isTextDefaultSize:|
  
+- (void)defaultTextSize
+{
+  nsCOMPtr<nsIContentViewer> contentViewer = getter_AddRefs([self getContentViewer]);
+  nsCOMPtr<nsIMarkupDocumentViewer> markupViewer(do_QueryInterface(contentViewer));
+  if (!markupViewer)
+    return;
+
+  markupViewer->SetTextZoom(1.0); //Default value
+}
+

We shouldn't be addref-ing here on |contentViewer|, instead use the |dont_AddRef| macro:
  nsCOMPtr<nsIContentViewer> contentViewer = dont_AddRef([self getContentViewer]);

From your comments via IRC, you should make this change on the methods you copied this code from.

+- (BOOL)textIsDefaultSize
+{
+  return ([self getTextZoom] == 1.0);
+}

It would be nice to define a constant for |1.0| since it is used frequently. Maybe something to the beat of |kDefaultTextSize|?

 -(IBAction) biggerTextSize:(id)aSender;
 -(IBAction) smallerTextSize:(id)aSender;
+-(IBAction) defaultTextSize:(id)aSender;

Since I requested a method name change above, probably should do it here as well.

Comment 20

[froodian (Ian Leue)]

Attachment: Doesn't addref

(In reply to comment #19)
> (From update of attachment 228315 [details] [diff] [review] [edit])
>  - (void)biggerTextSize;
>  - (void)smallerTextSize;
> +- (void)defaultTextSize;
> 
> Im not a big fan of the names of these methods, perhaps we can rename them to
> something like:
>   |makeTextBiggerSize:|, |makeTextSmallerSize:|, ...

I'd rather not do this, since those methods (and their name counterparts in BWC) are used all over the place, and the last time I did a major name change, it still hasn't landed.  I think |biggerTextSize| is fine.

This addresses all other comments.

Comment 21

[Nick Kreeger]

Find and Replace?? :-)

Comment 22

[froodian (Ian Leue)]

Attachment: Patch

I was hoping I wouldn't have to do this, but there's just too much overlap to keep them separate.  This makes the name changes, and incorporates bug 345657.

Comment 23

[froodian (Ian Leue)]

Attachment: New MainMenu.nib

Incorporates the name changes.

Comment 24

[Stuart Morgan]

(In reply to comment #19)
> (From update of attachment 228315 [details] [diff] [review] [edit])
>  - (void)biggerTextSize;
>  - (void)smallerTextSize;
> +- (void)defaultTextSize;
> 
> Im not a big fan of the names of these methods, perhaps we can rename them to
> something like:
>   |makeTextBiggerSize:|, |makeTextSmallerSize:|, ...

I agree the old names are bad, but those names don't read like English to me. How about just makeTextBigger and makeTextSmaller?

Comment 25

[Nick Kreeger]

(In reply to comment #24)
> (In reply to comment #19)
> > (From update of attachment 228315 [details] [diff] [review] [edit] [edit])
> >  - (void)biggerTextSize;
> >  - (void)smallerTextSize;
> > +- (void)defaultTextSize;
> > 
> > Im not a big fan of the names of these methods, perhaps we can rename them to
> > something like:
> >   |makeTextBiggerSize:|, |makeTextSmallerSize:|, ...
> 
> I agree the old names are bad, but those names don't read like English to me.
> How about just makeTextBigger and makeTextSmaller?
> 

Yes i agree with smorgan here, i was just suggesting some names... please use the ones smorgan recommends.

Comment 26

[froodian (Ian Leue)]

Attachment: Patch

Comment 27

[froodian (Ian Leue)]

Attachment: New MainMenu.nib

Comment 28

[Stuart Morgan]

Comment on attachment 231197 [details] [diff] [review]
Patch

>+const int kDefaultTextSize = 1.0;
>+
> #define MIN_TEXT_ZOOM 0.01f
> #define MAX_TEXT_ZOOM 20.0f

#define this as DEFAULT_TEXT_ZOOM for consistency (and because it's neither an int nor a size).

>+- (BOOL)isTextDefaultSize
>+{
>+  return ([self getTextZoom] == kDefaultTextSize);
>+}

This is unlikely to every return YES; you can't do meaningful equality tests on float values.

>+- (BOOL)shouldMakeTextBigger;
>+- (BOOL)shouldMakeTextSmaller;

Don't call methods that describe whether you can do something shouldFoo. It implies some sort of delegation model that doesn't exist here.

Comment 29

[froodian (Ian Leue)]

Attachment: fabsf

Addresses smorgan's comments.

Comment 30

[Stuart Morgan]

Comment on attachment 232285 [details] [diff] [review]
fabsf

Works as advertised.  Nits:

+  // essentially test [self getTextZoom] and DEFAULT_TEXT_ZOOM for equality
+  return (fabsf([self getTextZoom] - DEFAULT_TEXT_ZOOM) < .001);

Remove the comment. This is well-known, standard practice.

>+- (IBAction)makeTextBigger:(id)aSender
> {
>   BrowserWindowController* browserController = [self getMainWindowBrowserController];
>   if (browserController)
>-    [browserController biggerTextSize:aSender];
>+    [browserController makeTextBigger:aSender];
> }
> 
>-- (IBAction)smallerTextSize:(id)aSender
>+- (IBAction)makeTextSmaller:(id)aSender
> {
>   BrowserWindowController* browserController = [self getMainWindowBrowserController];
>   if (browserController)
>-    [browserController smallerTextSize:aSender];
>+    [browserController makeTextSmaller:aSender];
>+}
>+
>+- (IBAction)makeTextDefaultSize:(id)aSender
>+{
>+  BrowserWindowController* browserController = [self getMainWindowBrowserController];
>+  if (browserController)
>+    [[[browserController getBrowserWrapper] getBrowserView] makeTextDefaultSize];
> }

As long as you are touching these anyway, just make it
[[self getMainWindowBrowserController] makeText{Bigger,Smaller,DefaultSize}:aSender];
since the |if| doesn't accomplish anything

>+- (BOOL)validateMakeTextBigger;
>+- (BOOL)validateMakeTextSmaller;

canMakeText{Bigger,Smaller}

>+- (BOOL)validateMakeTextBigger
> {
>-  [[mBrowserView getBrowserView] biggerTextSize];
>+  return (![[self getBrowserWrapper] isEmpty] &&
>+          ![self bookmarkManagerIsVisible] &&
>+          [[[self getBrowserWrapper] getBrowserView] canMakeTextBigger]);
> }
>+
>-- (IBAction)smallerTextSize:(id)aSender
>+- (BOOL)validateMakeTextSmaller
> {
>-  [[mBrowserView getBrowserView] smallerTextSize];
>+  return (![[self getBrowserWrapper] isEmpty] &&
>+          ![self bookmarkManagerIsVisible] &&
>+          [[[self getBrowserWrapper] getBrowserView] canMakeTextSmaller]);
>+}

Get the browser wrapper as a local variable so you aren't calling getBrowserWrapper twice.


I'm not liking the lack of parallel structure in the menu names in the nib though--the "Size" at the end of default makes it look out of place. What do people think about:
Make Text Bigger
Make Text Default
Make Text Smaller

Comment 31

[froodian (Ian Leue)]

Attachment: New MainMenu.nib

Incorporates smorgan's suggestion.  a=irc.

Comment 32

[froodian (Ian Leue)]

Attachment: Patch

Addresses smorgan's comments.

Comment 33

[Stuart Morgan]

Comment on attachment 233728 [details] [diff] [review]
Patch

Sorry, one other thing I missed:

>+- (IBAction)makeTextDefaultSize:(id)aSender
>+{
>+  [[[[self getMainWindowBrowserController] getBrowserWrapper] getBrowserView] makeTextDefaultSize];
> }
...
>+  if (action == @selector(makeTextDefaultSize:))
>     return (browserController &&
>             ![[browserController getBrowserWrapper] isEmpty] &&
>+            ![[[browserController getBrowserWrapper] getBrowserView] isTextDefaultSize]);

These should look just the way makeText{Bigger,Smaller} look; having one doing things differently despite being essentially the same task is odd.  You'll need to add parallel makeTextDefaultSize: and canMakeTextDefaultSize (or isTextDefaultSize) methods in BWC.

Comment 34

[froodian (Ian Leue)]

Attachment: Patch

Heh.  Funnily enough, this is the approach I first took (see comment 13). :)

Comment 35

[Stuart Morgan]

Comment on attachment 235084 [details] [diff] [review]
Patch

This patch has bitrotted a bit because of the sendURL: validation changes, but looks good when applied by hand.

r=me for a respun version.

Comment 36

[froodian (Ian Leue)]

Attachment: r=smorgan patch

Pink, this blocks all the menu cleanup work for 1.1, so a speedy sr would be most helpful for me. :)

Comment 37

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 237374 [details] [diff] [review]
r=smorgan patch

sr=pink. are we sure those getter_addrefs -> dont_addrefs are correct?

Comment 38

[Stuart Morgan]

I spent some quality time with the COM docs when reviewing; getter_addRefs is for out parameters, dont_addRef is for what's happening here ("dont_AddRef is a similar directive that helps you when you assign in a pointer that has already been AddRefed, e.g., because you called a getter that returned the pointer as its function result.")

Comment 39

[froodian (Ian Leue)]

Checked in on trunk and 1.8branch

Comment 40

[Håkan Waara]

Instead of using dont_AddRef([self methodThatReturnsSomethingAddrefed]), you can define the method returning something addref'd as follows:

  - (already_AddRefed<nsIFoo*>)methodThatReturnsSomethingAddrefed;

Then nsCOMPtr will be smart about it:

  nsCOMPtr<nsIFoo> foo = [self methodThatReturnsSomethingAddrefed];

We have a few methods already that do this already.