Last Comment Bug 738194 - Composition: Add keyboard shortcuts for Zoom (Ctrl++, Ctrl+-, Ctrl+0: ux-consistency with message reader), change shortcuts for font size to Ctrl+<, Ctrl+>
: Composition: Add keyboard shortcuts for Zoom (Ctrl++, Ctrl+-, Ctrl+0: ux-cons...
Status: RESOLVED FIXED
[relnote = from TB 18][needs followup...
: ux-consistency, ux-control, ux-error-prevention
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
Mentors:
: 177510 459861 486151 493530 536134 856642 (view as bug list)
Depends on: 813295
Blocks: 512956 tb-keyboard-tracker tbkbd-doc-tracker 836889 512968 716214 919276
  Show dependency treegraph
 
Reported: 2012-03-22 04:02 PDT by Thomas D. (currently busy elsewhere; needinfo?me)
Modified: 2014-03-13 06:10 PDT (History)
17 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Composition: Proposed fix for Zoom vs. Font size keyboard shortcuts (for ux-consistency, ux-error-prevention, ux-control) (1.87 KB, text/plain)
2012-04-02 11:16 PDT, Thomas D. (currently busy elsewhere; needinfo?me)
bwinton: ui‑review+
Details
1/3 patch for Toolkit that is needed for the zoom to work. (1.25 KB, patch)
2012-09-16 07:46 PDT, :aceman
neil: review+
Details | Diff | Review
2/3 WIP patch for TB (11.25 KB, patch)
2012-09-16 07:49 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Review
3/3 Optional patch for Seamonkey (1.26 KB, patch)
2012-09-16 07:51 PDT, :aceman
neil: review+
Details | Diff | Review
1/3 patch for Toolkit that is needed for the zoom to work v2 (1.59 KB, patch)
2012-09-17 12:18 PDT, :aceman
acelists: review+
Details | Diff | Review
2/3 patch for TB v2 (18.80 KB, patch)
2012-09-27 13:12 PDT, :aceman
mkmelin+mozilla: review-
Details | Diff | Review
2/3 patch for TB v3 (20.41 KB, patch)
2012-09-28 12:33 PDT, :aceman
mkmelin+mozilla: review+
Details | Diff | Review

Description Thomas D. (currently busy elsewhere; needinfo?me) 2012-03-22 04:02:16 PDT
In preparation for compose-in-a-tab (Bug 449299), we should make zoom behaviour and its mouse and keyboard shortcuts consistent between message reader and message composition. At the same time, we'll fix some confusing inconsistencies and bugs in the current *zoom* vs. *change font size* UI/behaviour for composition, like bug 512968. I have recently documented the current confusion in Keyboard Shortcuts Article (1), so I know what I am talking about...

For easy reading, I'll refer to windows keyboard shortcuts only.
You'll can easily work out equivalents for other OS from Keyboard Shortcuts (1).

STR

1 in message reader (as in FF), zoom in/out: Ctrl++, Ctrl+- (or Ctrl+mouse wheel), and reset the zoom to 100%: Ctrl+0
2 in composition, try the same  (as in 1):
2a) zoom in/out/reset using keyboard only
2b) zoom in/out/reset using mouse+keyboard
2c) zoom in/out/reset using mouse only

Actual result

1 in message reader, everything's fine:
keyboard-only zoom works nicely, as in FF: Ctrl++, Ctrl+-, Ctrl+0 (ok)
mouse+keyboard: Ctrl+mouse wheel (ok)
mouse only: View > Zoom menu (ok)

2 in composition, confusion and bugs:

2a zoom *keyboard only*: not possible (no keyboard shortcuts implemented for zoom)
confusingly, zoom keyboard shortcuts ctrl++, ctrl+- are used to increase/decrease text size, albeit incomplete: no keyboard shortcut to reset the text size (ctrl+0 does nothing).
Bug 449299 (compose in a tab) will make this even more confusing.
Having different keyboard shortcuts for zoom in reader vs. composition  violates ux-consistency, and should be changed (ux-error-prevention).

2b zoom *keyboard+mouse*: Ctrl+mouse wheel (ok); but practically no reliable way of resetting zoom to 100%, bug 512968.

2c zoom *mouse only*: not possible

For people who need and use both: changing font size and zooming simultaneously, current UI/behaviour is ultimate confusion as we don't have any indication of the current zoom level nor any reliable way to reset it to 100% except guessing from sight (bug 512968). Try guessing 100% zoom for an increased font size...

Expected result (changes/improvements proposed by this bug):

* be ux-consistent
* be accessible
* be feature-complete

2a) *keyboard only*
- use the same zoom keyboard shortcuts for composition as in message reader (and FF), especially when we can compose in a tab, bug 449299:
Ctrl++, Ctrl+-, Ctrl+0 to zoom in/out/reset (anywhere); this will fix bug 512968.
- implement new, distinct keyboard shortcuts for changing font size:
Ctrl+> to increase font size
Ctrl+< to decrease font size
(and optionally, Ctrl+= to reset font size without resetting other text styles; otherwise there's Ctrl+Space to reset /all/ text styles incl. font size).

2b) *keyboard+mouse* 
- basically, we're fine here, if we do 2a which will fix bug 512968.
- optionally, consider adding a mouse+keyboard shortcut for resetting the zoom to 100%, so we could have:
Ctrl+mouse wheel up/down: zoom in/out
Ctrl+middle click (click on mouse wheel): reset zoom to 100% (or even toggle between 100% and the last custom zoom level).

2c) *mouse only*
For full mouse-only access:
- Add View > Zoom menu to composition window, same as message reader (easy, should be fixed in a separate bug)

(1) https://support.mozillamessaging.com/en-US/kb/keyboard-shortcuts
Comment 1 Thomas D. (currently busy elsewhere; needinfo?me) 2012-03-22 04:13:17 PDT
N.B. I suggested Ctrl+< and Ctrl+> for changing font size because MS Word (all versions) is using them for increasing/decreasing font size; and afasict they are not used for anything else so far, neither main window nor composition nor lightning.
Comment 2 Thomas D. (currently busy elsewhere; needinfo?me) 2012-04-02 11:16:15 PDT
Created attachment 611523 [details]
Composition: Proposed fix for Zoom vs. Font size keyboard shortcuts (for ux-consistency, ux-error-prevention, ux-control)

Blake, the current keyboard shortcuts for composition's *font size* (Ctrl++/ Ctrl+-) are obviously ux-inconsistent with message reader and FF where Ctrl++/Ctrl+-/Ctrl+0 are used for *Zoom*. This ux-inconsistency will be worse when we'll have compose-in-a-tab. And more problems in this corner of composition: E.g. zoom has no keyboard shortcuts, it's not mouse-accessible, and cannot be reliably reset to 100%. Changing zoom *and* font size will cause much confusion...

For the sake of ux-consistency, ux-error-prevention, and ux-control, here's my proposal to address all of these problems in a wholistic approach. Fortunately, it just needs a few obvious tweaks, mainly changing/adding a few keyboard shortcuts. This looks pretty straight-forward, so I'm asking ui-review for the attached detailed UI proposal; otherwise, it'll be a request for feedback.
Comment 3 Blake Winton (:bwinton) (:☕️) (PTO 'til London. Find me there for quick answers!) 2012-04-12 14:14:02 PDT
Comment on attachment 611523 [details]
Composition: Proposed fix for Zoom vs. Font size keyboard shortcuts (for ux-consistency, ux-error-prevention, ux-control)

So, I like this except for the "Ctrl+mouse wheel up/down" and "Ctrl+middle click" parts, since they feel to me like something that people would do accidentally, and then be confused and frustrated by.

But, ui-r=me with those bits removed.  :)

Thanks,
Blake.
Comment 4 Thomas D. (currently busy elsewhere; needinfo?me) 2012-04-12 15:57:21 PDT
Blake, thank you for ui-r+!
So this bug is ready for implementation and waiting for volunteers!

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #3)
> Comment on attachment 611523 [details]
> So, I like this except for the "Ctrl+mouse wheel up/down" and "Ctrl+middle
> click" parts, since they feel to me like something that people would do
> accidentally, and then be confused and frustrated by.

That caveat statement is a bit ambiguous, and perhaps my wording of that part wasn't ideal, either. Let's try to clarify:

I assume Blake is only against my little new idea of making "Ctrl+middle click" (i.e. Ctrl+click on mouse wheel) reset the zoom to 100% (on Windows & Unix; for Mac, that would be Command + Control + middle click). I don't really see the risk of accidents here (and they would be really harmless even if they happened), but I can live with without that little idea.

I'm sure Blake does *not* intend to remove existing "Ctrl+scrollwheel" behaviour for zooming in/out (where you hold Ctrl while turning the mouse wheel), because we have just implemented this useful and cross-OS/cross-application standard behaviour for TB10 in Bug 250415 (on Windows & Unix; on Mac OS X, that's Command + Control + scrollwheel because just control + scrollwheel is used by Mac OS X  Operating System Level zoom), and this bug would be the wrong place to change anything about that.
Comment 5 Blake Winton (:bwinton) (:☕️) (PTO 'til London. Find me there for quick answers!) 2012-04-13 06:49:14 PDT
Well, you're partially correct.  I didn't intend to remove it because it doesn't seem to work for me in Earlybird on Mac.  I'm not a big fan of the feature, but removing it does seem a little harsh, even for me.  ;)
Comment 6 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-15 14:47:33 PDT
Aceman, is this something you could fix?
Comment 7 :aceman 2012-09-15 15:27:58 PDT
I can try.
Comment 8 :aceman 2012-09-16 05:20:46 PDT
Thomas, for Ctrl+< and > you actually need to press shift too. Did you really mean it, or should the keys be Ctrl+, and Ctrl+. ?
Comment 9 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-16 07:36:26 PDT
(In reply to :aceman from comment #8)
> Thomas, for Ctrl+< and > you actually need to press shift too. Did you
> really mean it, or should the keys be Ctrl+, and Ctrl+. ?

I think it will be more memorable and less confusing for both users of English keyboard layout and other keyboard layouts to really use whatever keys are required to arrive at Ctrl+< and Ctrl+>. That's also externally consistent with MS Word (including recent version 2010), both German and English version.

For English keyboards, that's Ctrl+Shift+< and Ctrl+Shift+>.

There are a lot of keyboard layouts for which the mnemonic value of Ctrl+, and Ctrl+. is zero because the </> signs are not on those keys (see http://en.wikipedia.org/wiki/Keyboard_layout). Instead, there's only punctuation which is very hard to tell apart. So e.g. for German keyboards, we have , and ; on one key, and . and : on the neighbouring key. < and > are one their own same key in lower-left corner of keyboard, so for German layout that maps to Ctrl+< and Ctrl+Shift+> for the shortcuts proposed here.

Perhaps the only drawback is that some keyboard layouts need different modifier keys for Ctrl+< and Ctrl+>, whereas Ctrl+, and Ctrl+. would be internationally the same.

But even for English keyboards, I think it's much easier to make sense of and memorize Ctrl+Shift+<, and Ctrl+Shift+>, (also in Shortcut documentation), compared to Ctrl+, and Ctrl+. In fact, strictly speaking the shortcut is really Ctrl+< and Ctrl+>, which just happens to be mapped to Ctrl+Shift+< and Ctrl+Shift+> on English keyboards. Given the mnemonic and localization benefits, imo it's not too hard to add Shift, and we already have a number of formatting shortcuts with Shift in TB (like Ctrl+Shift+Y, Ctrl+Shift+R, Ctrl+Shift+K).
Comment 10 :aceman 2012-09-16 07:46:52 PDT
Created attachment 661596 [details] [diff] [review]
1/3 patch for Toolkit that is needed for the zoom to work.
Comment 11 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-16 07:49:20 PDT
I finally understood from looking at English Keyboard layout why on earth we have Ctrl+= as a second shortcut for Zoom in (or currently font size in composition). It's because their + needs shift modifier (Ctrl+Shift++), and = is the other character on the key with the + if you forget the shift modifier. So contrary to my comment 0, Ctrl+= for resetting font size to default size isn't a good idea.

What about Ctrl+Shift+: for resetting font size without removing any other formats? That's the same on every layout afasics, and not taken for anything else.
Comment 12 :aceman 2012-09-16 07:49:45 PDT
Created attachment 661598 [details] [diff] [review]
2/3 WIP patch for TB

This is the main part. I just copied what was needed from mailWindowOverlay.xul and .js. I am not sure if we need goUpdateMailMenuItems() or the zoom menu can be simplified more. Another ugly stuff is including messenger.dtd into messengercompose.xul to get the zoom key definitions. Maybe we should split those into a shared file?
Comment 13 :aceman 2012-09-16 07:51:04 PDT
Created attachment 661599 [details] [diff] [review]
3/3 Optional patch for Seamonkey

If the patch for toolkit is accepted, this simplification could be done in editingOverlay.js that is only used in SM.
Comment 14 :aceman 2012-09-16 07:52:55 PDT
Comment on attachment 661596 [details] [diff] [review]
1/3 patch for Toolkit that is needed for the zoom to work.

The patches are thrown into one bug so that the context is seen.
If the general idea of code changes is approved we can split the patches into separate bugs if needed.
Comment 15 :aceman 2012-09-16 07:58:06 PDT
(In reply to Thomas D. from comment #11)
> I finally understood from looking at English Keyboard layout why on earth we
> have Ctrl+= as a second shortcut for Zoom in (or currently font size in
> composition). It's because their + needs shift modifier (Ctrl+Shift++), and
> = is the other character on the key with the + if you forget the shift
> modifier. So contrary to my comment 0, Ctrl+= for resetting font size to
> default size isn't a good idea.
> 
> What about Ctrl+Shift+: for resetting font size without removing any other
> formats? That's the same on every layout afasics, and not taken for anything
> else.

If you see the patch 2/3 I have used > and . as alternatives for the "increase font size" as the existing code already allowed for 2 alternatives. Maybe we could add , in addition to < too.

I have not yet implemented the resetting of html font size. It would need to determine what is the base size and add more code as this function is not yet implemented.
Comment 16 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-16 08:32:11 PDT
(In reply to :aceman from comment #15)
> (In reply to Thomas D. from comment #11)
> > I finally understood from looking at English Keyboard layout why on earth we
> > have Ctrl+= as a second shortcut for Zoom in (or currently font size in
> > composition). It's because their + needs shift modifier (Ctrl+Shift++), and
> > = is the other character on the key with the + if you forget the shift
> > modifier. So contrary to my comment 0, Ctrl+= for resetting font size to
> > default size isn't a good idea.
> > 
> > What about Ctrl+Shift+: for resetting font size without removing any other
> > formats? That's the same on every layout afasics, and not taken for anything
> > else.
> 
> If you see the patch 2/3 I have used > and . as alternatives for the
> "increase font size" as the existing code already allowed for 2
> alternatives. Maybe we could add , in addition to < too.

+1, I think that would make sense (fallback shortcuts if users of English keyboard layout forget Shift modifier - adding to user's comfort, ux-error-recovery).

Perhaps we could add a comment in code why these secondary shortcuts were added (which would also enable addons or anybody to claim them back if needed).

> I have not yet implemented the resetting of html font size. It would need to
> determine what is the base size and add more code as this function is not
> yet implemented.

We could consider re-using "Format > Size > Medium" command for restoring base size, as we currently do for Ctrl+Shift+Y (Remove all formatting), ignoring user's default size setting for Tools > Options > Composition > General > HTML > Font Size (and arguably, that's useful behaviour because font size xxl isn't a good default font size).
Comment 17 neil@parkwaycc.co.uk 2012-09-16 13:47:44 PDT
Comment on attachment 661598 [details] [diff] [review]
2/3 WIP patch for TB

> <!ENTITY increaseFontSize.label "Larger">
> <!ENTITY increaseFontSize.accesskey "g">
>-<!ENTITY increaseFontSize.key "+">
>-<!ENTITY increaseFontSize.key2 "="> <!-- + is above this key on many keyboards -->
>+<!ENTITY increaseFontSize.key "&gt;">
>+<!ENTITY increaseFontSize.key2 "."> <!-- > is above this key on many keyboards -->
My understanding is that the adddition of the Ctrl+= alternative only applies to the use of Ctrl++; if you're changing the key then you no longer need the alternative (note that Ctrl+= should increase zoom of course.)

>+  <!ENTITY % messengerDTD SYSTEM "chrome://messenger/locale/messenger.dtd">
>+  %messengerDTD;
[SeaMonkey does this using a separate viewZoomOverlay]
Comment 18 neil@parkwaycc.co.uk 2012-09-16 14:08:43 PDT
Comment on attachment 661596 [details] [diff] [review]
1/3 patch for Toolkit that is needed for the zoom to work.

>                 readonly="true"
>                 onget="return this.docShell.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIWebBrowserFind);"/>
>       <property name="editingSession"
>                 readonly="true"
>                 onget="return this.webNavigation.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIEditingSession);"/>
>       <property name="commandManager"
>                 readonly="true"
>                 onget="return this.webNavigation.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsICommandManager);"/>
>+      <property name="markupDocumentViewer"
>+                readonly="true"
>+                onget="return this.docShell.contentViewer.QueryInterface(Components.interfaces.nsIMarkupDocumentViewer);"/>
Oh, I meant to suggest that this might look slightly less out of place above the webNavigation-derived properties instead of below.
Comment 19 :aceman 2012-09-16 15:22:49 PDT
(In reply to neil@parkwaycc.co.uk from comment #17)
> > <!ENTITY increaseFontSize.label "Larger">
> > <!ENTITY increaseFontSize.accesskey "g">
> >-<!ENTITY increaseFontSize.key "+">
> >-<!ENTITY increaseFontSize.key2 "="> <!-- + is above this key on many keyboards -->
> >+<!ENTITY increaseFontSize.key "&gt;">
> >+<!ENTITY increaseFontSize.key2 "."> <!-- > is above this key on many keyboards -->
> My understanding is that the adddition of the Ctrl+= alternative only
> applies to the use of Ctrl++; if you're changing the key then you no longer
> need the alternative (note that Ctrl+= should increase zoom of course.)
This is taking + and - away from doing a FONT SIZE change so that the + and - and = can do the normal zoom function. I am not changing the = alternative here:
http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.dtd#256

> >+  <!ENTITY % messengerDTD SYSTEM "chrome://messenger/locale/messenger.dtd">
> >+  %messengerDTD;
> [SeaMonkey does this using a separate viewZoomOverlay]

Yes, but it seems you reimplement the whole zoom manager (viewZoomOverlay.js). I just offered to split the entities into a separate .dtd file to be included from all places instead of full messenger.dtd.
Comment 20 Axel Grude [:realRaven] 2012-09-17 00:28:38 PDT
can we add a panel to. the status bar that indicates the current zoom level? this is something that the office suite also implements successfully. a slider for this might follow lateron.
Comment 21 :aceman 2012-09-17 01:01:23 PDT
Axel, if you file that bug, I can look at it. But it may not be easy as Firefox does not have it either so there may not be events of zoom level changes to hook into.
Comment 22 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-17 03:43:30 PDT
*** Bug 177510 has been marked as a duplicate of this bug. ***
Comment 23 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-17 03:55:48 PDT
(In reply to :aceman from comment #21)
> (In reply to Axel Grude [:realRaven] from comment #20)
>> can we add a panel to. the status bar that indicates the current zoom level?
>> this is something that the office suite also implements successfully. a
>> slider for this might follow lateron.
> Axel, if you file that bug, I can look at it.

That's bug 512956 - we have bugs for everything, it's only they have never been fixed... But with dedicated people like Aceman, there's hope after all...

> But it may not be easy as
> Firefox does not have it either so there may not be events of zoom level
> changes to hook into.

Is it possible/easier to do it the other way round: Include a few lines at the end of zoom commands that directly change the value of the proposed zoom widget in status bar?
Comment 24 :aceman 2012-09-17 04:02:30 PDT
> Is it possible/easier to do it the other way round: Include a few lines at
> the end of zoom commands that directly change the value of the proposed zoom
> widget in status bar?

That is what I have in mind :) But it is a hack, as the zoom level can be changed via mouse Ctrl+scroll and I have no idea yet where that is defined and how to see the zoom changes there.

But let's move to bug 512956.
Comment 25 neil@parkwaycc.co.uk 2012-09-17 04:28:13 PDT
(In reply to aceman from comment #19)
> (In reply to comment #17)
> > > <!ENTITY increaseFontSize.label "Larger">
> > > <!ENTITY increaseFontSize.accesskey "g">
> > >-<!ENTITY increaseFontSize.key "+">
> > >-<!ENTITY increaseFontSize.key2 "="> <!-- + is above this key on many keyboards -->
> > >+<!ENTITY increaseFontSize.key "&gt;">
> > >+<!ENTITY increaseFontSize.key2 "."> <!-- > is above this key on many keyboards -->
> > My understanding is that the adddition of the Ctrl+= alternative only
> > applies to the use of Ctrl++; if you're changing the key then you no longer
> > need the alternative (note that Ctrl+= should increase zoom of course.)
> This is taking + and - away from doing a FONT SIZE change
This is changing + and - to > and <. But changing = to . makes no sense.

> > >+  <!ENTITY % messengerDTD SYSTEM "chrome://messenger/locale/messenger.dtd">
> > >+  %messengerDTD;
> > [SeaMonkey does this using a separate viewZoomOverlay]
> Yes, but it seems you reimplement the whole zoom manager
> (viewZoomOverlay.js). I just offered to split the entities into a separate
> .dtd file to be included from all places instead of full messenger.dtd.
I was just suggesting a name for the .dtd file :-)
Comment 26 :aceman 2012-09-17 05:43:19 PDT
(In reply to neil@parkwaycc.co.uk from comment #25)
> (In reply to aceman from comment #19)
> > (In reply to comment #17)
> > > > <!ENTITY increaseFontSize.label "Larger">
> > > > <!ENTITY increaseFontSize.accesskey "g">
> > > >-<!ENTITY increaseFontSize.key "+">
> > > >-<!ENTITY increaseFontSize.key2 "="> <!-- + is above this key on many keyboards -->
> > > >+<!ENTITY increaseFontSize.key "&gt;">
> > > >+<!ENTITY increaseFontSize.key2 "."> <!-- > is above this key on many keyboards -->
> > > My understanding is that the adddition of the Ctrl+= alternative only
> > > applies to the use of Ctrl++; if you're changing the key then you no longer
> > > need the alternative (note that Ctrl+= should increase zoom of course.)
> > This is taking + and - away from doing a FONT SIZE change
> This is changing + and - to > and <. But changing = to . makes no sense.
Why not? We must make room for = to be taken over by Increase Zoom. And . is on the same key as > on English layout.
Comment 27 Axel Grude [:realRaven] 2012-09-17 09:26:22 PDT
(In reply to :aceman from comment #26)
> (In reply to neil@parkwaycc.co.uk from comment #25)
> > (In reply to aceman from comment #19)
> > > (In reply to comment #17)
> > > > > <!ENTITY increaseFontSize.label "Larger">
> > > > > <!ENTITY increaseFontSize.accesskey "g">
> > > > >-<!ENTITY increaseFontSize.key "+">
> > > > >-<!ENTITY increaseFontSize.key2 "="> <!-- + is above this key on many keyboards -->
> > > > >+<!ENTITY increaseFontSize.key "&gt;">
> > > > >+<!ENTITY increaseFontSize.key2 "."> <!-- > is above this key on many keyboards -->
> > > > My understanding is that the adddition of the Ctrl+= alternative only
> > > > applies to the use of Ctrl++; if you're changing the key then you no longer
> > > > need the alternative (note that Ctrl+= should increase zoom of course.)
> > > This is taking + and - away from doing a FONT SIZE change
> > This is changing + and - to > and <. But changing = to . makes no sense.
> Why not? We must make room for = to be taken over by Increase Zoom. And . is
> on the same key as > on English layout.

Let's Narrow that one down: American keyboard layout.

On UK/Irish keyboards you have the '/' key beside < and >. 

The mathematical operators -,+ and = are located on the top right just before the backspace key. It is simply impossible to declare an 'ideal' set of shortcuts because of the differences in keyboard layouts. Luckily, the < and > are on separate keys here as well (the , and . key).
Comment 28 :aceman 2012-09-17 11:22:57 PDT
And that's great:) The base locale is en-US so I assume we can go by US keyboard layout (it seems I have one). For UK keyboards there may be a en-GB locale. The keys are localizable as all other text strings.

Also, I do not use '/' in the patch and yes, I see it to the right of > key. So I do not see what is the problem.
Comment 29 :aceman 2012-09-17 12:18:38 PDT
Created attachment 661870 [details] [diff] [review]
1/3 patch for Toolkit that is needed for the zoom to work v2
Comment 30 Blake Winton (:bwinton) (:☕️) (PTO 'til London. Find me there for quick answers!) 2012-09-20 14:38:42 PDT
Comment on attachment 661598 [details] [diff] [review]
2/3 WIP patch for TB

This seems to work well.  My main concern is that people who are used to the old font-size changing keys won't notice the new behaviour, or will notice it, and be frustrated because it's not doing what they want, and they don't know how to get the font-size-changing behaviour.

So, I guess r=me, but I _really_ think we need some clear messaging, either on first use of the old keys, or on the what's new page.  And let Roland know that we're changing shortcut keys again, so he should brace for a flood of angry email.  ;)

Thanks,
Blake.
Comment 31 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-21 01:29:16 PDT
Comment on attachment 661598 [details] [diff] [review]
> 2/3 WIP patch for TB

Imho for ux-consistency we need to add secondary/alternative shortcut "," for "decrease font size", as talked about in comment 15 and comment 16.

Reason:

Many English keyboard layouts have [< ,] on one key and [> .] on another key. To invoke < and >, you need Shift (somewhat clumsy). So in the past we decided to offer the non-shifted variants of those two keys, too (ux-error-prevention) - that's why we have Ctrl+= as alternative shortcut for zoom-in.
Patch of attachment 661598 [details] [diff] [review] implements "." as the alternative keyboard shortcut for > (increase font size)...

>+<!ENTITY increaseFontSize.key "&gt;">
>+<!ENTITY increaseFontSize.key2 "."> <!-- > is above this key on many keyboards

...but "," as the alternative shortcut for < is missing!

>+<!ENTITY decreaseFontSize.key "&lt;">
> <!ENTITY increaseFontSize.label "Larger">

That's ux-inconsistent. We should do either both or none, not one out of two.
So I suggest just adding this line:

>+<!ENTITY decreaseFontSize.key "&lt;">
+<!ENTITY decreaseFontSize.key2 ","> <!-- > is above this key on many keyboards -->

Having the alternative shortcuts of "," and "." is also helpful because they are internationally in the same position and need no modifier keys for a very large number of keyboard layouts (while position and keys required to invoke < and > differ internationally).

Aceman, could you update the patch?
Comment 32 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-21 01:32:30 PDT
(In reply to Thomas D. from comment #31)
> Comment on attachment 661598 [details] [diff] [review]
> > 2/3 WIP patch for TB
> 
> So I suggest just adding this line:
> >+<!ENTITY decreaseFontSize.key "&lt;">
> +<!ENTITY decreaseFontSize.key2 ","> <!-- > is above this key on many
> keyboards -->

Typo fix:
+<!ENTITY decreaseFontSize.key2 ","> <!-- < is above this key on many keyboards -->
Comment 33 :aceman 2012-09-21 02:40:46 PDT
Thomas, yes, I have it in my WIP patch locally.
I also need to create the viewZoomOverlay.dtd file.

I also play with the resetting of font size to Medium, but the hotkey doesn't play well with the existing menu item for that size (the <command> is not disabled/enabled properly). I don't yet know what is the problem.
Comment 34 :aceman 2012-09-27 13:12:24 PDT
Created attachment 665618 [details] [diff] [review]
2/3 patch for TB v2

So this should work as intended. I split out a new viewZoomOverlay.dtd file so I do not need to include the full messenger.dtd.

I couldn't get the 'reset font size' to work so I'll skip that. It is new functionality so can get a separate bug.
Comment 35 :aceman 2012-09-27 13:29:55 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #30)
> So, I guess r=me, but I _really_ think we need some clear messaging, either
> on first use of the old keys, or on the what's new page.  And let Roland
> know that we're changing shortcut keys again, so he should brace for a flood
> of angry email.  ;)

I think info on the What's new page would be good. Who can do that?
Comment 36 Roland Tanglao :rolandtanglao 2012-09-27 13:47:07 PDT
(In reply to :aceman from comment #35)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #30)
> > So, I guess r=me, but I _really_ think we need some clear messaging, either
> > on first use of the old keys, or on the what's new page.  And let Roland
> > know that we're changing shortcut keys again, so he should brace for a flood
> > of angry email.  ;)
> 
> I think info on the What's new page would be good. Who can do that?
if this is for TB17, anne-marie can do it, if it's for post TB17 I have no idea!
Comment 37 :aceman 2012-09-27 13:55:09 PDT
If it lands it is for TB18+.
Comment 38 Magnus Melin 2012-09-28 12:16:06 PDT
Comment on attachment 665618 [details] [diff] [review]
2/3 patch for TB v2

Review of attachment 665618 [details] [diff] [review]:
-----------------------------------------------------------------

You forgot to hg add viewZoomOverlay.dtd

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4759,5 @@
> +// on the getBrowser function.
> +function getBrowser()
> +{
> +  // return our <editor> element
> +  return document.getElementById('content-frame');

prefer double quotes when there's no need for single

@@ +4764,5 @@
> +}
> +
> +function goUpdateMailMenuItems(commandset)
> +{
> +  for (var i = 0; i < commandset.childNodes.length; i++)

prefer let
Comment 39 :aceman 2012-09-28 12:33:02 PDT
Created attachment 666024 [details] [diff] [review]
2/3 patch for TB v3
Comment 40 Magnus Melin 2012-09-29 13:06:09 PDT
Comment on attachment 666024 [details] [diff] [review]
2/3 patch for TB v3

Review of attachment 666024 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thx! r=mkmelin
Comment 42 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-30 02:34:35 PDT
Wow, thanks Aceman & everybody, great teamwork. And a lightning-fast bug turnaround of only 6 months. I suppose I also contributed by serving this on a silver tablet ;) We should do that more often, it's great fun and very encouraging.

Can we land this on TB 17, too?
Comment 43 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-30 02:45:55 PDT
Per Blake's comment 30, this change of shortcut keys needs "clear documentation" in Release Notes and/or What's New Page, because of the potentially confusing effect that the old shortcut for changing font size will now change zoom only (and since zoom also let's the font appear bigger, you won't notice the difference in composition window, only possibly after sending or from the saved draft).

Roland, will you take care of that?

I will fix the main keyboard shortcuts documentation with appopriate version-specific switches.

I'm setting "relnote" keyword as we don't have any other keyword that I'm aware of to keep this on the documentation radar. Having something like a "needs-doc", "doc-needed" or "docme" keyword might be useful.

Oh wait, there's also tbkbd-doc-tracker!
Comment 44 :aceman 2012-09-30 05:00:48 PDT
When you have a not too complicated request and the right people spot it then it can be done quickly :)

For TB17, you would need to provide a good case why it should land there, but in this case you'd need to persuade the Firefox people too (one patch is for m-c) and they may not support the push for 17, as their 18 is in normal cycle, in contrast to ours...
Comment 45 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-30 06:43:49 PDT
(In reply to Thomas D. from comment #43)
> I will fix the main keyboard shortcuts documentation (1) with appopriate
> version-specific switches.

Done. Due to a bug in Sumo/Kitsune which wrongly shows sections correctly marked for future versions, somebody needs to remove the comment marks around <!--{for tb18}...--> blocks when tb18 gets listed as a version selector; otherwise, it's an amazing example exposing kitsune's bug 720243 and bug 720249.

(1) https://support.mozillamessaging.com/en-US/kb/keyboard-shortcuts#w_writing-messages
Comment 46 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-30 06:54:48 PDT
We also need a followup bug for comment 34:
Implement keyboard shortcut for "Reset font size", e.g. Ctrl+:
Comment 47 abourcier 2012-10-01 02:50:02 PDT
Hello. 
What's New page: Even if landed in TB18, I can prepare the text and hand it over to Mark for Beta 18. 
End-user discoverablity: Who ever changes the documentation should let me know and I'll make sure the new article will be highlighted on the Start page at TB18 release. AM
Comment 48 Gion-Andri Cantieni [:gion-andri] 2012-11-18 01:59:44 PST
If you change the content of 'decreaseFontSize.key', 'increaseFontSize.key' and 'increaseFontSize.key2' in b/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd, you should also change the entity names. Otherwise, some localizers won't find this changes.
Comment 49 Michael Carr 2013-01-30 18:03:12 PST
(In reply to Blake Winton (:bwinton) from comment #30)
> Comment on attachment 661598 [details] [diff] [review]
> 2/3 WIP patch for TB
> 
> This seems to work well.  My main concern is that people who are used to the
> old font-size changing keys won't notice the new behaviour, or will notice
> it, and be frustrated because it's not doing what they want, and they don't
> know how to get the font-size-changing behaviour.
> 
> So, I guess r=me, but I _really_ think we need some clear messaging, either
> on first use of the old keys, or on the what's new page.  And let Roland
> know that we're changing shortcut keys again, so he should brace for a flood
> of angry email.  ;)
> 
> Thanks,
> Blake.

Blake was talking about me--I'm not angry, but I wasted a lot of time getting to this thread.
(I noticed the change in SeaMonkey Composer and thought it might self-correct, but it didn't.)

TWO BUGS RELATED TO THE IMPLEMENTED CHANGES:

o SeaMonkey 2.15.1 > COMPOSER does not implement:
CTRL+, (aka, CTRL+<) *** does NOT work--PLEASE FIX ***
whereas these examples all work as per new expectations:
CTRL+. (aka, CTRL+>) works fine
CTRL+SHIFT+, (aka, CTRL+SHIFT+<) works fine
CTRL+SHIFT+. (aka, CTRL+SHIFT+>) works fine

o SeaMonkey 2.15.1 HELP > COMPOSER SHORTCUTS > contains OUTDATED DOCUMENTATION (that wastes a lot of time for folks like me who look at HELP before filing BUG reports):
Decrease Font Size
    Ctrl+- (minus sign)
    Cmd+- (minus sign)
    Ctrl+- (minus sign)
Increase Font Size
    Ctrl++ (plus sign)
    Cmd++ (plus sign)
    Ctrl++ (plus sign)

*** Please change the BUG STATUS back to ACTIVE BROKEN (or whatever) since it is no longer RESOLVED FIXED (and I don't know how to change it). ***

*** Thanks *** ... for all your hard work on making Mozilla the best browser available,

Michael Carr
Dallas, TX, USA
Comment 50 :aceman 2013-01-31 06:26:25 PST
This bug is for Thunderbird only, so it can stay closed (reopening could make a mess in which patch landed in which TB version). But I see we could have changed/broken stuff for Seamonkey :( I touched the file editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd with the key definitions and that one may be shared. Please file a separate bug for your report, thanks.
Comment 51 :aceman 2013-04-02 08:15:56 PDT
*** Bug 856642 has been marked as a duplicate of this bug. ***
Comment 52 :aceman 2013-06-25 00:39:46 PDT
Thomas D., can you please check the shortcuts page? I do not see the new keys listed there.
Comment 53 Thomas D. (currently busy elsewhere; needinfo?me) 2013-06-25 01:41:13 PDT
(In reply to :aceman from comment #52)
> Thomas D., can you please check the shortcuts page? I do not see the new
> keys listed there.

Well, release version stands at TB17, so we still have the old shortcuts and documentation is up to date and correct. Do you want me to show the upcoming new shortcuts now, with an indication of "New in TB24"?

As explained in comment 45, there are several bugs in Kitsune or whereever the {for} syntax comes from, which prevent us from doing the correct things efficiently for documentation. I recommend voting for those bugs or otherwise trying to move them forward, e.g. by getting them into right product/component and alerting the right folks. I've tried but there's no response whatsoever.

Bug 886699 - [kitsune] documentation sections marked as {for TBxx} with xx = future version are shown, but should be hidden

Bug 720249 - [kitsune] Handy dandy "Show for..." button produces useless {for} syntax because of wrong UI (article editor toolbar)

Bug 720243 - [kitsune] Simplify {for} syntax for multiple subsequent product versions: support {for tb5-tb9} instead of clumsy {for =tb5,=tb6,=tb7,=tb8,=tb9}

(In reply to Thomas D. from comment #45)
> (In reply to Thomas D. from comment #43)
> > I will fix the main keyboard shortcuts documentation (1) with appopriate
> > version-specific switches.
> 
> Done. Due to bug 886699 in Sumo/Kitsune which wrongly shows sections correctly
> marked for future versions, somebody needs to remove the comment marks
> around <!--{for tb18}...--> blocks when tb18 gets listed as a version
> selector; otherwise, it's an amazing example exposing kitsune's bug 720243
> and bug 720249.
> 
> (1)
> https://support.mozillamessaging.com/en-US/kb/keyboard-shortcuts#w_writing-
> messages
Comment 54 :aceman 2013-06-25 10:50:20 PDT
(In reply to Thomas D. from comment #53)
> Well, release version stands at TB17, so we still have the old shortcuts and
> documentation is up to date and correct. Do you want me to show the upcoming
> new shortcuts now, with an indication of "New in TB24"?

No, I think you know what you are doing there so please only do what you think is necessary.
Comment 55 Thomas D. (currently busy elsewhere; needinfo?me) 2013-07-14 07:57:27 PDT
*** Bug 486151 has been marked as a duplicate of this bug. ***
Comment 56 Thomas D. (currently busy elsewhere; needinfo?me) 2013-07-14 08:01:05 PDT
*** Bug 459861 has been marked as a duplicate of this bug. ***
Comment 57 Thomas D. (currently busy elsewhere; needinfo?me) 2013-07-14 08:03:04 PDT
*** Bug 493530 has been marked as a duplicate of this bug. ***
Comment 58 Thomas D. (currently busy elsewhere; needinfo?me) 2013-07-14 08:05:01 PDT
*** Bug 536134 has been marked as a duplicate of this bug. ***
Comment 59 Thomas D. (currently busy elsewhere; needinfo?me) 2013-09-22 02:54:02 PDT
(In reply to :aceman from comment #54)
> (In reply to Thomas D. from comment #53)
> > Well, release version stands at TB17, so we still have the old shortcuts and
> > documentation is up to date and correct. Do you want me to show the upcoming
> > new shortcuts now, with an indication of "New in TB24"?
> 
> No, I think you know what you are doing there so please only do what you
> think is necessary.

I have updated new shortcuts of this bug in the documentation per bug 919276:

https://support.mozillamessaging.com/en-US/kb/keyboard-shortcuts/revision/8184

Pls verify especially on MAC OS and Linux if all shortcuts work as documented, and if documentation covers all alternative shortcuts present on your OS (compare with Windows shortcuts). Report anything which requires updates of documentation to bug 919276.

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