Closed
Bug 651052
Opened 13 years ago
Closed 8 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)
Thunderbird
Message Compose Window
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 50.0
People
(Reporter: thomas8, Assigned: aceman)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(2 files, 4 obsolete files)
1.62 KB,
image/png
|
Details | |
2.33 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
afaict, STR from comment 0 will make the composition window completely inaccessible for keyboard-only users, which is a major accessibility problem.
Keywords: access
Reporter | ||
Updated•12 years ago
|
Blocks: tb-keyboard-tracker
Reporter | ||
Updated•12 years ago
|
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
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 3•11 years ago
|
||
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.
bwinton, please answer comment 3 too.
Attachment #764928 -
Attachment is obsolete: true
Attachment #764999 -
Flags: ui-review?(bwinton)
Comment 6•11 years ago
|
||
(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 8•11 years ago
|
||
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+
![]() |
Assignee | |
Comment 10•8 years ago
|
||
Fixed bwinton's requests in comment 8.
Attachment #764999 -
Attachment is obsolete: true
Flags: needinfo?(acelists)
Attachment #8762313 -
Flags: ui-review?(richard.marti)
Comment 11•8 years ago
|
||
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+
![]() |
Assignee | |
Comment 12•8 years ago
|
||
(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•8 years ago
|
||
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.
![]() |
Assignee | |
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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 16•8 years ago
|
||
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()?
![]() |
Assignee | |
Comment 17•8 years ago
|
||
Nice.
Attachment #8762417 -
Attachment is obsolete: true
Attachment #8762417 -
Flags: review?(mozilla)
Attachment #8762545 -
Flags: review?(mozilla)
Comment 18•8 years ago
|
||
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?
![]() |
Assignee | |
Comment 19•8 years ago
|
||
Yes, works for me.
Comment 20•8 years ago
|
||
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•8 years ago
|
||
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+
![]() |
Assignee | |
Comment 22•8 years ago
|
||
(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
![]() |
Assignee | |
Comment 23•8 years ago
|
||
(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.
![]() |
Assignee | |
Comment 24•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/d8e0c5ff9c11ec6b775cd01536d544de9196b089
Status: ASSIGNED → RESOLVED
Closed: 8 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.
Description
•