845870 - [AccessFu] Trust explicitly associated names when speaking certain elements

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This is a follow-up to our Firefox OS finding w.r.t. names for buttons and links, e.g. the dock icons, the explicitly associated names in alpha.app.net etc.

I'd like our name speaking to be smart enough to use explicitly associated names, e. g. through aria-label etc., in favor of possibly present inner text which may not be accurate.

Let's improve and review the patch you have for this, Eitan, and get it in ASAP. This will improve many pages, including, hopefully, mobile Twitter once they get around to fixing their stuff.

Comment 1

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

Will this patch affect the gecko naming algorithm?

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to David Bolter [:davidb] from comment #1)
> Will this patch affect the gecko naming algorithm?

No, it will make sure we use the one in nsIAccessible more often than we currently do from within AccessFu. AccessFu, sometimes, like when landing on text leaves below links, will try to read the inner text in favor of the actual link name nsIAccessible might have calculated. This bug aims to make sure we use the calculated name more often rather than trying to work against it. :)

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: smarter name inclusion. just a wip really, don't take it seriously.

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 719209 [details] [diff] [review]
smarter name inclusion. just a wip really, don't take it seriously.

This does not give us the result we want. We should implement support of the attribute introduced in bug 637578 that tells assistive technologies how the name was obtained, and always use it if authors give an explicit name.

Comment 5

[Eitan Isaacson [:eeejay]]

Marco, Could you please put some test cases here. I recall app.net and twitter.

Comment 6

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

Attachment: Patch for 845870 v1

Comment 7

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

Attachment: Tests for the patch v1

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 742196 [details] [diff] [review]
Patch for 845870 v1

f=me. This patch does exactly what it is supposed to. I tried it on the alpha.app.net when logged in, and the "star" and "re-post" items now have their aria-label splken instead of the unicode characters that are the text children of those links, because the speech synthesizer doesn't speak those. Nice work!

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 742197 [details] [diff] [review]
Tests for the patch v1

I'm wondering if the two test utterance files don't have quite some code in common that could be shared to reduce code duplication?

Also, a few general things:
1. Parameters of functions have a small cap a at the beginning. So aAccOrElmOrId is correct, but oldAAccOrElmOrId should be aOldAccOrElmOrId.
2. In the array of utterance expectations, these should not have an appended a, because they are properties of that particular value set, not function parameters. So accOrElmOrId and oldAccOrElmOrId and not aAcc.... etc.

I'd like to see a new patch with these changes, so cancelling feedback for this one.

Comment 10

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

Attachment: Tests for the patch v2

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 742196 [details] [diff] [review]
Patch for 845870 v1

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

This might be a tough problem to solve. My concern is that this is done above the specific role considerations in a sweeping fashion. What I mean by this is that we know we want this behavior for such things as buttons. But there are other roles where we might want to use the explicit name *and* the subtree, or ignore the explicit name entirely.

For example in your test, you have a <textarea/> with a title. When the user lands on it, they will only hear the explicit name and not the contents, when they really should hear both (and in a different order depending on description first/last). In these cases we might want to retrieve the accessible.value.

Also, I think this case will not work:
<section>
  <button aria-label="Orange">Garbage</button>
  <button aria-label="Apple">Garbage</button>
  <button aria-label="Pear">Garbage</button>
</section>

If the vc lands on the section, it will read something like "button, Orange, Garbage, button, Apple, Garbage, button, Pear, Garbage".

::: accessible/src/jsat/Utils.jsm
@@ +159,5 @@
>      aAccessible.getState(state, extState);
>      return [state.value, extState.value];
>    },
>  
> +  isNameExplicit: function isNameExplicit(aAccessible) {

This is used only by the utterance generator. So I don't think it belongs in the Utils module. What could go here is a method that extracts accessible attributes into a key/value js object. I could see that being useful some time in the future, as well as this case.

Comment 12

[Eitan Isaacson [:eeejay]]

Comment on attachment 742423 [details] [diff] [review]
Tests for the patch v2

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

More tests of nested items would be nice too :)

::: accessible/tests/mochitest/jsat/test_explicit_names.html
@@ +51,5 @@
> +        accOrElmOrID: "li_two",
> +        expected: ["list 2 items", "Test List", "Last item", "2. ", "list two"]
> +      }, {
> +        accOrElmOrID: "cell",
> +        expected: ["table", "Fruits and vegetables", "List of Fruits"]

This looks wrong to me. It should continue reading the contents and not stop at the explicit name. If it does than that reinforces the problem I was mentioning earlier.

Comment 13

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

Here's the log of the IRC discussion about this bug:

# [18:34] <@eeejay> yzen, there are problems with your approach. it is a really tough problem to solve
# [18:35] <yzen> eeejay: oh :( i had a feeling it might be no as easy as i thought :)
# [18:36] <yzen> YES! i was going to ask about the textarea :)
# [18:36] <@eeejay> yzen, yeah. but thanks to the tests you put in place, we could move forward without regressing too much
# [18:36] <@eeejay> yzen, in the past every time i tweaked the behavior i broke something else
# [18:38] <yzen> eeejay: i like your idea about the utils method
# [18:39] <yzen> eeejay: so i have a test for a button that does not read the content if there's an aria-label, i wonder if it would actually not read garbage, but i can add this as a test case
# [18:39] <yzen> eeejay: do you think there might be some sort of criteria to identify the need for accessible.value ?
# [18:39] <@firebot> [e-mail] denied review for attachment 742423 [details] [diff] [review] on bug 845870.
# [18:40] <@eeejay> yzen, right. and test with nesting, like my example
# [18:40] <yzen> eeejay: yep
# [18:45] <yzen> eeejay: i wonder if indeed this is the case about the cell, it reads it all if there's no aria-label on it (like in utterance order tests) but does not read the whole subtree with label, it still needs to be that verbose with explicit name too ?
# [18:45] <@eeejay> yzen, the hard part is the fact that the context object gives us flattened versions of the subtree when you want to sometimes not add certain descendants
# [18:45] <@eeejay> yzen, yeah
# [18:46] <@eeejay> yzen, i think so.
# [18:46] <@eeejay> yzen, think of the name as a caption
# [18:47] <yzen> eeejay: so it sounds like explicit name is quite specific , for things like buttons , anything else ? links ?
# [18:49] <@eeejay> yzen, http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/RoleMap.h
# [18:49] <@eeejay> every role that has eNameFromSubtreeRule
# [18:49] <yzen> eeejay: nice :) , i can do that then
# [18:50] <yzen> eeejay: thanks for pointing it to me
# [18:51] <@eeejay> yzen, it can't be copied verbatim, some thinking needs to go in each
# [18:51] <@eeejay> yzen, for example links
# [18:51] <@eeejay> yzen, both the subtree AND the title should probably be read
# [18:52] <@eeejay> yzen, which makes me wonder if perhaps subtrees should never be ignored
# [18:52] <@eeejay> yzen, it would be much easier to implement
# [18:52] <@eeejay> yzen, we should consult marco

Marco, what would you suggest?

Comment 14

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

^

Comment 15

[Eitan Isaacson [:eeejay]]

Here is the subtree from app.net:

<ul class="unstyled ul-horizontal yui3-u fixed-right ta-right">
  <li class="yui3-u">
    <a href="#star" data-starred="0" data-star-button="1" data-post-id="5098826">
      <i aria-label="star" class="icon-star-empty"></i>
    </a>
  </li>
  <li class="yui3-u repost">
    <a href="#repost" title="repost" data-repost-button="1" data-reposted="0" data-post-id="5098826">
      <i aria-label="repost" class="icon-repost"></i>
    </a>
  </li>
</ul>

Without styling they don't look like much. But it is harder to extract the correct style...

Comment 16

[Marco Zehe (:MarcoZ)]

The history of bug 637578 is that there were many cases where NVDA and other assistive technologies tried to get the most meaningful information for a link or button from the sub tree. But the important info was not always in the sub tree. Especially since the conception of aria-label, but sometimes also plainly through HTML author error like putting a title on a link and no alt on the child graphic.
In this example, Firefox would correctly deduce the accessible name for the link from the link's title, but NVDA would ignore that and still try to get meaning from the borked graphic child.
The same goes for aria-label.

Now with bug 637578 in place,,,,, if it is present, it is very likely that the accessible name provided by Firefox is probably more meaningful than the child content. Screen readers can still decide if they actually want to use that information rather than child content, but we at least tell it that this is what we most likely found the most meaningful.

In the app.net example Eitan shows above, the child content for each list item is a unicode character showing a star or some other symbol for the re-post action. This is most likely not going to be spoken by most speech synthesizers. So I asked the folks at app.net to add an aria-label, to tell screen readers more meaningful text. You can think of it as sort of alternative text.

Especially aria-label would very unlikely be used arbitrarily, so it is always good to trust ourselves when we decide what to speak. The likelyhood that sub tree content is not the best for our users is very high if the attribute is present indicating that this name was given explicitly.

Additionally, there's acc.description. Description is *suplementary* content to the acc.name property. Firefox is smart enough to only provide a title if it is actually relevant. Example:

<a href="http://www.mozilla.org" title="Mozilla home"><img src="moz.png" alt="Mozilla home" /></a>

will result in a name for both the link and child content of "Mozilla home". acc.description would be empty, because the title of the link and alt of the image have the same content, and thus the title does not provide any meaningful additional information.

On the other hand:

<a href="http://www.mozilla.org" title="Link back to Mozilla start page"><img src="moz.png" alt="Mozilla home" /></a>

would result in the acc.name of the link being "Mozilla home", which corresponds to the child content AKA image's name, and acc.description of the link being the title's content "Back to Mozilla start page". Even though in this example, the information is not really suplementary, it *could* be, and thus we would fill the acc.description property with the title's content. Same goes for aria-describedby pointing to an ID of some text. that would always be mapped to any acc.description.

So to enhance AccessFu to speak the most meaningful information, we should:

a) See if we want to trust the explicit name and use that if present.
b) Use sub tree for the name.
c) Speak the role and states.
d) If present, speak the description content *after* the rest to give the suplemental information.

For d) there is a separate bug filed, wose number escapes me at the moment. Currently, AccessFu does not do anything yet for acc.description IIRC.

Hope this clarifies things!

Comment 17

[Marco Zehe (:MarcoZ)]

Comment on attachment 742423 [details] [diff] [review]
Tests for the patch v2

The changes I asked for look good. However since we still need to discuss a few things re the names and such, cancelling review for this particular patch. The refactoring and common code use are correct, tough.

Comment 18

[Eitan Isaacson [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #16)
> a) See if we want to trust the explicit name and use that if present.
> b) Use sub tree for the name.

The main clarification needed is, do those two steps need to be mutually exclusive? Could we add explicit names AND subtree names all the time?

Comment 19

[Marco Zehe (:MarcoZ)]

(In reply to Eitan Isaacson [:eeejay] from comment #18)
> (In reply to Marco Zehe (:MarcoZ) from comment #16)
> > a) See if we want to trust the explicit name and use that if present.
> > b) Use sub tree for the name.
> 
> The main clarification needed is, do those two steps need to be mutually
> exclusive? Could we add explicit names AND subtree names all the time?

No, we should not do that. It may lead to confusing announcements. If somebody uses aria-label, we can trust that they most likely do it on purpose. Or if the name was derived by some other means, see all possible cases in bug 637578, that lead to an explicit name, not one derived by the sub tree if the rule normally specified so. So if the name from sub tree rule applies to a particular element, like a link, but the name got associated explicitly through some means, I believe the sub tree should be ignored.

Comment 20

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

Attachment: Patch for 845870 v2

Comment 21

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

Attachment: Tests for the patch v3

Comment 22

[Marco Zehe (:MarcoZ)]

Comment on attachment 743458 [details] [diff] [review]
Tests for the patch v3

<+    <!-- app.net -->

You forgot to actually add the IDs of these test cases to the tests array above. ;-)

Also, it may seem a bit whacky to some people that you can add the title attribute to an h1 element, but I have seen this in the wild. So it is definitely good to test this here.

r=me with the app.net IDs added to the test array.

Comment 23

[Marco Zehe (:MarcoZ)]

Comment on attachment 743458 [details] [diff] [review]
Tests for the patch v3

Strike my earlier comment, all is fine and the code is actually being tested. my bad, sorry!

Comment 24

[Eitan Isaacson [:eeejay]]

Comment on attachment 743457 [details] [diff] [review]
Patch for 845870 v2

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

The main implementation flaw in my mind is the fact that when the vc is on an object with an explicit name - it ignores the subtree. But in the case where the explicit name is in the subtree we don't ignore the sub-subtree of that descendant.

The other issue is that I believe there are some cases where we should *not* ignore the subtree. The table cell in your test is one example. Basically, all container roles should have their subtree uttered. I guess I would like to get a clarification from Marco on that, I'll also do some testing on my own. But in any case, I would like the architectural possibility of changing the behavior on a per-role basis.

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +88,5 @@
>          UtteranceGenerator.genForObject(aAccessible));
>      };
> +    // If the name is explicit, always use it instead of the subtree utterance.
> +    let explicitName = Utils.getAttributes(
> +      aContext.accessible)['explicit-name'];

I think the attribute is a string, so this would evaluate as true even if the string was 'false'. I don't think it would ever have the value of 'false', but still...

Comment 25

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

(In reply to Eitan Isaacson [:eeejay] from comment #24)
> ::: accessible/src/jsat/UtteranceGenerator.jsm
> @@ +88,5 @@
> >          UtteranceGenerator.genForObject(aAccessible));
> >      };
> > +    // If the name is explicit, always use it instead of the subtree utterance.
> > +    let explicitName = Utils.getAttributes(
> > +      aContext.accessible)['explicit-name'];
> 
> I think the attribute is a string, so this would evaluate as true even if
> the string was 'false'. I don't think it would ever have the value of
> 'false', but still...

Makes sense, I'll explicitly check for 'true'.

Comment 26

[Eitan Isaacson [:eeejay]]

Comment on attachment 743458 [details] [diff] [review]
Tests for the patch v3

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

As far as the tests go, r=me. I still have open questions about the expected results, but I already aired those grievances in the previous review. So once the implementation is tweaked and the tests updated, this looks good to me.

I like the addition of utterance.js.

::: accessible/tests/mochitest/jsat/test_explicit_names.html
@@ +70,5 @@
> +      }, {
> +        // Test pivot to "apples" link from the table cell.
> +        accOrElmOrID: "apples",
> +        oldAccOrElmOrID: "cell",
> +        expected: ["List of Fruits", "list 4 items", "First item", "link", "Apples"]

What would happen if the links had titles? I think you would get both the explicit name and the text leaf name. It would not be consistent with what would happen if the vc were on the link.

@@ +75,5 @@
> +      }, {
> +        // Test pivot to the table cell from the "apples" link.
> +        accOrElmOrID: "cell",
> +        oldAccOrElmOrID: "apples",
> +        expected: ["List of Fruits"]

Sorry to re-open this issue, but this still looks wrong to me. I think this should also speak the contents of the cell, I know Marco's comments suggest otherwise.
Also this will not be consistent if the pivot lands on the cell's parent. It will speak everything under it (which is, in my mind, the desired result).

Comment 27

[Marco Zehe (:MarcoZ)]

There is a set of rules for varying roles where one of the rules says "calculate the name from the sub tree". This does not apply, for example, to group boxes (html:fieldset), where the name must be provided by a html:legend or an ARIA label or labelledby.

So, the sub tree, if the name has been provided via an explicit means, should *only* be pruned/ignored if the role calls for it.

I think what we should do here in AccessFu is mimic the behavior found in the rest of the accessibility module, and not prune the sub tree if the name has been provided via an explicit means, for roles whole rule calls for name from subtree calculation otherwise. Others should have thir sub trees and accessibles intact, and the name used by the rule: if explicit, yes, and the normal calculation otherwise. But elelements whose name won't be composed from the sub tree usually also don't need special handling, and name can be trusted. The ambiguity only comes with elements whose name can be constructed from the sub tree.

See this file for which role requires which name calculation rule:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/RoleMap.h

Comment 28

[Eitan Isaacson [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #27)
> There is a set of rules for varying roles where one of the rules says
> "calculate the name from the sub tree". This does not apply, for example, to
> group boxes (html:fieldset), where the name must be provided by a
> html:legend or an ARIA label or labelledby.
> 
> So, the sub tree, if the name has been provided via an explicit means,
> should *only* be pruned/ignored if the role calls for it.
> 
> I think what we should do here in AccessFu is mimic the behavior found in
> the rest of the accessibility module, and not prune the sub tree if the name
> has been provided via an explicit means, for roles whole rule calls for name
> from subtree calculation otherwise. Others should have thir sub trees and
> accessibles intact, and the name used by the rule: if explicit, yes, and the
> normal calculation otherwise. But elelements whose name won't be composed
> from the sub tree usually also don't need special handling, and name can be
> trusted. The ambiguity only comes with elements whose name can be
> constructed from the sub tree.
> 
> See this file for which role requires which name calculation rule:
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/RoleMap.h

Agreed. Back to the test cases above, it means that they should be fixed to utter the children of cells or groupings with labels.

Comment 29

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

Attachment: Patch for 845870 v3

Comment 30

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

Attachment: Tests for the patch v4

Comment 31

[Marco Zehe (:MarcoZ)]

Comment on attachment 749900 [details] [diff] [review]
Patch for 845870 v3

r=me, this looks right.

Comment 32

[Marco Zehe (:MarcoZ)]

Comment on attachment 749902 [details] [diff] [review]
Tests for the patch v4

Yes, this is what I think should be done. But Eitan should definitely also look at this in case I missed some case he thinks we should handle differently. But from my end, this looks correct.

Comment 33

[Eitan Isaacson [:eeejay]]

Comment on attachment 749902 [details] [diff] [review]
Tests for the patch v4

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

::: accessible/tests/mochitest/jsat/test_explicit_names.html
@@ +140,5 @@
> +    <!-- app.net -->
> +    <ul id="app.net" class="unstyled ul-horizontal yui3-u fixed-right ta-right" style="list-style-type: none;">
> +      <li class="yui3-u">
> +        <a href="#star" data-starred="0" data-star-button="1" data-post-id="5098826">
> +          <i aria-label="star" class="icon-star-empty"></i>

In the app.net case above, lets say this labeled element would have a text:
<i aria-label="star" class="icon-star-empty">Garbage</i>

The resulted utterance, would be (AFAICT):
["list 2 items", "First item", " ", "link", "star", "Garbage", "Last item", " ", "link", "repost"]

That is because the implementation only prunes the subtree of the pivot position, but it does not prune the subtrees of child items.

I am debating whether this is just good enough for now, and maybe it is worth landing. Pruning the subtree of child object is less than trivial and we would need to change the way the PivotContext caches subtrees, since we just get a flat list with no knowledge of the hierarchy. I think it could be possible with an internal (to PivotContext) tree format, and generator functions that take arguments on whether to ignore the children of the current item.

I think we should land this as-is, and open a new bug with a failing test case.

Comment 34

[Eitan Isaacson [:eeejay]]

Comment on attachment 749900 [details] [diff] [review]
Patch for 845870 v3

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

Once you address the issue below, this is good enough. We need to open a new bug with known failing test cases, and maybe a comment to that bug in this implementation.

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +204,5 @@
>      return [gStringBundle.GetStringFromName(
>                aIsEditing ? 'editingMode' : 'navigationMode')];
>    },
>  
> +  nameRuleMap: {

This should probably be merged with the verbosityRoleMap below since they are both role-bitfield maps.

Comment 35

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

Attachment: Patch for 845870 v4

Updated to the latest head.

Comment 36

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

Attachment: Tests for the patch v5

Carrying over the r+

Comment 37

[Eitan Isaacson [:eeejay]]

Comment on attachment 754530 [details] [diff] [review]
Patch for 845870 v4

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

Looking good.

Comment 38

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/556b4f13391b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2af88d919c

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/556b4f13391b
https://hg.mozilla.org/mozilla-central/rev/1e2af88d919c