Closed Bug 468497 Opened 16 years ago Closed 6 years ago

Switching tabs using Ctrl+Tab does not generate an accessible focus event on the newly focused control.

Categories

(Thunderbird :: Toolbars and Tabs, defect)

defect
Not set
major

Tracking

(thunderbird_esr60 fixed, thunderbird62 wontfix, thunderbird63 fixed)

RESOLVED FIXED
Thunderbird 63.0
Tracking Status
thunderbird_esr60 --- fixed
thunderbird62 --- wontfix
thunderbird63 --- fixed

People

(Reporter: MarcoZ, Assigned: Jamie)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 3 obsolete files)

STR:
1. Open Shredder.
2. With your default Inbox folder open, right-click a different folder and choose "Open in new tab".
3. Focus the message list.
4. Press Ctrl+Tab.

Expected: Contents of message list changes to your Inbox. Screen reader should notify the user.
Actual: Contents changes, but screen reader does not receive notification. Only after hitting up and down arrow once is there a refresh in content.
Blocks: tabsmeta
Flags: blocking-thunderbird3?
Marco, what do we need to do to fix this bug?  Is it just a matter of calling .focus() on the DOM element corresponding to the tab panel?
(In reply to comment #1)
> Marco, what do we need to do to fix this bug?  Is it just a matter of calling
> .focus() on the DOM element corresponding to the tab panel?

No, because the focus never leaves the tree table. The question is: Do you reuse the same tree table and simply repopulate it with new content, which is what seems to be happening, or do you create new tree tables each time?

A trick Aaron Leventhal once told me is this: Fire a DOMMenuItemActive event on the  selected tree child once the tabs are switched. A11y
code translates this into a regular focus event if fired on anything other than
a real menu item.
Sounds pretty painful from accessibility pov.
Component: Mail Window Front End → Toolbars and Tabs
Flags: blocking-thunderbird3? → blocking-thunderbird3+
QA Contact: front-end → toolbars-tabs
Target Milestone: --- → Thunderbird 3.0rc1
Pulling this in to beta 4.  The fix sounds easy but the mozmill test implications will require investigation.  Ideally I would like some form of accessibility 'listener' that we could use to support assertions about events having been generated that are appropriate for screen readers and the like.

We can take a fix for this without a unit test if someone wants to provide one.

Tsk/Standard8, have you perchance seen any signs of this already existing for mozmill?
OS: Windows Vista → All
Hardware: x86 → All
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b4
This bug also applies (i.e. a focus event is not fired) when pressing enter to open a message with "Open messages in a new tab" configured. Closing a message tab also fails to fire an appropriate focus event. In these cases, pressing up/down arrows when returning to a message list tab often isn't enough to restore focus; one has to press tab once or twice.
(In reply to comment #5)
> This bug also applies (i.e. a focus event is not fired) when pressing enter to
> open a message with "Open messages in a new tab" configured.
Just saw that this is covered by bug 501308.
We're getting to the point in the release cycle where we have to start letting go of bugs which don't effect a sufficiently large percentage of users.  As much as we want this for Thunderbird 3, I don't think we'd let it hold back all the other goodness that's landed since Tb2 if it were the last bug standing.  Marking as blocking-, wanted+, in the hopes that someone does find the bandwidth to come up with a patch.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Any chance of a fix for this in 3.1? Is Marco's solution (though arguably a bit of a hack) as described in comment 2 (using DOMMenuItemActive) acceptable?

Interestingly, the focused state is set on the new active item as it should be, but no focus event is fired.
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.4) Gecko/20100608 Thunderbird/3.1

this behaviour persists. Furthermore if browsing through messages by <ctrl><tab> or <ctrl><shift><tab> you see the proper messages but the "tab focus" is never displayed or moved unless the number of messages is less or equal to the number that fit in the current windows width.
Advise please.
Blocks: 725469
(In reply to Michał from comment #9)
> Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.4) Gecko/20100608
> Thunderbird/3.1
> 
> this behaviour persists. Furthermore if browsing through messages by
> <ctrl><tab> or <ctrl><shift><tab> you see the proper messages but the "tab
> focus" is never displayed or moved unless the number of messages is less or
> equal to the number that fit in the current windows width.
> Advise please.

I filed that as bug 725469, maybe two sides of the same coin. It's unbelievable. I still see this behaviour in TB10.
See also: 695285, 577655.
(In reply to Michał from comment #11)
> See also: 695285, 577655.

Thanks, Michał, for pointing out some connections and describing the problem of comment 9, which is now bug 725469.
Yes, these bugs/rfes also refer to the tab bar, and insofar are related.
Otherwise, bug 695285 is unrelated to this bug, and I forward-duplicated bug 577655 against more concise bug 725469, which might well be related or even a duplicate of this one.
No longer blocks: tbird3access
Attached patch WIP patch (obsolete) — Splinter Review
Inform FocusManager when the tree's view changes. This seems to work... sometimes. I've no idea why it doesn't work all the time. I've confirmed that the item I'm passing definitely isn't the same as the previous item FocusManager knew about, but it still doesn't fire an event in some cases.
I've also confirmed that in all cases, the right item is definitely getting passed to FocusManager::ActiveItemChanged. It always has a state of invisible at this point, even in the cases where it works.

I'm starting to think the event somehow gets blocked because the tree hasn't been updated visually yet. I tried moving the TreeViewChanged a11y call in nsTreeBodyFrame to ReflowFinished (with a flag of course), but it doesn't seem to get called. This is probably a bad idea anyway. :)

Frustratingly, I can't use a11y logging to debug this because when I do, the bug goes away, which suggests it's definitely timing related.
Attached patch Correct WIP patch (obsolete) — Splinter Review
Bah. Patch accidentally included unrelated stuff. Sorry.
Attachment #8474043 - Attachment is obsolete: true
Neil, any thoughts about the problems/questions raised by Jamie in comment #14?
Flags: needinfo?(enndeakin)
(In reply to James Teh [:Jamie] from comment #14)
> I'm starting to think the event somehow gets blocked because the tree hasn't
> been updated visually yet. I tried moving the TreeViewChanged a11y call in
> nsTreeBodyFrame to ReflowFinished (with a flag of course), but it doesn't
> seem to get called. This is probably a bad idea anyway. :)
> 

A reflow will only occur if the tree needs to change its size or layout. TreeViewChanged is only called when the view is changed. Is the view being changed here, or just the data returned by the view?
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #17)
> TreeViewChanged is only called when the view is changed. Is the view being
> changed here, or just the data returned by the view?
I'm pretty sure the view is being changed. TreeViewChanged does seem to get called. As noted in comment 14, I even know that I'm retrieving the item that should get focus, but it's always invisible at the time TreeViewChanged is called. I haven't worked out yet why accessibility sometimes fires the focus event and other times refuses, but the fact that it's invisible at that point means something is clearly not ready at that point.
Neil, any further thoughts relative to comment 18?
Severity: normal → major
Flags: needinfo?(enndeakin)
Target Milestone: Thunderbird 3.0b4 → ---
Nope.
Flags: needinfo?(enndeakin)
Attached patch Rebased patch (obsolete) — Splinter Review
Jamie's patch, rebased to current code base.

I can confirm that this fixes the bug. If I have two folders open and focus is in both messages lists, ctrl+tab will reliably speak the newly focused item in the new tab. Note that the tab itself is not spoken, but I think that's more a screen reader side of thing than ours. Also if I open a message from either, so it is wedged between the two folder trees, ctrl+tabbing between all three tabs will always speak the focused item. Jamie, what do you think, should we take this? Asking Surkov for review since we kinda both worked on this. ;)
Attachment #8474093 - Attachment is obsolete: true
Flags: needinfo?(jteh)
Attachment #8989726 - Flags: review?(surkov.alexander)
(In reply to Marco Zehe (:MarcoZ) from comment #21)
> Note that the tab itself is not spoken, but I think
> that's more a screen reader side of thing than ours.

From a screen reader perspective, there's nothing that would cause the tab to be reported. IIRC, message list tabs don't have their own tab panel/property page accessible in the hierarchy, so the focus ancestry doesn't change and thus there's no new "context" to report aside from the newly focused item. You could perhaps argue a screen reader should watch for selection events on tabs, but that would probably need to be app specific. In a super ideal world, each tab would have its own container and the ancestry would thus change, but that's a massive change and would probably prevent reuse of the tree.

> Jamie, what do you think, should we take this?

If it fixes the bug, I don't see why not. Ideally, we'd have some kind of test for it, but I think writing a test which covers this is going to be pretty complicated and I'm not sure it's worth the effort given that this has only ever been seen in Thunderbird. We're using XUL less and less these days.
Flags: needinfo?(jteh)
Comment on attachment 8989726 [details] [diff] [review]
Rebased patch

As this is Jamie's patch, which I just rebased, I am giving this an r+ as a module peer. Jamie, feel free to land at your discretion. Since this is Gecko, and only concerning Thunderbird, it'll probably be hard to get it backported to 60ESR, which the next TB release will use, but going forward, this should then be fixed for TB users.
Attachment #8989726 - Flags: review?(surkov.alexander) → review+
Happy to get this landed. I agree uplift to 60 ESR is probably not something we can justify, though.
Keywords: checkin-needed
The patch currently has no author and commit message, so you may want to update it with that.
Ug, forgot about that. Thanks for the heads up. I might re-submit this as a mozreview request to make my life easier if you don't mind r+ing it again.
Keywords: checkin-needed
Attachment #8989726 - Attachment is obsolete: true
Comment on attachment 8989909 [details]
Bug 468497: Inform the accessibility FocusManager when a XUL tree's view changes.

https://reviewboard.mozilla.org/r/254916/#review261796
Attachment #8989909 - Flags: review?(mzehe) → review+
(In reply to James Teh [:Jamie] from comment #24)
> Happy to get this landed. I agree uplift to 60 ESR is probably not something
> we can justify, though.

But if it only affects Thunderbird, then what's the harm in uplift?
Thunderbird uses a branch off mozilla-central 60, with customizations, too so you can ask to uplift it to that branch too instead if not accepted to mozilla-central directly.
(In reply to Wayne Mery (:wsmwk) from comment #29)
> But if it only affects Thunderbird, then what's the harm in uplift?

The code relates to XUL trees, so it could theoretically affect Firefox. I've just never seen this bug in Firefox, but that doesn't mean there isn't a potential for unforeseen regressions. The risk is low, but it's still there, which makes it hard to justify uplift to mozilla-central.

(In reply to Magnus Melin from comment #30)
> Thunderbird uses a branch off mozilla-central 60, with customizations, too
> so you can ask to uplift it to that branch too instead if not accepted to
> mozilla-central directly.

Ah. Is there a flag to request this directly? I'm very reluctant to ask for uplift to m-c.
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4fd60abd9726
Inform the accessibility FocusManager when a XUL tree's view changes. r=MarcoZ
https://hg.mozilla.org/mozilla-central/rev/4fd60abd9726
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
Assignee: nobody → jteh
(In reply to James Teh [:Jamie] from comment #31)
> Ah. Is there a flag to request this directly? I'm very reluctant to ask for
> uplift to m-c.

Not directly, but you can ask for approval‑comm‑esr60 uplift and just add info to the comment.
Comment on attachment 8989909 [details]
Bug 468497: Inform the accessibility FocusManager when a XUL tree's view changes.

[Approval Request Comment]
Regression caused by (bug #): None.
User impact if declined: Screen reader users will not receive any notification when they switch between folder tabs with the keyboard, so they won't know what message is currently focused.
Testing completed (on c-c, etc.): Another user/developer verified that this patch fixes the bug in comment 21.
Risk to taking this patch (and alternatives if risky): Low risk. Only affects accessibility, and only when a tree is completely changed (its view changes).

Note that this patch is a mozilla-* patch, not a comm-* patch. I haven't requested uplift to Mozilla ESR because while it's low risk, I don't know of any manifestation of this bug in Firefox, so I can't justify the uplift for mozilla ESR. See comment 30 and comment 34.
Attachment #8989909 - Flags: approval-comm-esr60?
Comment on attachment 8989909 [details]
Bug 468497: Inform the accessibility FocusManager when a XUL tree's view changes.

Something is wrong here. This as an M-C bug and you're asking for inclusion in TB 60 (and consequently also 62) via a release branch using comm approval flags. Hmm.

OK, I'll do it.
Attachment #8989909 - Flags: approval-comm-esr60?
Attachment #8989909 - Flags: approval-comm-esr60+
Attachment #8989909 - Flags: approval-comm-beta+
(In reply to Jorg K (GMT+2) from comment #36)
> Something is wrong here. This as an M-C bug and you're asking for inclusion
> in TB 60 (and consequently also 62) via a release branch using comm approval
> flags. Hmm.

I tried to explain the reasons for this in the uplift request, and I requested comm approval based on the advice in comment 34. I don't know of another way to request uplift to the comm specific fork of mozilla 60 ESR. Apologies if I did this the wrong way; I'm not familiar with the Thunderbird release process for m-c changes given that it's quite separate from Firefox.

In terms of Firefox, this probably wouldn't be something I'd uplift to 60. However, given that Thunderbird releases are less regular, I didn't want users to have to wait so long for the fix.

If this is all just too messy, that's fine and reasonable; we can just wait until the next Thunderbird after 60.
Trick to get this off the books for uplift since my query looks for releases/comm-esr60/rev ;-)
Attachment #8989909 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.