Composition's fast focus ring disfunctional, especially backwards (Ctrl+[Shift+]+Tab / [Shift+]F6])
Categories
(Thunderbird :: Message Compose Window, defect)
Tracking
(Not tracked)
People
(Reporter: thomas8, Assigned: thomas8)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
10.42 KB,
patch
|
aleca
:
review+
|
Details | Diff | Splinter Review |
Composition's fast focus ring isn't working (again), regressed by several causes, mostly disfunctional. Focused stops vary depending on the shortcut, starting point, and direction, iow, there's no ring.
Fast focus ring is operated by these keyboard shortcuts:
Ctrl+Tab
or F6
--> forwards
Ctrl+Shift+Tab
or Shift+F6
--> backwards
Forwards:
- F6/Ctrl+Tab from contacts side bar (search input) skips 'From' field.
Backwards:
- Shift+F6 from contacts side bar cycles only To, From, and Contacts Side Bar; skips message body, subject, and any addressing fields except 'To'.
- Ctrl+Shift+Tab from contacts side bar gets stuck in alternating between To and MoreRecipients button (which isn't even part of the fast focus ring).
- Ctrl+Shift+Tab from message body is no different from Shift+Tab, i.e. it will stop at any milk can on the way, like close buttons of any addressing rows, before getting stuck between To and MoreRecipients button.
Regressions:
- on anonymous search input, input.parentNode now returns
Shadow Root
instead of the search textbox, so we default to focusingTo
addressing field - new recipient area is too greedy in consuming 'tab', unaware of Ctrl+[Shift+]Tab
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Let's just fix the breakage within the existing clumsy focus ring framework.
Also simiplified/normalized some corners and polished some comments and function comments.
The rest is for another day, something like bug 1612761.
Comment 2•4 years ago
|
||
Comment on attachment 9145224 [details] [diff] [review] 1634889_fixFastFocusRing.diff Review of attachment 9145224 [details] [diff] [review]: ----------------------------------------------------------------- This is good, thanks for taking care of it. I'm gonna lunch a try-run to see if we're breaking any test, even if I doubt it as we're not fully covering this area with tests. Just update those couple of nits and this should be good. ::: mail/components/compose/content/MsgComposeCommands.js @@ +7589,2 @@ > * > + * @return {HTMLElement} An element node of the fast-track focus ring if the If we might return a null value, let's update this with @return {HTMLElement | null} @@ +7630,5 @@ > ) { > return currentNode; > } > + // Iterate parent nodes until we find one that matches. Applicable especial- > + // ly for Contacts Side Bar when the search input or a contact has focus. Let's update this with: // Iterate through parent nodes until we find one that matches. // Applicable for Contacts sidebar when the search input or a contact has focus.
Comment 3•4 years ago
|
||
Assignee | ||
Comment 4•4 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #3)
Thank you for the try. I've caused a test failure, here:
https://searchfox.org/comm-central/source/mail/test/browser/composition/browser_focus.js#55
Assert.equal(contentElement, controller.window.WhichElementHasFocus());
https://searchfox.org/comm-central/source/mail/test/browser/composition/browser_focus.js#38
let contentElement = controller.window.content;
It's because I changed the element returned for focus on message body, before: window.content, now: the editor element, which makes things simpler and more consistent with how we handle other elements here.
So I just have to fix the test accordingly.
Assignee | ||
Comment 5•4 years ago
|
||
Tried to fix the test, but cannot test that. Another try run?
Updated•4 years ago
|
Comment 6•4 years ago
|
||
Comment on attachment 9145863 [details] [diff] [review] 1634889_fixFastFocusRing.diff Review of attachment 9145863 [details] [diff] [review]: ----------------------------------------------------------------- If you have daily built locally, you can run `./mach mochitest comm/mail/test/browser/composition/browser_focus.js` from the `source` folder. The tests are still failing. If you're not able to run the tests locally, let me know and I'll take care of fixing them.
Comment 7•4 years ago
|
||
I fixed the issue.
The problem was the return
if the event.ctrlKey
was true, which was preventing the focus ring from moving properly during the tests.
It seems that we need to respect the pressing of the CTRL key in our tests when triggering controller.keypress(null, key, { ctrlKey: ctrlTab });
, otherwise the focus ring can't move during the tests.
Take a look at the diff and let me know if it makes sense.
Launched a new try run to confirm that everything works: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=21e333da942da69cde3b4569febd5b9fe08c15b6
Assignee | ||
Comment 8•4 years ago
|
||
Comment on attachment 9146061 [details] [diff] [review] 1634889_fixFastFocusRing-fixed_tests.diff I am failing to see what you changed, but I think you'll figure this out! Also, I am irritated by the distributed handling of "Tab", some flavors handled in keydown, some elsewhere. Maybe that should be bundled into keydown?
Updated•4 years ago
|
Updated•4 years ago
|
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/1defa8e63d75
Fix composition's fast focus ring (Ctrl+[Shift+]+Tab / [Shift+]F6]). r=aleca
Description
•