738194 - Composition: Add keyboard shortcuts for Zoom (Ctrl++, Ctrl+-, Ctrl+0: ux-consistency with message reader), change shortcuts for font size to Ctrl+<, Ctrl+>

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

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. (:thomas8)]

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. (:thomas8)]

Attachment: 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) (:☕️)]

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. (:thomas8)]

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) (:☕️)]

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. (:thomas8)]

Aceman, is this something you could fix?

Comment 7

[:aceman]

I can try.

Comment 8

[:aceman]

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. (:thomas8)]

(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]

Attachment: 1/3 patch for Toolkit that is needed for the zoom to work.

Comment 11

[Thomas D. (:thomas8)]

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]

Attachment: 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]

Attachment: 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]

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]

(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. (:thomas8)]

(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

[[e-mail]]

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

[[e-mail]]

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]

(In reply to [e-mail] 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]]

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]

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 23

[Thomas D. (:thomas8)]

(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]

> 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

[[e-mail]]

(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]

(In reply to [e-mail] 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]]

(In reply to :aceman from comment #26)
> (In reply to [e-mail] 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]

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]

Attachment: 1/3 patch for Toolkit that is needed for the zoom to work v2

Comment 30

[Blake Winton (:bwinton) (:☕️)]

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. (:thomas8)]

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. (:thomas8)]

(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]

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]

Attachment: 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]

(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 needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

(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]

If it lands it is for TB18+.

Comment 38

[Magnus Melin [:mkmelin]]

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]

Attachment: 2/3 patch for TB v3

Comment 40

[Magnus Melin [:mkmelin]]

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 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0b325032484
https://hg.mozilla.org/comm-central/rev/328ac7ab1686
https://hg.mozilla.org/comm-central/rev/aee422d2fa9f

Comment 42

[Thomas D. (:thomas8)]

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. (:thomas8)]

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]

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. (:thomas8)]

(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. (:thomas8)]

We also need a followup bug for comment 34:
Implement keyboard shortcut for "Reset font size", e.g. Ctrl+:

Comment 47

[abourcier]

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]]

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]

(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]

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 52

[:aceman]

Thomas D., can you please check the shortcuts page? I do not see the new keys listed there.

Comment 53

[Thomas D. (:thomas8)]

(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]

(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 59

[Thomas D. (:thomas8)]

(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.

Comment 60

[[e-mail]]

(In reply to Thomas D. (currently busy elsewhere) from comment #59)
> Pls verify especially on MAC OS and Linux if all shortcuts work as documented
⌘, is supposed to be Preferences on Mac OS, but this broke it (although as mentioned in comment #49, SeaMonkey escaped).

Comment 61

[Bernie Innocenti]

(In reply to Thomas D. (currently busy elsewhere) from comment #59)
> 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.

All the editor and mail shortcuts I tried seem correct on Linux. Thank you.