1612761 - Rewrite composition's fast focus ring code and finetune behaviour (Ctrl+Tab, F6, Shift+...)

Status: ASSIGNED

(Thunderbird :: Message Compose Window, task)

Description

[Thomas D. (:thomas8)]

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

Composition's fast focus ring code could need some cleanup:

• too bulky
• lots of code duplication
• too hard-coded / not generic enough
• useless cycles
• partly obselete

For the avoidance of doubt, Alessandro did a good job in Bug 1602372, but it all had to fit into the odd framework of the existing old code.

I am offering a complete rewrite with the following improvements:

• Plain vanilla array of element IDs to define the focus ring
• Generic focus ring iteration algorithm (well, as generic as it possibly can be with some of those little special cases which we need to handle)
• Modular approach for easier special casing and maintenance
• Almost 100 lines less code (rewrite, unneeded helper functions removed)
• Improved behaviour and 1 nitfix

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1612761-composeFastFocusRingRewrite.patch

The patch. For details, see comment 0.

Please note:

• I can't test the new tests.
• For some reason, searchfox is not yet showing the changes of Bug 1602372, although it's already 4 days old. So I couldn't double-check on some of the occurences of stuff which I removed.

Comment 2

[Thomas D. (:thomas8)]

Attachment: 1612761-composeFastFocusRingRewrite.patch

Oh for the new comfort of changing the fast focus ring: One element in the wrong order, just change the array of fast focus stop IDs, done. :-)

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9123987 [details] [diff] [review]
1612761-composeFastFocusRingRewrite.patch

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

::: mail/base/content/mailWidgets.js
@@ +2027,4 @@
>          setupAutocompleteInput(input, this.highlightNonMatches);
>  
>          input.addEventListener("keypress", event => {
> +          if (event.key == "Tab" && event.shiftKey && !event.ctrlKey) {

the existing code is better, since with an early return you don't have to keep too many combinations in mind, which makes reasoning easier

@@ +2546,5 @@
>      }
>  
> +    getFirstAvailableAddressRowInput() {
> +      return this.querySelector(".address-row:not(.hidden)")
> +                 .querySelector(".address-input");

Helpers like these are not really that helpful. There's quite a mental overhead of keeping track of small helpers that could just as easily be inlined.

Also, the query should be combined.
.querySelector(".address-row:not(.hidden) .address-input")

::: mail/components/compose/content/MsgComposeCommands.js
@@ +7121,5 @@
> + * 
> + * @param {String} id - The ID of the element to receive focus.
> + * 
> + */
> +function setFocusTo(id) {

please don't create helper functions when not necessary, like this one.

Comment 4

[Richard Marti (:Paenglab)]

Comment on attachment 9123987 [details] [diff] [review]
1612761-composeFastFocusRingRewrite.patch

Removing the f? as my JS knowledge is less than yours.

Comment 5

[Alessandro Castellani [:aleca]]

Comment on attachment 9123987 [details] [diff] [review]
1612761-composeFastFocusRingRewrite.patch

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

I'm removing the ui-r requests as there are no UI changes involved in this patch, so there's not a need for that.
11 tests are failing in the `browser_focus.js` file you updated, so this is a no-go for me.

It's better to ping Magnus and/or myself for a full review as we've been working on updating this section for a few months now, so we might have a better grasp of the code base.
That's not always the case, but overall Magnus should be the one reviewing these sort of substantial change :-)

Comment 6

[Thomas D. (:thomas8)]

Mass-changing bugs around the new recipient area (pills) from product/component MailNews Core/Composition to Thunderbird/Message Compose Window, because composition frontend code is not shared with SM. Mostly cloned from Bug 440377 which started out in MailNews Core long back.

20200614001RecipientPillsProductChangeTypeBug