Closed Bug 181712 Opened 18 years ago Closed 14 years ago

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

Categories

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

PowerPC
macOS
enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.5

People

(Reporter: stf, Assigned: froodian)

References

Details

(Keywords: fixed1.8.1)

Attachments

(3 files, 12 obsolete files)

29.53 KB, image/png
Details
24.22 KB, application/zip
Details
15.46 KB, patch
mikepinkerton
: superreview+
Details | Diff | Splinter Review
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:
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).
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)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: View - Page Text Sizes → Somehow indicate current text zoom value
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!
-->sfraser for triage
Assignee: brade → sfraser
I agree with this feature request. Seems reasonable enough for 1.0 as it's an
accessibility issue.

Targeting for 1.0.
Priority: -- → P4
Target Milestone: --- → Camino1.0
Ping again.

Is this something that's as simple as a nib change or two? How easy would this
be for 09?
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".
(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.
Target Milestone: Camino1.0 → Camino1.2
*** Bug 314289 has been marked as a duplicate of this bug. ***
Summary: Somehow indicate current text zoom value → Somehow indicate current text/font zoom value/size (include default size)
Attached image 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...
-> 1.1, part of menus cleanup.
QA Contact: bugzilla → toolbars
Target Milestone: Camino1.2 → Camino1.1
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%
Summary: Somehow indicate current text/font zoom value/size (include default size) → Menu item for "default font size" (also indicate current text/font zoom value/size?)
Assignee: sfraser_bugs → stridey
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #226505 - Flags: review?(mozilla)
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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)
Attachment #226505 - Attachment is obsolete: true
Attachment #228315 - Flags: review?(nick.kreeger)
Attachment #226505 - Flags: review?(mozilla)
Attached file New MainMenu.nib (obsolete) —
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.
Attachment #228316 - Flags: review?(alqahira)
Blocks: 272171
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.)
Attachment #228316 - Flags: review?(alqahira) → review-
Attached file New MainMenu.nib (obsolete) —
No submenu - just adds 'Default Text Size' between the existent bigger/smaller text menu items.
Attachment #228580 - Flags: review?(alqahira)
Comment on attachment 228580 [details]
New MainMenu.nib

Much more sane, r=me
Attachment #228580 - Flags: review?(alqahira) → review+
Attachment #228316 - Attachment is obsolete: true
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.
Attachment #228315 - Flags: review?(nick.kreeger) → review-
Attached patch Doesn't addref (obsolete) — Splinter Review
(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.
Attachment #228315 - Attachment is obsolete: true
Attachment #231015 - Flags: review?(nick.kreeger)
Find and Replace?? :-)
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #231015 - Attachment is obsolete: true
Attachment #231028 - Flags: review?(nick.kreeger)
Attachment #231015 - Flags: review?(nick.kreeger)
Attached file New MainMenu.nib (obsolete) —
Incorporates the name changes.
Attachment #228580 - Attachment is obsolete: true
Attachment #231028 - Flags: review?(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?
(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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #231028 - Attachment is obsolete: true
Attachment #231197 - Flags: review?(nick.kreeger)
Attachment #231028 - Flags: review?(stuart.morgan)
Attachment #231028 - Flags: review?(nick.kreeger)
Attached file New MainMenu.nib (obsolete) —
Attachment #231030 - Attachment is obsolete: true
Attachment #231197 - Flags: review?(stuart.morgan)
No longer blocks: 272171
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.
Attachment #231197 - Flags: review?(stuart.morgan) → review-
Attached patch fabsf (obsolete) — Splinter Review
Addresses smorgan's comments.
Attachment #231197 - Attachment is obsolete: true
Attachment #232285 - Flags: review?(stuart.morgan)
Attachment #231197 - Flags: review?(nick.kreeger)
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
Attachment #232285 - Flags: review?(stuart.morgan) → review-
Attached file New MainMenu.nib
Incorporates smorgan's suggestion.  a=irc.
Attachment #231198 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Addresses smorgan's comments.
Attachment #232285 - Attachment is obsolete: true
Attachment #233728 - Flags: review?(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.
Attachment #233728 - Flags: review?(stuart.morgan) → review-
Attached patch Patch (obsolete) — Splinter Review
Heh.  Funnily enough, this is the approach I first took (see comment 13). :)
Attachment #233728 - Attachment is obsolete: true
Attachment #235084 - Flags: review?(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.
Attachment #235084 - Flags: review?(stuart.morgan) → review+
Attached patch r=smorgan patchSplinter Review
Pink, this blocks all the menu cleanup work for 1.1, so a speedy sr would be most helpful for me. :)
Attachment #235084 - Attachment is obsolete: true
Attachment #237374 - Flags: superreview?(mikepinkerton)
Comment on attachment 237374 [details] [diff] [review]
r=smorgan patch

sr=pink. are we sure those getter_addrefs -> dont_addrefs are correct?
Attachment #237374 - Flags: superreview?(mikepinkerton) → superreview+
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.")
Checked in on trunk and 1.8branch
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.