Closed Bug 955056 Opened 10 years ago Closed 10 years ago

Some screen readers say the word "frame" a lot when moving the selection in the contacts list

Categories

(Instantbird Graveyard :: Contacts window, defect)

x86
Other
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: aleth)

References

()

Details

(Whiteboard: [accessibility])

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 1626 at 2012-08-09 10:06:00 UTC ***

I haven't reproduced this myself, but several different users mentioned it on Twitter. I think it happens at least with NVDA, but maybe other screen readers too.

Someone got an NVDA developer to look at it, he said on Twitter:
04:01:24 - jcsteh: @tmantalks @fqueze I assume you mean the contact list. It's actually literally throwing focus to a random frame object just before it +
04:02:23 - jcsteh: @tmantalks @fqueze focuses each buddy list item. I have no idea what the object it; it doesn't provide any useful info.


I'm wondering if it could be caused by the change of XBL binding that happens when a contact is selected.
Whiteboard: [accessibility]
Summary: Screen ready say the word "frame" a lot when moving the selection in the contacts list → Some screen readers say the word "frame" a lot when moving the selection in the contacts list
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2012-08-09 10:38:37 UTC ***

Fwiw, this doesn't happen when you focus on the contact group headers (tag: group), only on the contacts themselves (tag: contact).

Further debugging reveals that the frame accessible is not really a frame accessible at all. What's actually happening is that we get two focus events for the contact, but the first refers to an accessible which dies before we have a chance to query it. It's like the contact list item mutates somehow. Unfortunately, I don't know enough about XUL internals to provide any more info than that.
*** Original post on bio 1626 at 2012-08-15 22:46:10 UTC ***

(In reply to comment #1)
> What's actually happening is that we get two focus events
> for the contact, but the first refers to an accessible which dies before we
> have a chance to query it. It's like the contact list item mutates somehow.
> Unfortunately, I don't know enough about XUL internals to provide any more info
> than that.

As flo suggests, this is likely due to the contact binding being exchanged via CSS:
http://lxr.instantbird.org/instantbird/source/instantbird/content/blist.css#6
becomes
http://lxr.instantbird.org/instantbird/source/instantbird/content/blist.css#12

It seems you receive a focus event for the contact binding, and then another one for the contact-big binding which replaces it after the contact is marked selected. The former is one too many.

I'm not sure what can be done about this.
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2012-08-16 01:37:45 UTC ***

This is probably a question to ask the Mozilla accessibility team. I don't know much about bindings, despite a quick read. I assume the contact node itself stays the same (even though its binding is changed)?
*** Original post on bio 1626 at 2012-08-16 09:48:26 UTC ***

(In reply to comment #3)
> I assume the contact node itself 
> stays the same (even though its binding is changed)?
Yes - the two bindings are that of the unselected contact (a single line) and that of the selected contact (two lines (with status), expandable to show merged contacts). I am guessing the sequence of events is something like this: Focus moves to a contact. This fires a focus event which you pick up, and also bubbles up to the richlistbox containing the contacts, which marks the contact as selected. CSS then replaces the binding to that appropriate to the selected contact. Another focus event is fired as the new binding gets focus.
*** Original post on bio 1626 at 2012-08-17 20:35:31 UTC ***

I think the focus is supposed to stay on the richlistbox (so that we can continue to use up/down arrows to change the selected contact), so another possible explanation would be that a button (either "start conversations" or "expand") of the expanded (2 lines) contact gets briefly focused before the richlistbox is focused again.
*** Original post on bio 1626 at 2012-08-17 20:39:09 UTC ***

(In reply to comment #5)
> I think the focus is supposed to stay on the richlistbox (so that we can
> continue to use up/down arrows to change the selected contact)

Yes, and no focus events arrive at the contact binding (I checked). I don't know what the screen reader hooks into though - I assumed they report "selected" as "focused", eliding the difference.
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2012-08-18 16:10:04 UTC ***

(In reply to comment #6)
> Yes, and no focus events arrive at the contact binding (I checked). I don't
> know what the screen reader hooks into though - I assumed they report
> "selected" as "focused", eliding the difference.
Screen readers use accessibility APIs. This is definitely focus (not selection) from an accessibility API standpoint. However, I don't know how Gecko maps to accessibility focus internally in this case.
*** Original post on bio 1626 by alexander surkov <surkov.alexander AT gmail.com> at 2012-08-19 02:59:19 UTC ***

A binding change causes the accessible tree recreation. Current item change in the richlistbox causes accessible focus event when the richlistbox is focused.

Do I understand right that you receive a focus event for defunct list item accessible and then another focus event for recreated list item accessible? Is the problem you have an event for defunct accessible?
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2012-08-19 22:45:43 UTC ***

(In reply to comment #8)
> Do I understand right that you receive a focus event for defunct list item
> accessible and then another focus event for recreated list item accessible? Is
> the problem you have an event for defunct accessible?
The problem is the first event. Out-of-process, the accessible is actually forgotten by Gecko before we even have a chance to fetch it; i.e. IAccessible::accChild doesn't even return it. If we were able to fetch it fast enough, I guess it would probably go defunct. In-process, it isn't defunct when it is fetched, but that's because in-process is synchronous.
*** Original post on bio 1626 by alexander surkov <surkov.alexander AT gmail.com> at 2012-08-20 01:48:17 UTC ***

I see. Fixing it on instantbird side sounds like workaround since what they do it seems is a legal usage of JS/XUL/XBL. I'm not sure what we can do on Gecko side (perhaps to keep event target accessibles alive longer but how long?). Would it be reasonable to fix the problem on NVDA side - just ignore dead accessibles?
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2012-08-20 02:28:52 UTC ***

(In reply to comment #10)
> I see. Fixing it on instantbird side sounds like workaround since what they do
> it seems is a legal usage of JS/XUL/XBL. I'm not sure what we can do on Gecko
> side (perhaps to keep event target accessibles alive longer but how long?).
Keeping them alive longer wouldn't necessarily help anyway. Out-of-process ATs would still need a special exception to ignore focus on defunct accessibles and in-process ATs would get focus on two seemingly different list items. The only useful solution on the Gecko/Instantbird side is to eliminate that first focus event altogether, but I assume there's no way to do this. Is there some way to detect focus is about to change and change the binding before the focus event is actually fired?

> Would it be reasonable to fix the problem on NVDA side - just ignore dead
> accessibles?
I can certainly special case this in NVDA for Instantbird (or even Gecko if you think this is likely to occur elsewhere with Gecko), but it's obviously a hack and doesn't fix it for other ATs. That said, other ATs may already handle this somehow; I'm not sure.
*** Original post on bio 1626 by alexander surkov <surkov.alexander AT gmail.com> at 2012-08-20 02:50:29 UTC ***

(In reply to comment #11)
> The only
> useful solution on the Gecko/Instantbird side is to eliminate that first focus
> event altogether, but I assume there's no way to do this. Is there some way to
> detect focus is about to change and change the binding before the focus event
> is actually fired?

In Gecko we don't don't know what will be next so it seems there's no way to fix it on Gecko side. It can be fixed on Instantbird side (since they can handle that richlistbox selection is about to change) but as I said I think it's a hack.

> > Would it be reasonable to fix the problem on NVDA side - just ignore dead
> > accessibles?
> I can certainly special case this in NVDA for Instantbird (or even Gecko if you
> think this is likely to occur elsewhere with Gecko), but it's obviously a hack
> and doesn't fix it for other ATs. That said, other ATs may already handle this
> somehow; I'm not sure.

For Gecko at least since I believe it's not unique usecase when AT gets a dead accessible if it listens events out of process. Btw I think it may be true for any browser (app?).

But why is it a hack? The server sends an event for accessible and this accessible goes away in nearest future. If AT listens events in in-process then it gets this accessible but if it listens events out-of-process then it gets dead accessible. It sounds as a common scenario.
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2012-08-20 06:12:34 UTC ***

(In reply to comment #12)
> It can be fixed on Instantbird side (since they can
> handle that richlistbox selection is about to change)
Right; that's the sort of thing I was thinking.
> but as I said I think
> it's a hack.
From an accessibility standpoint, it isn't; focus should never be fired twice when it only moved once. However, I guess you mean it's a hack because changing the binding on focus is a valid use case in XUL.

> > I can certainly special case [ignoring dead focus] in NVDA for Instantbird (or even Gecko if you
> > think this is likely to occur elsewhere with Gecko), but it's obviously a hack
...
> But why is it a hack? The server sends an event for accessible and this
> accessible goes away in nearest future. If AT listens events in in-process then
> it gets this accessible but if it listens events out-of-process then it gets
> dead accessible. It sounds as a common scenario.
A server firing events on an accessible which dies before the event even arrives is understandable when objects are changing very fast. However, it's particularly nasty for focus and really shouldn't happen. Focus is very user centric; other changes can happen very fast, but focus should really only change when the user needs to or must care about it. If the focus moves once, only one focus event should be fired. In this case, an in-process AT would end up speaking two focus changes when only one really occurred from the user's perspective.
*** Original post on bio 1626 by alexander surkov <surkov.alexander AT gmail.com> at 2012-08-20 07:29:17 UTC ***

(In reply to comment #13)

> However, I guess you mean it's a hack because changing
> the binding on focus is a valid use case in XUL.

right, if XUL developer should keep things like this in mind then it must be unreasonable difficult language to develop interfaces.

> A server firing events on an accessible which dies before the event even
> arrives is understandable when objects are changing very fast. However, it's
> particularly nasty for focus and really shouldn't happen. Focus is very user
> centric; other changes can happen very fast, but focus should really only
> change when the user needs to or must care about it. If the focus moves once,
> only one focus event should be fired. In this case, an in-process AT would end
> up speaking two focus changes when only one really occurred from the user's
> perspective.

true. but I don't see how we can bypass these technical restrictions though. Accessible object can be very unstable thing.
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2012-08-20 07:40:24 UTC ***

(In reply to comment #14)
> > A server firing events on an accessible which dies before the event even
> > arrives is understandable when objects are changing very fast. However, it's
> > particularly nasty for focus and really shouldn't happen.
> true. but I don't see how we can bypass these technical restrictions though.
Sure, which is why I'm happy to implement the work around in NVDA. It's still an ugly hack, though. :)

So, as to arriving at a solution, we have two options:
1. Work around it on the Instantbird side by somehow intercepting the richlistbox selection change and changing the binding on the new selection *before* it really changes. The advantage of this is that it will solve the problem for multiple screen readers.
2. If that isn't feasible, I'll implement a work around in NVDA. This is fairly trivial to do.
*** Original post on bio 1626 by alexander surkov <surkov.alexander AT gmail.com> at 2012-08-20 08:50:33 UTC ***

(In reply to comment #15)

> So, as to arriving at a solution, we have two options:
> 1. Work around it on the Instantbird side by somehow intercepting the
> richlistbox selection change and changing the binding on the new selection
> *before* it really changes. The advantage of this is that it will solve the
> problem for multiple screen readers.
> 2. If that isn't feasible, I'll implement a work around in NVDA. This is fairly
> trivial to do.

both of these seems to be reasonable to implement
*** Original post on bio 1626 by alexander surkov <surkov.alexander AT gmail.com> at 2012-08-21 09:27:04 UTC ***

(In reply to comment #15)

> So, as to arriving at a solution, we have two options:
> 1. Work around it on the Instantbird side by somehow intercepting the
> richlistbox selection change and changing the binding on the new selection
> *before* it really changes. The advantage of this is that it will solve the
> problem for multiple screen readers.

another option is to bind the binding to element inside listitem
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2012-08-23 03:11:16 UTC ***

Any thoughts from Instantbird devs on a resolution? If this isn't likely to be changed on the Instantbird side (whether ever or at least in the near future), I'll work around it in NVDA.
*** Original post on bio 1626 at 2012-08-23 10:01:26 UTC ***

We discussed this briefly on IRC. Our general feeling is that we dislike most of the hacks proposed here, especially because they would be fragile and likely to be easily broken by a developer who wouldn't know about this bug and accessibility concerns.

That said, if the issue is actually on our side, we would be happy to fix it, but we aren't going to release just for this, so a fix in Instantbird would only be available in nightlies until we do another stable release; which is unlikely to happen before a few months.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1626 as attmnt 1831 at 2012-08-23 11:16:00 UTC ***

James, could you try this patch and see if it helps? It is a hack, but a fairly mild one. (It will only work if the DOMattr event fires before the first accessibility focus event, which I can't test.)

The problem with this bug is that what Instantbird is doing is in fact the way you are supposed to change bindings. So this is a Gecko issue and will also occur in other settings/applications. 

For the CSS to realize that a contact listitem has been selected, it has to have been marked 'selected', in which case it seems the accessibility focus event has already been fired. Suppressing the second event would not help as the accessible it points to no longer exists after the binding changes, and the first event is likely the same one that is needed for the CSS to respond. So it's hard to see how one would fix this cleanly - maybe a mozilla expert on the precise sequence of event handling would be able to help. 

Is it a large overhead for screen readers to check that events refer to accessibles that still exist? Or is this in fact a problematic thing to do in general?
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2012-08-23 11:44:31 UTC ***

(In reply to comment #20)
> James, could you try this patch and see if it helps?
Unfortunately, I don't currently have a Mozilla/Instantbird build environment set up. I may be able to do this some day, but am not likely to have the time for a while. Sorry! :(

> Suppressing the second event would not help as
> the accessible it points to no longer exists after the binding changes, and the
> first event is likely the same one that is needed for the CSS to respond.
Which event do you mean when you say "second event"? We get two accessibility focus events and the second one is the one we want (it fires on the new accessible), but I'm assuming this isn't what you're referring to.

> Is it a large overhead for screen readers to check that events refer to
> accessibles that still exist?
Not particularly. It's just hacky, there's no "standard" way to do it and it has to be done for each affected screen reader. However, I can easily make this a Mozilla specific change in NVDA; it's fairly trivial.
*** Original post on bio 1626 at 2012-08-23 12:12:48 UTC ***

(In reply to comment #21)
> > James, could you try this patch and see if it helps?
> Unfortunately, I don't currently have a Mozilla/Instantbird build environment
> set up. I may be able to do this some day, but am not likely to have the time
> for a while. Sorry! :(
You don't need a build environment for this change: You'd only have to open the omni.ja file of your Instantbird and modify the correct file within according to the patch.

> > Suppressing the second event would not help as
> > the accessible it points to no longer exists after the binding changes, and the
> > first event is likely the same one that is needed for the CSS to respond.
> Which event do you mean when you say "second event"? We get two accessibility
> focus events and the second one is the one we want (it fires on the new
> accessible), but I'm assuming this isn't what you're referring to.
Sorry, "it" was ambiguous here: What I meant was that trying to suppress the second event wouldn't help as the first one is useless, and trying to suppress the first event is likely difficult as it is tied to the contact getting selected in the first place, which will always happen before the CSS can respond. The patch above is the only way I can think of to work around that.
*** Original post on bio 1626 at 2012-08-23 12:14:51 UTC ***

(In reply to comment #22)
> You don't need a build environment for this change: You'd only have to open the
> omni.ja file of your Instantbird and modify the correct file within according
> to the patch.
Oh, and start Instantbird with the -purgecaches command line parameter to be on the safe side.
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2012-08-28 01:29:43 UTC ***

Thanks for the tip regarding omni.ja!

(In reply to comment #20)
> James, could you try this patch and see if it helps?
It definitely helps. When arrowing up and down in the contact list, a single focus event is fired per item as it should be. However, if you move focus away from the list completely and then move it back (e.g. tab then shift+tab), the extraneous focus event is fired then.
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1626 as attmnt 1863 at 2012-08-28 13:18:00 UTC ***

Please test this which should hopefully fix the issue.
Attachment #8353621 - Flags: feedback?
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8353621 [details] [diff] [review]
Patch v2

*** Original change on bio 1626 attmnt 1863 at 2012-08-28 13:19:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353621 - Flags: feedback?
Comment on attachment 8353621 [details] [diff] [review]
Patch v2

*** Original change on bio 1626 attmnt 1863 at 2012-08-28 13:22:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353621 - Flags: feedback?(bugzilla)
Comment on attachment 8353621 [details] [diff] [review]
Patch v2

*** Original change on bio 1626 attmnt 1863 by jamie AT nvaccess.org at 2012-08-28 23:01:58 UTC ***

Perfect. That said, if this is still considered too hacky and prone to regression, I'm still happy to make the change in NVDA. (I'm not sure whether this affects other screen readers, but it is very likely unless they're filtering it out.)
Attachment #8353621 - Flags: feedback?(bugzilla) → feedback+
*** Original post on bio 1626 at 2012-08-29 00:29:54 UTC ***

Filed a corresponding BMO bug, so we can remove the hack if/when it gets fixed. I cc'd the participants in this comment thread so you can correct what I put there ;)
https://bugzilla.mozilla.org/show_bug.cgi?id=786508
Attached patch PatchSplinter Review
*** Original post on bio 1626 as attmnt 1869 at 2012-08-29 00:53:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353627 - Flags: review?(florian)
Comment on attachment 8353590 [details] [diff] [review]
Patch

*** Original change on bio 1626 attmnt 1831 at 2012-08-29 00:53:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353590 - Attachment is obsolete: true
Comment on attachment 8353621 [details] [diff] [review]
Patch v2

*** Original change on bio 1626 attmnt 1863 at 2012-08-29 00:53:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353621 - Attachment is obsolete: true
Comment on attachment 8353627 [details] [diff] [review]
Patch

*** Original change on bio 1626 attmnt 1869 at 2012-08-29 22:18:16 UTC ***

(In reply to comment #26)

> Perfect. That said, if this is still considered too hacky and prone to
> regression, I'm still happy to make the change in NVDA.

I discussed this with aleth over IRC. It's hacky but not too horrible so I'm OK taking it, but looking forward to remove it as soon as the real Mozilla bug is fixed.
You may still want to fix it temporarily in NVDA too if it affects many of your users, as I don't think you will want to advise them to use nightly builds :-). That part is up to you.
Attachment #8353627 - Flags: review?(florian) → review+
*** Original post on bio 1626 at 2012-08-30 02:47:51 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/668dc68e5905

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2013-09-12 02:16:52 UTC ***

This seems to have regressed in the current nightly (20130905042202). I haven't had time to debug yet to see whether it's exactly the same in terms of events, but it certainly behaves the same from a user perspective.
*** Original post on bio 1626 at 2013-09-12 08:27:36 UTC ***

(In reply to comment #31)
> This seems to have regressed in the current nightly (20130905042202). I haven't
> had time to debug yet to see whether it's exactly the same in terms of events,
> but it certainly behaves the same from a user perspective.

See bug 955182 (bio 1750), I think we wanted to ask you for feedback after landing the patch from that bug; I'm afraid we forgot.
Depends on: 955631
*** Original post on bio 1626 by James Teh <jamie AT nvaccess.org> at 2013-10-23 04:02:18 UTC ***

(In reply to comment #32)
> > This seems to have regressed in the current nightly (20130905042202).
> See bug 955182 (bio 1750), I think we wanted to ask you for feedback after landing the patch
> from that bug; I'm afraid we forgot.
Shall I file a follow up bug to get this regression fixed?
*** Original post on bio 1626 at 2013-10-23 10:55:25 UTC ***

(In reply to comment #33)

> Shall I file a follow up bug to get this regression fixed?

aleth has already filed bug 955631 (bio 2186). I'm afraid we currently have no idea of how we can fix it though, so if you have ideas they would be welcome.
You need to log in before you can comment on or make changes to this bug.