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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Toolbars & Menus
P4
enhancement
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: Stephane Moureau, Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1
Dependency tree / graph

Details

Attachments

(3 attachments, 12 obsolete attachments)

29.53 KB, image/png
Details
24.22 KB, application/zip
Details
15.46 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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

15 years ago
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

15 years ago
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

Comment 3

15 years ago
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

15 years ago
-->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)
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...
Blocks: 328173
-> 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)

Updated

11 years ago
Assignee: sfraser_bugs → stridey
(Assignee)

Comment 13

11 years ago
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.
Attachment #226505 - Flags: review?(mozilla)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 14

11 years ago
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)
Attachment #226505 - Attachment is obsolete: true
Attachment #228315 - Flags: review?(nick.kreeger)
Attachment #226505 - Flags: review?(mozilla)
(Assignee)

Comment 15

11 years ago
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.
Attachment #228316 - Flags: review?(alqahira)
(Assignee)

Updated

11 years ago
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-
(Assignee)

Comment 17

11 years ago
Created attachment 228580 [details]
New MainMenu.nib

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+
(Assignee)

Updated

11 years ago
Attachment #228316 - Attachment is obsolete: true

Comment 19

11 years ago
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-
(Assignee)

Comment 20

11 years ago
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.
Attachment #228315 - Attachment is obsolete: true
Attachment #231015 - Flags: review?(nick.kreeger)

Comment 21

11 years ago
Find and Replace?? :-)
(Assignee)

Comment 22

11 years ago
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.
Attachment #231015 - Attachment is obsolete: true
Attachment #231028 - Flags: review?(nick.kreeger)
Attachment #231015 - Flags: review?(nick.kreeger)
(Assignee)

Comment 23

11 years ago
Created attachment 231030 [details]
New MainMenu.nib

Incorporates the name changes.
Attachment #228580 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #231028 - Flags: review?(stuart.morgan)

Comment 24

11 years ago
(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

11 years ago
(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.
(Assignee)

Comment 26

11 years ago
Created attachment 231197 [details] [diff] [review]
Patch
Attachment #231028 - Attachment is obsolete: true
Attachment #231197 - Flags: review?(nick.kreeger)
Attachment #231028 - Flags: review?(stuart.morgan)
Attachment #231028 - Flags: review?(nick.kreeger)
(Assignee)

Comment 27

11 years ago
Created attachment 231198 [details]
New MainMenu.nib
Attachment #231030 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #231197 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
No longer blocks: 272171

Comment 28

11 years ago
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-
(Assignee)

Comment 29

11 years ago
Created attachment 232285 [details] [diff] [review]
fabsf

Addresses smorgan's comments.
Attachment #231197 - Attachment is obsolete: true
Attachment #232285 - Flags: review?(stuart.morgan)
Attachment #231197 - Flags: review?(nick.kreeger)

Comment 30

11 years ago
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-
Blocks: 345657
(Assignee)

Comment 31

11 years ago
Created attachment 233727 [details]
New MainMenu.nib

Incorporates smorgan's suggestion.  a=irc.
Attachment #231198 - Attachment is obsolete: true
(Assignee)

Comment 32

11 years ago
Created attachment 233728 [details] [diff] [review]
Patch

Addresses smorgan's comments.
Attachment #232285 - Attachment is obsolete: true
Attachment #233728 - Flags: review?(stuart.morgan)

Comment 33

11 years ago
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-
(Assignee)

Comment 34

11 years ago
Created attachment 235084 [details] [diff] [review]
Patch

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 35

11 years ago
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+
(Assignee)

Comment 36

11 years ago
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. :)
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+

Comment 38

11 years ago
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.")
(Assignee)

Comment 39

11 years ago
Checked in on trunk and 1.8branch
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Comment 40

11 years ago
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.