785144 - [AccessFu] Braille doesn't update when navigating through content

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Reported by Jason over Twitter:
When navigating through content via a braille display, navigation works, but the braille display does not get the actual utterances. It does not show any of the content. It works fine in the native UI parts like the Awesome Screen.

Comment 1

[Max Li [:maxli]]

Attachment: Patch

As far as I can tell, this is the desired behaviour, but would prefer some confirmation.

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 744599 [details] [diff] [review]
Patch

This looks right. It needs more testing with TalkBack, to see if populating that field doesn't mess with things. If it doesn't we could land this as-is. After that, there are a few things to do:
1. Populate the rest of the node info with data (clickable/checkable) that will help the display show the state, for example "(x)" for checked check buttons.
2. Implement performAction for each of those actions (this may already work).

More challenging bits:

3. Figure out a way to have TalkBack speech and Braille output be different from each other. If that is possible, the braille output should be much less verbose. Possibly less role names, and no new context info (ie. don't prepend container names when stepping into the first node of that container).
4. Populate the adjacent nodes as well, the braille output seems to make use of them too. This would probably require an addition to the nsIAccessiblePivot API with methods such as peekNext/peekPrevious because you would want to know what the next and previous nodes are without actually traversing to them.

Comment 3

[Eitan Isaacson [:eeejay]]

Comment on attachment 744599 [details] [diff] [review]
Patch

Actually lets suspend this r+ until it is tested more with TalkBack.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Eitan Isaacson [:eeejay] from comment #3)
> Comment on attachment 744599 [details] [diff] [review]
> Patch
> 
> Actually lets suspend this r+ until it is tested more with TalkBack.

Who is doing that? :)

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #4)
> (In reply to Eitan Isaacson [:eeejay] from comment #3)
> > Comment on attachment 744599 [details] [diff] [review]
> > Patch
> > 
> > Actually lets suspend this r+ until it is tested more with TalkBack.
> 
> Who is doing that? :)

Max, you should probably get familiar with smoke-testing Firefox.

Comment 6

[Max Li [:maxli]]

(In reply to Eitan Isaacson [:eeejay] from comment #5)
> (In reply to David Bolter [:davidb] from comment #4)
> > (In reply to Eitan Isaacson [:eeejay] from comment #3)
> > > Comment on attachment 744599 [details] [diff] [review]
> > > Patch
> > > 
> > > Actually lets suspend this r+ until it is tested more with TalkBack.
> > 
> > Who is doing that? :)
> 
> Max, you should probably get familiar with smoke-testing Firefox.

The patch does not seem to cause any changes in TalkBack's behaviour.

Comment 7

[Eitan Isaacson [:eeejay]]

Comment on attachment 744599 [details] [diff] [review]
Patch

Lets have a module peer review this as well..

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 744599 [details] [diff] [review]
Patch

Interesting! Wonder why our speech interface worked at all, when we didn't even set the contentDescription previously? Or is this call now being executed twice and should be removed from a different spot?

Comment 9

[Max Li [:maxli]]

(In reply to Marco Zehe (:MarcoZ) from comment #8)
> Comment on attachment 744599 [details] [diff] [review]
> Patch
> 
> Interesting! Wonder why our speech interface worked at all, when we didn't
> even set the contentDescription previously? Or is this call now being
> executed twice and should be removed from a different spot?

If there was no information on the node, TalkBack would fall back to taking the text from the event (which was getting populated).

Comment 10

[Marco Zehe (:MarcoZ)]

Thanks, Max!

Would it make sense to include BrailleBack as one of the recognized services, similar to TalkBack, Spiel etc.?

Comment 11

[Max Li [:maxli]]

(In reply to Marco Zehe (:MarcoZ) from comment #10)
> Thanks, Max!
> 
> Would it make sense to include BrailleBack as one of the recognized
> services, similar to TalkBack, Spiel etc.?

Given that BrailleBack depends on having TalkBack also on to do navigation, I don't think it's necessary.

Comment 12

[Marco Zehe (:MarcoZ)]

I just ran with this patch, and it causes a variation of bug 854006 to reoccur. It appears that the event text and node info contentDescription cause repetition of the last node's info when the screen is being touched in some completely different place. Instead of speaking the new item under the finger, the last item is repeated, and until the user actually hits a new element by dragging the finger around, TalkBack keeps retaining and repeating that info. This is only when exploring, not when swiping left and right.

Braille works with this patch. When swiping or using the controls on the braille display, info is flawlessly being updated. When exploring, braille also displays the older node info when a new spot on the screen is touched.

Comment 13

[Max Li [:maxli]]

Attachment: Patch v2

So the issue is that there was a focus event being dispatched when you start exploring by touch. Since we're now populating the node info now with text, TalkBack was reading it. I think the best solution to this is to (try to) not associate these events with the current virtual cursor node.

Marco, could you try this out and see if this causes other regressions?

Comment 14

[Marco Zehe (:MarcoZ)]

Comment on attachment 749999 [details] [diff] [review]
Patch v2

f=me. This works and still speaks everything it should. It also eliminates seemingly random speech when exploring, and one gets an even better feel for where what is on a page. :)

Comment 15

[Eitan Isaacson [:eeejay]]

Comment on attachment 749999 [details] [diff] [review]
Patch v2

Review of attachment 749999 [details] [diff] [review]:
-----------------------------------------------------------------

This looks right.

Comment 16

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f5496fdca8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f9f5496fdca8