682424 - Add a stop for composition's Contacts Sidebar to Ctrl+Tab / F6 fast-track focus ring

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Thomas D. (:thomas8)]

Followup from bug 303712

Now that bug 473901 has been fixed, the last missing piece of the accessibility puzzle for message composition is this:

Implement Ctrl+Tab stop and F6 stop for contacts sidebar.

STR
1) message compose window (until such time that it will become a tab), F9 to open contacts sidebar
2) press Ctrl+Tab repeatedly for fast-track focus cycle (until such time that message composition will be in a tab)
3) press F6 repeatedly for fast-track focus cycle

Actual results
2) Contacts sidebar not included in Ctrl+Tab cycle
3) Contacts sidebar not included in F6 cycle

Expected results
2) Contacts sidebar should be included in Ctrl+Tab cycle (add single stop)
3) Contacts sidebar should be included in F6 cycle (add single stop)

Implementation Details
- imo, the most useful element for the stop within contacts sidebar is the search field
- within the cycle, the stop for the sidebar should be after the text body stop:
address > subject > [attachments] > text body > [contacts sidebar] > From > address (stops are arranged left>right/top-down, resulting in something like a clockwise rotation since we start out from the middle)

Reasons:
- For users who deliberately have the sidebar open, we can safely assume it's an important part of the compose UI for them (for all other users, this bug does not change anything)
- Any major / important part of the UI should be included in the Ctrl+Tab/F6 fast-track cycles (which also ensure fast access to all the lesser elements of that major part)
- Note that shift+tab from "From:" dropdown cycles back into contacts sidebar (and, technically correct, lands on its last element, the add-to-bcc button, since we are in the backwards cycle), so the sidebar is included in the tab cycle
- forward-tab gets stuck in the input area for message body where tab becomes part of the text, adding to the need for Ctrl+Tab / F6 stop as a cycle-forward alternative

Comment 1

[Thomas D. (:thomas8)]

Another interesting observation from Bug 303712 (where this problem was first mentioned in bug 303712, comment 0):

With focus anywhere inside the contacts sidebar, Ctrl+Tab/F6 will shift focus to subject field, but Shift+Ctrl+Tab/Shift+F6 will never cycle back into the sidebar (clearly breaking ux-consistency).

Equivalent Seamonkey bug:
Bug 433879 - Contacts sidebar panel in Mail Compose window not keyboard reachable

Comment 3

[Thomas D. (:thomas8)]

Attachment: Screenshot1: Add Ctrl+Tab and F6 stop in contacts side bar

Blake, another 2-mins no-brainer ui-review, ready for consumption:

We need a single Ctrl+Tab and F6 Stop on Contacts Sidebar for the existing fast-track focus cycle in Compose Window, as shown in the attached screenshot for your convenience.

Of course, we only stop there if the sidebar is actually shown, so for folks not using it we are not changing a thing. We've recently done the same thing by adding a Ctrl+Tab and F6 stop for the attachment pane on the right (if shown), and in exact analogy, we should have one for the sidebar on the left (if shown). Anything else obviously violates ux-consistency and accessibility principles. Some more reasons in comment 0. I can lay out the reasons in a more systematical way should there be any doubts.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 587780 [details]
Screenshot1: Add Ctrl+Tab and F6 stop in contacts side bar

Yeah, this annoyed me when I was thinking about the delete/enter behaviour in the other bug, so ui-r=me.

One thing I'm not sure about is whether or not to include the list of addresses in the tab order, or just the search box…  Thoughts?

Thanks,
Blake.

Comment 5

[Thomas D. (:thomas8)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #4)
> Comment on attachment 587780 [details]
> Screenshot1: Add Ctrl+Tab and F6 stop in contacts side bar
>
> One thing I'm not sure about is whether or not to include the list of
> addresses in the tab order, or just the search box…  Thoughts?

We need to distinguish between TAB cycle (stop everywhere) vs. CTRL+TAB / F6 (fast-track).

For fast-track CTRL+TAB / F6 cycle, I think single stop at contacts search box is ideal:
- Number of *fast-track* stops should be as low as possible, otherwise we are violating the *fast-track* principle
- It makes a lot of sense to filter addresses with a search term before picking recipients, so this seems a reasonable default scenario for the majority of users
- List of contacts is just a single TAB away
- List of address books is just a single Shift+Tab away

For stop everywhere plain TAB cycle, we already have the correct behaviour:
- Shift+TAB from From: field will focus "Add to BCC" button, which is the correct behaviour, because we are in the *backwards* TAB cycle, so the last element of contacts side bar must come first, in reversal of the *forwards* tab cycle.
- With focus on Address Books dropdown list, TAB forward correctly moves on to Search, contacts list, "Add to..."-Buttons etc.
- The reason why we can't enter the contacts side bar in the forward tab cycle is that tab gets stuck in the message editor box where it correctly becomes a character of the message text (but after fixing this bug, we can reach contacts side bar in the *Ctrl+TAB / F6* forward cycle instead).

Having said which, I'm not entirely against having an extra fast-track Ctrl+Tab / F6 Stop at the contacts list. But I'm hesitant because inflation of fast-track stops is something we should avoid. The point is, with single fast-track stop in the contacts search field only, we are encouraging the most efficient behaviour, which is certainly using the search before manually picking from the list.

Blake?

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Yeah, that makes sense.  Thanks!

Comment 7

[Thomas D. (:thomas8)]

Bug 473901 included the attachment pane into fast track Ctrl+Tab/F6 focus cycle, so that might have ideas how to fix this.

Comment 8

[Jim Porter (:squib)]

We don't really use [good first bug] anymore. Usually we add the flags for mentored bugs.

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to Jim Porter (:squib) from comment #8)
> We don't really use [good first bug] anymore. Usually we add the flags for
> mentored bugs.

Where are those?

Comment 10

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

Fixed! :)

Jörg, if you are not reviewing this, pls pass on.

This implements my proposal seen in the annotated screenshot of attachment 587780 [details], which has ui-r+ by Blake Winton in comment 4, combined with comment 6. For details, see my comments in the planning phase.

I stumbled over the fast-track focus ring function while doing other work in MsgComposeCommands.js. It was trickier than I thought, because contacts sidebar sits in its own browser, and some things are and some things aren't available to the main document, and it's kinda twisted. I'm not claiming to fully understand the deep stuff of this nesting, but this works correctly as it should, and also comes with a nice code cleanup.

There's one thing which I'm failing to understand:
>  let currentNode = top.document.commandDispatcher.focusedElement;
>  while (currentNode) {  console.log("currentNode id=" + currentNode.id + " tagName=" + currentNode.tagName);
>    if (currentNode == GetMsgIdentityElement() ||
>        currentNode == GetMsgAddressingWidgetTreeElement() ||
>        currentNode == GetMsgSubjectElement() ||
>        currentNode == GetMsgAttachmentElement() ||
>        // Contacts Side Bar <page> element
>        currentNode == top.document.getElementById("abContactsPanel"))
>      return currentNode;
>
>    currentNode = currentNode.parentNode;

From my understanding, this should stop iterating and return currentNode when it finds a match. And this seems to work/match for abContactsPanel as focus is correctly processed. BUT looking at the console.log, after matching abContactsPanel, it's still trying another currentNode whose ID and tagName are undefined. What am I missing?

Comment 11

[Thomas D. (:thomas8)]

Comment on attachment 8940756 [details] [diff] [review]
Patch V.1: Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ -5857,5 @@
>  {
> -  var msgIdentityElement             = GetMsgIdentityElement();
> -  var msgAddressingWidgetTreeElement = GetMsgAddressingWidgetTreeElement();
> -  var msgSubjectElement              = GetMsgSubjectElement();
> -  var msgAttachmentElement           = GetMsgAttachmentElement();

Oh, I realize that these should come back here as let's. I wrongly thought they are only used once below, but as they are inside the while loop, it's better to initialize here as they'll be used recurrently.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8940756 [details] [diff] [review]
Patch V.1: Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

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

I tried it and it works. I haven't looked very closely at the code changes given that Aceman is also nominated as reviewer since I haven't done nothing for a month :-( - Sorry about that. Let's get this moving.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5858,5 @@
>  
> +function getPeopleSearchInputElement() {
> +  let sidebarBrowser = document.getElementById("sidebar");
> +  let sidebarDocument = sidebarBrowser.contentDocument ||
> +                        sidebarBrowser.contentWindow.document;

Why this ||?

Comment 13

[:aceman]

(In reply to Thomas D. (currently busy elsewhere) from comment #10)
> From my understanding, this should stop iterating and return currentNode
> when it finds a match. And this seems to work/match for abContactsPanel as
> focus is correctly processed. BUT looking at the console.log, after matching
> abContactsPanel, it's still trying another currentNode whose ID and tagName
> are undefined. What am I missing?

It is because top.document.getElementById("abContactsPanel") returns null every time, whether sidebar is open or not. You need to find another way to find it.

Comment 14

[:aceman]

Comment on attachment 8940756 [details] [diff] [review]
Patch V.1: Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

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

Thanks, this seems to do what was intended, but it is a mistery why when the querying of the sidebar is not working. Anyway, that needs to be fixed yet.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4408,5 @@
> + * Show contacts sidebar and focus the search box.
> + */
> +function focusContactsSidebarSearchInput() {
> +  // Ensure that contacts side bar is visible.
> +  if (document.getElementById("sidebar-box").hidden)

There is sidebar_is_hidden() for this.

@@ +5876,2 @@
>  
>    if (top.document.commandDispatcher.focusedWindow == content)

I wonder where this 'content' comes from.

@@ +5883,5 @@
> +        currentNode == GetMsgAddressingWidgetTreeElement() ||
> +        currentNode == GetMsgSubjectElement() ||
> +        currentNode == GetMsgAttachmentElement() ||
> +        // Contacts Side Bar <page> element
> +        currentNode == top.document.getElementById("abContactsPanel"))

Doesn't find the element.

@@ +5914,5 @@
> +        SetMsgIdentityElementFocus();
> +        break;
> +      case gMsgIdentityElement:
> +        // Only set focus to the contacts side bar (search box) if it is shown.
> +        if (!document.getElementById("sidebar-box").hidden)

sidebar_is_hidden()

@@ +5919,5 @@
> +          focusContactsSidebarSearchInput();
> +        else
> +          SetMsgBodyFrameFocus();
> +        break;
> +      case document.getElementById("abContactsPanel"):

I think this also doesn't work (always null).

@@ +5959,5 @@
> +          focusContactsSidebarSearchInput();
> +        else
> +          SetMsgIdentityElementFocus();
> +        break;
> +      case document.getElementById("abContactsPanel"):

May not work again.

@@ +5974,5 @@
>  {
> +  let sidebarBox = document.getElementById("sidebar-box");
> +  let sidebarSplitter = document.getElementById("sidebar-splitter");
> +  let sidebar = document.getElementById("sidebar");
> +  let elt = document.getElementById("viewAddressPicker");

Not your fault, but 'elt' could use a better name :)

Comment 15

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #14)
> Review of attachment 8940756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this seems to do what was intended, but it is a mistery why when the
> querying of the sidebar is not working. Anyway, that needs to be fixed yet.

When I wrote the first patch, I was having a hard time finding out how to access abContactsPanel and peopleSearchInput within, but I actually succeeded, so I don't know what stopped me from properly applying that to the other functions.

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +4408,5 @@
> > + * Show contacts sidebar and focus the search box.
> > + */
> > +function focusContactsSidebarSearchInput() {
> > +  // Ensure that contacts side bar is visible.
> > +  if (document.getElementById("sidebar-box").hidden)
> 
> There is sidebar_is_hidden() for this.

Yeah, more concise so more readable. But would this not be minimally less performant as it adds another function call?

> @@ +5876,2 @@
> >  
> >    if (top.document.commandDispatcher.focusedWindow == content)
> 
> I wonder where this 'content' comes from.

https://developer.mozilla.org/en-US/docs/Web/API/Window/content

> @@ +5883,5 @@
> > +        currentNode == GetMsgAddressingWidgetTreeElement() ||
> > +        currentNode == GetMsgSubjectElement() ||
> > +        currentNode == GetMsgAttachmentElement() ||
> > +        // Contacts Side Bar <page> element
> > +        currentNode == top.document.getElementById("abContactsPanel"))
> 
> Doesn't find the element.

Fixed, and all the others, too.

Comment 16

[Thomas D. (:thomas8)]

Attachment: Patch V.2: Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

Thanks Aceman for finding the hidden faults in my code even though it was ostensibly working ;-) Now doing this correctly, much better...

I added a more generic function to get elements from the sidebar <browser>'s contentDocument, and it can even check if that element has been found on the right <page>, as addons might show other pages in the sidebar browser and those pages might accidentally have an element with the same id.

> function sidebarDocumentGetElementById(aId, aPageId) {

Comment 17

[Thomas D. (:thomas8)]

Attachment: Patch V.2.1: (nitifxes, more fail-proof) Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

Nitfix: another instance of sidebar_is_hidden()

Existing toggleAddressPicker() function is based on the somewhat frail assumption that abContactsPanel.xul is the only possible src for the sidebar <browser>. This holds true for the default TB installation, but add-ons might use the sidebar to show something else. So I added some comments and made the focus ring fail-proof against that. I'm assuming that any xul file can only ever have a single <page> element (which is like a <window> element); unfortunately, documentation is vague.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/page

Comment 18

[Thomas D. (:thomas8)]

Attachment: Patch V.2.2: (nitifx) Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

Testing helps. Nitfix spelling error. For review of this patch, pls see comment 16 and comment 17.

Comment 19

[:aceman]

(In reply to Thomas D. from comment #15)
> > There is sidebar_is_hidden() for this.
> 
> Yeah, more concise so more readable. But would this not be minimally less
> performant as it adds another function call?

Very minimally, thanks to all those optimizing JIT JS compilers in gecko :)
Anyway, this is not a hot path executed 1000x per second, so we should strive for readability and maintainability (e.g. not hardcoding IDs and element hierarchies) by default.

Comment 20

[:aceman]

Comment on attachment 8952669 [details] [diff] [review]
Patch V.2.2: (nitifx) Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +228,5 @@
>    }
>  }
>  
>  function sidebar_is_hidden() {
> +  let sidebar_box = document.getElementById("sidebar-box");

'let' and 'var' are the same in top level context of a function so this change isn't needed by itself. But if you touch the quotes and want to change the var, I can live with it ;)

@@ +4414,5 @@
> +  // Caveat: toggleAddressPicker() does not reliably enforce abContactsPanel.xul
> +  // as src of the sidebar <browser>, but currently we don't show anything else
> +  // in the sidebar. Add-ons might.
> +  if (sidebar_is_hidden())
> +    toggleAddressPicker();

Why is this needed? You only call this function when you already checked sidebar is not hidden.

@@ +4421,5 @@
> +  // Per the above caveat, this would fail if any src other than
> +  // abContactsPanel.xul were shown in the sidebar, so that should be
> +  // ensured before calling this function. Otherwise we let it fail so that
> +  // respective design errors can be noticed.
> +  sidebarDocumentGetElementById("peopleSearchInput", "abContactsPanel").focus();

If you expect addons open something else in the sidebar, then there may be no peopleSearchInput and this will return null on which .focus() will fail.

@@ +6080,5 @@
> +function sidebarDocumentGetElementById(aId, aPageId) {
> +  let sidebarDocument = document.getElementById("sidebar").contentDocument;
> +  if (aPageId) {
> +    if (sidebarDocument.getElementById(aPageId)) {
> +      return sidebarDocument.getElementById(aId);

You do not check that aId is a descendant of aPageId as you write in the function description.

@@ +6107,5 @@
> +  let msgIdentityElement = GetMsgIdentityElement();
> +  let msgAddressingWidgetTreeElement = GetMsgAddressingWidgetTreeElement();
> +  let msgSubjectElement = GetMsgSubjectElement();
> +  let msgAttachmentElement = GetMsgAttachmentElement();
> +  let abContactsPanelElement = sidebarDocumentGetElementById("abContactsPanel");

This can return null if the sidebar wasn't loaded.

@@ +6118,5 @@
>      if (currentNode == msgIdentityElement ||
>          currentNode == msgAddressingWidgetTreeElement ||
>          currentNode == msgSubjectElement ||
> +        currentNode == msgAttachmentElement ||
> +        currentNode == abContactsPanelElement) {

If abContactsPanelElement is null, and currentNode is too, we would return currentNode (thus null) here. Is that wanted? I'd say we should check for null explicitly somewhere.

@@ +6143,5 @@
> +  let focusedElement = WhichElementHasFocus();
> +
> +  if (event && event.shiftKey) {
> +    // Backwards focus ring: e.g. Ctrl+Shift+Tab | Shift+F6
> +    switch(focusedElement) {

Space after 'switch'.

@@ +6155,5 @@
> +          focusContactsSidebarSearchInput();
> +        else
> +          SetMsgBodyFrameFocus();
> +        break;
> +      case sidebarDocumentGetElementById("abContactsPanel"):

What if this is null? If focusedElement is null too, you take this path. But you probabbly wanted to take "default".

@@ +6175,4 @@
>      }
> +  } else {
> +    // Forwards focus ring: e.g. Ctrl+Tab | F6
> +    switch(focusedElement) {

This whole block is a reverse of the previous block with all the same calls. So there should be a way to write this more smart without duplicating it, but it is not straightforward. But you do not need to bother if you do not want to. I can think about it later.

Comment 21

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #20)
> Comment on attachment 8952669 [details] [diff] [review]
> Patch V.2.2: (nitifx) Add stop for Contacts Sidebar to fast-track focus
> ring; related code clean-up
> 
> Review of attachment 8952669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +228,5 @@
> >    }
> >  }
> >  
> >  function sidebar_is_hidden() {
> > +  let sidebar_box = document.getElementById("sidebar-box");
> 
> 'let' and 'var' are the same in top level context of a function so this
> change isn't needed by itself. But if you touch the quotes and want to
> change the var, I can live with it ;)

Yes, they are the same, but we agreed that function-level variables should be declared with let, and only top-level global variables should use var.

> @@ +4414,5 @@
> > +  // Caveat: toggleAddressPicker() does not reliably enforce abContactsPanel.xul
> > +  // as src of the sidebar <browser>, but currently we don't show anything else
> > +  // in the sidebar. Add-ons might.
> > +  if (sidebar_is_hidden())
> > +    toggleAddressPicker();
> 
> Why is this needed? You only call this function when you already checked
> sidebar is not hidden.

Yeah, we can let the caller do the checking.

> @@ +4421,5 @@
> > +  // Per the above caveat, this would fail if any src other than
> > +  // abContactsPanel.xul were shown in the sidebar, so that should be
> > +  // ensured before calling this function. Otherwise we let it fail so that
> > +  // respective design errors can be noticed.
> > +  sidebarDocumentGetElementById("peopleSearchInput", "abContactsPanel").focus();
> 
> If you expect addons open something else in the sidebar, then there may be
> no peopleSearchInput and this will return null on which .focus() will fail.

That's where we deliberately let it fail so that broken design gets noticed.
As you advocated for above, let's just leave the checking to the caller.
We could catch this, but it's just wasting cycles every time, whereas doing nothing when a change of focus is expected isn't very helpful and should be avoided by design.

> @@ +6080,5 @@
> > +function sidebarDocumentGetElementById(aId, aPageId) {
> > +  let sidebarDocument = document.getElementById("sidebar").contentDocument;
> > +  if (aPageId) {
> > +    if (sidebarDocument.getElementById(aPageId)) {
> > +      return sidebarDocument.getElementById(aId);
> 
> You do not check that aId is a descendant of aPageId as you write in the
> function description.

Hmmm, no, I didn't exactly write that. I first wrote:
> the ID of a page in the sidebar browser; only return the element if the page exists.
For most cases, this boils down to descendancy:
<browser id="sidebar">
 <page id="abContactsPanel">
  <input id="peopleSearchInput">

Documentation is a bit vague, but says that <page> is like <window>, so I assume that every xul file can only have *one* <page> OR *one* <window> (like you can't define more than one <body> in an HTML file. If that's true, then any <page> which has been confirmed present in the sidebar browser will always be the visible top-level element of the current src.xul shown, But you are right, that doesn't automatically make any other element found in src.xul a descendant of page, might be sibling, or child of a sibling.
<browser id="sidebar">
 <page id="abContactsPanel">
 <sibling/>
  <input id="peopleSearchInput">

Ok, let me clarify the description.

> @@ +6107,5 @@
> > +  let msgIdentityElement = GetMsgIdentityElement();
> > +  let msgAddressingWidgetTreeElement = GetMsgAddressingWidgetTreeElement();
> > +  let msgSubjectElement = GetMsgSubjectElement();
> > +  let msgAttachmentElement = GetMsgAttachmentElement();
> > +  let abContactsPanelElement = sidebarDocumentGetElementById("abContactsPanel");
> 
> This can return null if the sidebar wasn't loaded.

Yes.

> @@ +6118,5 @@
> >      if (currentNode == msgIdentityElement ||
> >          currentNode == msgAddressingWidgetTreeElement ||
> >          currentNode == msgSubjectElement ||
> > +        currentNode == msgAttachmentElement ||
> > +        currentNode == abContactsPanelElement) {
> 
> If abContactsPanelElement is null, and currentNode is too, we would return
> currentNode (thus null) here.

Normally it's not possible for currentNode to be null unless addons have added elements which can have focus.
If currentNode is null, we skip the conditional because it's in a while(currentNode) loop, so we don't return currentNode, but we'd still return null (default return of the function).

> Is that wanted? I'd say we should check for
> null explicitly somewhere.

Ok, thanks, I've added that check in SwitchElementFocus(). Less code, more readable. Great.

> @@ +6155,5 @@
> > +          focusContactsSidebarSearchInput();
> > +        else
> > +          SetMsgBodyFrameFocus();
> > +        break;
> > +      case sidebarDocumentGetElementById("abContactsPanel"):
> 
> What if this is null? If focusedElement is null too, you take this path. But
> you probabbly wanted to take "default".

Fixed, see above.

> @@ +6175,4 @@
> >      }
> > +  } else {
> > +    // Forwards focus ring: e.g. Ctrl+Tab | F6
> > +    switch(focusedElement) {
> 
> This whole block is a reverse of the previous block with all the same calls.
> So there should be a way to write this more smart without duplicating it,
> but it is not straightforward. But you do not need to bother if you do not
> want to. I can think about it later.

Definitely not straightforward, I doubt that it's worth it, and probably ends up less performant. I was also considering if we should use IDs rather than elements, but again that's not straightforward, and I have a feeling that whoever wrote this was deliberately trying to use abstractions as much as possible. I guess we can just leave it as it is, as long as it works.

Comment 22

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #21)
> > @@ +6118,5 @@
> > >      if (currentNode == msgIdentityElement ||
> > >          currentNode == msgAddressingWidgetTreeElement ||
> > >          currentNode == msgSubjectElement ||
> > > +        currentNode == msgAttachmentElement ||
> > > +        currentNode == abContactsPanelElement) {
> > 
> > If abContactsPanelElement is null, and currentNode is too, we would return
> > currentNode (thus null) here.
> 
> Normally it's not possible for currentNode to be null unless addons have
> added elements which can have focus.

Sorry, still wrong, let's try that again:
Normally, it's not possible for currentNode to be null, as there should always be a focused element:
>   let currentNode = top.document.commandDispatcher.focusedElement;
Normally, it's not possible for this function to return null, because focus will always be on one of the focus ring stops, unless addons have added elements which can have focus (or maybe edge cases like focus on attachment reminder info bar etc.).

If for some reason currentNode were null (no focus)...
> ... we skip the conditional because it's in a
> while(currentNode) loop, so we don't return currentNode, but we'd still
> return null (default return of the function).

If focus isn't on any member of our pre-defined focus ring (e.g. on an addon element), the function will also return null.

> > Is that wanted? I'd say we should check for
> > null explicitly somewhere.
> 
> Ok, thanks, I've added that check in SwitchElementFocus(). Less code, more
> readable. Great.

Comment 23

[Thomas D. (:thomas8)]

Attachment: Patch V.2.3: (more fail-proof) Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

This addresses comment 20, see my comment 21 and comment 22.

Comment 24

[Thomas D. (:thomas8)]

Attachment: Patch V.2.4: (restore missing cases) Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

Testing helps. Default cases of the switch statements cover the last-but-one focus stop in each cycle (forwards/backwards), so they can't be removed. Added respective comments, and more precise function comment for WhichElementHasFocus().

Comment 25

[Thomas D. (:thomas8)]

O.T. We should file a new bug for fixing toggleAddressPicker() so that it actually ensures that abContactsPanel is shown in the sidebar and not some other page (via add-ons).

Comment 26

[:aceman]

Comment on attachment 8953355 [details] [diff] [review]
Patch V.2.4: (restore missing cases) Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

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

Thanks, I think I'm happy with the current version.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6138,5 @@
> +    // None of the pre-defined focus ring elements has focus: This should never
> +    // happen with the default installation, but might happen with add-ons.
> +    // In that case, default to focusing the address widget as the first element
> +    // of the focus ring.
> +    SetMsgAddressingWidgetTreeElementFocus();

I think you want 'return' here, otherwise we still enter the null-problematic switch below.

Comment 27

[:aceman]

(In reply to Thomas D. from comment #25)
> O.T. We should file a new bug for fixing toggleAddressPicker() so that it
> actually ensures that abContactsPanel is shown in the sidebar and not some
> other page (via add-ons).

Yes, that could be useful.

Comment 28

[Thomas D. (:thomas8)]

Attachment: Patch V.2.5: (nitfix return) Add stop for Contacts Sidebar to fast-track focus ring; related code clean-up

Alright, thanks for the fast review, I've added the missing return statement, so with Aceman's r+ from previous patch we're done here.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1c9918ea5681
Add a stop for Contacts Sidebar to composition's fast-track focus ring (Ctrl+Tab / F6). r=aceman, ui-r=bwinton