753984 - [AccessFu] Make utterance order configurable

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

Probably 2 settings are enough. Description first, description last.
Example: "Search link", "link Search".

Comment 1

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

Attachment: Inital patch to get some feedback.

I was hoping to get some feedback whether I'm doing something similar to what you had in mind.

I was also trying to find tests for accessfu but the closest thing I found was Bug 811307. Would you have any suggestions as to how to test this besides building an apk and running it with different prefs?

Thanks!

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 725996 [details] [diff] [review]
Inital patch to get some feedback.

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

This is the right approach. Mochitests should be coming soon, that would be the ideal place to test for this.

Comment 3

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

Attachment: Some initial thoughts for the test.

Given the way utterance order preference was implemented, I am not sure if testing UtteranceGenerator.genForObject is necessary, since the order is decided within UtteranceGenerator._addName.

Comment 4

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

Attachment: Tests for utterance order preference.

I think I got a good set of tests for all possible utterance generator functions.

There is one problem, however. I have one case where the test fails - when testing list items. For this markup

<li id="li_one">list one</li>

I am getting a childCount of 2 and thus the INCLUDE_NAME flag is never set. Would you know if my markup is wrong or there might be a problem somewhere else?

Thanks!

Comment 5

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

Probably one child is the bullet, which is expected IIRC.

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #4)
> Created attachment 728276 [details] [diff] [review]
> Tests for utterance order preference.
> 
> I think I got a good set of tests for all possible utterance generator
> functions.
> 
> There is one problem, however. I have one case where the test fails - when
> testing list items. For this markup
> 
> <li id="li_one">list one</li>
> 
> I am getting a childCount of 2 and thus the INCLUDE_NAME flag is never set.
> Would you know if my markup is wrong or there might be a problem somewhere
> else?
> 
> Thanks!

We don't include the name of list items, the child text leaf's name is added, and thus the text is spoken. Like David said, their are two children, one is the text leaf, and the other (i think first child) is a static text node which is the styled bullet. We don't speak that.

Comment 7

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

As per IRC conversation with eeejay (http://krijnhoetmer.nl/irc-logs/accessibility/20130322#l-280), complete utterance generation should move to UtteranceGenerator from Presentation. Order should happen for the overall output, not just for a specific nsIAccessible.

Comment 8

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

Attachment: Moved all utterance generation into Utterance Generator.

Comment 9

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

Attachment: Tests for utterance order.

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 729542 [details] [diff] [review]
Moved all utterance generation into Utterance Generator.

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

Definitely in the right direction. But not there yet. See notes below.

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +75,5 @@
> +  /**
> +   * Generates an utterance for a PresenterContext.
> +   * @param {PresenterContext} aContext object that generates and caches
> +   *    context information for a given accessible and its relationship with
> +   *    another accessible.

I find it strange that this function takes a PresenterContext as an argument while Presentation.jsm is not an official dependency. Maybe PresenterContext should be renamed and put in Utils.jsm so the dependency graph remains clean. If it moves to Utils is should be renamed to something more generic and fitting to its role. It is basically a caching mechanism so that we don't repeatedly dig the tree. Can't think of a good name for it, maybe PivotCache since its main association is an accessible and a previous accessible. Or PointOfRegard since that is screen-reader terminology for the virtual cursor position of the screen reader.

@@ +93,5 @@
> +    aContext.subtreePreorder.forEach(addUtterance);
> +
> +    // Reverse the utterance array if description is preferred first.
> +    return utteranceOrder === UTTERANCE_DESC_FIRST ?
> +      utterance : utterance.reverse();

So we discussed this a bit on IRC. And I was distracted and I forgot why post order subtree would be required as opposed to simple reversal. Now I recall :) Say the user it traversing by table cell, and they land on a cell in a new table in the document:

<table>
 <caption>Fruits and vegetables</caption>
 <tr>
  <td>
    <ul>
      <li><a href="...">Apples</a></li>
      <li><a href="...">Bananas</a></li>
      <li><a href="...">Peaches</a></li>
      <li><a href="...">Plums</a></li>
    </ul>
  </td>
  ...
 </tr>
 ...
</table>

In a description-first situation, the speech would be something like:
"table, Fruits and vegetables, cell, list 4 items, First item, link, Apples, link, Bananas, link, Peaches, Last item, link, Plums"

With your patch, description-last, it would look like this:
"Plums, link, Last item, Peaches, link, Bananas, link, Apples, link, First Item, list 4 items, cell, Fruits and vegetables, table"

When what we really want is:
"Apples, link, First item, Bananas, link, Peaches, link, Plums, link, Last item, list 4 items, cell, Fruits and vegetables, table"

To get what we want we would first have to generate the utterance for the subtree in post-order[1], then attach the current node, and then its (child->parent ordered) ancestry.

1. http://en.wikipedia.org/wiki/Tree_traversal

@@ +324,5 @@
> +    let name = (aFlags & INCLUDE_NAME) ? (aAccessible.name || '') : '';
> +    if (name) {
> +      utterance.push(name);
> +    }
> +  },

Good addition. With the info I gave on top, it might be worth checking the order pref here and either appending or prepending the name like you did in the previous patch. Since description last is not a trivial utterance traversal.

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 729543 [details] [diff] [review]
Tests for utterance order.

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

This is a very welcome addition. I am excited to see it land. It is not very relevant because the previous patch does not work correctly yet, so I am r-ing it as well.

::: accessible/tests/mochitest/jsat/test_bug753984.html
@@ +41,5 @@
> +      }
> +
> +      // Test UtteranceGenerator.genForContext utterance order for
> +      // a particular accessible context.
> +      function testContextUterance(utteranceOrder, aAccOrElmOrID) {

Utterance with two "t"s

@@ +57,5 @@
> +          utterance.indexOf(objectUtteranceFirst) <=
> +            utterance.indexOf(objectUtteranceLast) :
> +          utterance.indexOf(objectUtteranceFirst) >=
> +            utterance.indexOf(objectUtteranceLast),
> +          "Utterance order is correct for " + aAccOrElmOrID);

This is hard to read, maybe this should be broken up with a real "if". Also, since we aren't testing for simple list reversal, it might be worth just doing is() with expected utterance outcomes.

@@ +61,5 @@
> +          "Utterance order is correct for " + aAccOrElmOrID);
> +      }
> +
> +      // Test UtteranceGenerator.genForObject for a particular aAccOrElmOrID.
> +      function testUtterance(aAccOrElmOrID) {

An interesting thing to test for is different movement vectors (ie. the utterance is different when oldAccessible is set to different things), so you could simulate a move from one part of the document to another. I wouldn't block review on that though, having oldAccessible be null has its dvantages too because you get the full context utterance each time.

@@ +86,5 @@
> +          "heading",
> +          "list",
> +          "dlist",
> +          "li_one"
> +        ];

Maybe add some more complex containers like the example I gave in the other patch review.

@@ +93,5 @@
> +        aAccOrElmOrIDs.forEach(function testAAccOrElmOrID(aAccOrElmOrID) {
> +          var utteranceOrderValues = [0, 1];
> +          utteranceOrderValues.forEach(
> +            function testUtteranceOrder(utteranceOrder) {
> +              Services.prefs.setIntPref(PREF_UTTERANCE_ORDER, utteranceOrder);

This should be set through the SpecialPowers API.

@@ +101,5 @@
> +          testUtterance(aAccOrElmOrID);
> +        });
> +
> +        // If there was an original utterance order preference, revert to it.
> +        Services.prefs.setIntPref(PREF_UTTERANCE_ORDER, originalUtteranceOrder);

SpecialPowers.clearUserPref()

Comment 12

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

Attachment: Addressed all comments.

Comment 13

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

Attachment: Updated Patch with addressed latest comments.

This passes in isolation but not in the whole suite.

Comment 14

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

Comment on attachment 732812 [details] [diff] [review]
Updated Patch with addressed latest comments.

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

Nothing is jumping out at me WRT the suite failure. How does it fail? (Which tests?)

::: accessible/tests/mochitest/jsat/Makefile.in
@@ +11,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +MOCHITEST_A11Y_FILES =\
> +test_bug753984.html \

Nit: our naming style for test files is to be descriptive. E.g. "test_utterance_order.html"

::: accessible/tests/mochitest/jsat/test_bug753984.html
@@ +8,5 @@
> +    <meta charset="utf-8">
> +    <link rel="stylesheet" type="text/css"
> +          href="chrome://mochikit/content/tests/SimpleTest/test.css" />
> +    <style type="text/css">
> +      .offscreen {

nit: remove unused css style block.

@@ +39,5 @@
> +          oldAccessible = getAccessible(oldAAccOrElmOrID);
> +        }
> +        var context = new PivotContext(accessible, oldAccessible);
> +        var utterance = UtteranceGenerator.genForContext(context);
> +        isDeeply(utterance, expected,

TIL there is a deep object comparison test :)

@@ +61,5 @@
> +
> +      function doTest() {
> +        // Test the following aAccOrElmOrID (with optional old aAccOrElmOrID).
> +        // Note: each aAccOrElmOrID entry maps to a unique object utterance
> +        // generator function wihtin the UtteranceGenerator.

nit: 'within'

Comment 15

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

Attachment: Updated test patch with davidb's comments.

Comment 16

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

(In reply to David Bolter [:davidb] from comment #14)
> @@ +39,5 @@
> > +          oldAccessible = getAccessible(oldAAccOrElmOrID);
> > +        }
> > +        var context = new PivotContext(accessible, oldAccessible);
> > +        var utterance = UtteranceGenerator.genForContext(context);
> > +        isDeeply(utterance, expected,
> 
> TIL there is a deep object comparison test :)

Yes, I was convinced there must've been one :)

Comment 17

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

Attachment: Latest patch.

Latest patch that takes into account changes made for bug 857749.

Comment 18

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

Attachment: Latest tests patch.

All tests now pass after addressing bug 857749. Also, now controlling the root of the pivot context so the tests are consistent when they are run both in suite and individually.

Comment 19

[Eitan Isaacson [:eeejay]]

Comment on attachment 733164 [details] [diff] [review]
Latest patch.

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

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +15,5 @@
>  
> +const UTTERANCE_DESC_FIRST = 0;
> +
> +// Read and observe changes to a setting for utterance order.
> +let utteranceOrder;

this should have a preceding 'g' so we know it is a global - gUtteranceOrder.

@@ +94,5 @@
> +      aContext.subtreePreorder.forEach(addUtterance);
> +    } else {
> +      aContext.subtreePostorder.forEach(addUtterance);
> +      addUtterance(aContext.accessible);
> +      aContext.newAncestry.reverse().forEach(addUtterance);

Nice!

::: mobile/android/app/mobile.js
@@ +633,5 @@
>  // Enable accessibility mode if platform accessibility is enabled.
>  pref("accessibility.accessfu.activate", 2);
>  pref("accessibility.accessfu.quicknav_modes", "Link,Heading,FormElement,ListItem");
> +// Setting for an utterance order (0 - description first, 1 - description last).
> +pref("accessibility.accessfu.utterance", 0);

This is just a tangent: We should create some tweaks extension people could install and control this and other things, like the quicknav modes and eventually the verbosity level too.

In any case, I wonder what Marco would have to say about description-last mode. Maybe it should be the default, at least in B2G. And maybe in Android for uniformity. Anyway, that would mean some sort of migration plan that won't upset currently happy users.

Comment 20

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #18)
> Created attachment 733165 [details] [diff] [review]
> Latest tests patch.
> 
> All tests now pass after addressing bug 857749. Also, now controlling the
> root of the pivot context so the tests are consistent when they are run both
> in suite and individually.

Do they pass on a try server too? I could post it there to make sure.

Comment 21

[Eitan Isaacson [:eeejay]]

Comment on attachment 733165 [details] [diff] [review]
Latest tests patch.

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

Overall, looks great. Like I said previously, I would like to see this run on try before landing. I'll push these two patches now.

::: accessible/tests/mochitest/jsat/test_utterance_order.html
@@ +23,5 @@
> +
> +      // Test UtteranceGenerator.genForContext utterance order for
> +      // a particular accessible context with an optional old accessible.
> +      function testContextUtterance(expected, aAccOrElmOrID, oldAAccOrElmOrID) {
> +        oldAAccOrElmOrID = oldAAccOrElmOrID || "root";

I like that solution for having a common root no matter how nested this test is.

@@ +59,5 @@
> +          aAccOrElmOrID: "textarea",
> +          expected: [[
> +            "text area", "Test Text Area", "This is the text area text.", "\n"
> +          ], [
> +            "This is the text area text.", "\n", "Test Text Area", "text area"

What are those newlines? We probably should be filtering those. I guess that is a separate bug?

@@ +94,5 @@
> +            "Last item", " ", "link", "Plums"
> +          ], [
> +            " ", "Apples", "link", "First item", " ", "Bananas", "link", " ",
> +            "Peaches", "link", " ", "Plums", "link", "Last item",
> +            "list 4 items", "Fruits and vegetables", "table"

Same here, we are sending whitespace, I don't think we should. I created bug 858130 for that.

Comment 22

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

> ::: accessible/tests/mochitest/jsat/test_utterance_order.html
> @@ +23,5 @@
> > +
> > +      // Test UtteranceGenerator.genForContext utterance order for
> > +      // a particular accessible context with an optional old accessible.
> > +      function testContextUtterance(expected, aAccOrElmOrID, oldAAccOrElmOrID) {
> > +        oldAAccOrElmOrID = oldAAccOrElmOrID || "root";
> 
> I like that solution for having a common root no matter how nested this test
> is.
> 

+1. Makes things less fragile.

Comment 23

[Eitan Isaacson [:eeejay]]

https://tbpl.mozilla.org/?tree=Try&rev=350ff8192143

Comment 24

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

Attachment: Final patch with comments from eeejay

Renamed utteranceOrder to gUtteranceOrder

Comment 25

[Eitan Isaacson [:eeejay]]

The try results are good enough...

https://hg.mozilla.org/integration/mozilla-inbound/rev/8354b14f5036
https://hg.mozilla.org/integration/mozilla-inbound/rev/55985195ce90

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8354b14f5036
https://hg.mozilla.org/mozilla-central/rev/55985195ce90