Closed Bug 758675 Opened 7 years ago Closed 6 years ago

[AccessFu] Speak accDescription if available

Categories

(Core :: Disability Access APIs, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: eeejay, Assigned: MarcoZ)

References

Details

Attachments

(1 file, 3 obsolete files)

This is a very easy change. Just need to make a new link utterance function that uses the description, if it is available.
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → marco.zehe
Status: NEW → ASSIGNED
Attachment #628690 - Flags: review?(eitan)
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".
Attachment #628690 - Flags: review?(eitan) → review-
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?
(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.
Attached patch Patch2 (obsolete) — Splinter Review
As you suggested.
Attachment #628690 - Attachment is obsolete: true
Attachment #629188 - Flags: review?(eitan)
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.
Attachment #629188 - Flags: review?(eitan) → review+
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.
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.
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.
Depends on: 845870
Summary: [AccessFu] Speak link title → [AccessFu] Speak accDescription if available
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.
Attachment #629188 - Attachment is obsolete: true
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 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.
Attachment #778472 - Flags: feedback?(yura.zenevich)
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 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;
}
Attachment #778472 - Flags: feedback?(yura.zenevich) → feedback+
If I do it this way, I will *always* use the explicit name, thereby defeating the purpose of the name from subtree rule.
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 + ')';
Addressed Yura's suggestions and added tests.
Attachment #779103 - Flags: review?(eitan)
Attachment #778472 - Attachment is obsolete: true
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.
Attachment #779103 - Flags: review?(eitan) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/bf53001aa2c7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.