651052 - Contacts sidebar: Bogus focus after F9 to hide it -> F9 to view again fails; renders compose window keyboard-inaccessible

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Thomas D. (:thomas8)]

STR

1 compose msg, view > contacts sidebar = on
2 place cursor (focus) into sidebar's address search field (!)
3 press F9 to hide the sidebar
4 press F9 again to show the sidebar

Actual result:
nothing

Expected result
show sidebar again

The problem
When focus is in the sidebar, and user hides it, we leave the focus in the now hidden sidebar, where it gets lost, so the keyboard shortcut to unhide the sidebar no longer works.

The solution
I suppose we need to set the focus back to the message.
Probably as a cursor-focus behind the last recipient, if there is one, otherwise in the first To-recipient input box.

Comment 1

[Thomas D. (:thomas8)]

afaict, STR from comment 0 will make the composition window completely inaccessible for keyboard-only users, which is a major accessibility problem.

Comment 2

[:aceman]

Attachment: patch

So this seems to work. The only problem is I can't detect if the mail body area is focused (<editor> element). So even in that case the first address row is selected upon Contacts sidebar close.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 764928 [details] [diff] [review]
patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4681,5 @@
> +    let focusedElement = composerBox.querySelector(":focus") ||
> +                         composerBox.querySelector('[focused="true"]');
> +    if (!focusedElement) {
> +      document.getElementById("addressCol2#1").focus();
> +    }

no need for braces. also i wonder if subject, or message body would be better. or what happens if we just blur() it?

looks like bug 570835 - add a comment if you agree

::: mail/components/compose/content/messengercompose.xul
@@ +789,5 @@
>      </vbox>
>  
>      <splitter id="sidebar-splitter" hidden="true"/>
>  
> +    <vbox flex="1" id="compose-box">

please put the id attr first

Comment 4

[:aceman]

(In reply to Magnus Melin from comment #3)
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +4681,5 @@
> > +    let focusedElement = composerBox.querySelector(":focus") ||
> > +                         composerBox.querySelector('[focused="true"]');
> > +    if (!focusedElement) {
> > +      document.getElementById("addressCol2#1").focus();
> > +    }
> 
> no need for braces. also i wonder if subject, or message body would be
> better.
That is up for discussion, we can ask UX.

> or what happens if we just blur() it?
.blur what? I tried blur() in the whole contacts panel but that didn't work.

Comment 5

[:aceman]

Attachment: patch v2

bwinton, please answer comment 3 too.

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #4)
> > or what happens if we just blur() it?
> .blur what? I tried blur() in the whole contacts panel but that didn't work.

Probably on the search field. I don't think blur() does anything unless the element has focus.

Comment 7

[:aceman]

It didn't work for me with blur() when called on the element that was supposed to have focus. The problem is textboxes do not really have focus but only a "focused=true" attribute, that is why I check for that in the patch.

Comment 8

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

Comment on attachment 764999 [details] [diff] [review]
patch v2

(In reply to :aceman from comment #4)
> (In reply to Magnus Melin from comment #3)
> > also i wonder if subject, or message body would bebetter.
> That is up for discussion, we can ask UX.

How hard would it be to re-focus the element that was previously focused?  If it's not too hard, then I think that's the optimal solution.

If it is too hard, then I think we should focus the subject, since if they've just closed the address bar, then they're probably done with the addressing part of writing email.  Actually, thinking about it a little maybe we should focus the subject bar only if it's empty, and the message body if we already have a subject.

>+++ b/mail/components/compose/content/MsgComposeCommands.js
>@@ -4669,16 +4669,25 @@ function toggleAddressPicker()
>     sidebarBox.setAttribute("sidebarVisible", "true");
>   }
>   else
>   {
>     sidebarBox.hidden = true;
>     sidebarSplitter.hidden = true;
>     sidebarBox.setAttribute("sidebarVisible", "false");

I think, if we're going to focus another element, we should also clear any focused attributes in the sidebarBox.
(Otherwise when we re-open the sidebar, it will display as if it's focused, but won't actually have the focus.)

So, ui-r=me with that fixed, and the re-focusing (as long as it's not too hard).

Thanks,
Blake.

Comment 9

[Wayne Mery (:wsmwk)]

this got UI-r+

Comment 10

[:aceman]

Attachment: patch v3

Fixed bwinton's requests in comment 8.

Comment 11

[Richard Marti (:Paenglab)]

Comment on attachment 8762313 [details] [diff] [review]
patch v3

ui-r=me

Works like Blake suggested.

The only thing I saw is when something was focused in the sidebar and the sidebar is closed and reopened, the focus is still active. This makes it looking weird when you move the focus to the sidebar again and two elements are focused. If it's possible to unfocus everything in the sidebar on opening this would be perfect.

Comment 12

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #11)
> If it's possible to unfocus everything in the sidebar on
> opening this would be perfect.
This is also what Blake said and I added code to unfocus anything in the sidebar. How can I reproduce what you see? which element should I focus in sidebar to see it having focus again when sidebar is reopened?

Comment 13

[Richard Marti (:Paenglab)]

Attachment: focus.png

On Linux it's not so obvious because the buttons don't show a focus ring (at least on my systems). But it works also with the textbox:
- Select the textbox, the border around is highlighted.
- Press F9, try tabbing between the fields in compose header.
- Press again F9, the textbox border is still highlighted.
- Tab backwards until the To: menulist is no more focused. Now the focus is in the sidebar.
- Tab still backwards (5 times) until the cursor blinks in the textbox.
- One tab more and it is no more highlighted.

On Windows its better visible as also the buttons have a focusring. When for example the "Add to To:" button has focus and I press F9 twice the button has still the focusring. But when I backwards tab the "Add to Bcc:" button becomes the focus. The same with the textbox, then the textbox and the "Add to Bcc:" button have the focusring.

Comment 14

[:aceman]

Attachment: patch v3.1

The code didn't find any focused elements t de-focus, because the sidebar is hiding in a browser :) Try this please.

Comment 15

[Richard Marti (:Paenglab)]

Comment on attachment 8762417 [details] [diff] [review]
patch v3.1

Works perfectly now. Thanks for the fix.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8762417 [details] [diff] [review]
patch v3.1

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4736,5 @@
> +    if (focusedElement) {
> +      focusedElement.focus();
> +    } else {
> +      if (!document.getElementById("msgSubject").value)
> +        document.getElementById("msgSubject").focus();

GetMsgSubjectElement()?
SetMsgSubjectElementFocus()?

Comment 17

[:aceman]

Attachment: patch v3.2

Nice.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8762545 [details] [diff] [review]
patch v3.2

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4738,5 @@
> +    } else {
> +      if (!GetMsgSubjectElement().value)
> +        SetMsgSubjectElementFocus();
> +      else
> +        SetMsgBodyFrameFocus();

That's a strange function, don't you think?
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4563
You've tried it, right?

Comment 19

[:aceman]

Yes, works for me.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8762545 [details] [diff] [review]
patch v3.2

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

Looks OK and works. Please fix the comment as indicated.

Personally, I don't like it. If you hide something that had focus, you should *not* artificially focus something else at random. Can't we leave it with nothing focused and let the user click where they want to go?

I'm happy to give you r+, but I'd like to discuss this option first.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4726,5 @@
>      sidebarBox.setAttribute("sidebarVisible", "false");
>      elt.removeAttribute("checked");
> +
> +    // If nothing is focused in the main compose frame, focus the
> +    // first addressing row. Otherwise if focus stays inside the closed

That's not right. You focus on the subject (if empty) or the body.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8762545 [details] [diff] [review]
patch v3.2

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

Ignore what I said, with nothing focused, the F9 doesn't work. I tried without the SetMsgSubjectElementFocus()/SetMsgBodyFrameFocus();

Comment 22

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #20)
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +4726,5 @@
> > +    // If nothing is focused in the main compose frame, focus the
> > +    // first addressing row. Otherwise if focus stays inside the closed
> 
> That's not right. You focus on the subject (if empty) or the body.
Thanks. Remnant of previous version.

Comment 23

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #20)
> Personally, I don't like it. If you hide something that had focus, you
> should *not* artificially focus something else at random. Can't we leave it
> with nothing focused and let the user click where they want to go?

It is actually written in the comment. We need to focus something, or the accesskeys (so probably F9 too) will not work. The code tries to return focus where it was (the focused=yes tests), but if that fails focus the subject/body.

Comment 24

[:aceman]

https://hg.mozilla.org/comm-central/rev/d8e0c5ff9c11ec6b775cd01536d544de9196b089