1716823 - Inconvenient focus in contacts sidebar after "Keep recipients public" from public bulk mail notification

Status: RESOLVED FIXED

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

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #1710245 +++
TB 90.0b1 / 91.0a1 (2021-06-15) (64-bit)

STR

1. Compose with Contacts Sidebar open
2. Add 15 recipients in To or Cc
   -> Public bulk mail notification appears
3. Click Keep recipients public button (or press Alt+K) and observe focus

Actual result

• Focus jumps over to the address book context menu button (≡) in the sidebar - that's unexpected and not useful! Pretty hard to get back to a useful focus position using keyboard.

Expected result

• Focus a more suitable element (ideally the element which had focus before)

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1716823_keepRecipientsPublicFocusFix.diff

This patch improves the focus behaviour after Keep Recipients Public was chosen and the notification disappeared:

• Try to restore focus to where it was before the notification got focus.
• Otherwise focus message body.

Some workflows:

• Add 15 recipients, notification pops up, press Alt+K to get rid of that instantly, and just continue adding more recipients without any focus hassles (perfect keyboard convenience!).
• Ignore the notification at first, start working on message body, notice unwanted notification, click Keep Recipients Public, and just continue with message body where you left (one clumsy click to refocus body avoided).
• If you deliberately focused something else before dismissing the notification, we'll also try to restore that focus, assuming that you had your reasons, and possibly aren't done there yet.

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9227489 [details] [diff] [review]
1716823_keepRecipientsPublicFocusFix.diff

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

Interesting approach.
I think the direction is good but I'm not sure about assigning a custom attribute to the target, because as a general rule we're trying to avoid it.
Maybe we could have a `var lastFocusedElement;` declared in the file and use it for this scenario.
This new variable might also come in handy in the future if we need to more carefully restore the focus in other scenarios (eg. when opening and closing the attachment via keyabord, would be nice to move the focus back to the previously focused item).
What do you think?

Comment 3

[Thomas D. (:thomas8)]

Attachment: 1716823_keepRecipientsPublicFocusFix.diff

Alright, thanks. Changes made per review of comment 2.

I've also reworked the behaviour for a more consistent UX, and to support the most likely workflows.

• Restore focus in address-rows if it was there - user might well want to just click the notification away and continue adding more recipients.
• Otherwise if subject is still empty, focus that as a courtesy of ux-error-prevention.
• Otherwise focus message body: central focus point near the notification.

Comment 4

[Alessandro Castellani [:aleca]]

Comment on attachment 9228013 [details] [diff] [review]
1716823_keepRecipientsPublicFocusFix.diff

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +215,5 @@
> +document.addEventListener("focusin", event => {
> +  // Listen for focusin event in composition. gLastFocusElement might well be
> +  // null, e.g. when focusin enters a different document like contacts sidebar.
> +  gLastFocusElement = event.relatedTarget;
> +});

Since we only care about this element if it's an address input, we could instead set it only in the `addressInputOnFocus()` method.
And set it to `null` on blur.
By doing so we can avoid the condition of the element containing the address-input class.

@@ +5195,5 @@
> +        : (() => {
> +            return !msgSubject.value
> +              ? msgSubject
> +              : document.getElementById("content-frame");
> +          })()

This is smart, but it's not super readable.
I see you tried to overcome the limitation of the linter which doesn't allow indented ternary operations.
I think we should make this a bit more plain and readable.

      // After closing notification with `Keep Recipients Public`, actively
      // manage focus to prevent weird focus change e.g. to Contacts Sidebar.
      // If focus was in addressing area before, restore that as the user might
      // dismiss the notification when it appears while still adding recipients.
      if (gLastFocusElement?.classList.contains("address-input")) {
        gLastFocusElement.focus();
        return false;
      }

      // Otherwise if there's no subject yet, focus that (ux-error-prevention).
      let msgSubject = document.getElementById("msgSubject");
      if (!msgSubject.value) {
        msgSubject.focus();
        return false;
      }

      // Otherwise default to focusing message body.
      document.getElementById("content-frame").focus();
      return false;

It's more verbose and adds more lines of code, but it's way cleaner and easier to read.

Comment 5

[Thomas D. (:thomas8)]

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

Review of attachment 9228013 [details] [diff] [review] ⧉[details] [diff] [review]:
::: mail/components/compose/content/MsgComposeCommands.js
@@ +215,5 @@

+document.addEventListener("focusin", event => {
+  // Listen for focusin event in composition. gLastFocusElement might well be
+  // null, e.g. when focusin enters a different document like contacts sidebar.
+  gLastFocusElement = event.relatedTarget;
+});

Since we only care about this element if it's an address input, we could
instead set it only in the addressInputOnFocus() method.
And set it to null on blur.

As discussed, this would not work.
Even if we'd change it to make it work, per your own comment 2, we are actually expecting more uses of gLastFocusElement, so instead of placing small focus listeners everywhere, it would seem better to just have a single central focus listener and to know that gLastFocusElement will always have the correct element (except where it can't, due to the multi-document nature of composition).

@@ +5195,5 @@

+        : (() => {
+            return !msgSubject.value
+              ? msgSubject
+              : document.getElementById("content-frame");
+          })()

This is smart, but it's not super readable.
I see you tried to overcome the limitation of the linter which doesn't allow
indented ternary operations.

You got me!!! 😎
It's a pity that Mozilla's linter doesn't like nested ternaries as a perfectly terse notation - and as you get to know them, they become very readable, too! We should actually call them chained ternaries, and here's a nice explanation why nested ternaries are great!

Compare: Our section condensed with chained ternaries vs. plain vanilla procedural conditionals using if (see Aleca's rewrite below).

      let msgSubject = document.getElementById("msgSubject");
      (gLastFocusElement?.classList.contains("address-input")
        ? gLastFocusElement
        : !msgSubject.value
        ? msgSubject
        : document.getElementById("content-frame")
      ).focus();
      return false;

I think we should make this a bit more plain and readable.

      // After closing notification with `Keep Recipients Public`, actively
      // manage focus to prevent weird focus change e.g. to Contacts Sidebar.
      // If focus was in addressing area before, restore that as the user might
      // dismiss the notification when it appears while still adding recipients.

      if (gLastFocusElement?.classList.contains("address-input")) {
        gLastFocusElement.focus();
        return false;
      }

      // Otherwise if there's no subject yet, focus that (ux-error-prevention).
      let msgSubject = document.getElementById("msgSubject");
      if (!msgSubject.value) {
        msgSubject.focus();
        return false;
      }

      // Otherwise default to focusing message body.
      document.getElementById("content-frame").focus();
      return false;

It's more verbose and adds more lines of code, but it's way cleaner and
easier to read.

It's quite verbose and soooo boring...! 🥱
Let's go for it then as we don't have a choice yet! 🤪

Comment 6

[Thomas D. (:thomas8)]

Attachment: 1716823_keepRecipientsPublicFocusFix.diff

Let's do this! 🚀

Address comment 4 as explained in my comment 5.

Comment 7

[Alessandro Castellani [:aleca]]

Comment on attachment 9228495 [details] [diff] [review]
1716823_keepRecipientsPublicFocusFix.diff

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

Looks good, thanks.

Comment 8

[Thomas D. (:thomas8)]

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

Review of attachment 9228495 [details] [diff] [review] ⧉[details] [diff] [review]:
Looks good, thanks.

👍🏻

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/abd56590d89a
Fix focus after Keep Recipients Public from public bulk mail notification. r=aleca

Comment 10

[Lasana Murray]

Thomas did you figure out why the focus is lost and set to the contacts sidebar?

Comment 11

[Thomas D. (:thomas8)]

(In reply to Lasana Murray from comment #10)

Thomas did you figure out why the focus is lost and set to the contacts sidebar?

No. Notification gets focus when you click a notification button, then notification goes away, meaning the focused element is gone. Then I guess there's a mechanism either attached to the notifications or to composition in general that if visible focus was lost, it should be reset to the first visible element - with sidebar, that's sidebar AB context button, without sidebar, that's identity selector (and I saw focus there after clicking notification button before my patch).

It would be good if we could figure out where exactly focus is controlled, but otoh it may not help much: Fallback focus (in case of focus lost) might be different from what we want for those notifications, which is active focus control depending on scenario.

I can add focus control as a followup to bug 1671891 if you want.
We can probably reuse the focus control code from this bug 1716823 for encrypted-Bcc notification, but attachment reminder might need something different to preserve a previous focus in subject even when it's not empty.

Comment 12

[Lasana Murray]

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

(In reply to Lasana Murray from comment #10)

Thomas did you figure out why the focus is lost and set to the contacts sidebar?

No.
...

Thanks for the reply. I'll still take a look and see if we can at least figure out why. Maybe just setting the autofocus property somewhere? Having to introduce custom code to restore focus after adding a new notification is not a good experience. If I can't figure anything out, your patch should do.

Comment 13

[Thomas D. (:thomas8)]

(In reply to Lasana Murray from comment #12)

I'll still take a look and see if we can at least figure out why. Maybe just setting the autofocus property somewhere? Having to introduce custom code to restore focus after adding a new notification is not a good experience.

We already have a lot of "custom code" around focus in composition. Let's avoid looking at this from a ux-implementation-level perspective. What really matters is the overall user experience - in this case, focus behaviour has been carefully crafted (yet still simple and predictable) to support maximum ux-efficiency, avoid ux-interruption, and effect ux-error-prevention, especially for keyboard users where focus matters most. Typically you won't get that with "just setting the autofocus property somewhere". See my comment 1 for some workflows, and my comment 3 for the current behaviour.

Comment 14

[Thomas D. (:thomas8)]

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

We can probably reuse the focus control code from this bug 1716823 for encrypted-Bcc notification

When I said re-use, I probably meant that the recipient-based notifications can share the same focus control code if we just put it in a function.

but attachment reminder might need something different to preserve a previous focus in subject even when it's not empty.

Maybe we can also handle this deviation in a shared post-notification focus handling function if we pass in the id of the notification or some such.