795957 - [AccessFu] Support live regions

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

Right now we do nothing that respects or supports live regions.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi Jamie,

I was wondering if you might have some suggestions regarding handling consequent show on child|text inserted on parent events (and/or hide on child|text removed on parent events) for live regions.

I am trying to figure out whether handling of text inserted/removed is appropriate where it's the result of the child of a live region being shown or hidden.

Thanks!

Comment 3

[James Teh [:Jamie]]

You do need to report changes to descendants. Note that aria-relevant determines which changes. Also, aria-atomic and aria-busy affect whether and how changes should be reported. (FWIW, NVDA doesn't yet handle aria-atomic and aria-busy. :))

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 795957 v1

This is a first WIP.

At the moment there are several assumptions inside the event handlers.
Also I started with SpeechPresente so the rest of the presenters will follow.
No support yet for atomic or busy.

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 777602 [details] [diff] [review]
Patch for 795957 v1

Here's some initial feedback:

1. Please replace all instances of "decendant" with "descendant".

2. Why are you using aria-label instead of ordinary inner text? Most text that will be in live regions will actually be visible on the screen rather than aria-labels, which are not. So tests should reflect that.

3. Please add a localization comment that the word "hidden" will be spoken when some information disappears.

Other than that, the tests look good so far.

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 795957.patch

Updated based on Marco's comments. There's an issue with announcing hidden text at the moment: it looks like the utterance generator does not include the text node when removed.

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 777743 [details] [diff] [review]
795957.patch

This looks better.

One question: The changes to accessfu.jsm at the top of the patch seem unrelated to this bug on first glance. Are they? And if not, what purpose do they serve?

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Marco Zehe (:MarcoZ) from comment #7)
> Comment on attachment 777743 [details] [diff] [review]
> 795957.patch
> 
> This looks better.
> 
> One question: The changes to accessfu.jsm at the top of the patch seem
> unrelated to this bug on first glance. Are they? And if not, what purpose do
> they serve?

Since I was fixing the jsatcommon.js I also addressed some of the warnings related to pref getters and setters in the tests.

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 795957 v3

Still a WIP.
Fixed and updated the tests.
Pivot context can now optionally handle hidden subtree (for announcing hide events). 
Added preliminary Android support.

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 778320 [details] [diff] [review]
Patch for 795957 v3

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

Nice patch, its a lot. I'll give it a better look later today, worst case first thing Monday.

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 778320 [details] [diff] [review]
Patch for 795957 v3

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

Overall this looks good. Like you mentioned on IRC, you don't want to explicitly announce hidden and then shown content, since that is to chatty and it does not reflect what the user experience should be: changing text, not hiding and showing other text.

::: accessible/src/jsat/EventManager.jsm
@@ +235,5 @@
> +        if (!liveRegion) {
> +          break;
> +        }
> +        // Show for text is handled by the EVENT_TEXT_INSERTED handler.
> +        if (aEvent.accessible.DOMNode.nodeType === TEXT_NODE) {

I think this should check for a role of text leaf instead of checking the node type.

::: accessible/src/jsat/Presentation.jsm
@@ +56,4 @@
>     */
>    textChanged: function textChanged(aIsInserted, aStartOffset,
>                                      aLength, aText,
> +                                    aModifiedText, aIsPolite) {},

I think textChanged should remain for user input text changes only.

@@ +108,5 @@
> +   * Announce a live region shown.
> +   * @param  {PivotContext} aContext context object for an accessible.
> +   * @param  {boolean} aIsPolite A politeness level for a live region.
> +   */
> +  objectShown: function objectShown(aContext, aIsPolite) {},

Object hidden/shown is too generic. This needs to be named something live region specific. Same with the utterance methods. It should also take an optional string similar to aModifiedText in textChanged.

::: accessible/src/jsat/Utils.jsm
@@ +510,5 @@
> +      if (this._includeInvisible) {
> +        include = true;
> +      } else {
> +        let state = {};
> +        child.getState(state, {});

We have a Utils function for state so you could do all this in one line.

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Patch for 795957 v4

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Tests for 795957 v1

Comment 14

[Marco Zehe (:MarcoZ)]

Comment on attachment 789867 [details] [diff] [review]
Tests for 795957 v1

This looks very thorough! Also really cool to have the queuing in place! r=me.

Comment 15

[Eitan Isaacson [:eeejay]]

Comment on attachment 789867 [details] [diff] [review]
Tests for 795957 v1

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

r++ These tests look good! A lot of fancy additions. I like the log watching, I didn't know you could do that. It is really cool and useful.

::: accessible/tests/mochitest/jsat/test_live_regions.html
@@ +28,5 @@
> +    }
> +
> +    function updateText(id, text) {
> +      var element = document.getElementById(id);
> +      element.textContent = text;

I imagine innerHTML is used often in the wild. Will it work exactly like textContent? What if they add more markup with innerHTML?

Comment 16

[Eitan Isaacson [:eeejay]]

Comment on attachment 789866 [details] [diff] [review]
Patch for 795957 v4

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

This looks good. Pretty dense, but worst case we could catch and refine things later. The thing that I am most concerned about is performance. hide/show and text events are frequent. I don't have any brilliant optimizations, but we need to be careful and selective. I am also a bit concerned about the 1ms timeout which seems arbitrary, and fragile on different systems and setups.

::: accessible/src/jsat/AccessFu.jsm
@@ +161,5 @@
>      Services.obs.removeObserver(this, 'Accessibility:ActivateObject');
>      Services.obs.removeObserver(this, 'Accessibility:MoveByGranularity');
>  
> +    delete this._quicknavModesPref;
> +    delete this._notifyOutputPref;

Good catch. I guess these are non-related flyby fixes?

::: accessible/src/jsat/EventManager.jsm
@@ +148,5 @@
> +        // XXX Bug 442005 results in DocAccessible::getDocType returning
> +        // NS_ERROR_FAILURE. Checking for aEvent.accessibleDocument.docType ==
> +        // 'window' does not currently work.
> +        (aEvent.accessibleDocument.DOMDocument.doctype &&
> +          aEvent.accessibleDocument.DOMDocument.doctype.name === 'window')) {

nit: misaligned indent.

@@ +232,5 @@
> +      {
> +        let {liveRegion, isPolite} = this._handleLiveRegion(aEvent,
> +          ['additions', 'all']);
> +        // Only handle show if it is a relevant live region.
> +        if (!liveRegion) {

from what I can gather from _handleLiveRegion(), it always returns a truthy value, {}. You may want to change it to return null if no relevant live region is found.

@@ +264,5 @@
>        case EVENT_TEXT_REMOVED:
>        {
>          if (aEvent.isFromUserInput) {
> +          // Handle all text mutations coming from the user.
> +          this._handleText(aEvent);

_handleText is an ambiguous function name. Anyway, I would have _handleLiveRegion check and return immediately on isFromUserInput == true. Then you could keep _handleText inline here. If you still want to keep it a separate function, call it _handTextEvent.

@@ +338,5 @@
> +          busy: attrs['container-busy'],
> +          atomic: attrs['container-atomic'],
> +          memberOf: attrs['member-of']
> +        };
> +      }

This function should always return something. lets avoid explicit null returns.

@@ +398,5 @@
> +    }
> +    let eventHandler = {
> +      eventType: aEventType,
> +      timeoutID: Utils.win.setTimeout(this.present.bind(this),
> +        1, // Wait for a possible EVENT_SHOW or EVENT_TEXT_INSERTED event.

Does a HIDE always precede a SHOW when replacing text in a node? How reliable is 1ms? You should push both this and the test patch to try and see if it is happy. Besides the user-ended issues. I am concerned that this introduces brittle mochitests that could fail when the build servers have a high CPU load.

I wonder if window.requestAnimationFrame might be a more bomb-proof solution.

::: accessible/src/jsat/Utils.jsm
@@ +265,5 @@
>  
>      return true;
>    },
>  
> +  matchAttributeValue: function matchAttributeValue(aAttributeValue, values) {

I wish javascript had better list/set comprehension with a difference function. In any case, you could slightly optimize here by using a set.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #15)
> Comment on attachment 789867 [details] [diff] [review]
> Tests for 795957 v1
> 
> Review of attachment 789867 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r++ These tests look good! A lot of fancy additions. I like the log
> watching, I didn't know you could do that. It is really cool and useful.
> 
> ::: accessible/tests/mochitest/jsat/test_live_regions.html
> @@ +28,5 @@
> > +    }
> > +
> > +    function updateText(id, text) {
> > +      var element = document.getElementById(id);
> > +      element.textContent = text;
> 
> I imagine innerHTML is used often in the wild. Will it work exactly like
> textContent? What if they add more markup with innerHTML?

I believe it's recommended to use textContent, but you are right, I should absolutely add the test case with innerHTML as well.

Comment 18

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #16)
> Comment on attachment 789866 [details] [diff] [review]
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +161,5 @@
> >      Services.obs.removeObserver(this, 'Accessibility:ActivateObject');
> >      Services.obs.removeObserver(this, 'Accessibility:MoveByGranularity');
> >  
> > +    delete this._quicknavModesPref;
> > +    delete this._notifyOutputPref;
> 
> Good catch. I guess these are non-related flyby fixes?

Yes, since I was improving the jsatcommon, I figured I'll address some intermittent warnings as well.

> 
> @@ +232,5 @@
> > +      {
> > +        let {liveRegion, isPolite} = this._handleLiveRegion(aEvent,
> > +          ['additions', 'all']);
> > +        // Only handle show if it is a relevant live region.
> > +        if (!liveRegion) {
> 
> from what I can gather from _handleLiveRegion(), it always returns a truthy
> value, {}. You may want to change it to return null if no relevant live
> region is found.

In case {} is returned liveRegion and isPolite will be undefined with this multiple assignment.

> @@ +398,5 @@
> > +    }
> > +    let eventHandler = {
> > +      eventType: aEventType,
> > +      timeoutID: Utils.win.setTimeout(this.present.bind(this),
> > +        1, // Wait for a possible EVENT_SHOW or EVENT_TEXT_INSERTED event.
> 
> Does a HIDE always precede a SHOW when replacing text in a node? 

As far as the spec goes, yes. I also did some logging and it seems to be consistent with the spec.

> How reliable is 1ms? You should push both this and the test patch to try and see
> if it is happy. Besides the user-ended issues. I am concerned that this
> introduces brittle mochitests that could fail when the build servers have a
> high CPU load.
> 
> I wonder if window.requestAnimationFrame might be a more bomb-proof solution.

Thanks, I'll try and test the other approach.

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

Pushed to try https://tbpl.mozilla.org/?tree=Try&rev=6027ba6cae63

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15c5fec95754
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9902fb61ee

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15c5fec95754
https://hg.mozilla.org/mozilla-central/rev/9e9902fb61ee