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

RESOLVED FIXED in Thunderbird 60.0

Status

defect
RESOLVED FIXED
8 years ago
Last year

People

(Reporter: bugzilla2007, Assigned: bugzilla2007)

Tracking

(Blocks 2 bugs, {access, ux-consistency})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has ui-review+] [good first bug])

Attachments

(2 attachments, 6 obsolete attachments)

96.72 KB, image/png
bwinton
: ui-review+
Details
13.83 KB, patch
bugzilla2007
: review+
bugzilla2007
: ui-review+
Details | Diff | Splinter Review
Assignee

Description

8 years ago
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
Assignee

Comment 1

8 years ago
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
See Also: → 433879, 473901
Assignee

Updated

8 years ago
Summary: Contacts sidebar excluded from Ctrl+Tab/F6 cycles (implement Ctrl+Tab/F6 stop for keyboard accessibility) → Contacts sidebar excluded from Ctrl+Tab/F6 cycles in message compose window (implement Ctrl+Tab/F6 stop for keyboard accessibility)
Assignee

Updated

8 years ago
No longer blocks: 303712
Duplicate of this bug: 303712
Assignee

Updated

8 years ago
Keywords: ux-consistency
Assignee

Comment 3

8 years ago
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.
Attachment #587780 - Flags: ui-review?(bwinton)
Assignee

Updated

8 years ago
Attachment #587780 - Attachment description: Screenshot1: Add Ctrl+Tab and F6 stop in contacts side bar.png → Screenshot1: Add Ctrl+Tab and F6 stop in contacts side bar
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.
Attachment #587780 - Flags: ui-review?(bwinton) → ui-review+
Assignee

Comment 5

8 years ago
(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?
Whiteboard: [has ui-review+]
Yeah, that makes sense.  Thanks!
Assignee

Comment 7

5 years ago
Bug 473901 included the attachment pane into fast track Ctrl+Tab/F6 focus cycle, so that might have ideas how to fix this.
Whiteboard: [has ui-review+] → [has ui-review+] [good first bug]
We don't really use [good first bug] anymore. Usually we add the flags for mentored bugs.
Assignee

Updated

5 years ago
See Also: → 998230
(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?

Updated

2 years ago
Blocks: 1364659
Assignee

Updated

Last year
Summary: Contacts sidebar excluded from Ctrl+Tab/F6 cycles in message compose window (implement Ctrl+Tab/F6 stop for keyboard accessibility) → Add a stop for composition's Contacts Sidebar to Ctrl+Tab / F6 fast-track focus ring
Assignee

Comment 10

Last year
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?
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Flags: needinfo?(acelists)
Attachment #8940756 - Flags: ui-review+
Attachment #8940756 - Flags: review?(jorgk)
Assignee

Updated

Last year
Attachment #8940756 - Flags: review?(acelists)
Assignee

Comment 11

Last year
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 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 ||?
Attachment #8940756 - Flags: feedback+

Comment 13

Last year
(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

Last year
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 :)
Attachment #8940756 - Flags: review?(jorgk)
Attachment #8940756 - Flags: review?(acelists)
Attachment #8940756 - Flags: feedback+
Assignee

Comment 15

Last year
(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.
Assignee

Comment 16

Last year
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) {
Attachment #8940756 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Attachment #8952651 - Flags: review?(acelists)
Assignee

Comment 17

Last year
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
Attachment #8952651 - Attachment is obsolete: true
Attachment #8952651 - Flags: review?(acelists)
Attachment #8952667 - Flags: ui-review+
Attachment #8952667 - Flags: review?(acelists)
Assignee

Comment 18

Last year
Testing helps. Nitfix spelling error. For review of this patch, pls see comment 16 and comment 17.
Attachment #8952667 - Attachment is obsolete: true
Attachment #8952667 - Flags: review?(acelists)
Attachment #8952669 - Flags: ui-review+
Attachment #8952669 - Flags: review?(acelists)

Comment 19

Last year
(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

Last year
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.
Attachment #8952669 - Flags: review?(acelists) → feedback+
Assignee

Comment 21

Last year
(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.
Assignee

Comment 22

Last year
(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.
Assignee

Comment 23

Last year
This addresses comment 20, see my comment 21 and comment 22.
Attachment #8952669 - Attachment is obsolete: true
Attachment #8953354 - Flags: review?(acelists)
Assignee

Comment 24

Last year
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().
Attachment #8953354 - Attachment is obsolete: true
Attachment #8953354 - Flags: review?(acelists)
Attachment #8953355 - Flags: ui-review+
Attachment #8953355 - Flags: review?(acelists)
Assignee

Comment 25

Last year
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

Last year
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.
Attachment #8953355 - Flags: review?(acelists) → review+

Comment 27

Last year
(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.
Assignee

Comment 28

Last year
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.
Attachment #8953355 - Attachment is obsolete: true
Attachment #8953817 - Flags: ui-review+
Attachment #8953817 - Flags: review+
Assignee

Updated

Last year
Keywords: checkin-needed

Comment 29

Last year
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

Last year
Target Milestone: --- → Thunderbird 60.0

Updated

Last year
Blocks: 1444771
You need to log in before you can comment on or make changes to this bug.