Closed Bug 651052 Opened 9 years ago Closed 3 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 50.0

People

(Reporter: bugzilla2007, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 4 obsolete files)

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.
afaict, STR from comment 0 will make the composition window completely inaccessible for keyboard-only users, which is a major accessibility problem.
Keywords: access
Summary: Contacts sidebar: Bogus focus after F9 to hide it -> F9 to view again fails → Contacts sidebar: Bogus focus after F9 to hide it -> F9 to view again fails; renders compose window keyboard-inaccessible
Assignee: nobody → acelists
Component: Address Book → Message Compose Window
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #764928 - Flags: review?(mkmelin+mozilla)
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
Attachment #764928 - Flags: review?(mkmelin+mozilla) → review+
(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.
Status: NEW → ASSIGNED
Attached patch patch v2 (obsolete) — Splinter Review
bwinton, please answer comment 3 too.
Attachment #764928 - Attachment is obsolete: true
Attachment #764999 - Flags: ui-review?(bwinton)
(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.
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 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.
Attachment #764999 - Flags: ui-review?(bwinton) → ui-review+
this got UI-r+
Flags: needinfo?(acelists)
Attached patch patch v3 (obsolete) — Splinter Review
Fixed bwinton's requests in comment 8.
Attachment #764999 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Attachment #8762313 - Flags: ui-review?(richard.marti)
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.
Attachment #8762313 - Flags: ui-review?(richard.marti) → ui-review+
(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?
Attached image 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.
Attached patch patch v3.1 (obsolete) — Splinter Review
The code didn't find any focused elements t de-focus, because the sidebar is hiding in a browser :) Try this please.
Attachment #8762313 - Attachment is obsolete: true
Attachment #8762417 - Flags: ui-review?(richard.marti)
Comment on attachment 8762417 [details] [diff] [review]
patch v3.1

Works perfectly now. Thanks for the fix.
Attachment #8762417 - Flags: ui-review?(richard.marti) → ui-review+
Attachment #8762417 - Flags: review?(mozilla)
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()?
Attached patch patch v3.2Splinter Review
Nice.
Attachment #8762417 - Attachment is obsolete: true
Attachment #8762417 - Flags: review?(mozilla)
Attachment #8762545 - Flags: review?(mozilla)
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?
Yes, works for me.
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 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();
Attachment #8762545 - Flags: review?(mozilla) → review+
(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.
Keywords: checkin-needed
(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.
https://hg.mozilla.org/comm-central/rev/d8e0c5ff9c11ec6b775cd01536d544de9196b089
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
You need to log in before you can comment on or make changes to this bug.