758675 - [AccessFu] Speak accDescription if available

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

This is a very easy change. Just need to make a new link utterance function that uses the description, if it is available.

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: Patch

1. If the name is not retrieved and a description is available, it is being used unconditionally regardles of the INCLUDE_NAME flag. This is in assumption that the description only is generated if there is information there that the user *must* hear.

2. Moved the state check for STATE_TRAVERSED from the general function to the link utterance function, since STATE_TRAVERSED is only used by links, and we can save the check for all other elements that way.

Comment 2

[Eitan Isaacson [:eeejay]]

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

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

You should set up a build environment to test this with!

I am not certain what the correct way to have titled links spoken. Take this html for example:
<a href="http://monotonous.org" title="Eitan's blog">My blog</a>

So, now (before this patch) it would speak: "link, My blog".
After this patch (assuming it does what you expect) it would speak: "link, Eitan's blog, My blog".
I am thinking that since it is a "named link" maybe the name should come before the link? Something like: "Eitan's blog link, My blog".

Don't know. Note, in the current design, you can't opt out of speaking the links "name", since it will read the subtree in any case.

I am rejecting this patch on the account of it not being correct Javascript :)

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +200,5 @@
> +
> +      if (aFlags & INCLUDE_DESC) {
> +        let desc = this._getLocalizedStates(aStates);
> +        let roleStr = (aStates.base & Ci.nsIAccessibleStates.STATE_TRAVERSED) ?
> +                      gStringBundle.GetStringFromName('stateTraversed') + ' ' : '';

This doesn't really save us much by taking it out of the _getLocalizedStates() method. The only advantage I see is if we want to see "visited link" be localizable atomically, like we did with multiline edit boxes. Since this patch does not do that, I would suggest taking it out for now.

Although, I think that would be a nice future enhancement.

@@ +201,5 @@
> +      if (aFlags & INCLUDE_DESC) {
> +        let desc = this._getLocalizedStates(aStates);
> +        let roleStr = (aStates.base & Ci.nsIAccessibleStates.STATE_TRAVERSED) ?
> +                      gStringBundle.GetStringFromName('stateTraversed') + ' ' : '';
> +        let roleStr = roleStr + this._getLocalizedRole(aRoleStr);

You can't redeclare a variable twice. In other words you can't do "let" twice.

@@ +207,5 @@
> +          desc.push(roleStr);
> +        utterance.push(desc.join(' '));
> +      }
> +
> +      let name = (aFlags & INCLUDE_NAME) ? (aAccessible.name || '') : '';

We are never interested in link names, since they are just a concatenation of all the children names, which we speak now anyway.

@@ +208,5 @@
> +        utterance.push(desc.join(' '));
> +      }
> +
> +      let name = (aFlags & INCLUDE_NAME) ? (aAccessible.name || '') : '';
> +      let name = (!name) ? (aAccessible.description || '') : '';

Again, you redeclared "name".

Comment 3

[Marco Zehe (:MarcoZ)]

Thanks, Eitan! Silly me, I totally ignored the fact that "let" is a declarative, not an assigning, keyword!

I thought that pulling the check for STATE_TRAVERSED would save us quite a number of cycles when navigating, since we don't have to re-check the STATE_TRAVERSED on something that would never use it. Again, STATE_TRAVERSED will only appear on link accessibles.

As for your question:

(In reply to Eitan Isaacson [:eeejay] from comment #2)
> So, now (before this patch) it would speak: "link, My blog".
> After this patch (assuming it does what you expect) it would speak: "link,
> Eitan's blog, My blog".
> I am thinking that since it is a "named link" maybe the name should come
> before the link? Something like: "Eitan's blog link, My blog".

Yes, that's the intended behavior. I guess I'm not sure in what order one has to push these things onto the utterance for TalkBack to read them in correct order. If I understood you correctly, I should simply push the description first, and then simply do the rest as in the defaultFunc, right?

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #3)
> Thanks, Eitan! Silly me, I totally ignored the fact that "let" is a
> declarative, not an assigning, keyword!
> 
> I thought that pulling the check for STATE_TRAVERSED would save us quite a
> number of cycles when navigating, since we don't have to re-check the
> STATE_TRAVERSED on something that would never use it. Again, STATE_TRAVERSED
> will only appear on link accessibles.
> 

It is a simple bit operation. I don't think it will give us any real performance gains.

> As for your question:
> 
> (In reply to Eitan Isaacson [:eeejay] from comment #2)
> > So, now (before this patch) it would speak: "link, My blog".
> > After this patch (assuming it does what you expect) it would speak: "link,
> > Eitan's blog, My blog".
> > I am thinking that since it is a "named link" maybe the name should come
> > before the link? Something like: "Eitan's blog link, My blog".
> 
> Yes, that's the intended behavior. I guess I'm not sure in what order one
> has to push these things onto the utterance for TalkBack to read them in
> correct order. If I understood you correctly, I should simply push the
> description first, and then simply do the rest as in the defaultFunc, right?

I would regard the link title as part of the description fragment (where the role and state strings are assigned. So the output of the functions would be something like:
['visited Eitan's blog link'], with no second "name" string.

Essentially do everything like defaultFunc, but leave out the name bit, and add the description in between the states and role name.

Comment 5

[Marco Zehe (:MarcoZ)]

Attachment: Patch2

As you suggested.

Comment 6

[Eitan Isaacson [:eeejay]]

Comment on attachment 629188 [details] [diff] [review]
Patch2

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

r=me

This Javascript works!

Just a small caution, I am testing this and it gets verbose and tiring quickly on wordpress sites that redundantly title links. So we may want to visit this again for usability. But it works good enough for now.

Comment 7

[Eitan Isaacson [:eeejay]]

Marco,

This patch is good for landing. I'll leave it up to you if this is too verbose or not. If you think it is worth landing, go ahead, if not we could shelve it for now.

Comment 8

[Marco Zehe (:MarcoZ)]

Considering that title is widely misused on the web, I think we should shelve it until we have refactored the utterances to push descriptions as the very last thing spoken.

Comment 9

[Marco Zehe (:MarcoZ)]

After bug 845870 lands, it is worth revisiting this to speak accessible descriptions if they have content after the name, role and states. It should be investigated if we should do this for current objects, or the whole ancestry, too. An example of how this works can be seen in NVDA when tabbing through elements on a page, and some may have accDescriptions.

Changing summary to reflect this change of intentions for this bug.

Comment 10

[Marco Zehe (:MarcoZ)]

Comment on attachment 629188 [details] [diff] [review]
Patch2

This patch os now obsolete, and a new patch more aligned to the new bug purpose will be posted after bug 845870.

Comment 11

[Marco Zehe (:MarcoZ)]

Attachment: Speak accessible description if available,

This patch asks for the description. If one is available, it asks for the name even if flags specify the name should be spoken from the sub tree. Then, it compares name and description, and if they do not match, appends the description as part of the name, making it an addendum in parentheses. My question is: Is there a more elegant way to do this?

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 778472 [details] [diff] [review]
Speak accessible description if available,

See comment above for an explanation of what the patch does, and my thinking behind it.

Comment 13

[Marco Zehe (:MarcoZ)]

So if I have:

<a href="http://mozilla.org" title="Brings you back to the Mozilla home page">Mozilla</a>

The name would be "Mozilla" before this patch, and "Mozilla (Brings you back to the Mozilla home page)" after this patch.

However, if it said:

<a href="http://www.mozilla.org" title="Mozilla">Mozilla</a>

the name would still be "Mozilla", because inner text and title (which gets converted to the description) would be identical, thus the information given would be superflous. However, since web authors too often duplicate the link text in the title, see at http://heise.de/newsticker, we need to suppress duplicated title text to not drive our users crazy.

Comment 14

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

Comment on attachment 778472 [details] [diff] [review]
Speak accessible description if available,

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

Looks really nice :) Would you be able to add some tests as well ?

::: accessible/src/jsat/OutputGenerator.jsm
@@ +163,5 @@
>        (aFlags & INCLUDE_NAME)) {
>        name = aAccessible.name;
>      }
>  
> +    let description = aAccessible.description;

You can put the check for description before the original check for the explicit name or flag. This way it will look a little simpler:

let name;
let description = aAccessible.description;

if (description) {
  let accessibleName = aAccessible.name;
  if (accessibleName && (description !== accessibleName)) {
    name = accessibleName + ' (' + description + ')';
  }
} else if (
  Utils.getAttributes(aAccessible)['explicit-name'] === 'true' ||
  (aFlags & INCLUDE_NAME)) {
  name = aAccessible.name;
}

Comment 15

[Marco Zehe (:MarcoZ)]

If I do it this way, I will *always* use the explicit name, thereby defeating the purpose of the name from subtree rule.

Comment 16

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

Comment on attachment 778472 [details] [diff] [review]
Speak accessible description if available,

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

::: accessible/src/jsat/OutputGenerator.jsm
@@ +167,5 @@
> +    let description = aAccessible.description;
> +    if (description) {
> +      // Compare against the calculated name unconditionally, regardless of name rule,
> +      // so we can make sure we don't speak duplicated descriptions
> +      let tmpName = name ? name : aAccessible.name;

let tmpName = name || aAccessible.name;

@@ +169,5 @@
> +      // Compare against the calculated name unconditionally, regardless of name rule,
> +      // so we can make sure we don't speak duplicated descriptions
> +      let tmpName = name ? name : aAccessible.name;
> +      if (tmpName && (description !== tmpName)) {
> +        name = name ? name : '';

name = name || '';

@@ +170,5 @@
> +      // so we can make sure we don't speak duplicated descriptions
> +      let tmpName = name ? name : aAccessible.name;
> +      if (tmpName && (description !== tmpName)) {
> +        name = name ? name : '';
> +        name = name + ' (' + description + ')';

name += ' (' + description + ')';

Comment 17

[Marco Zehe (:MarcoZ)]

Attachment: Speak accessible description if available,

Addressed Yura's suggestions and added tests.

Comment 18

[Eitan Isaacson [:eeejay]]

Comment on attachment 779103 [details] [diff] [review]
Speak accessible description if available,

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

r+ with some suggested changes below.

::: accessible/src/jsat/OutputGenerator.jsm
@@ +170,5 @@
> +      // so we can make sure we don't speak duplicated descriptions
> +      let tmpName = name || aAccessible.name;
> +      if (tmpName && (description !== tmpName)) {
> +        name = name || '';
> +        name += ' (' + description + ')';

Some synth engines would speak the parenthesis, no? that seems chatty and annoying. Also, I would either append or prepend, depending on utterance order.

Comment 19

[Marco Zehe (:MarcoZ)]

OK, I've changed the separator to an " - " instead of putting the description in parentheses. This may still be spoken by some synths, and it is good that we have an indicator for the separation, but it is less intrusive than parentheses at the beginning and end of the accDescription.

Comment 20

[Marco Zehe (:MarcoZ)]

Pushed with comments addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf53001aa2c7

Comment 21

[Ed Morley [:emorley]]

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