1731837 - Wrong l10n labels on cmd_delete in 3-pane with focus in search fields: Delete message, Remove contact (both out of context, esp. on search input context menu)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect, P3)

Description

[Thomas D. (:thomas8)]

Something went haywire around cmd_delete in 3-pane, which gets the wrong labels in search inputs' context menu and main menu.
I remember that this command is quite overloaded to fit all the focus locations where deletion is possible and update the labels accordingly.
I'm failing to understand where the "Remove contact" comes from, because that does not seem to apply anywhere in the entire 3-pane UI. This started to go wrong in TB 78.

STR

91.1.1 (64-bit) and 94.0a1 (2021-09-21) (64-bit), Win10; similar on 78

1. Select a msg
2. Type some text "foo" into Global Search input, and select it
3. Right-click on selected search text, check out item label under Paste
4. Click on Delete Message (sic, this bug) from search input context menu
5. With no search text, Delete Message from search input context menu again
6. From main menu (sic!): choose Edit, and check out item label under Paste
7. Repeat step 2 to 3 (select text in search input, right-click, check out item label)

Actual

• context menu on selected search text shows Delete Message, enabled - this action does not belong here! This is the toolkit context menu on text inputs...
• Delete Message is wrong label, but correctly deletes selected search text
• When there's no search text, Delete Message from search context is still enabled (that's wrong), still out of context (also wrong), and luckily still correctly not deleting the message.
• Edit > Delete %%% shows Remove contact - what!? no selected contacts here, and I'm not aware that this action can be done anywhere in 3-pane
• After that, search input context menu on selected text now also shows Remove Contact, which remains disabled (meaning you can no longer use Delete via mouse on search text)

Expected

• Always show just Delete on search input context menu
• Always enable contextual Delete if there's selected text
• Never show Delete message on search input context menu
• Never show Remove contact in 3-pane

I guess that's where we'd feel like ripping out cmd_delete, but be warned that there'll be dragons - it'll be a totally non-trivial task as we have all sorts of special cases, e.g.:

• with focus in msg preview, DEL should delete the message, and some even like that behaviour when text is selected
• even with focus and selection on account nodes in folder list, we'll have Delete message in the account node context menu (for safety reasons iirc), which is really only to enable DEL for deleting the message, and should probably be disabled on the context menu.

We certainly do want the main menu (and app menu?) to update and inform the user which item is going to get deleted.

Comment 1

[Thomas D. (:thomas8)]

Attachment: Screencast: cmd_delete labels gone haywire

For ease of understanding, here's a video of what currently happens in 91.1.1 (64-bit) wrt wrong labels for cmd_delete.

Comment 2

[Geoff Lankow (:darktrojan)]

It looks to me like the string from chat.dtd is used instead of the one from utilityOverlay.dtd. Both are included in messenger.xhtml, so I guess it's just used the first value it found.

https://searchfox.org/comm-central/search?q=deletecmd.label&path=mail%2Flocales&case=false&regexp=false

Comment 3

[Thomas D. (:thomas8)]

Alex, could you find an assignee for this?

Comment 4

[Alessandro Castellani [:aleca]]

This seems to be a conflict between our XUL command defined in the mainCommandSet.inc.xhtml
https://searchfox.org/comm-central/rev/19830be6418c2c94149a6ebfee8d8e0123a03c9b/mail/base/content/mainCommandSet.inc.xhtml#108

which dynamically changes the value based on the active selection/item in this method: https://searchfox.org/comm-central/rev/19830be6418c2c94149a6ebfee8d8e0123a03c9b/mail/base/content/mailWindowOverlay.js#1870-1882

Theoretically, we should improve this method to detect if the focus is currently on a text field, and set the correct label, but I'm not sure if that might be like putting band-aid on concrete crack.
I think we should investigate this a bit further before assigning someone to fix it as we need to define a proper direction.

Some questions:

• Do we know why we're using the same context menu for messages and input fields?
• Shouldn't that context menu be completely separate and independent?
• Why is the mailEditMenuItems leaking around the application and not recognizing the currently focused element?

Comment 5

[Thomas D. (:thomas8)]

I think we can safely assume that this is a regression.

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Alessandro Castellani [:aleca] from comment #4)

This seems to be a conflict between our XUL command defined in the mainCommandSet.inc.xhtml
https://searchfox.org/comm-central/rev/19830be6418c2c94149a6ebfee8d8e0123a03c9b/mail/base/content/mainCommandSet.inc.xhtml#108

which dynamically changes the value based on the active selection/item in this method: https://searchfox.org/comm-central/rev/19830be6418c2c94149a6ebfee8d8e0123a03c9b/mail/base/content/mailWindowOverlay.js#1870-1882

Theoretically, we should improve this method to detect if the focus is currently on a text field, and set the correct label, but I'm not sure if that might be like putting band-aid on concrete crack.
I think we should investigate this a bit further before assigning someone to fix it as we need to define a proper direction.

Some questions:

• Do we know why we're using the same context menu for messages and input fields?
• Shouldn't that context menu be completely separate and independent?
• Why is the mailEditMenuItems leaking around the application and not recognizing the currently focused element?

Does this need the eyes of someone other than Thomas?

Comment 7

[Alessandro Castellani [:aleca]]

This seems to be fixed in 102, not sure what did it.

Comment 8

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani [:aleca] from comment #7)

This seems to be fixed in 102, not sure what did it.

Have you tried all STR of comment 0, as demonstrated in the screencast of attachment 9242314 [details]?

Still fails pretty much exactly as described on 102.0 (64-bit) and 104.0a1 (2022-06-29) (64-bit), something in the cmd_delete machinery goes wrong. Please use the screencast for guidance, and follow all steps of comment 0 religiously (not sure how the bug gets closed without rechecking STR and screencast!). As noted in steps, you need to start with selecting a message before you go into the global search box. Don't click elsewhere between steps.

The behaviour also changes during steps, so even though initally the Delete Message on selected text in global search box may not occur (saw that once I think, maybe no message selected), it still happens after going through the other steps. Edit > Remove Contact also still seen in 3-pane when global search box has focus, and also in search box context.

Comment 9

[Thomas D. (:thomas8)]

This has gotten worse on 113.0a1 (2023-04-03) (64-bit), Win10.
Without using any special steps that I'm aware of, I now have Edit > Remove Contact (Del) label permanently set even when messages are selected, even after restart. Which makes no sense at all, and is pretty irritating - now I don't know which contact it will delete if I use that command from the menu.

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1731837 - Handle cmd_delete differently depending on what has focus. r=aleca

This renames cmd_delete handling (for the mail views) to cmd_deleteMessage, then creates new
cmd_delete handlers which wrapscmd_deleteFolder or cmd_deleteMessage depending on whether the
folder tree has focus.

If a text box has focus, cmd_delete behaves as expected, although the strings are still wrong,
which is fixed in the next patch.

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1731837 - Fix multiple problems with the labelling of Delete commands. r=aleca

This fixes the Edit menu Delete command to say "Delete" again by default, or a context appropriate
string if the current tab is a mail tab.

It also fixes the context menu in mail tabs to say "Undelete" in the IMAP delete-in-place mode.

Depends on D177219

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1731837 - Test the Edit menu's Delete item says the right thing and does what it says. r=aleca

Depends on D177220

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c86b0c26b3c1
Handle cmd_delete differently depending on what has focus. r=aleca
https://hg.mozilla.org/comm-central/rev/067019c08382
Fix multiple problems with the labelling of Delete commands. r=aleca
https://hg.mozilla.org/comm-central/rev/c3e37db8a937
Test the Edit menu's Delete item says the right thing and does what it says. r=aleca

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9331730 [details]
Bug 1731837 - Handle cmd_delete differently depending on what has focus. r=aleca

[Triage Comment]
Approved for beta

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9331731 [details]
Bug 1731837 - Fix multiple problems with the labelling of Delete commands. r=aleca

[Triage Comment]
Approved for beta

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9332162 [details]
Bug 1731837 - Test the Edit menu's Delete item says the right thing and does what it says. r=aleca

[Triage Comment]
Approved for beta

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 114.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/72dc2c7b6932
https://hg.mozilla.org/releases/comm-beta/rev/6414640c4265
https://hg.mozilla.org/releases/comm-beta/rev/6114fff6198e