Closed Bug 1637657 Opened 11 months ago Closed 11 months ago

Selection of recipient pills with modifier keys (Shift+▶, Ctrl+▶) unexpectedly lost when reaching row input

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 78.0

People

(Reporter: thomas8, Assigned: thomas8)

References

(Blocks 1 open bug)

Details

(4 keywords)

Attachments

(1 file, 2 obsolete files)

Some "edge cases" are actually common cases...

STR

  1. Intend to select the last n pills of a given addressing row (say pills 5 to 10) fast and without fail. Start by selecting pill 5.
  2. Press and hold Shift+Cursor-right as that would select all pills to the end.
  3. As a variation (different case), Ctrl+Space to select some pills in front (1 and 3), intend to Ctrl-select the last pill (10), so press and hold Ctrl+Cursor-right to navigate to last pill whilst preserving existing selection.

Actual Result

  • Your existing selection silently and annoyingly disappears as focus ends up in row input. That's good for plain navigation, but not for selection mode (Shift-selection/Ctrl-navigation).

Expected Result

  • We really have to stop at the last pill for selections/navigations with Shift/Ctrl+navigation keys and thus preserve user's hand-picked selection.
Summary: Selection of recipient pills with modifier keys (Shift, Ctrl) unexpectedly lost when reaching row input → Selection of recipient pills with modifier keys (Shift+▶, Ctrl+▶) unexpectedly lost when reaching row input

Indeed, this behaviour is a bit annoying and we should address it.

OK, so this fixes the basic cases of Shift+Cursor-right and Ctrl+cursor-right.
So if you hold Shift|Ctrl+Cursor-right (ongoing selection), we'll now stop at the last pill instead of focusing row input, so as to preserve your selection.

Makes a good workaround for the more complex cases involving other navigation keys like END and page-up/down (bug 1602397).

Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #9148062 - Flags: review?(alessandro)
Comment on attachment 9148062 [details] [diff] [review]
1637657_recipientPillsSelectOnEdge.diff

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

Some comments on things I removed assuming they are unused - Alex, pls verify!
Setting f+ so that I can file these comments as a review in the code.

::: mail/base/content/mailWidgets.js
@@ -2376,5 @@
>            }
>            break;
>  
>          case "ArrowRight":
> -          if (pill.nextElementSibling.hasAttribute("hidden")) {

I couldn't figure out the purpose of this "hidden" stuff. Do we have hidden pills or inputs? So I'm assuming that it's no longer used.

@@ -2486,2 @@
>        if (event.shiftKey) {
> -        if (this.hasAttribute("selected") && element.hasAttribute("selected")) {

`this` points to the attachment area. So this code was removing a "selected" attribute from the entire recipient area when the navigation target (pill/input) had a previous "selected" attribute. Again, I assume we're no longer using this, but Alex might know more. Please verify!

@@ -2490,3 @@
>          }
> -
> -        this.setAttribute("selected", "selected");

dito. Removed under the assumption of "unused".
Attachment #9148062 - Flags: review?(alessandro) → feedback+
Attachment #9148062 - Flags: feedback+
Comment on attachment 9148062 [details] [diff] [review]
1637657_recipientPillsSelectOnEdge.diff

sorry
Attachment #9148062 - Flags: review?(alessandro)
Comment on attachment 9148062 [details] [diff] [review]
1637657_recipientPillsSelectOnEdge.diff

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

Great job.
This works almost perfectly. I think we should also handle the `End` keypress to complete this polishing.

::: mail/base/content/mailWidgets.js
@@ -2377,5 @@
>            break;
>  
>          case "ArrowRight":
> -          if (pill.nextElementSibling.hasAttribute("hidden")) {
> -            pill.nextElementSibling.removeAttribute("hidden");

The "hidden" attribute is indeed a leftover from the first implementation, where I was hiding the input fields when not focused to handle spacing. Not necessary anymore since we implemented a better layout.

@@ +2393,3 @@
>            break;
>  
>          case "End":

We should also handle the End keypress in case the user has CTRL or SHIFT pressed.
For those cases, we should jump to the last pill and not the recipient field.

@@ +2482,3 @@
>       */
> +    checkKeyboardSelected(event, targetElement) {
> +

nit: remove blank row.

@@ +2509,5 @@
> +      // If targetElement is a pill, focus it.
> +      if (targetPill) {
> +        targetPill.focus();
> +      }
> +

nit: remove extra blank row.
Attachment #9148062 - Flags: review?(alessandro) → feedback+

(In reply to Alessandro Castellani (:aleca) from comment #5)

Review of attachment 9148062 [details] [diff] [review]:

A thoughtful review which gently instituted some more polishing.

Great job.
This works almost perfectly.

:-))

I think we should also handle the End keypress to complete this polishing.

What a clever thought! You fox ;-)
Polished, and some more.
We're getting there.

The "hidden" attribute is indeed a leftover from the first implementation,

Since you didn't protest, I assume same applies for the selected attribute on the recipient area, right?

@@ +2393,3 @@

       break;

     case "End":

We should also handle the End keypress in case the user has CTRL or SHIFT
pressed. For those cases, we should jump to the last pill and not the recipient field.

Done for Ctrl. Cleaned up for Shift: As we do not handle Shift+End/Home yet, let's ignore shift here and treat it as unmodified, to avoid creating false expectations and use patterns.

Attachment #9148062 - Attachment is obsolete: true
Attachment #9148898 - Flags: review?(alessandro)
Comment on attachment 9148898 [details] [diff] [review]
1637657_recipientPillsSelectOnEdge.diff

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

This looks good, thanks for the update and polishing.
Just a few nits to update, and than it's a r+.

I can't launch a try-run now since we're currently busted, but no test should be affected by this.

Most of these nits can be prevented by setting up eslint and prettier in your code editor.
Which one are you using? Do you need help in setting it up?

::: mail/base/content/mailWidgets.js
@@ +2386,5 @@
>          case "Home":
> +          let firstPill = pill
> +            .closest(".address-container")
> +            .querySelector("mail-address-pill");
> +          if(!event.ctrlKey) {

missing space before parenthesis.

@@ +2400,5 @@
> +          if (!event.ctrlKey) {
> +            // Unmodified navigation: focus row input.
> +            // ### Todo: We can't handle Shift+End yet, so it ends up here.
> +            pill.rowInput.focus();
> +          } else {

Let's break; early here so we avoid the `else` callback.

@@ +2402,5 @@
> +            // ### Todo: We can't handle Shift+End yet, so it ends up here.
> +            pill.rowInput.focus();
> +          } else {
> +          // Navigation with Ctrl modifier key: focus last pill.
> +            pill

Wrong indentation of the comment, but if you remove the else is correct :D
Attachment #9148898 - Flags: review?(alessandro) → feedback+

Ok, nits fixed.

Attachment #9148898 - Attachment is obsolete: true
Attachment #9149111 - Flags: review?(alessandro)

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

Most of these nits can be prevented by setting up eslint and prettier in
your code editor. Which one are you using? Do you need help in setting it up?

Let's talk about that on PM some time.

Attachment #9149111 - Flags: review?(alessandro) → review+

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

Set off a try build: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5ddb625fcee92cdc7b36995540d03bf95a19c096

Thanks. Loads of failures, but I didn't see any which I could relate to this bug. Alas, I'm not an expert on reading test results, can someone verify?

Flags: needinfo?(mkmelin+mozilla)

We'll have to do another run later once the errors are cleared. They are not at least mainly from this bug but from bug 1638288 (and maybe something else).

Flags: needinfo?(mkmelin+mozilla)

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/94c8faf148df
Prevent loss of recipient pills selection by stopping at last pill for Ctrl|Shift+Cursor-right|+End. r=aleca

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 78.0
You need to log in before you can comment on or make changes to this bug.