876475 - [AccessFu] Make braille output less verbose

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Max Li [:maxli]]

Right now, we give the same amount of context information as in speech.

It would be good to do this to make best use of available space.

Comment 1

[Max Li [:maxli]]

Attachment: WIP

Here's a quick proof of concept. There's many things wrong with the patch as is code-wise, but I think the behaviour is quite decent.

Comment 2

[Eitan Isaacson [:eeejay]]

This patch pulls in some google java files that are used for interfacing with the selfbraille service provided by braillback. Obviously we would need to integrate it into our tree in a way that keeps its license intact.

Here is the Android project:
http://code.google.com/p/eyes-free/source/browse/#svn%2Ftrunk%2Fbraille%2Fclient%253Fstate%253Dclosed

And here are the source files that need to be included:
http://code.google.com/p/eyes-free/source/browse/#svn%2Ftrunk%2Fbraille%2Fclient%2Fsrc%2Fcom%2Fgooglecode%2Feyesfree%2Fbraille%2Fselfbraille

Brad, do you know how/if we could go about integrating those files into our tree? The advantage is having solid braille support is obvious. We are going to need a braille solution at some point in b2g as well, so this seems like a good investment for us. And it will give our Android users an added benefit.

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to Max Li [:maxli] from comment #1)
> Created attachment 754744 [details] [diff] [review]
> WIP
> 
> Here's a quick proof of concept. There's many things wrong with the patch as
> is code-wise, but I think the behaviour is quite decent.

Max, did you test this with different braille translation tables like US Grade 2? Does it affect our output? I hope it does. It is worth also seeing if Chrome gets different output as well depending on translation table.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

good news, MPL3 is compatible with Apache. You can just include those files with the original license headers in a patch.

CC'ing gerv to make sure I'm not lying to you.

Comment 5

[Max Li [:maxli]]

(In reply to Eitan Isaacson [:eeejay] from comment #3)
> Max, did you test this with different braille translation tables like US
> Grade 2? Does it affect our output? I hope it does. It is worth also seeing
> if Chrome gets different output as well depending on translation table.

Brailleback handles the different translation tables for us.

Comment 6

[Gervase Markham [:gerv]]

Yes, you may certainly include entire files licensed under the Apache License 2.0 in the Mozilla tree with no need to adjust about:license.

However, if you want to copy code from Apache files into MPLed files or vice versa, you need to consult us.

Gerv

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 754744 [details] [diff] [review]
WIP

OK, so if I understand this correctly, we're setting ourselves as a self-brailling service so BrailleBack accepts from us what we want to braille. ut this patch does not yet include alternative text itself for control types such as links etc., correct?

Comment 8

[Max Li [:maxli]]

(In reply to Marco Zehe (:MarcoZ) from comment #7)
> Comment on attachment 754744 [details] [diff] [review]
> WIP
> 
> OK, so if I understand this correctly, we're setting ourselves as a
> self-brailling service so BrailleBack accepts from us what we want to
> braille. ut this patch does not yet include alternative text itself for
> control types such as links etc., correct?

Yes, that is correct.

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 754744 [details] [diff] [review]
WIP

f=me, as this is needed to enable us to braille separate strings than speak. Since we'll also need this for Firefox OS (of course except the Android part), this lays a foundation.

Comment 10

[Max Li [:maxli]]

Attachment: WIP v2

So here's an updated WIP. It now does shortening of (certain) role names and also now has all previous functionality of the non-self-brailling solution. 

A lot of functionality is still borrowed from the UtteranceGenerator, which makes the code kind of ugly. Thoughts?

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 757191 [details] [diff] [review]
WIP v2

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

I see what you mean about duplicating a lot of the utterance generator. I think ideally, we would have an utterance and braille generator, with a common base class. The module could be called OutputGenerator or something like that. Just like speech, you may want to do custom things for different roles, so a function mapping could be useful like we do now in UtteranceGenerator. There is a lot of js wizardry that could make this interesting!

Luckily, we have some test coverage for the utterance generator, so refactoring and playing with it should be possible.

Marco should look over the abbreviated roles. I think there are too many, and the braille output should only contain roles (or state indicators) of actionable items. Along with some formatting helpers, like asterisks in list items.

::: accessible/src/jsat/AccessFu.jsm
@@ +460,5 @@
>    Haptic: function Haptic(aDetails, aBrowser) {
>      Utils.win.navigator.vibrate(aDetails.pattern);
>    },
>  
> +  Braille: function Braille(aDetails, aBrowser) {

Throw in a debug output here, could be interesting.

::: accessible/src/jsat/Presentation.jsm
@@ -226,5 @@
>                          clickable: aContext.accessible.actionCount > 0,
> -                        checkable: !!(state &
> -                                      Ci.nsIAccessibleStates.STATE_CHECKABLE),
> -                        checked: !!(state &
> -                                    Ci.nsIAccessibleStates.STATE_CHECKED)});

Do these need to be removed for this to work? If not, it is worth keeping so that other random ATs will do the right thing.

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +23,5 @@
>  var gStringBundle = Cc['@mozilla.org/intl/stringbundle;1'].
>    getService(Ci.nsIStringBundleService).
>    createBundle('chrome://global/locale/AccessFu.properties');
>  
> +this.EXPORTED_SYMBOLS = ['UtteranceGenerator', 'gStringBundle'];

That is obviously not great.

::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
@@ +132,5 @@
>  quicknav_Separator   = Separators
>  quicknav_Table       = Tables
> +quicknav_Checkbox    = Check boxes
> +
> +# Shortened role names

This looks good. Mention braille here, "Shortened role names for braille".

@@ +133,5 @@
>  quicknav_Table       = Tables
> +quicknav_Checkbox    = Check boxes
> +
> +# Shortened role names
> +listAbbr           =       lst

I think this is excessive for braille.

@@ +134,5 @@
> +quicknav_Checkbox    = Check boxes
> +
> +# Shortened role names
> +listAbbr           =       lst
> +listitemAbbr       =       lstitm

Instead of lstitm, there could be an asterisk before the list item.

@@ +137,5 @@
> +listAbbr           =       lst
> +listitemAbbr       =       lstitm
> +linkAbbr           =       lnk
> +pushbuttonAbbr     =       btn
> +checkbuttonAbbr    =       chkbtn

This would have a state indicator, and should probably not have a role.

@@ +138,5 @@
> +listitemAbbr       =       lstitm
> +linkAbbr           =       lnk
> +pushbuttonAbbr     =       btn
> +checkbuttonAbbr    =       chkbtn
> +radiobuttonAbbr    =       rdiobtn

Same, although the state indicator might be different, maybe a square bracket?

@@ +140,5 @@
> +pushbuttonAbbr     =       btn
> +checkbuttonAbbr    =       chkbtn
> +radiobuttonAbbr    =       rdiobtn
> +passwordtextAbbr   =       passwdtxt
> +togglebuttonAbbr   =       togglebtn

No role, same state indicator as a check button.

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 757191 [details] [diff] [review]
WIP v2

>+listitemAbbr       =       lstitm

This should definitely not preceede every list item. Instead, the bullet or numbering accessibles should be sufficient to do the trick. And if none is given because the list stype doesn't give them in the first place, that's OK.

>+definitionAbbr     =       defn

Similar thing is true for this. Don't think this is needed and only wastes space.

The others look OK.

Comment 13

[Max Li [:maxli]]

Attachment: v3

Comment 14

[Eitan Isaacson [:eeejay]]

Comment on attachment 760239 [details] [diff] [review]
v3

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

Overall this looks like the right way forward. When it is ready for review, flag both yzen and I. He has most recently done work on the utterance generator, so he could pick stuff up.

::: accessible/src/jsat/Presentation.jsm
@@ +221,5 @@
>  
> +    if (!this._braillePresenter)
> +      this._braillePresenter = new BraillePresenter();
> +    let brailleText = this._braillePresenter.pivotChanged(aContext, aReason).
> +                           details.text;

Probably worth checking for jelly bean, don't need this in ICS and GB.

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +466,5 @@
> +
> +      if (aAccessible.indexInParent === 1 &&
> +          aAccessible.parent.role == Ci.nsIAccessibleRole.ROLE_LISTITEM &&
> +          aAccessible.previousSibling.role == Ci.nsIAccessibleRole.ROLE_STATICTEXT) {
> +        if (aAccessible.parent.parent.DOMNode.nodeName == 'UL')

This breaks if there is no grandparent or DOMNode for grandparent. I am not sure in what case that should be, but it is still worth being safe here.

@@ +486,5 @@
> +
> +
> +    statictext: function statictext(aAccessible, aRoleStr, aStates, aFlags) {
> +      if (aAccessible.parent.role == Ci.nsIAccessibleRole.ROLE_LISTITEM) {
> +        return [];

Add a comment explaining why you do this.

Comment 15

[Max Li [:maxli]]

Attachment: Patch

Comment 16

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

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

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

Looks good!

It'd be nice if braille.js and utterance.js (given that they are the same) were condensed into one file. Perhaps output.js?

::: accessible/src/jsat/OutputGenerator.jsm
@@ +28,2 @@
>  
> +function OutputGenerator() {}

I believe you can avoid making a constructor and defining prototype in favor of a plain object literal:
this.OutputGenerator = {...};
this.UtteranceGenerator = {
  __proto__: OutputGenerator,
  ...
};
...

@@ +450,5 @@
> +
> +this.BrailleGenerator = {
> +  __proto__: OutputGenerator.prototype,
> +
> +  defaultOutputOrder: 1,

This should probably be defined as a constant:

const OUTPUT_DESC_LAST = 1;

@@ +465,5 @@
> +        braille.push(desc.join(' '));
> +      }
> +
> +      this._addName(braille, aAccessible, aFlags);
> +

The block above is common with the defaultFunc for the UtteranceGenerator. Perhaps it could be taken out as a separate function?

@@ +471,5 @@
> +          aAccessible.parent.role == Ci.nsIAccessibleRole.ROLE_LISTITEM &&
> +          aAccessible.previousSibling.role == Ci.nsIAccessibleRole.ROLE_STATICTEXT) {
> +        if (aAccessible.parent.parent && aAccessible.parent.parent.DOMNode &&
> +            aAccessible.parent.parent.DOMNode.nodeName == 'UL')
> +          braille.unshift('*');

NIT: Needs to be wrapped in {}.

@@ +473,5 @@
> +        if (aAccessible.parent.parent && aAccessible.parent.parent.DOMNode &&
> +            aAccessible.parent.parent.DOMNode.nodeName == 'UL')
> +          braille.unshift('*');
> +        else
> +          braille.unshift(aAccessible.previousSibling.name);

NIT: Needs to be wrapped in {}.

@@ +520,5 @@
> +    togglebutton: function radiobutton(aAccessible, aRoleStr, aStates, aFlags) {
> +      return this.objectOutputFunctions._useStateNotRole.apply(this, arguments);
> +    },
> +
> +    entry: function entry(aAccessible, aRoleStr, aStates, aFlags) {

This method is identical to the one in the UtteranceGenearator.

::: accessible/src/jsat/Presentation.jsm
@@ +9,5 @@
>  const Cu = Components.utils;
>  const Cr = Components.results;
>  
>  Cu.import('resource://gre/modules/accessibility/Utils.jsm');
> +Cu.import('resource://gre/modules/accessibility/OutputGenerator.jsm');

Feel free to load modules lazily.

@@ +220,5 @@
>      let state = Utils.getStates(aContext.accessible)[0];
>  
> +    let brailleText = '';
> +    if (Utils.AndroidSdkVersion >= 16) {
> +      if (!this._braillePresenter)

NIT: This needs to be wrapped in {}.

@@ +387,5 @@
> +
> +  type: 'Braille',
> +
> +  pivotChanged: function BraillePresenter_pivotChanged(aContext, aReason) {
> +    if (!aContext.accessible)

NIT: this needs to wrapped in {}.

Comment 17

[Max Li [:maxli]]

Attachment: Patch v2

Comment 18

[Eitan Isaacson [:eeejay]]

Comment on attachment 761451 [details] [diff] [review]
Patch v2

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

A few question:
What API version do the braille additions depend on?
I am assuming this will build fine, but will it crash on pre-JB devices?
Also, what if BrailleBack is not installed?
Does activating nodes "just work"?
Maybe all the google braille java files should be moved to com/googlecode/eyesfree/braille/selfbraille?

Worth sending to try to see what breaks.

::: accessible/tests/mochitest/jsat/test_braille.html
@@ +88,5 @@
> +      <a href="example.com" id="link">Link</a>
> +      <button id="button">I am a button</button>
> +      <label for="password_input">Secret Password</label><input id="password_input" type="password"></input>
> +      <label for="checkbox_unchecked">checkboxtext</label><input id="checkbox_unchecked" type="checkbox"></input>
> +      <label for="checkbox_checked">checked</label><input id="checkbox_checked" type="checkbox" checked></input>

Change the name from "checked", that is confusing and overlaps with utterances.

::: accessible/tests/mochitest/jsat/test_explicit_names.html
@@ +86,5 @@
>            "Apples", "link", "Bananas", "link", "Peaches", "Last item", "link",
>            "Plums"]
>        }];
>  
> +      SpecialPowers.setIntPref(PREF_UTTERANCE_ORDER, 0);

Why was this addition necessary?

::: mobile/android/base/GeckoAccessibility.java
@@ +71,5 @@
>                              if (sEnabled)
>                                  break;
>                          }
> +                        if (sEnabled && sSelfBrailleClient == null)
> +                            sSelfBrailleClient = new SelfBrailleClient(GeckoAppShell.getContext(), true); // XXX

XXX?
This should not get instantiated on pre-JB. Also, what happens if BrailleBack is not installed?

::: mobile/android/base/Makefile.in
@@ +1179,5 @@
>  	@echo "MKDIR jars"
>  	$(NSINSTALL) -D jars
>  
> +aidl:
> +	$(ANDROID_PLATFORM_TOOLS)/aidl -I$(addprefix $(srcdir)/,braille) $(addprefix $(srcdir)/,braille/com/googlecode/eyesfree/braille/selfbraille/ISelfBrailleService.aidl)

You probably should do this with a static pattern[1].
Something like:
$(AIDL_AUTOGEN_FILES): %.java: %.aidl

1. http://www.gnu.org/software/make/manual/make.html#Static-Pattern

Comment 19

[Max Li [:maxli]]

> What API version do the braille additions depend on?

I'm not too sure what you mean by this. Brailleback only works on Jellybean, so I guess 16?

> I am assuming this will build fine, but will it crash on pre-JB devices?

Doesn't seem to crash on Gingerbread.

> Also, what if BrailleBack is not installed?

Then the SelfBrailleClient will fail to bind to the SelfBrailleService and it will fail gracefully.

> Does activating nodes "just work"?

Well, besides the ACTION_NEXT_AT_MOVEMENT_GRANULARITY ugliness, yes.

> Maybe all the google braille java files should be moved to
> com/googlecode/eyesfree/braille/selfbraille?

Sure.

> Why was this addition necessary?

I'm not too sure why, but the tests were failing otherwise because of the ordering being wrong.

> XXX?
> This should not get instantiated on pre-JB. Also, what happens if
> BrailleBack is not installed?

Oops. That was for debugging purposes and I forgot to change it back. (The value of true allows the use of a debug version of brailleback.)

Comment 20

[Max Li [:maxli]]

Attachment: Patch v3

Comment 21

[Max Li [:maxli]]

Attachment: Patch v3.1

Now actually applies onto the latest m-c.

Also tested on ICS now too. No crashes or otherwise odd behaviour.

Comment 22

[Eitan Isaacson [:eeejay]]

Comment on attachment 762061 [details] [diff] [review]
Patch v3.1

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

This looks good to me. Lets get kat's review for the Java bits.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 762061 [details] [diff] [review]
Patch v3.1

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

I don't fully understand what the code is doing but the Java bits look fine to me. Some comments below. Also I think you should get a build system peer to sign off on the Makefile.in changes, because it's the first .aidl file conversion in the mobile code and I don't know if that will interfere with the moz.build changes going on.

::: mobile/android/base/GeckoAccessibility.java
@@ +72,5 @@
>                                  break;
>                          }
> +                        if (sEnabled && sSelfBrailleClient == null &&
> +                            Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN)
> +                            sSelfBrailleClient = new SelfBrailleClient(GeckoAppShell.getContext(), false);

Please add braces even for single-line if bodies.

@@ +228,5 @@
>  
>          }
>      }
>  
> +    private static void sendBrailleText (final View view, final String text) {

nit: no space before "("

::: mobile/android/base/braille/com/googlecode/eyesfree/braille/selfbraille/WriteData.aidl
@@ +15,5 @@
> + */
> +
> +package com.googlecode.eyesfree.braille.selfbraille;
> +
> +parcelable WriteData;

Does this file need to be checked in? It's not used in the Makefile; it looks like the WriteData.java file was generated from it and then modified and now we're just using that directly. So we probably don't need this.

Comment 24

[Max Li [:maxli]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> Does this file need to be checked in? It's not used in the Makefile; it
> looks like the WriteData.java file was generated from it and then modified
> and now we're just using that directly. So we probably don't need this.

Yes. The ISelfBrailleService.aidl imports WriteData, which means the aidl tool needs WriteData.aidl to be there or it would otherwise raise an error.

Comment 25

[Max Li [:maxli]]

Attachment: Patch v3.1 with nits fixed

Comment 26

[Kartikaya Gupta (email:[e-mail])]

(In reply to Max Li [:maxli] from comment #24)
> Yes. The ISelfBrailleService.aidl imports WriteData, which means the aidl
> tool needs WriteData.aidl to be there or it would otherwise raise an error.

Ah, I missed that. Thanks for the explanation!

Comment 27

[(not currently active) Ted Mielczarek]

Comment on attachment 762166 [details] [diff] [review]
Patch v3.1 with nits fixed

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

The build bits look fine, just one nit and a question.

::: mobile/android/base/Makefile.in
@@ +1219,5 @@
>  jars:
>  	@echo "MKDIR jars"
>  	$(NSINSTALL) -D jars
>  
> +$(AIDL_AUTOGEN_FILES): %.java: %.aidl

We are on a mission to move custom rules out of Makefiles, but this Makefile is already sort of a mess, so I guess this doesn't make it much worse.

@@ +1220,5 @@
>  	@echo "MKDIR jars"
>  	$(NSINSTALL) -D jars
>  
> +$(AIDL_AUTOGEN_FILES): %.java: %.aidl
> +	$(ANDROID_PLATFORM_TOOLS)/aidl -I$(addprefix $(srcdir)/,braille) $<

You don't really need the addprefix, with only one argument in there it's equivalent to just writing "$(srcdir)/braille".

Do we know that aidl is always in this directory across SDK versions? We've had some issues with other tools changing locations in newer SDK versions.

Comment 28

[Max Li [:maxli]]

> Do we know that aidl is always in this directory across SDK versions? We've
> had some issues with other tools changing locations in newer SDK versions.

It seems it's always been there.

Comment 29

[Max Li [:maxli]]

Attachment: Patch v4

Comment 30

[Marco Zehe (:MarcoZ)]

Pushed on MaxLi's behalf: https://hg.mozilla.org/integration/mozilla-inbound/rev/459376ae11c7

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/459376ae11c7

Comment 32

[cajbir (:cajbir)]

This patch landing has broken my Android builds:

JAR gecko-browser.jar
/src/mozilla/android/android/mobile/android/base/braille/com/googlecode/eyesfree/braille/selfbraille/ISelfBrailleService.java:30: warning: [cast] redundant cast to android.os.IInterface
android.os.IInterface iin = (android.os.IInterface)obj.queryLocalInterface(DESCRIPTOR);
                            ^
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
1 warning
make[6]: *** [jars/gecko-browser.jar] Error 1

That warning results in the build failing. Reverting the patch landing lets things work. Do I have some configuration setting wrong?

Comment 33

[Marco Zehe (:MarcoZ)]

Hi Chris, works fine for me. OS X 10.8.4 with latest Xcode, building against Android NDK R8D, and SDK-16. I haven't changed this configuration in quite a while, and adding the patch for this bug didn't break me.

Comment 34

[Francesco Lodolo [:flod]]

Any advice on how localizers should work with these abbreviations? TBH I don't understand if I'm supposed to localize them and how, if there are some references out there I could use, etc.

Comment 35

[Marco Zehe (:MarcoZ)]

My best bet would be the translation of the open source screen reader NVDA at http://nvda-project.org. There is a way to get the source from Github, and in there are all the localizations. These include braille abbreviations as well. And if everything else fails, try to make a sensible abbreviation out of the word used in the section above in the same .properties file for speech.

Comment 36

[Francesco Lodolo [:flod]]

Thanks Marco, this helps a lot. I also think it would be great adding this explanation directly to the .properties file.

Comment 37

[Kartikaya Gupta (email:[e-mail])]

(In reply to Marco Zehe (:MarcoZ) from comment #33)
> Hi Chris, works fine for me. OS X 10.8.4 with latest Xcode, building against
> Android NDK R8D, and SDK-16. I haven't changed this configuration in quite a
> while, and adding the patch for this bug didn't break me.

This is likely due to the java compiler version in use. Java 7's compiler is more strict. It would be a good idea to remove the redundant cast pointed out in the warning message.

Comment 38

[Marco Zehe (:MarcoZ)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> This is likely due to the java compiler version in use. Java 7's compiler is
> more strict. It would be a good idea to remove the redundant cast pointed
> out in the warning message.

I filed bug 885298 to get this fixed.