Closed
Bug 876475
Opened 11 years ago
Closed 11 years ago
[AccessFu] Make braille output less verbose
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: maxli, Assigned: maxli)
References
Details
Attachments
(1 file, 8 obsolete files)
70.90 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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.
Attachment #754744 -
Flags: feedback?(marco.zehe)
Comment 2•11 years ago
|
||
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.
Flags: needinfo?(blassey.bugs)
Comment 3•11 years ago
|
||
(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•11 years ago
|
||
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.
Flags: needinfo?(blassey.bugs)
Assignee | ||
Comment 5•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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?
Assignee | ||
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
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.
Attachment #754744 -
Flags: feedback?(marco.zehe) → feedback+
Assignee | ||
Comment 10•11 years ago
|
||
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?
Attachment #754744 -
Attachment is obsolete: true
Attachment #757191 -
Flags: feedback?(eitan)
Comment 11•11 years ago
|
||
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.
Attachment #757191 -
Flags: feedback?(eitan)
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #757191 -
Attachment is obsolete: true
Attachment #760239 -
Flags: feedback?(eitan)
Comment 14•11 years ago
|
||
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.
Attachment #760239 -
Flags: feedback?(eitan) → feedback+
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #760239 -
Attachment is obsolete: true
Attachment #760936 -
Flags: review?(eitan)
Assignee | ||
Updated•11 years ago
|
Attachment #760936 -
Flags: review?(yura.zenevich)
Comment 16•11 years ago
|
||
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 {}.
Attachment #760936 -
Flags: review?(yura.zenevich)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #760936 -
Attachment is obsolete: true
Attachment #760936 -
Flags: review?(eitan)
Attachment #761451 -
Flags: review?(eitan)
Assignee | ||
Updated•11 years ago
|
Attachment #761451 -
Flags: review?(yura.zenevich)
Comment 18•11 years ago
|
||
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
Attachment #761451 -
Flags: review?(yura.zenevich)
Assignee | ||
Comment 19•11 years ago
|
||
> 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.)
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #761451 -
Attachment is obsolete: true
Attachment #761451 -
Flags: review?(eitan)
Attachment #761797 -
Flags: review?(eitan)
Assignee | ||
Comment 21•11 years ago
|
||
Now actually applies onto the latest m-c. Also tested on ICS now too. No crashes or otherwise odd behaviour.
Attachment #761797 -
Attachment is obsolete: true
Attachment #761797 -
Flags: review?(eitan)
Attachment #762061 -
Flags: review?(eitan)
Comment 22•11 years ago
|
||
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.
Attachment #762061 -
Flags: review?(eitan)
Attachment #762061 -
Flags: review?(bugmail.mozilla)
Attachment #762061 -
Flags: review+
Comment 23•11 years ago
|
||
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.
Attachment #762061 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 24•11 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 26•11 years ago
|
||
(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!
Assignee | ||
Updated•11 years ago
|
Attachment #762166 -
Flags: review?(ted)
Comment 27•11 years ago
|
||
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.
Attachment #762166 -
Flags: review?(ted) → review+
Assignee | ||
Comment 28•11 years ago
|
||
> 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 30•11 years ago
|
||
Pushed on MaxLi's behalf: https://hg.mozilla.org/integration/mozilla-inbound/rev/459376ae11c7
Comment 31•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/459376ae11c7
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 32•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Thanks Marco, this helps a lot. I also think it would be great adding this explanation directly to the .properties file.
Comment 37•11 years ago
|
||
(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•11 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•