Open Bug 1612761 Opened 6 months ago Updated 2 months ago

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

Categories

(Thunderbird :: Message Compose Window, task)

task
Not set
normal

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: bugzilla2007, Assigned: bugzilla2007)

References

Details

(Keywords: access, ux-efficiency)

Attachments

(1 file, 1 obsolete file)

+++ 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

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.
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #9123982 - Flags: ui-review?(alessandro)
Attachment #9123982 - Flags: review?(acelists)
Attachment #9123982 - Flags: feedback?(richard.marti)

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. :-)

Attachment #9123982 - Attachment is obsolete: true
Attachment #9123982 - Flags: ui-review?(alessandro)
Attachment #9123982 - Flags: review?(acelists)
Attachment #9123982 - Flags: feedback?(richard.marti)
Attachment #9123987 - Flags: ui-review?(alessandro)
Attachment #9123987 - Flags: review?(acelists)
Attachment #9123987 - Flags: feedback?(richard.marti)
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 on attachment 9123987 [details] [diff] [review]
1612761-composeFastFocusRingRewrite.patch

Removing the f? as my JS knowledge is less than yours.
Attachment #9123987 - Flags: feedback?(richard.marti)
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 :-)
Attachment #9123987 - Flags: ui-review?(alessandro)
Attachment #9123987 - Flags: review?(acelists)
Depends on: 1634889

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

Component: Composition → Message Compose Window
Product: MailNews Core → Thunderbird
You need to log in before you can comment on or make changes to this bug.