1558393 - Shift+Tab doesn't focus shadow host when slot doesn't have focusable children

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Josef Krasny]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

If I go with tab through elements in main window.

Actual results:

If focus lands in message list, I cannot continue tabbing. If I press Shift + tab, focus skips to tabs. It happens in daily builds, in Thunderbird 60.7.0 is tab order OK.

Expected results:

Tab from message list should move focus to tab control element, then to folder tree, message list... Shift + tab from message list should move focus to folder tree, then to tab control, message list...

Comment 1

[Jorg K (CEST = GMT+2)]

Is this still OK in TB 68 beta being released soon, or get one from here:
http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-beta/

Comment 2

[Josef Krasny]

No, it is broken too. :-(

Comment 3

[Josef Krasny]

Version 67.0a1 from February 1st is OK, version 67.0a1 from March 1st has tabbing broken.

Comment 4

[Stanislav Plachý]

Confirm, Thunderbird 67 to 69 has tabbing in main window broken.

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to Josef Krasny from comment #0)

If focus lands in message list, I cannot continue tabbing.

Hmm, I have a TB 68 beta here and when the message preview is enabled, tab from the message list will take me to the header pane and so on. If I disable the preview, tabbing is stuck.

So STR:
Switch off message preview. Select a message in a folder, press tab.

Undesired: Nothing happens.
Desired: Focus moves, first to the folder pane, than to the tabs on top.

Alice, can you find the regression, rough range in comment #3. Thanks in advance!

Comment 6

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=1a4da3cb06f1e8971b1111251072c034d3b3500a&tochange=9c16e1ffb7feb63f29cf8e6a183f2761e0b896b5
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e92ff56d2be21676b447c6fbb87b4c4479539bc9&tochange=93c4470ee3af339226fe5ccb8db273be87b9935a

Comment 7

[Stanislav Plachý]

(In reply to Jorg K (GMT+2) from comment #5)

(In reply to Josef Krasny from comment #0)

If focus lands in message list, I cannot continue tabbing.

Hmm, I have a TB 68 beta here and when the message preview is enabled, tab from the message list will take me to the header pane and so on. If I disable the preview, tabbing is stuck.

I confirm, with message pane turned on is tab behavior OK. But we, blind people, turn off message pane. It is better for orientation in Thunderbird.

Comment 8

[Alessandro Castellani [:aleca]]

Another odd behaviour happening in daily, but not happening in TB 60.

Using TAB starting from the Inbox Tab moves the focus through these elements:

• Search form
• Folders Tree
• Messages Tree
• Message Pane
• Continues through various child elements
• Today Pane
• Back to Inbox tab, and so on

Pressing SHIFT + TAB follows the same order in reverse, but completely skips the Folders Tree and Messages Tree.
When the focus is on the Message Pane, pressing SHIFT + TAB moves the focus directly to the Search form.

Comment 9

[Jorg K (CEST = GMT+2)]

Thanks, Alice, but that's bad news. In the regression ranges we have nothing interesting in C-C, but we have this in M-C:
a6606e85276716b93695029cb914a678beb51708 Victor Porof — Bug 1523957 - Convert trees to custom elements, r=bgrins, mak, standard8, MattN

Since the thread pane (message list) is a tree, we're seeing some fallout from this bug here. Trees have very little use in M-C, so there will be very little motivation to fix things for us.

Comment 10

[Alessandro Castellani [:aleca]]

So, what should we do here?
I was thinking about 3 possible actions.

1. Ask the Firefox devs to fix this for us. How doable is this, and what could be the time expectation?

2. Fork the Tree and maintain our own version to fix this issue.

3. Intercept the TAB key and manually deal with keyboard navigation.

I was experimenting a bit with solution 3, and the main downfall is that by intercepting it we won't be able to trigger the default behaviour, meaning that every focus change needs to be done manually. That might be slightly insane to support.

Thoughts?

Comment 11

[Jorg K (CEST = GMT+2)]

One won't happen. Magnus fixed the last few tree regressions himself. Two is pretty much impossible. I don't know about three.

I think the first step should be to check where FF still uses the tree and how bad the focus issue is there.

To my knowledge, the saved logins in the FF options still use a tree (since you can select tree columns there) as does the certificate manager. So can you check what happens with tabbing there.

Also, Alice, can you observer any changes to tabbing on FF's saved logins and certificate manager panels (which are trees under the cover).

Comment 12

[Magnus Melin [:mkmelin]]

Well I'm pretty sure m-c would accept a patch. It just requires finding what and how to fix it, which is not always easy of course.

Comment 13

[Jorg K (CEST = GMT+2)]

Yes, sure. But option one said: "Ask the Firefox devs to fix this for us". Two is equally unnecessary and impossible. If we find a fix, surely M-C will take it.

Comment 14

[Alessandro Castellani [:aleca]]

Sounds good.
I will investigate this tree regression and try to fix it in order to send a patch to M-C.
Wish me luck :D

Comment 15

[Alice0775 White]

(In reply to Jorg K (GMT+2) from comment #11)

Also, Alice, can you observer any changes to tabbing on FF's saved logins
and certificate manager panels (which are trees under the cover).

About Saved Login Dialog and Certificate manager, after landing of Bug 1523957, Shift+TAB is broken.
I think similar dialog(such as page info) using tree widget is also affected.

STR:

1. Open Saved login
   --- observe, now, search field gets focus as expected
2. TAB TAB
   --- observe, now, tree gets focus as expected.(When tree is focused, Up/Down arrow key select next/previous row)
3. TAB
   --- observe, now, checkbox label gets focus as expected
4. Shift+TAB

Actual Results:
Search Field gets focus

Expected Results:
Tree should get focus(When tree is focused, Up/Down arrow key select next/previous row)

STR:

1. Open Certificate manager
   --- observe, now, [Authorities] tab gets focus as expected.
2. TAB
   --- observe, now, tree gets focus as expected.(When tree is focused, Up/Down arrow key select next/previous row)
3. TAB
   --- observe, now, button gets focus as expected
4. Shift+TAB

Actual Results:
Authorities tab gets focus

Expected Results:
Tree should get focus(When tree is focused, Up/Down arrow key select next/previous row)

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1657fe18be9bc445c7838e3b4e93a9bfc4f85e7f&tochange=a6606e85276716b93695029cb914a678beb51708

Regressed by: a6606e85276716b93695029cb914a678beb51708 Victor Porof — Bug 1523957 - Convert trees to custom elements, r=bgrins, mak, standard8, MattN

Comment 16

[Alice0775 White]

Oops, s/2. TAB TAB/2. TAB/

Comment 17

[Jorg K (CEST = GMT+2)]

Thanks for confirming, Alice, much appreciated.

Victor and Brian, can you lend a hand fixing that regression, see comment #15 for effects in Firefox.

Comment 18

[Brian Grinstead [:bgrins]]

Alternate STR:

• Open https://www.mozilla.org/en-US/privacy/firefox/ -> Page Info
• Switch to Media tab
• Press TAB -> tree is focused
• Press TAB -> "Location" is focused
• Press Shift+TAB -> "Dimensions" gets focused but I'd expect the tree to get focused again

I was thinking originally it may be something related to focus event retargeting inside shadow dom (stuff like https://developers.google.com/web/fundamentals/web-components/shadowdom#focus) but it doesn't seem like this is specific to any individual page / piece of UI that would be listening to focus events in JS. So maybe this is a bug in the focus manager with focusing backwards into trees and/or shadow DOM. I don't know much about the focus implementation. Maybe Alex or Emilio have an idea - I know they've been working through a number of Shadow DOM focus bugs lately for wizard, dialog, etc.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

This is a focus manager bug.

Reduced test-case:

<style>
#host {
  border: 1px solid red;
  padding: 10px;
}

:focus {
  background: green;
}
</style>
<input>
<div id="host" tabindex=0><div></div></div>
<input>
<script>
host.attachShadow({ mode: "open" }).innerHTML = `<slot></slot>`;
</script>

Comment 20

[Emilio Cobos Álvarez (:emilio)]

I think I have a low-risk fix, kinda, but a big of a deeper question: Olli, this is caused by the interaction of the frame tree traversal that regular content does and the content traversal that we use for shadow dom. Given the current state of things, I kind of thing we should just simplify the setup and only have the shadow dom version of the focus traversal, wdyt?

That's a separate bug anyway.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1558393 - Don't keep top level scope owner when going backwards. r=smaug,edgar

When going backwards the host is the last thing of the scope we see, not the
first, so we need to make sure to properly switch to the document scope.

To be clear, the order this is happening when bogusly going backwards from
second to first is:

• We're doing frame traversal so start off the <div>.
• The text is the first frame in preorder we find, we figure out that the
  current top-level owner is the host. But it's not focusable so we carry on.
• Now we go to the next frame and find the <span>. oldTopLevelScopeOwner is
  still the span itself, and we don't change it (that's what my patch fixes),
  so we go into the "We're within non-document scope, continue", and thus skip
  trying to focus the host.

With this fix we actually realize that the current top level scope owner has
changed and thus don't skip it and try to focus the slot properly.

Comment 22

[alexander :surkov (:asurkov)]

bug 1558990 seems related to this one - advanceFocusIntoSubtree hangs on a slotted element when no focusable, https://phabricator.services.mozilla.com/D34785 has a mochitest.

Comment 23

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #20)

I think I have a low-risk fix, kinda, but a big of a deeper question: Olli,
this is caused by the interaction of the frame tree traversal that regular
content does and the content traversal that we use for shadow dom. Given the
current state of things, I kind of thing we should just simplify the setup
and only have the shadow dom version of the focus traversal, wdyt?

Well, frame tree traversal is in theory better approach. It more likely gives the behavior user expects.
So if we change the behavior, we at least need to test browser UI quite a bit.
a11y folks may have an opinion.

Comment 25

[Stanislav Plachý]

It is not only Shift + tab behavior. :-(
With closed message pane, go using tab from folder list to message list.
Next tab press should move focus to tabs, from tabs to folder list etc.
But focus does not change if you hit tab in message list.
Shift + tab moves focus from message list to tabs, but should go to folder list first and from folder list to tabs, from tabs to message list etc.
Tab and Shift + tab behavior is bad.
For blind people is it a big blocker. :-(

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c3402f0d512
Don't keep top level scope owner when going backwards. r=smaug

Comment 27

[Jorg K (CEST = GMT+2)]

Thanks for the fix.

This bug started as a TB bug and the description was (comment #0):

If I go with tab through elements in main window. If focus lands in message list, I cannot continue tabbing.

So in TB 68 beta it's clearly observable that once the message list (tree control) was hit by tabbing forward, further tabbing doesn't work and we're stuck.

The second issue reported in comment #0, shift-tab from the message list (tree control) not cycling back to where we were coming from, the folder list (also a tree control), is fixed as far as I can see.

So, the tab being stuck altogether is not fixed yet. Looks like we need to file another bug, this time we can't show the malfunction in FF.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Note that all this stuff is generally reproducible with plain HTML (see comment 19). I'm happy to investigate them if you can somehow repro them in HTML.

Comment 29

[Jorg K (CEST = GMT+2)]

Thanks, Emilio. I filed bug 1561288 for the "can't tab away from tree" issue.

Comment 30

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2c3402f0d512

Comment 31

[Jorg K (CEST = GMT+2)]

Emilio, can you please request uplift for this.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9073329 [details]
Bug 1558393 - Don't keep top level scope owner when going backwards. r=smaug,edgar

Beta/Release Uplift Approval Request

• User impact if declined: Broken backward tab navigation on XUL trees and other UI.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 18 for example.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Focusmanager is never trivial, but this change is extremely small.
• String changes made/needed: none

Comment 33

[Julien Cristau [:jcristau]]

Comment on attachment 9073329 [details]
Bug 1558393 - Don't keep top level scope owner when going backwards. r=smaug,edgar

regression fix for 68.0b14

Comment 34

[Jorg K (CEST = GMT+2)]

Thanks for approving this, Julien. This was really pretty bad as tabbing/shift-tabbing through attachment 9073883 [details] in FF 67 or 68 shows.

Comment 35

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f4a9a2f7f1d4

Comment 36

[Daniel Cicas [:dcicas], Release QA]

Hello,

I could reproduce this issue on Fx Nightly 69.0a1 BuildID: 20190610093815. I verified that this issue is fixed on the latest Nightly 69.0a1 BuildID: 20190626215508 and can confirm that this issue is fixed in Nightly. Will wait for Fx Beta 68.0b14 to verify this fix.

Comment 37

[Jean-Philippe MENGUAL]

I confirme the bug is fixed, many thanks.

Best regards

Comment 38

[Daniel Cicas [:dcicas], Release QA]

Hello,

I verified that this bugs is fixed in Fx Beta 68.0b14 on Windows 10 x64, Ubuntu 18.04 and mac OS 10.13.