1710245 - Sticky pill selection and erratic focus after "Use Bcc instead" from public bulk mail notification

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect, P2)

Description

[Thomas D. (:thomas8)]

Attachment: Screencast: sticky selection and unhelpful focus after "Use Bcc instead"

+++ This bug was initially created as a clone of Bug #1706204 +++

STR

1. compose, add 15 recipients in To or Cc (with Bcc row initially hidden - default)
   -> Public bulk mail notification appears
2. Hit Use Bcc instead and observe focus and selection thereafter.
3. Click into other spots like subject, body, or contacts sidebar, trying to get rid of pills selection.

Actual result (see attached screencast)

• step 2) moved pills are selected, but focus is in Bcc recipient input - this should never happen. When pills are selected, a pill must have focus - if no pill has focus, there must not be selected pills.
• step 3) Pill selection stubbornly remains - possible, but not easy enough to get rid off.

If you start out with contacts sidebar and Bcc row already open before the move, same problem of sticky selection, but focus jumps over to the address book context menu button - that's even less useful.

Expected result

• Last selected pill should have focus after bulk moving recipients. Selection provides action success feedback, focus on last pill makes it easy to deselect via cursor-right into recipient input.

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1710245_publicBulkRecipientsFixFocus.diff

This fixes the sticky recipient pills selection by focusing the last selected pill after the move.
Some other nits fixed.

Comment 2

[Lasana Murray]

Comment on attachment 9220978 [details] [diff] [review]
1710245_publicBulkRecipientsFixFocus.diff

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

Works for me, just include the type in the jsdoc please.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5233,2 @@
>   *
> + * @returns {Array} All <mail-address-pill> elements in the "To" and "CC" fields.

Specify the array type here so it's clear what it actually contains (in JS not DOM). It may not do much now, but if we ever get into generating API docs from source or type checking it will be very helpful.

Comment 3

[Thomas D. (:thomas8)]

(In reply to Lasana Murray from comment #2)

Review of attachment 9220978 [details] [diff] [review] ⧉[details] [diff] [review]:

• • @returns {Array} All <mail-address-pill> elements in the "To" and "CC" fields.

Specify the array type here so it's clear what it actually contains (in JS
not DOM). It may not do much now, but if we ever get into generating API
docs from source or type checking it will be very helpful.

I'm not sure what to change here - can you propose a change?
I've tried to follow the documentation:
https://jsdoc.app/tags-returns.html
It's a plain Javascript Array of HTML Custom Elements - so {Array} looks correct, and the description provides the details.

Comment 4

[Magnus Melin [:mkmelin]]

perhaps @returns {Element[]}

Comment 5

[Lasana Murray]

(In reply to Thomas D. (:thomas8) from comment #3)

I'm not sure what to change here - can you propose a change?
I've tried to follow the documentation:
https://jsdoc.app/tags-returns.html
It's a plain Javascript Array of HTML Custom Elements - so {Array} looks correct, and the description provides the details.

Oh wow, jsdoc still does not support this yet, my bad.
https://github.com/jsdoc/jsdoc/issues/1073

I use the TypeScript reference because use it in other projects:
https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#type

Your call then, personally I think it's helpful; with the YouCompleteMe extension for Vim I get code completion for example.

Comment 6

[Thomas D. (:thomas8)]

Attachment: 1710245_publicBulkRecipientsFixFocus.diff

(In reply to Lasana Murray from comment #2)

Review of attachment 9220978 [details] [diff] [review] ⧉[details] [diff] [review]:

Thanks for the review!

Works for me, just include the type in the jsdoc please.
> + * @returns {Array} All <mail-address-pill> elements in the "To" and "CC" fields.

(In reply to Magnus Melin [:mkmelin] from comment #4)

perhaps @returns {Element[]}

OK, that looks good - done.

r=lasana from comment 2.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8f9fb6e4b672
Prevent sticky recipients selection by focusing last pill after moving public bulk recipients to Bcc. r=lasana

Comment 8

[Thomas D. (:thomas8)]

(In reply to Pulsebot from comment #7)

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8f9fb6e4b672
Prevent sticky recipients selection by focusing last pill after moving public bulk recipients to Bcc. r=lasana

Thank you for pushing this to the repository, that was quick! :-)