1527551 - Composition's Formatting Bar wrongly disabled when using paragraph format or font selector menulist dropdowns

Status: VERIFIED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Thomas D. (:thomas8)]

Regression: not seen on 60.5.0 (32-bit), seen on 67.0a1 (2019-02-11) (64-bit).
This does not seem to impede functionality, but it's irritating and looks unprofessional.

STR

1. Compose new message
2. Focus msg body (even after typing and selecting text)
3. Click into paragraph format or font selector to change them (menulist dropdowns on formatting bar)

Actual result

Formatting bar disabled

Expected result

Formatting bar not disabled

Comment 1

[Magnus Melin [:mkmelin]]

I don't see this on trunk.

Comment 2

[Jorg K (CEST = GMT+2)]

I'm not seeing this in TB 68 fortunately.

Comment 3

[Thomas D. (:thomas8)]

Using STR of comment 0, easily and reliably reproducable on TB 70.0a1 (2019-08-14) (64-bit).

STR

1. Compose
2. Focus msg body (initially correctly enables formatting toolbar); optionally select some msg body text.
3. Single-Click into either of the following dropdown selectors: Paragraph format | Font (dropdown now open), and observe toolbar status
4. Click on disabled top part of dropdown selector again to close it (I need the other one...), and observe toolbar status

Actual result:

• after step 3(!): first normal click on selector correctly opens the dropdown, but incorrectly disables the entire toolbar including the toolbar item (top part) of the currently active selector - clearly wrong. (Note: The dropdown list itself is enabled, has focus and is functional unless for cases of step 4.)
• after step 4: second click on disabled selector or anywhere else on formatting toolbar closes the dropdown and now makes the entire disabled toolbar inaccessible, without any indication of focus. Also very wrong, especially when the last focus was on selected text, so all formatting commands should be enabled.

Expected result:

• after step 3: When focus/selection is in body/editor, just using items of the formatting toolbar should never disable the same at any time.
• after step 4: close the dropdown (with focus if clicked), keep toolbar enabled, allow using any other formatting items

Comment 4

[Thomas D. (:thomas8)]

This looks like a fallout from de-XBL/de-XUL where technical focus is now different from what it used to be (active element vs. focused part of that element), so TB wrongly believes that focus is NOT on body and NOT on formatting toolbar, which is conceptually wrong. As long as focus is on anything from formatting toolbar (like selector dropdowns), obviously the bar should be enabled to allow, well, formatting of the selected body text, including 0-length selection.

Comment 5

[Jorg K (CEST = GMT+2)]

Damn, how foolish of me to close this, it's totally visible, albeit not affecting usability. My apologies.

Alice, can you please find the regression for us. If the bug report is correct, somewhere at TB 67.

Comment 6

[:aceman]

This is what I have investigated:
When clicking into compose body, the focusedwinow in CommandUpdate_MsgCompose() is "Window about:blank"

When clicking into recipients, the focusedwindow in CommandUpdate_MsgCompose() is "ChromeWindow chrome://messenger/content/messengercompose/messengercompose.xul"

The same value appears now also when clicking the "font" menulist. That is kinda understandable, we are in the composer UI, not the message body.
What this bug is about is actually to special-case the toolbar so that even if we are not in the message body, but are in the toolbar, it should not be disabled. The current code probably does not check where we clicked in the non-body window specifically so disables the toolbar unconditionally.

I haven't yet found which code disables the formatting toolbar.
But in updateComposeItems() (called from CommandUpdate_MsgCompose()) we call goUpdateCommand("cmd_renderedHTMLEnabler"); ("font" and other items on the toolbar observe this command) . And at that spot the command is disabled according to where we are focused in the UI.
The strange thing is, this command is handled by the editor code at https://searchfox.org/comm-central/source/editor/ui/composer/content/ComposerCommands.js#407, but only when we are in the message body. When outside it, something else handles and disables it.

Comment 7

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=66296c90046564a6b068ffdfe6bf92dc071464e8&tochange=9289d8a37eb2dae82f2ecc0076ec8396126a5526
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=88cfe9dfcb973c643cb2a2ce3a15262d3454c57c&tochange=e4e2245fc1428c33db179ca2f242b922ad988d79

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks so much, Alice!

So this goes back about one year :-( - Nothing obvious, but it could be this:
Sriharsha sriharsh007jc — Bug 1484949 Remove obsolete .toolbar-focustarget styling r=dao
https://hg.mozilla.org/mozilla-central/rev/fab25a0a45fa9ea78b1fcf5a0a030bd2ee4980c8

Actually, I have a patch that fixes it.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1527551-fix-focus-compose.patch

This fixes it.

Thank you so much once again, Alice. Without your work we would have been searching for a solution for days.

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 9085682 [details] [diff] [review]
1527551-fix-focus-compose.patch

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

Perfect, this works again.

::: mail/themes/shared/mail/messengercompose.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
> +/* removed from toolkit/themes/NNN/global/global.css in bug 1484949 */

/* removed from global.css in bug 1484949 */
would be enough.

Comment 11

[Richard Marti (:Paenglab)]

Comment on attachment 9085682 [details] [diff] [review]
1527551-fix-focus-compose.patch

Ah forgot this: Please move it below after the /* :::: Format toolbar :::: */ where it belongs.

Comment 12

[Magnus Melin [:mkmelin]]

And maybe in the comment explain why it's needed instead of saying it was removed in ,,,,

Comment 13

[Jorg K (CEST = GMT+2)]

Well, what comment do you suggest? People can look up the bug where it was added. I don't claim to understand what that does, so I can't add a qualified comment here :-(

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1527551-fix-focus-compose.patch

I tried my best to come up with a useful comment.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/768ececa54d4
Restore .toolbar-focustarget in messengercompose.css after removal in bug 1484949. r=Paenglab DONTBUILD

Comment 16

[Jorg K (CEST = GMT+2)]

TB 69 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/510a90a516e77db9a24322b56681dbd1884032e1

Comment 17

[Jorg K (CEST = GMT+2)]

TB 68.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/8ffcfb241e6062554f146fc78c82c3635949857b

Comment 18

[Jorg K (CEST = GMT+2)]

Working again in TB 68 ESR, thanks, Thomas!