Last Comment Bug 181712 - Menu item for "default font size" (also indicate current text/font zoom value/size?)
: Menu item for "default font size" (also indicate current text/font zoom value...
Status: RESOLVED FIXED
: fixed1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Toolbars & Menus (show other bugs)
: unspecified
: PowerPC Mac OS X
P4 enhancement (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
: 314289 (view as bug list)
Depends on:
Blocks: 328173 345657
  Show dependency treegraph
 
Reported: 2002-11-24 01:15 PST by Stephane Moureau
Modified: 2006-09-13 08:52 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
MacIE font zoom menu (29.53 KB, image/png)
2005-12-28 22:36 PST, Samuel Sidler (old account; do not CC)
no flags Details
Patch (7.12 KB, patch)
2006-06-21 10:19 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Patch (5.30 KB, patch)
2006-07-06 11:14 PDT, froodian (Ian Leue)
nick.kreeger: review-
Details | Diff | Splinter Review
New MainMenu.nib (24.37 KB, application/zip)
2006-07-06 11:16 PDT, froodian (Ian Leue)
alqahira: review-
Details
New MainMenu.nib (24.27 KB, application/zip)
2006-07-08 23:13 PDT, froodian (Ian Leue)
alqahira: review+
Details
Doesn't addref (6.90 KB, patch)
2006-07-27 15:58 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Patch (15.31 KB, patch)
2006-07-27 17:25 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
New MainMenu.nib (24.26 KB, application/zip)
2006-07-27 17:26 PDT, froodian (Ian Leue)
no flags Details
Patch (15.22 KB, patch)
2006-07-28 22:20 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
New MainMenu.nib (24.26 KB, application/zip)
2006-07-28 22:20 PDT, froodian (Ian Leue)
no flags Details
fabsf (16.09 KB, patch)
2006-08-04 23:52 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
New MainMenu.nib (24.22 KB, application/zip)
2006-08-14 23:29 PDT, froodian (Ian Leue)
no flags Details
Patch (15.19 KB, patch)
2006-08-14 23:30 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Patch (15.54 KB, patch)
2006-08-23 09:45 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
Details | Diff | Splinter Review
r=smorgan patch (15.46 KB, patch)
2006-09-08 14:05 PDT, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Stephane Moureau 2002-11-24 01:15:51 PST
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 User image S Woodside 2002-11-24 19:15:08 PST
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 User image Greg K. 2002-11-25 10:10:07 PST
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 User image Jasper 2002-12-01 10:24:10 PST
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 User image Kathleen Brade 2003-01-10 10:35:02 PST
-->sfraser for triage
Comment 5 User image Samuel Sidler (old account; do not CC) 2005-04-03 04:44:41 PDT
I agree with this feature request. Seems reasonable enough for 1.0 as it's an
accessibility issue.

Targeting for 1.0.
Comment 6 User image Samuel Sidler (old account; do not CC) 2005-05-21 00:52:24 PDT
Ping again.

Is this something that's as simple as a nib change or two? How easy would this
be for 09?
Comment 7 User image Samuel Sidler (old account; do not CC) 2005-07-28 22:26:31 PDT
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2005-07-28 23:23:20 PDT
(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 User image Samuel Sidler (old account; do not CC) 2005-10-28 22:33:23 PDT
*** Bug 314289 has been marked as a duplicate of this bug. ***
Comment 10 User image Samuel Sidler (old account; do not CC) 2005-12-28 22:36:36 PST
Created attachment 207055 [details]
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 User image Samuel Sidler (old account; do not CC) 2006-03-23 21:41:47 PST
-> 1.1, part of menus cleanup.
Comment 12 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-23 23:59:19 PDT
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 User image froodian (Ian Leue) 2006-06-21 10:19:56 PDT
Created attachment 226505 [details] [diff] [review]
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 User image froodian (Ian Leue) 2006-07-06 11:14:12 PDT
Created attachment 228315 [details] [diff] [review]
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 User image froodian (Ian Leue) 2006-07-06 11:16:04 PDT
Created attachment 228316 [details]
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-07-06 23:20:53 PDT
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 User image froodian (Ian Leue) 2006-07-08 23:13:48 PDT
Created attachment 228580 [details]
New MainMenu.nib

No submenu - just adds 'Default Text Size' between the existent bigger/smaller text menu items.
Comment 18 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-07-11 21:19:12 PDT
Comment on attachment 228580 [details]
New MainMenu.nib

Much more sane, r=me
Comment 19 User image Nick Kreeger 2006-07-27 13:14:10 PDT
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 User image froodian (Ian Leue) 2006-07-27 15:58:40 PDT
Created attachment 231015 [details] [diff] [review]
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 User image Nick Kreeger 2006-07-27 16:06:23 PDT
Find and Replace?? :-)
Comment 22 User image froodian (Ian Leue) 2006-07-27 17:25:43 PDT
Created attachment 231028 [details] [diff] [review]
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 User image froodian (Ian Leue) 2006-07-27 17:26:36 PDT
Created attachment 231030 [details]
New MainMenu.nib

Incorporates the name changes.
Comment 24 User image Stuart Morgan 2006-07-27 21:08:05 PDT
(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 User image Nick Kreeger 2006-07-28 21:45:22 PDT
(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 User image froodian (Ian Leue) 2006-07-28 22:20:38 PDT
Created attachment 231197 [details] [diff] [review]
Patch
Comment 27 User image froodian (Ian Leue) 2006-07-28 22:20:59 PDT
Created attachment 231198 [details]
New MainMenu.nib
Comment 28 User image Stuart Morgan 2006-08-03 20:55:33 PDT
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 User image froodian (Ian Leue) 2006-08-04 23:52:46 PDT
Created attachment 232285 [details] [diff] [review]
fabsf

Addresses smorgan's comments.
Comment 30 User image Stuart Morgan 2006-08-12 20:49:15 PDT
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 User image froodian (Ian Leue) 2006-08-14 23:29:38 PDT
Created attachment 233727 [details]
New MainMenu.nib

Incorporates smorgan's suggestion.  a=irc.
Comment 32 User image froodian (Ian Leue) 2006-08-14 23:30:17 PDT
Created attachment 233728 [details] [diff] [review]
Patch

Addresses smorgan's comments.
Comment 33 User image Stuart Morgan 2006-08-23 07:51:50 PDT
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 User image froodian (Ian Leue) 2006-08-23 09:45:53 PDT
Created attachment 235084 [details] [diff] [review]
Patch

Heh.  Funnily enough, this is the approach I first took (see comment 13). :)
Comment 35 User image Stuart Morgan 2006-09-08 07:53:46 PDT
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 User image froodian (Ian Leue) 2006-09-08 14:05:08 PDT
Created attachment 237374 [details] [diff] [review]
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 User image Mike Pinkerton (not reading bugmail) 2006-09-11 06:30:00 PDT
Comment on attachment 237374 [details] [diff] [review]
r=smorgan patch

sr=pink. are we sure those getter_addrefs -> dont_addrefs are correct?
Comment 38 User image Stuart Morgan 2006-09-11 07:21:46 PDT
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 User image froodian (Ian Leue) 2006-09-11 20:57:09 PDT
Checked in on trunk and 1.8branch
Comment 40 User image Håkan Waara 2006-09-12 07:25:13 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.