174854 - enhance view -> text size menu (was: no clear way to get text size immediately back to 100%)

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Azrael AOD]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021016 Phoenix/0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021016 Phoenix/0.3

From the View menu you can Increase or Decrease text size. But no way to jump
straight back to 100%.

The Mozilla menu optiosn for this are better, and I assume they will be copied.
However if they are not, I suggest the addition of 1 more option between the
Increae/Decrease, of 'Text Size: 100%'

Reproducible: Always

Steps to Reproduce:

Comment 1

[Asa Dotzler [:asa]]

Setting bug status to new. We need to restore Ctrl+0 to restore the default size
(like Mozilla)

Comment 2

[Blake Ross]

I restored Ctrl+0.

Comment 3

[Jeremy M. Dolan]

ctrl+0 is not at all discoverable. I agree with the original poster, this is the
*ONE* case where Mozilla does have better UI. It showed you what the current
level was, and let you jump back to 100% easily.

I generally zoom in with the keyboard, but sometimes it's a while before I
finish reading the page, and want to zoom back out. If I forget how many times i
hit CTRL++, in Mozilla I'd just hit 'alt-v' and check. I've been just closing
the tab in Phoenix, until I found this bug which mentions ctrl+0. How many other
users will do the same?

Comment 4

[Brant Gurganus]

REOPENED Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030714
Mozilla Firebird/0.6.

This CTRL-0 access key is fine, but it is not discoverable.  Until I saw this
bug, I was about to file this issue.  We need to see where we are at (with a
bullet) and where we can be (a listing like Seamonkey).

Comment 5

[Simon Paquet [:sipaq]]

Taking QA Contact as designated owner of Firebird-Menus. Sorry for bugspam.

Comment 6

[Grey Nicholson (they/them)]

For now, "Reset Text Size   Ctrl+0" could be added to the View menu, below
Decrease Text Size.

Comment 7

[Premek Brada]

I have one additional suggestion: provide some visual feedback about the current
text magnification (zoom %) e.g. in the menu or status bar.  

This would help zooming in/out back to 100% apart from the ctrl+0 hotkey.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: Add "Reset Text Size" to View menu

This is my first ever attempt at a mozilla patch, hopefully I got the diff
right.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: Add "Reset Text Size" to View menu (small cleanup)

Argh! Slight text alignment mistake.
Sorry for the bugspam.

Comment 10

[Asaf Romano (gone)]

Gavin, please ask for review.

Comment 11

[Asaf Romano (gone)]

I'm requesting blocking1.0PR? since we have a patch (with l10n inpact).

Comment 12

[chris hofmann]

I'll plus for now, but there is a large backlog of reviewed patches so this
might not make PR.  blake can you have a look?

Comment 13

[Blake Ross]

I don't believe text size warrants three top level View menu items, especially
since our View -> Text Size doesn't even persist (like IE's). If we're going to
tackle this for 1.0, I think we should adopt a submenu a la IE's. This isn't
hard since Seamonkey already has the submenu. We'd just want to use Largest,
Larger, Medium, Smaller, Smallest instead of the percentages, I think.

For PR: can someone produce a patch that simply adds the following text strings:

Reset Text Size
Text Size
Largest
Larger
Medium
Smaller
Smallest

This will allow us to handle the l10n freeze and give us the greatest possible
flexibility for 1.0.

Comment 14

[Prognathous]

From actually using IE text-size control and Mozilla/Firefox text zoom, I can
safely declare the latter as a *much* better implementation. Fine grained
non-persisting zoom is so much more convenient and useful than what IE comes
with. Is IE-parity really a good enough reason to reduce the functionality of
Firefox?

Prog.

Comment 15

[Asaf Romano (gone)]

(In reply to comment #13)

So, instead of having a way to discover Accel-0 (the "orginal" problem), you 
suggest to create the same UI problem for Accel-+ and Accel--. Or are we going 
to have a cluttered menu with both "zoom in" item, "zoom out" item and zooms 
sub menu? *and we'll still hide Accel+0 from new users*.

IMHO, three menu items (reset text zomm, zoom in. zoom out) are fine.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch attempt #1

Here's a try at what was asked for.

This patch has a few issues.
1) The chosen option in the menu is not updated if you change text size another
way (shortcut key).
2) The menu does not change the text size at the moment.

I will look into Seamonkey's inmplementation and try to sort out the issues,
will hopefully have a new patch ready later today.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Attachment: Image of menu with Patch attempt #1

This is what it looks like. I left in the increase, decrease and reset (like in
Mozilla).

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch attempt #2

The options now scale text to 75%, 90%, 100%, 125%, and 150%. Still not the
prettiest way to do this, but will probably be acceptable for PR.

The issue remaining is that the menu option does not necessarily reflect the
current text zoom, because changing it with the shortcut keys does not update
the menu.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Attachment: Image of menu with Patch attempt #2

I added the "other" option, so that it can be selected when text size is set
independently by the user.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch attempt #2 (fixed)

Sorry for all this bugspam.

There was a mistake in the last patch.

Comment 21

[Asaf Romano (gone)]

Comment on attachment 156597 [details] [diff] [review]
Patch attempt #2 (fixed)

If i'm not missing something, this is going to be -

The following doesn't seem to be connected any-place in your patch to tbe
reduse, enlarge and reset commands. say I chose Largest and then started to
press Accel--, where does it get-the-change? (the submenu)


>+                  <menupopup id="viewTextSizeMenu">
>+                  <menuitem key="key_textZoomEnlarge" label="&textZoomEnlargeCmd.label;" accesskey="&textZoomEnlargeCmd.accesskey;"
>+                            command="cmd_textZoomEnlarge"/>
>+                  <menuitem key="key_textZoomReduce"  label="&textZoomReduceCmd.label;"  accesskey="&textZoomReduceCmd.accesskey;"
>+                            command="cmd_textZoomReduce"/>
>+ 	                <menuitem key="key_textZoomReset"   label="&textZoomResetCmd.label;"   accesskey="&textZoomResetCmd.accesskey;"
>+                            command="cmd_textZoomReset"/>
>+                  <menuseparator/>
>+                  <menuitem type="radio" name="textZoom" key="key_textZoomLargest"  label="&textZoomLargest.label;"  accesskey="&textZoomLargest.accesskey;"
>+ 	                          command="cmd_textZoomLargest"/>
>+ 	                <menuitem type="radio" name="textZoom" key="key_textZoomLarger"   label="&textZoomLarger.label;"   accesskey="&textZoomLarger.accesskey;"
>+ 	                          command="cmd_textZoomLarger"/>
>+                  <menuitem checked="true" type="radio" name="textZoom" key="key_textZoomMedium"   label="&textZoomMedium.label;"   accesskey="&textZoomMedium.accesskey;"
>+                            command="cmd_textZoomMedium"/>
>+                  <menuitem type="radio" name="textZoom" key="key_textZoomSmaller"  label="&textZoomSmaller.label;"  accesskey="&textZoomSmaller.accesskey;"
>+                            command="cmd_textZoomSmaller"/>
>+                  <menuitem type="radio" name="textZoom" key="key_textZoomSmallest" label="&textZoomSmallest.label;" accesskey="&textZoomSmallest.accesskey;"
>+                            command="cmd_textZoomSmallest"/>
>+                  <menuseparator/>
>+                  <menuitem type="radio" name="textZoom" key="key_textZoomOther" label="&textZoomOther.label;" accesskey="&textZoomOther.accesskey;"
>+                            command="cmd_textZoomOther"/>
>+                  </menupopup>
>+                </menu>

Comment 22

[:Gavin Sharp [email: [e-mail]]]

If you read the comments correctly, I stated that the menu is not updated when
you change the text size any other way. I know this.

It's something I'm looking into, but Blake wanted a "strings only" menu for the
PR, and what I have done is, I believe, better than that.

Comment 23

[Asaf Romano (gone)]

(In reply to comment #22)
> If you read the comments correctly, I stated that the menu is not updated when
> you change the text size any other way. I know this.

Oh Missed it, but:

> It's something I'm looking into, but Blake wanted a "strings only" menu for the
> PR, and what I have done is, I believe, better than that.

He didn't say menu, he said "strings", that means *dtd file changes" only, we
don't need a broken feature for a release (don't you see all the upcomming
duplicates?).

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Ok, I understand what you're saying. I'll submit a patch soon without the
smaller-larger-etc options but with the reset text size (keeping the strings of
course).

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch attempt #3 (Includes strings for l10n)

Ok, here is my final proposal.

1) Includes strings for the Smaller, Smallest, etc.
2) Submenu containing Increase, Decrease, Reset
Text Size -> Increase
	     Decrease
	     Reset

This patch has much of the menu XUL already completed for when we will have the
options, but they are commented out.

Basically, if this menu format is acceptable for now, check this patch in, but
if you really don't want a submenu for the text size option at this point, use
the older patch.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Attachment: Image of menu with Patch attempt #3

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Attachment: Strings only

Only add the strings for l10n

I'll shut up now.

Comment 28

[Blake Ross]

Gavin,

Thanks very much for all your work on this. I talked this over with Asa a bit.
We believe patch #3 will be the best route forward for 1.0.

In the longer term, however, we believe patch #2 is actually the route we'd want
to take. We're concerned that we can't really do that for 1.0 unless you can
make it work so that choosing Increase and Decrease moves properly among only
the listed levels (e.g. if I'm at Medium and I hit Ctrl++, I should now be at
"Larger" and the menu should reflect that), and so that Largest and Smallest
really are the largest and smallest sizes you can zoom to. How hard is that?

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Well, if the resizing is limited to those 5 levels, it may be easier than if it
were not. I can look through the actual resizing code to limit it to those 5
intervals, and have it update the menu accordingly.

I would need to know what intervals are appropriate (in percentage). Mozilla
uses 50, 75, 90, 100, 120, 150, 200 for its intervals, but to corellate with the
5 sizes in IE I suggested something more like 75%, 90%, 100%, 125%, and 150%.

I'll try and get a working patch in in the next few hours, if I'm not too busy.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch attempt #4

This latest patch addresses the issues above, however, its not perfect.

Issues:
1) The menu is not being update when you zoom with Ctrl+Scrollwheel, at least
not on my setup. This is due to the us handling the zooming in a different way
in that case (not through viewZoomOverlay.js), and I can't figure out how that
works. Any ideas here would be appreciated.
2) Although it works, this is probably not the best way of doing this... I'm
not an expert in javascript. Hopefully someone who is can help me out here.
3) Some people may not like being restricted to those 5 levels. At the moment,
the only way to get around that is to used the mousewheel, and that was
unintentional.

I'd like to get some input on this patch, and testing would be greatly
appreciated.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Attachment: Image of menu with Patch attempt #4

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 156679 [details] [diff] [review]
Patch attempt #4

>+  // Update the menu
>+  var menuUpd = document.getElementById('viewTextSizeMenu').childNodes[9 - zoomIndex];
>+  menuUpd.setAttribute('checked', 'true');
>+}

That 9 should be an 8.

>+  // Update the menu
>+  var menuUpd = document.getElementById('viewTextSizeMenu').childNodes[9 - zoomIndex];
>+  menuUpd.setAttribute('checked', 'true');
>+}

Is the correct way to do it.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch attempt #4 (revised)

Mistake in the last patch.

Comment 34

[Zachariah]

(In reply to comment #26)
> Created an attachment (id=156604)
> Image of menu with Patch attempt #3
> [ http://bugzilla.mozilla.org/attachment.cgi?id=156604&action=view ]

IMO this is (above) the correct way the menu should look. 

This:
Text Size -> Increase
         Decrease
         Reset
is better than the "smallest, smaller, ..." approach.  
And, having both approaches is redundant.

Leave that second way to being an extension.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Looking back at some of the stuff I have done, it seems like most of it is a bit
of a mess. Basically, if blake/asa/somebody can decide what approach they want,
I'll make sure I have the right patch and submit it again.

It seems to me that since I cannot get the menu to sync right with the mouse
scrollwheel, that we should probably go with approach #3, but with a cleaner
patch. I'll wait for some input before submitting anything as I don't want to
further complicate things.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch attempt #5

Latest patch, I think it addresses everything that needs to be addressed.

Text Size ->	Increase
		Decrease
		Reset
		-----------
		Largest
		Larger
		Medium
		Smaller
		Smallest
		-----------
		Other

1) The larger, smaller, etc options correspond to 80, 90, 100, 110, and 120%.
2) The menu represents the current text zoom, regardless of how it was obtained
(menu, keyboard, scrollwheel). So for example, setting it to medium, then doing
Ctrl+- twice would cause "Smallest" to be selected. Ctrl-MouseWheelDown twice
would have the same effect.
3) If the text zoom is bigger or smaller than the presets (larger, largest,
etc) then "Other" is selected

Possible issues:
1) The "Other" option does nothing when clicked, could be confusing
2) The zoom is not persistent across sessions or even windows, so if you set
the zoom to largest and then open a new tab, the zoom in that new tab reverts
to medium. This is the behaviour currently though so this patch jut leaves it
as is. Using a pref would be best, but I don't really know how to hook that up.

3) User is not limited to the presets as Blake requested, but I think most
people wouldn't like being restricted, plus the implementation of that is
slightly more complicated.

Comment 37

[Jeremy M. Dolan]

The new menu seems somewhat confusing to me. "Medium"? It's not a medium size,
it's whatever size the page specified. It could be huge, it could be tiny. How
about "default"? "as specified"? "page default"? "standard", "normal",
"natural", "regular", "author specified"...

Personally, I think "Page Default" most clearly translates how HTML works, which
is really the limiting factor in users having a comfortable reading size.

Also, with one of the above "100%-size" labels introducing a sensible name, this
should eliminate the need for a "Reset" entry, as they do the exact same thing
(which makes the current menu even more confusing). The access key could be
listed next the the middle-size entry for exposure.

Comment 38

[:Gavin Sharp [email: [e-mail]]]

That's a good idea, it simplifies the menu, and you're right, it is more
representative.

"Page Default" sounds good to me, but I'll wait for word from blake before I
make a new patch.

Comment 39

[Blake Ross]

Gavin,

Many thanks for all your work here. It will not go to waste. For 1.0, we have
decided to go with the following approach:

View
  Text Size >  Increase     Ctrl+=
               Decrease     Ctrl+-
               -----------
               Normal       Ctrl+0


And for 1.5+, we will use the other work you have attached here. Thank you!

Comment 40

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch attempt #6

Well, for for whatever it's worth, here's another. Same as before with a new
menu layout.

Text Size -> Increase
	     Decrease
	     ----------
	     Largest
	     Larger
	     Normal
	     Smaller
	     Smallest
	     ----------
	     Other

Comment 41

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch attempt #6 (fixed)

Argh. This is getting ridiculous.

Comment 42

[Blake Ross]

This is fixed on the aviary branch now.

Comment 43

[Greg Campbell]

(In reply to comment #29)
> I would need to know what intervals are appropriate (in percentage). Mozilla
> uses 50, 75, 90, 100, 120, 150, 200 for its intervals, but to corellate with the
> 5 sizes in IE I suggested something more like 75%, 90%, 100%, 125%, and 150%.

It seems to make sense to use the scaling factors recommended in CSS 2.1
(http://www.w3.org/TR/CSS21/fonts.html#font-size-props) for the CSS x-small,
small, medium, large, etc.

In other words: 60%, 75%, 88.88%, 100%, 120%, 150%, 200%

Comment 44

[Fareed Rizkalla]

i'm seeing a conflicat on build 20040820, i can't use ctrl+O to set font size to
original

Comment 45

[Prognathous]

That's probably Bug 198233. Try Ctrl+0 instead of Ctrl+NumPad0. 

Prog.

Comment 46

[Jacob W. Klein]

I see a minor problem with this patch in it's current state.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040826
Firefox/0.9.1+

If I press "ctrl + -" or "ctrl + mousewheel, eventually it will get to its
SMALLEST possible text size... but if I KEEP scrolling/pushing it (or hold down
the keyboard buttons for a bit), it goes beyond, loops back to ABOVE-NORMAL
size, and then finally stops.

Reproducible:  Always, upon intial view of any site.

Fix:  Once the page is at the smallest text size allowed, make sure it stops
there, even if the user keeps pushing "ctrl + -" or using "ctrl + mousewheel".

Comment 47

[Asaf Romano (gone)]

(In reply to comment #46)

This is not a bew issue, it's there fo years.

Comment 48

[:Gavin Sharp [email: [e-mail]]]

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

Comment 49

[daf]

Seems that: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3)
Gecko/20041009 Firefox/0.10.1 includes the Normal Size (Ctrl+0) option.

Just has a small bug in whitch the numpad 0 doesn't work, besides that i'd call
this fixed.

Comment 50

[Asaf Romano (gone)]

> Just has a small bug in whitch the numpad 0 doesn't work, besides that i'd call
> this fixed.

comment 45

Comment 51

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #49)
> Seems that: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3)
> Gecko/20041009 Firefox/0.10.1 includes the Normal Size (Ctrl+0) option.
> 
> Just has a small bug in whitch the numpad 0 doesn't work, besides that i'd call
> this fixed.

This bug was originally about exposing Crl-0 in the menu, but got changed to
"enhance text-size menu" when a temporary fix was checked in for the 1.0
release. That is why this bug is still open. See comment 39.

Changing summary.

Comment 52

[Aaron Leventhal]

I see View -> Text Size -> Normal, which lists Ctrl + 0.
Ctrl + 0 works as as does the menu

Comment 53

[Asaf Romano (gone)]

see bug 208767