Closed Bug 1561288 Opened 6 years ago Closed 6 years ago

Cannot tab off the thread pane/message list in TB 68 beta and trunk

Categories

(Thunderbird :: Toolbars and Tabs, defect)

Product:
Thunderbird
Component:
Toolbars and Tabs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr68+ fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 69.0
Tracking Flags:
Tracking Status
thunderbird_esr68 + fixed

People

(Reporter: jorgk-bmo, Assigned: emilio)

Details

(Keywords: access, regression)

Attachments

(3 files, 1 obsolete file)

tree.html - Attempt to recreate the situation in a HTML document
531 bytes, text/html
Details
debug-patch.patch (v2)
11.01 KB, patch
Details | Diff | Splinter Review
Bug 1561288 - Don't override currentContent when focusing something on a shadow root. r=smaug
47 bytes, text/x-phabricator-request
Details | Review

+++ This bug was initially created as a clone of Bug #1533291 +++

Bug 1533291 reported various issues related to navigation of the TB main window using the TAB and Shift+TAB keys.

Bug 1558393 fixed the Shift+TAB issue, but the issue also reported in bug 1558393 comment #0 "If focus lands in message list, I cannot continue tabbing" isn't fixed yet.

The funny thing is that the folder pane, just like the message list, is a tree control and we can tab away from it.

Let's see whether Alex can find a reduced test case or check whether there is anything special about the thread pane's tree setup.

Assignee: nobody → alessandro
Status: NEW → ASSIGNED
Keywords: regression

Another observation: If the message preview is switched on, one can tab off the message list. It tabs into the header of the message in the preview pane. So it looks like the collapsed element is blocking it.

<hbox xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" id="msgHeaderView" class="main-header-area" collapsed="true">

Sweet, thanks for opening this dedicated bug.
I'll try to dive deep and find what the heck is going on :D

As per bug 1558393 comment #28, I tried to make it fail in FF alone, but no luck so far. Of course you'd have to use FF 68 beta or Nightly.

In FF 67.0.4, SHIFT+TAB skips the A and B divs, while with TAB it cycles through them.
Does it happen to you too?

Sure, the Shift+TAB was broken until fixed today in bug 1558393. Now we're looking for an example to make it stop like it stops on the thread pane in TB.

Emilio, I can't find a HTML test case. Can you please assist me with debugging (unles you want to build TB yourself). I've debugged a bit in nsFocusManager::GetNextTabbableContent() and when trying to tab off the thread pane, I get *aResultContent==0 at
https://searchfox.org/mozilla-central/rev/7e158713cf5a8514fa8161dd4a239737b05da64d/dom/base/nsFocusManager.cpp#3588

So there was no next content found.

Flags: needinfo?(emilio)
Attached patch debug-patch.patch (obsolete) — — Splinter Review

This prints
=== *aResultContent==nullptr
when tabbing off the thread pane with the following message previews collapsed, otherwise it doesn't print anything as that code isn't reached. I'll sprinkle some more debug into it, but some assistance would be appreciated.

So when the message preview following the thread pane is enabled, we see:

=== nsFocusManager::GetNextTabbableContent
XUL* mail-emailaddress@000002AC0D40F4C0 headerName="from" class="emailDisplayBut
ton" context="emailAddressPopup" popup="emailAddressPopup" label="Richard Marti
<richard.marti@gmail.com>" emailAddress="richard.marti@gmail.com" fullAddress="R
ichard Marti <richard.marti@gmail.com>" displayName="Richard Marti" hascard="tru
e" tooltipstar="Edit Contact" state=[20800000] flags=[03000002] primaryframe=000
002AC115A9020 refcount=10<
  XUL* label@000002AC0DAD2040 class="emaillabel" value="Richard Marti <richard.m
arti@gmail.com>" state=[20800000] flags=[00000002] primaryframe=000002AC115A91D8
 refcount=4<>
  XUL* image@000002AC0DAD20D0 class="emailStar" context="emailAddressPopup" hasc
ard="true" tooltiptext="Edit Contact" state=[20000000] flags=[01000002] primaryf
rame=000002AC115A9130 refcount=4<>
  XUL* image@000002AC0DAD21F0 class="emailPresence" state=[20000000] flags=[0000
0002] primaryframe=0000000000000000 refcount=2<>
>
=== return 10

The failing case with the message pane disabled gives:

=== nsFocusManager::GetNextTabbableContent
=== *aResultContent==nullptr
=== return 12
=== nsFocusManager::GetNextTabbableContent
XUL* tree@000002AC10428EA0 id="threadTree" persist="lastfoldersent width" treeli
nes="true" flex="2" enableColumnDrag="true" _selectDelay="250" class="plain" las
tfoldersent="false" keepcurrentinview="true" disableKeyNavigation="true" context
="mailContext" onkeydown="ThreadPaneKeyDown(event);" onselect="ThreadPaneSelecti
onChanged();" hidevscroll="true" hidehscroll="true" clickthrough="never" focusri
ng="true" state=[88020800006] flags=[01000006] primaryframe=000002AC122FBB90 ref
count=47<
  XUL* treecols@000002AC114B8310 id="threadCols" is="thread-pane-treecols" picke
[snip]
  XUL* treechildren@000002AC105C4550 ondragstart="ThreadPaneOnDragStart(event);"
 ondragover="ThreadPaneOnDragOver(event);" ondrop="ThreadPaneOnDrop(event);" slo
t="treechildren" state=[20800004] flags=[00040002] primaryframe=000002AC12F38838
 refcount=33<>
>
=== return 11

So in that case, there are two calls, the first one returns nothing, the second one returns the entire tree. To me, this looks like a bug in the focus manager. Surely it should tab to something different.

Attachment #9074258 - Attachment is obsolete: true

I don't mind building TB if needed. Actually I use daily so I can try to poke at building a test-case that repros in Firefox as well... But I don't know how to repro this one, tabbing seems to work alright.

Ah, I do have the message preview turned on... I guess I can try to debug Thunderbird sometime this week. Poked a bit at it and I'm a bit confused about what can be going on. No promises of course, since that'd be free-time kind of work. But I use daily so the more I can help the better ;)

Thanks, Emilio. Yes, when the message preview is turned on, it works. But your blind users don't need the extra information, so they have it turned off, and then it doesn't work strangely.

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

Thanks, Emilio. Yes, when the message preview is turned on, it works. But
your blind users don't need the extra information, so they have it turned
off, and then it doesn't work strangely.

I am not sure I understand, but indeed, the problem does not seem to appear if the preview message window is displayed. And yes, we disable it as blind users. Because it is not convinient when browsing the messages at keyboard, but especially because, and it is not a regression, it creates high performance problems with assistive technologies. Down arrow keys create a strong lag, etc. So disabling this window is necessary, hence the importance of this bug.

Best regards,

Taking, this is another focus manager bug, will explain what's going on on the commit message.

Assignee: alessandro → emilio
Flags: needinfo?(emilio)

It is wrong, what we're trying to focus is frame->GetContent() still.

This is not reproducible outside of Thunderbird because for it to happen the
focus manager needs to wrap around the start content.

Here's what's going on:

  • We start with a <tree> focused, which is a shadow host.
  • There's nothing else to focus after it the tree, so we wrap around and start
    from the root, with aCurrentTabIndex = 1, since it's a forward navigation.
  • All the focusable elements in the page have tabIndex = 0 (we rely on wrapping
    around another time so that we start looking with aCurrentTabIndex = 0), so
    we end up entering the shadow tree content again.
  • The frame tree traversal finds a frame for a non-focusable child of the
    <tree>, but then this code sets currentContent = <tree>. So frame is still
    the frame of the non-focusable kid, but currentContent is tree.
  • Since the child frame is not focusable, we wrongly end up at 1, and
    returning early, since currentContent != frame->GetContent(), instead of
    keeping looking for the right thing to focus.

As I said this cannot reproduce on content, as far as I can tell, so I'm not
sure how to go about testing this, but all the shadow dom tab navigation tests
keep passing with this change.

Thanks, Emilio. While some of us know our way around well our own mail related code, we mostly struggle debugging issues of the underlying platform code.

Yeah, I understand, no worries, I'm happy to help :)

FWIW the way I debugged this was with rr and MOZ_LOG=FocusNavigation:5, which is helpful to see how focus moves.

(Ni? for a phabricator comment.)

Flags: needinfo?(emilio)

I don't see any comment there, maybe unpublished?

Flags: needinfo?(emilio) → needinfo?(bugs)
Flags: needinfo?(bugs)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/78d99b96c104 Don't override currentContent when focusing something on a shadow root. r=smaug

https://hg.mozilla.org/mozilla-central/rev/78d99b96c104

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 69.0

Hi,

I confirm, the bug is fixed. Now tab enables to switch betwen all the screen areas, and shift-tab too.

Many thanks for this fantastic job

Regards

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: