739498 - Add Javascript layer for mobile accessibility

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

In mobile we are doing more than simple API support - we are driving the AT. These modules introduce some new user-facing core features for mobile browsing, like virtual cursor control and display. It could be run from desktop Firefox as well (without any useful speech output) by doing in Scratchpad:

Components.utils.import("resource://gre/modules/accessibility/MobileAccessibility.jsm");
MobileAccessibility.init(win);
MobileAccessibility.startup();

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Added Javascript modules for mobile accessibility.

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 609578 [details] [diff] [review]
Added Javascript modules for mobile accessibility.

Axel, you asked a while back to keep you in the loop on any localization we will be doing for mobile a11y. Well, this is it! And I don't think I did it completely right...

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 609578 [details] [diff] [review]
Added Javascript modules for mobile accessibility.

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

nice work!

it makes sense to keep files under jsAT folder so that we say they can be reused on desktop and have Android presenter iif Android platform is detected

general notice: you do 
gAccRetrieval = Cc['@mozilla.org/accessibleRetrieval;1']
  .getService(Ci.nsIAccessibleRetrieval);

but usually we do

gAccRetrieval = Cc['@mozilla.org/accessibleRetrieval;1'].
  getService(Ci.nsIAccessibleRetrieval);

following the rule to not keep operators at new line. Does this style goes with coding style guide?

also you do
name: function name() {
}
but we do
name: function name()
{
}
however it doesn't go with https://developer.mozilla.org/en/JavaScript_Tips

also
onkeypress: function onkeypress(event) {
defaultFunc(aAccessible, flags) {
usually we do aArgumentName instead argumentName

I'd love to see a short comment before each method

I think I have to have next round, too many comments, hard to read the diff because of them

::: accessible/src/mobile/Makefile.in
@@ +18,5 @@
> +# Portions created by the Initial Developer are Copyright (C) 2011
> +# the Initial Developer. All Rights Reserved.
> +#
> +# Contributor(s):
> +#   Panos Astithas <[e-mail]>

interesting creative pseudonym, anyway follow http://www.mozilla.org/MPL/headers/

::: accessible/src/mobile/MobileAccessibility.jsm
@@ +77,5 @@
> +      this.startup();
> +  },
> +
> +  _android_a11y_check: function _android_a11y_check() {
> +  },

what is this empty method for?

@@ +82,5 @@
> +
> +  startup: function startup() {
> +    dump('MobileAccessibility startup');
> +
> +    this.presenters = [new AndroidPresenter(), new VisualPresenter()];

it makes sense to create Android Presenter iif android platform is detected
btw keeping presenters in array you think you could have more than two these presenters?

@@ +127,5 @@
> +        let activatedAcc = getAccessible(aEvent.originalTarget);
> +        let state = {};
> +        activatedAcc.getState(state, {});
> +        if (state.value & Ci.nsIAccessibleStates.STATE_CHECKABLE)
> +          return;

note: DOMActivate is not in sync with a11y processing, for example, you can get DOMActive for a button which doesn't have an accessible yet

@@ +145,5 @@
> +    let browserApp = this.chromeWin.BrowserApp || this.chromeWin.gBrowser;
> +    function getDocAccessibleInternal() {
> +      let docAcc = getAccessible(browserApp.selectedBrowser.contentDocument);
> +      if (!docAcc) {
> +        this.chromeWin.setTimeout(getDocAccessibleInternal.bind(this), 0);

what if it never gets accessible? I'd suggest to listen reorder event on document container

::: accessible/src/mobile/Presenters.jsm
@@ +62,5 @@
> +
> +  /**
> +   * Uninitialize function.
> +   */
> +  uninit: function uninit() {},

maybe attach/detach?

@@ +91,5 @@
> +   *    tree node.
> +   * @param {nsIAccessible} aObject the object that has been toggled.
> +   * @param {boolean} isExpanded whether the object has just been expanded.
> +   */
> +  objectExpanded: function objectExpanded(aObject, isExpanded) {},

we have a number of actions (you didn't covered all of them), maybe it's nicer to have one method for all of them like
actionInvoked: function actionInvoked(aObject, aActionName)

@@ +96,5 @@
> +
> +  /**
> +   * Text has changed, either by the user or by the system.
> +   */
> +  textChanged: function textChanged() {},

text can be changed often (in terms of text inserted/deleted accessible events) and due to different reasons like window reszie

@@ +116,5 @@
> +  pageStateChanged: function pageStateChanged() {},
> +
> +  /**
> +   * The tab has changed.
> +   * @param {nsIAccessible} aObject the document object of the new tab.

The current tab has changed.
document containing new tab or tab document?

@@ +131,5 @@
> +/**
> + * Visual presenter. Draws a box around the virtual cursor's position.
> + */
> +
> +function VisualPresenter() {}

I hope you aware of bug 727942, your code won't work properly until bug is fixed ;)

@@ +141,5 @@
> +  borderWidth: 1,
> +  borderRadius: 2,
> +  borderColor: 'orange',
> +  boxShadow: '1px 1px 1px #444'
> +};

this should be part of CSS file

@@ +147,5 @@
> +VisualPresenter.prototype.init = function(aWindow) {
> +  this.chromeWin = aWindow;
> +
> +  let contentElement = this.chromeWin.document.getElementById('appcontent') ||
> +      this.chromeWin.document.getElementById('main-window');

it sounds like you have a browser specific code here and in other places, can you handle it nicer like
if (isFenix)
else if (isFirefox)
else if (isSeamonkey)

@@ +157,5 @@
> +  // style it
> +  this.highlightBox.style.position = 'fixed';
> +  this.highlightBox.style.borderStyle = 'solid';
> +  this.highlightBox.style.pointerEvents = 'none';
> +  this.highlightBox.style.display = 'none';

should be a part of CSS file too

@@ +180,5 @@
> +
> +  return this.highlightBox;
> +};
> +
> +VisualPresenter.prototype.uninit = function() {

maybe attach/unattach too?

@@ +212,5 @@
> +  this.highlightBox.style.width =
> +    (bounds.width + this.style.padding * 2) + 'px';
> +  this.highlightBox.style.height =
> +    (bounds.height + this.style.padding * 2) + 'px';
> +  this.highlightBox.style.display = 'block';

from none to block to avoid flickering? if so then makes sense to comment it

@@ +216,5 @@
> +  this.highlightBox.style.display = 'block';
> +};
> +
> +VisualPresenter.prototype.getBounds = function(aObject, aZoom, aStart, aEnd) {
> +  let ax = {}, ay = {}, aw = {}, ah = {};

what is 'a' prefix for? I'd preferred xObj

@@ +225,5 @@
> +      textAcc.getRangeExtents(
> +          aStart, aEnd, ax, ay, aw, ah,
> +          Ci.nsIAccessibleCoordinateType.COORDTYPE_SCREEN_RELATIVE);
> +      return {left: ax.value * aZoom, top: ay.value * aZoom,
> +              width: aw.value * aZoom, height: ah.value * aZoom};

a11y coordinates are returned in device pixels (which takes zoom into account) but you need numbers in CSS pixels relative chrome document. Thus it looks you should do something like:
(objXInDevPixels - chromeDocXInDevPixels) / zoomInChromeDoc (which is 1.0 I believe)

@@ +237,5 @@
> +  let dx = {}, dy = {}, dw = {}, dh = {};
> +  let docRoot = aObject.rootDocument.QueryInterface(Ci.nsIAccessible);
> +  docRoot.getBounds(dx, dy, dw, dh);
> +
> +  let rv = {left: Math.round((ax.value - dx.value) * aZoom),

you do COORDTYPE_SCREEN_RELATIVE for text and window relative for object

@@ +298,5 @@
> +      eventType: ANDROID_TYPE_VIEW_FOCUSED,
> +      text: output
> +    }
> +  });
> +};

thinking about tabSelected, shouldn't you announce something when tab is changed, maybe announce current pivot? I'm thinking about if we can combine tabSelected and pivotChanged methods into one

::: accessible/src/mobile/UtteranceGenerator.jsm
@@ +50,5 @@
> +  .getService(Ci.nsIStringBundleService)
> +  .createBundle('chrome://accessibility/locale/MobileAccessibility.properties');
> +
> +var gAccRetrieval = Cc['@mozilla.org/accessibleRetrieval;1']
> +  .getService(Ci.nsIAccessibleRetrieval);

shouldn't be these two globals be a part of UtteranceGenerator object?

@@ +55,5 @@
> +
> +var EXPORTED_SYMBOLS = ['UtteranceGenerator'];
> +
> +var UtteranceGenerator = {
> +  utteranceForObject: function(aAccessible, forceName) {

I'm not sure it makes sense to dupe utterance word in object name and methods name, probably
var UtteranceGenerator = {
  getForObject: function(aAccessible, forceName) {
or
  genForObject: function(aAccessible, forceName) {

otherwise you end up with tautology
UtteranceGenerator.utteranceForObject();

@@ +56,5 @@
> +var EXPORTED_SYMBOLS = ['UtteranceGenerator'];
> +
> +var UtteranceGenerator = {
> +  utteranceForObject: function(aAccessible, forceName) {
> +    let roleString = gAccRetrieval.getStringRole(aAccessible.role);

why to not use aAccessible.role instead?

@@ +59,5 @@
> +  utteranceForObject: function(aAccessible, forceName) {
> +    let roleString = gAccRetrieval.getStringRole(aAccessible.role);
> +
> +    let func = this.objectUtteranceFunctions[roleString] ||
> +        this.objectUtteranceFunctions.defaultFunc;

wrong indent

@@ +81,5 @@
> +  },
> +
> +  utteranceForClicked: function(aObject) {
> +    return stringBundle.GetStringFromName('actionClicked');
> +  },

I'd say these names should include 'action' word

@@ +136,5 @@
> +    'listbox': INCLUDE_ROLE},
> +
> +  objectUtteranceFunctions: {
> +    defaultFunc: function defaultFunc(aAccessible, flags) {
> +      let name = (flags & INCLUDE_NAME) ? (aAccessible.name || '').trim() : '';

again trim on accessible name

@@ +149,5 @@
> +      aAccessible.getState(state, extState);
> +
> +      if (state.value & Ci.nsIAccessibleStates.STATE_CHECKABLE) {
> +        let stateStr = (state.value & Ci.nsIAccessibleStates.STATE_CHECKED) ?
> +            'objChecked' : 'objNotChecked';

use two space indentation pls

@@ +162,5 @@
> +
> +      return [desc, name];
> +    },
> +
> +    'heading': function(aAccessible, flags) {

funny? 'heading', having heading without quotes I think you still can do this.objectUtteranceFunctions[roleString]

@@ +171,5 @@
> +                                                   [level.value], 1);
> +      return [desc, name];
> +    },
> +
> +    'listitem': function(aAccessible, flags) {

I wonder how you're going to deal with other listitem roles like option in comboboxes or rich list options used XUL listboxes or trees what group information makes sense for. I'd say this.objectUtteranceFunctions[roleString] mechanism is not flexible at all

::: accessible/src/mobile/VirtualCursorController.jsm
@@ +57,5 @@
> +  },
> +
> +  uninit: function uninit() {
> +    this.chromeWin.document.removeEventListener('keypress', this.onkeypress);
> +  },

maybe attach/deattach?

@@ +62,5 @@
> +
> +  onkeypress: function onkeypress(event) {
> +    let window = VirtualCursorController.chromeWin;
> +    let document = (window.gBrowser) ? window.gBrowser.contentDocument :
> +        window.BrowserApp.selectedBrowser.contentDocument;

wrong indent

@@ +67,5 @@
> +
> +    dump('keypress ' + event.keyCode);
> +
> +    if (event.keyCode == event.DOM_VK_DOWN)
> +      VirtualCursorController.nextObject(document, event.shiftKey);

maybe shift+down arrow == end key is ok for android but it looks strange for desktop (having lines window.gBrowser.contentDocument and window.BrowserApp. makes me think it's supposed to work on desktop too)

@@ +72,5 @@
> +    else if (event.keyCode == event.DOM_VK_UP)
> +      VirtualCursorController.previousObject(document, event.shiftKey);
> +    else if (event.keyCode == event.DOM_VK_ENTER)
> +      VirtualCursorController.activateCurrent(document);
> +    else return;

use switch instead

@@ +78,5 @@
> +    event.preventDefault();
> +    event.stopPropagation();
> +  },
> +
> +  nextObject: function nextObject(document, last) {

a little bit strange because next does not mean last usually.

@@ +96,5 @@
> +      if (!virtualCursor.movePrevious(this.SimpleTraversalRule) &&
> +          Services.appinfo.OS == 'Android') {
> +        Cc['@mozilla.org/android/bridge;1']
> +          .getService(Ci.nsIAndroidBridge).handleGeckoMessage(
> +            JSON.stringify({ gecko: { type: 'ToggleChrome:Focus' } }));

please add a comment since it's not evident what android bridge is supposed to do with that

@@ +100,5 @@
> +            JSON.stringify({ gecko: { type: 'ToggleChrome:Focus' } }));
> +        virtualCursor.position = null;
> +      }
> +    }
> +  },

maybe moveNext/movePrev? no action in the name might be confusing

@@ +111,5 @@
> +        continue;
> +      acc.doAction(0);
> +      break;
> +    } while ((acc = acc.parent) &&
> +             acc.role != Ci.nsIAccessibleRole.ROLE_DOCUMENT);

that's a little bit weird but something similar we do for MSAA (see nsLinkableAccessible). How is your logic different from MSAA's one?

@@ +126,5 @@
> +      return 0;
> +    },
> +
> +    preFilter: Ci.nsIAccessibleTraversalRule.PREFILTER_DEFUNCT |
> +        Ci.nsIAccessibleTraversalRule.PREFILTER_INVISIBLE,

looks like wrong indent

@@ +132,5 @@
> +    match: function(aAccessible) {
> +      let rv = Ci.nsIAccessibleTraversalRule.FILTER_IGNORE;
> +      if (aAccessible.childCount == 0) {
> +        let ignoreRoles = [Ci.nsIAccessibleRole.ROLE_WHITESPACE,
> +                           Ci.nsIAccessibleRole.ROLE_STATICTEXT];

the purpose to skip things like CSS generated content? Why so?

@@ +133,5 @@
> +      let rv = Ci.nsIAccessibleTraversalRule.FILTER_IGNORE;
> +      if (aAccessible.childCount == 0) {
> +        let ignoreRoles = [Ci.nsIAccessibleRole.ROLE_WHITESPACE,
> +                           Ci.nsIAccessibleRole.ROLE_STATICTEXT];
> +        let hasName = (aAccessible.name && aAccessible.name.trim());

iirc we trim accessible name on c++ side,

btw, something to keep in mind: we have two different kind of empty names: name was not specified and name is provided as empty. In the first case you might want to repair name, in the second case you should leave it empty.

::: accessible/src/mobile/locales/en-US/chrome/MobileAccessibility.properties
@@ +1,1 @@
> +menubar        =       Menu bar

what about MPL header?

@@ +1,3 @@
> +menubar        =       Menu bar
> +scrollbar      =       Scroll bar
> +grip           =       Grip

we need to make sure these are kept updated, maybe add a comment to Role.h?

@@ +69,5 @@
> +objNotChecked  =       Not checked %S
> +objExpanded    =       Expanded %s
> +objCollapsed   =       Collapsed %s
> +
> +# Actions for clicked events

in general actions are not about clicked events, maybe "Accessible actions"?

::: accessible/src/mobile/locales/jar.mn
@@ +1,1 @@
> +#filter substitution

shouldn't you have MPL header here?

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 609578 [details] [diff] [review]
Added Javascript modules for mobile accessibility.

> DIRS += \
>   base \
>   html \
>+  mobile \

we should only include this in the build where its going to be used, no?

>diff --git a/accessible/src/mobile/Makefile.in b/accessible/src/mobile/Makefile.in
>new file mode 100644
>index 0000000..24a53c8
>--- /dev/null
>+++ b/accessible/src/mobile/Makefile.in
>@@ -0,0 +1,49 @@
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is mozilla.org code.
>+#
>+# The Initial Developer of the Original Code is
>+# The Mozilla Foundation.
>+# Portions created by the Initial Developer are Copyright (C) 2011
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#   Panos Astithas <[e-mail]>
>+#
>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the GPL or the LGPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK *****

I thought we were supposed to use mpl2 in new files?

>+
>+DEPTH     = ../../..
>+topsrcdir = @top_srcdir@
>+srcdir    = @srcdir@
>+VPATH     = @srcdir@
>+DIRS      = locales
>+
>+include $(DEPTH)/config/autoconf.mk
>+
>+include $(topsrcdir)/config/rules.mk
>+
>+libs::
>+	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/accessibility

this isn't absolutely trivial, so you should probably get a build peer to review this.

>+function getAccessible(node) {
>+  try {
>+    return gAccRetrieval.getAccessibleFor(node)
>+      .QueryInterface(Ci.nsIAccessible);

I thought it returned an nsIAccessible, so the qi should be a no op afaik

>+  /**
>+   * The virtual cursor's position changed.
>+   * @param {nsIAccessible} aObject the new position.
>+   * @param {nsIAccessible[]} aNewContext the ancestry of the new position that
>+   *    is different from the old virtual cursor position.

that's pretty confusing, do you mean the first common parent of the old nd new positions?

>+   * Text has changed, either by the user or by the system.
>+   */
>+  textChanged: function textChanged() {},

kind of weird it takes no arguments, and same below.

>+  activateCurrent: function activateCurrent(document) {
>+    let virtualCursor = this.getVirtualCursor(document);
>+    let acc = virtualCursor.position;
>+    do {
>+      if (acc.numActions == 0)
>+        continue;
>+      acc.doAction(0);
>+      break;
>+    } while ((acc = acc.parent) &&
>+             acc.role != Ci.nsIAccessibleRole.ROLE_DOCUMENT);

I feel like you probably want to break on at least grouping controlls as well.

>diff --git a/accessible/src/mobile/locales/Makefile.in b/accessible/src/mobile/locales/Makefile.in
>new file mode 100644
>index 0000000..5756cba
>--- /dev/null
>+++ b/accessible/src/mobile/locales/Makefile.in

I wonder if we should keep this and our other localization files (dom/locales/en-US/chrome/accessibility/) together.

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 609578 [details] [diff] [review]
Added Javascript modules for mobile accessibility.

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

The l10n file should be together with the other a11y files in dom/locales/en-US/chrome/accessibility.

Generally, taking keys from code for l10n content is tricky, as it's hard to change the keys if we need to fix a string. For one, it'd be good to have a thorough review of the first iteration of those to make sure we probably don't have to change 'em. In addition, maybe it's good to think about adding a "redirect" from code-generated to used keys. Might be as simple as adding an empty object that you check if the key wants to be different for l10n. Just so that people wanting to change strings know how to go about it.

::: accessible/src/mobile/locales/en-US/chrome/MobileAccessibility.properties
@@ +63,5 @@
> +figure         =       Figure
> +
> +# More sophisiticated object descriptions
> +headingLevel   =       Heading level %S
> +objItemOf      =       %S %S of %S

Please use explicit ordering here, that is, %1$S %2$S of %3$S, and add a comment to each of these what the variables get replaced with.

@@ +65,5 @@
> +# More sophisiticated object descriptions
> +headingLevel   =       Heading level %S
> +objItemOf      =       %S %S of %S
> +objChecked     =       Checked %S
> +objNotChecked  =       Not checked %S

How do those strings read in practices? "Not checked Foo" sounds like bad English.

Comment 6

[Eitan Isaacson [:eeejay]]

Here is a partial reply, running out of battery. so I will continue this soon.
The work in progress, with your feedback incorporated could be seen at
https://github.com/eeejay/mozilla-central/commits/mobile-a11y

(In reply to alexander :surkov from comment #3)
> Comment on attachment 609578 [details] [diff] [review]
> Added Javascript modules for mobile accessibility.
> 
> Review of attachment 609578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> nice work!
> 
> it makes sense to keep files under jsAT folder so that we say they can be
> reused on desktop and have Android presenter iif Android platform is detected
> 

I was looking for a name besides "mobile"! jsat seems to be a good option.

> general notice: you do 
> gAccRetrieval = Cc['@mozilla.org/accessibleRetrieval;1']
>   .getService(Ci.nsIAccessibleRetrieval);
> 
> but usually we do
> 
> gAccRetrieval = Cc['@mozilla.org/accessibleRetrieval;1'].
>   getService(Ci.nsIAccessibleRetrieval);
> 
> following the rule to not keep operators at new line. Does this style goes
> with coding style guide?
> 

No, you are right, fixed.

> also you do
> name: function name() {
> }
> but we do
> name: function name()
> {
> }
> however it doesn't go with https://developer.mozilla.org/en/JavaScript_Tips
> 

Yeah, most js files in the project keep the curly braces on the same line as the function decleration. I know that our mochitests are an exception. But I figured since this is new code, it is time to correct it. If you feel strongly about that style, I could change it. BTW, I am using gjslint as a style corrector. As mentioned in the Gaia hacking guide (which I adopted becuase it is recent and 'modern').

> also
> onkeypress: function onkeypress(event) {
> defaultFunc(aAccessible, flags) {
> usually we do aArgumentName instead argumentName

Done.

> 
> I'd love to see a short comment before each method
> 
> I think I have to have next round, too many comments, hard to read the diff
> because of them
> 
> ::: accessible/src/mobile/Makefile.in
> @@ +18,5 @@
> > +# Portions created by the Initial Developer are Copyright (C) 2011
> > +# the Initial Developer. All Rights Reserved.
> > +#
> > +# Contributor(s):
> > +#   Panos Astithas <[e-mail]>
> 
> interesting creative pseudonym, anyway follow
> http://www.mozilla.org/MPL/headers/

Done, it is all now MPL 2.

> 
> ::: accessible/src/mobile/MobileAccessibility.jsm
> @@ +77,5 @@
> > +      this.startup();
> > +  },
> > +
> > +  _android_a11y_check: function _android_a11y_check() {
> > +  },
> 
> what is this empty method for?
> 

Nothing :)

> @@ +82,5 @@
> > +
> > +  startup: function startup() {
> > +    dump('MobileAccessibility startup');
> > +
> > +    this.presenters = [new AndroidPresenter(), new VisualPresenter()];
> 
> it makes sense to create Android Presenter iif android platform is detected
> btw keeping presenters in array you think you could have more than two these
> presenters?
> 

Yeah, that is the end goal (to only load the android one in android) (TODO). As for the array, yes, there could be more than two, for example braille, haptic feedback, earcons.

> @@ +127,5 @@
> > +        let activatedAcc = getAccessible(aEvent.originalTarget);
> > +        let state = {};
> > +        activatedAcc.getState(state, {});
> > +        if (state.value & Ci.nsIAccessibleStates.STATE_CHECKABLE)
> > +          return;
> 
> note: DOMActivate is not in sync with a11y processing, for example, you can
> get DOMActive for a button which doesn't have an accessible yet
> 

That sucks. We should have an a11y event for activation.
TODO

> @@ +145,5 @@
> > +    let browserApp = this.chromeWin.BrowserApp || this.chromeWin.gBrowser;
> > +    function getDocAccessibleInternal() {
> > +      let docAcc = getAccessible(browserApp.selectedBrowser.contentDocument);
> > +      if (!docAcc) {
> > +        this.chromeWin.setTimeout(getDocAccessibleInternal.bind(this), 0);
> 
> what if it never gets accessible? I'd suggest to listen reorder event on
> document container

TODO

> 
> ::: accessible/src/mobile/Presenters.jsm
> @@ +62,5 @@
> > +
> > +  /**
> > +   * Uninitialize function.
> > +   */
> > +  uninit: function uninit() {},
> 
> maybe attach/detach?

Good idea, done.

> 
> @@ +91,5 @@
> > +   *    tree node.
> > +   * @param {nsIAccessible} aObject the object that has been toggled.
> > +   * @param {boolean} isExpanded whether the object has just been expanded.
> > +   */
> > +  objectExpanded: function objectExpanded(aObject, isExpanded) {},
> 
> we have a number of actions (you didn't covered all of them), maybe it's
> nicer to have one method for all of them like
> actionInvoked: function actionInvoked(aObject, aActionName)
> 

Good idea. Done. With the suggestion above (an a11y activation event), there should be some correlation between the action interface action names and these. I hope the changes that I put in help with that.

> @@ +96,5 @@
> > +
> > +  /**
> > +   * Text has changed, either by the user or by the system.
> > +   */
> > +  textChanged: function textChanged() {},
> 
> text can be changed often (in terms of text inserted/deleted accessible
> events) and due to different reasons like window reszie
> 

Yeah, as you can tell I didn't work out the heuristics of this method yet. But I am pretty sure it will not be called on text reflows. Probably just editable text changes and live regions. Just keeping it in this patch as a stub. I'll add a TODO comment to all methods that are still not done.

> @@ +116,5 @@
> > +  pageStateChanged: function pageStateChanged() {},
> > +
> > +  /**
> > +   * The tab has changed.
> > +   * @param {nsIAccessible} aObject the document object of the new tab.
> 
> The current tab has changed.
> document containing new tab or tab document?
> 

tab document. I'll make that clearer.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #6)
> > it makes sense to keep files under jsAT folder so that we say they can be
> > reused on desktop and have Android presenter iif Android platform is detected
> I was looking for a name besides "mobile"! jsat seems to be a good option.

other alternatives are jslib or jstoolkit, or even just toolkit, perhaps jsAT is nicer :)

> Yeah, most js files in the project keep the curly braces on the same line as
> the function decleration. I know that our mochitests are an exception. But I
> figured since this is new code, it is time to correct it. If you feel
> strongly about that style, I could change it. BTW, I am using gjslint as a
> style corrector. As mentioned in the Gaia hacking guide (which I adopted
> becuase it is recent and 'modern').

ok, fine with me

> > it makes sense to create Android Presenter iif android platform is detected
> > btw keeping presenters in array you think you could have more than two these
> > presenters?
> > 
> 
> Yeah, that is the end goal (to only load the android one in android) (TODO).

this TODO you're going to fix next round or somewhere later?

> As for the array, yes, there could be more than two, for example braille,
> haptic feedback, earcons.

ok, cool

> > note: DOMActivate is not in sync with a11y processing, for example, you can
> > get DOMActive for a button which doesn't have an accessible yet
> > 
> 
> That sucks. We should have an a11y event for activation.
> TODO

no one AT needs it, probably you don't need it as well. Can you announce the action before you do the action?
like
invokeAction()
{
  var actionName = accessible.getActionName(0);
  say(actionName);
  accessible.doAction(actionName);
}

> Yeah, as you can tell I didn't work out the heuristics of this method yet.
> But I am pretty sure it will not be called on text reflows. Probably just
> editable text changes and live regions. Just keeping it in this patch as a
> stub. I'll add a TODO comment to all methods that are still not done.

well, accessible event provides isFromUserInput flag you should rely on if that's what you want. However it doesn't work properly :) One more reason to fix it.

Comment 8

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

(In reply to alexander :surkov from comment #3)
> Review of attachment 609578 [details] [diff] [review]:
> nice work!

+1

Regarding the folder name "jsat" WFM too.

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #7)
> (In reply to Eitan Isaacson [:eeejay] from comment #6)
> > > it makes sense to keep files under jsAT folder so that we say they can be
> > > reused on desktop and have Android presenter iif Android platform is detected
> > I was looking for a name besides "mobile"! jsat seems to be a good option.
> 
> other alternatives are jslib or jstoolkit, or even just toolkit, perhaps
> jsAT is nicer :)
> 

I made it all lowercase, I hope that works. I also need to change MobileAccessibility to something else, maybe AccessibilityHelper?

> > Yeah, most js files in the project keep the curly braces on the same line as
> > the function decleration. I know that our mochitests are an exception. But I
> > figured since this is new code, it is time to correct it. If you feel
> > strongly about that style, I could change it. BTW, I am using gjslint as a
> > style corrector. As mentioned in the Gaia hacking guide (which I adopted
> > becuase it is recent and 'modern').
> 
> ok, fine with me
> 
> > > it makes sense to create Android Presenter iif android platform is detected
> > > btw keeping presenters in array you think you could have more than two these
> > > presenters?
> > > 
> > 
> > Yeah, that is the end goal (to only load the android one in android) (TODO).
> 
> this TODO you're going to fix next round or somewhere later?
> 

It is just to remind myself about less trivial changes that I need to get back to. It will all be incorporated in the next patch.

> > As for the array, yes, there could be more than two, for example braille,
> > haptic feedback, earcons.
> 
> ok, cool
> 
> > > note: DOMActivate is not in sync with a11y processing, for example, you can
> > > get DOMActive for a button which doesn't have an accessible yet
> > > 
> > 
> > That sucks. We should have an a11y event for activation.
> > TODO
> 
> no one AT needs it, probably you don't need it as well. Can you announce the
> action before you do the action?
> like
> invokeAction()
> {
>   var actionName = accessible.getActionName(0);
>   say(actionName);
>   accessible.doAction(actionName);
> }
> 

The actions are not necessarily invoked from the AT. For example, a user with residual vision could alternately press d-pad center, or directly interact with widgets, but they expect the same feedback. And btw, I would say ATs *do* need it.. Android's screen reader speaks click events it receives from the a11y API. With touch screens, having audible and alternative feedback for interactions is important.

> > Yeah, as you can tell I didn't work out the heuristics of this method yet.
> > But I am pretty sure it will not be called on text reflows. Probably just
> > editable text changes and live regions. Just keeping it in this patch as a
> > stub. I'll add a TODO comment to all methods that are still not done.
> 
> well, accessible event provides isFromUserInput flag you should rely on if
> that's what you want. However it doesn't work properly :) One more reason to
> fix it.

Right!

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #3)
> Comment on attachment 609578 [details] [diff] [review]
> Added Javascript modules for mobile accessibility.
> 
> Review of attachment 609578 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +131,5 @@
> > +/**
> > + * Visual presenter. Draws a box around the virtual cursor's position.
> > + */
> > +
> > +function VisualPresenter() {}
> 
> I hope you aware of bug 727942, your code won't work properly until bug is
> fixed ;)
> 

Looks like it is fixed! Anyway, as Jamie said, the zoom level does not matter, the a11y cooridnates return whatever is rendered.

> @@ +141,5 @@
> > +  borderWidth: 1,
> > +  borderRadius: 2,
> > +  borderColor: 'orange',
> > +  boxShadow: '1px 1px 1px #444'
> > +};
> 
> this should be part of CSS file

Which CSS file? We are trying to be app-agnostic, so we would need to put it in every browser.xul.

> 
> @@ +147,5 @@
> > +VisualPresenter.prototype.init = function(aWindow) {
> > +  this.chromeWin = aWindow;
> > +
> > +  let contentElement = this.chromeWin.document.getElementById('appcontent') ||
> > +      this.chromeWin.document.getElementById('main-window');
> 
> it sounds like you have a browser specific code here and in other places,
> can you handle it nicer like
> if (isFenix)
> else if (isFirefox)
> else if (isSeamonkey)
> 

TODO

> @@ +157,5 @@
> > +  // style it
> > +  this.highlightBox.style.position = 'fixed';
> > +  this.highlightBox.style.borderStyle = 'solid';
> > +  this.highlightBox.style.pointerEvents = 'none';
> > +  this.highlightBox.style.display = 'none';
> 
> should be a part of CSS file too
> 
> @@ +180,5 @@
> > +
> > +  return this.highlightBox;
> > +};
> > +
> > +VisualPresenter.prototype.uninit = function() {
> 
> maybe attach/unattach too?

Done.

> 
> @@ +212,5 @@
> > +  this.highlightBox.style.width =
> > +    (bounds.width + this.style.padding * 2) + 'px';
> > +  this.highlightBox.style.height =
> > +    (bounds.height + this.style.padding * 2) + 'px';
> > +  this.highlightBox.style.display = 'block';
> 
> from none to block to avoid flickering? if so then makes sense to comment it
> 

Done.

> @@ +216,5 @@
> > +  this.highlightBox.style.display = 'block';
> > +};
> > +
> > +VisualPresenter.prototype.getBounds = function(aObject, aZoom, aStart, aEnd) {
> > +  let ax = {}, ay = {}, aw = {}, ah = {};
> 
> what is 'a' prefix for? I'd preferred xObj
> 

Done.

> @@ +225,5 @@
> > +      textAcc.getRangeExtents(
> > +          aStart, aEnd, ax, ay, aw, ah,
> > +          Ci.nsIAccessibleCoordinateType.COORDTYPE_SCREEN_RELATIVE);
> > +      return {left: ax.value * aZoom, top: ay.value * aZoom,
> > +              width: aw.value * aZoom, height: ah.value * aZoom};
> 
> a11y coordinates are returned in device pixels (which takes zoom into
> account) but you need numbers in CSS pixels relative chrome document. Thus
> it looks you should do something like:
> (objXInDevPixels - chromeDocXInDevPixels) / zoomInChromeDoc (which is 1.0 I
> believe)
> 

This magically works. Also, don't have any real text functionality in the VC yet, so it might make sense to remove this for now. Also, the aZoom parameter is Fennec specific for now.

> @@ +237,5 @@
> > +  let dx = {}, dy = {}, dw = {}, dh = {};
> > +  let docRoot = aObject.rootDocument.QueryInterface(Ci.nsIAccessible);
> > +  docRoot.getBounds(dx, dy, dw, dh);
> > +
> > +  let rv = {left: Math.round((ax.value - dx.value) * aZoom),
> 
> you do COORDTYPE_SCREEN_RELATIVE for text and window relative for object
> 

Oops, fixed.

> @@ +298,5 @@
> > +      eventType: ANDROID_TYPE_VIEW_FOCUSED,
> > +      text: output
> > +    }
> > +  });
> > +};
> 
> thinking about tabSelected, shouldn't you announce something when tab is
> changed, maybe announce current pivot? I'm thinking about if we can combine
> tabSelected and pivotChanged methods into one
> 

yes, something should be announced. I don't think they should be the same thing, but it makes sense in some presenters, like the Android one, to call pivotChanged from tabSelected with a full context array.

> ::: accessible/src/mobile/UtteranceGenerator.jsm
> @@ +50,5 @@
> > +  .getService(Ci.nsIStringBundleService)
> > +  .createBundle('chrome://accessibility/locale/MobileAccessibility.properties');
> > +
> > +var gAccRetrieval = Cc['@mozilla.org/accessibleRetrieval;1']
> > +  .getService(Ci.nsIAccessibleRetrieval);
> 
> shouldn't be these two globals be a part of UtteranceGenerator object?
> 

They could be, it doesn't make a difference since js modules are encapsulated.

> @@ +55,5 @@
> > +
> > +var EXPORTED_SYMBOLS = ['UtteranceGenerator'];
> > +
> > +var UtteranceGenerator = {
> > +  utteranceForObject: function(aAccessible, forceName) {
> 
> I'm not sure it makes sense to dupe utterance word in object name and
> methods name, probably
> var UtteranceGenerator = {
>   getForObject: function(aAccessible, forceName) {
> or
>   genForObject: function(aAccessible, forceName) {
> 
> otherwise you end up with tautology
> UtteranceGenerator.utteranceForObject();
> 

True, done.

> @@ +56,5 @@
> > +var EXPORTED_SYMBOLS = ['UtteranceGenerator'];
> > +
> > +var UtteranceGenerator = {
> > +  utteranceForObject: function(aAccessible, forceName) {
> > +    let roleString = gAccRetrieval.getStringRole(aAccessible.role);
> 
> why to not use aAccessible.role instead?
> 

It keeps it more readable, imho. The mapping is not complete, so you would get a non continous sequence of integers. I could change it, if you think it makes more sense.

> @@ +59,5 @@
> > +  utteranceForObject: function(aAccessible, forceName) {
> > +    let roleString = gAccRetrieval.getStringRole(aAccessible.role);
> > +
> > +    let func = this.objectUtteranceFunctions[roleString] ||
> > +        this.objectUtteranceFunctions.defaultFunc;
> 
> wrong indent
> 

Weirdly enough, gjslint requires 4 space indents when wrapping lines like that. I fixed all of the indent issues you saw, but what you say we go with gjslint styling?

> @@ +81,5 @@
> > +  },
> > +
> > +  utteranceForClicked: function(aObject) {
> > +    return stringBundle.GetStringFromName('actionClicked');
> > +  },
> 
> I'd say these names should include 'action' word
> 

I changed these methods to map you suggestion above for the presenters: to include all actions in one method.

> @@ +136,5 @@
> > +    'listbox': INCLUDE_ROLE},
> > +
> > +  objectUtteranceFunctions: {
> > +    defaultFunc: function defaultFunc(aAccessible, flags) {
> > +      let name = (flags & INCLUDE_NAME) ? (aAccessible.name || '').trim() : '';
> 
> again trim on accessible name
> 

I don't understand :) That weird syntax is needed because the accessible name could be null instead of an empty string.

> @@ +149,5 @@
> > +      aAccessible.getState(state, extState);
> > +
> > +      if (state.value & Ci.nsIAccessibleStates.STATE_CHECKABLE) {
> > +        let stateStr = (state.value & Ci.nsIAccessibleStates.STATE_CHECKED) ?
> > +            'objChecked' : 'objNotChecked';
> 
> use two space indentation pls
> 

Done.

> @@ +162,5 @@
> > +
> > +      return [desc, name];
> > +    },
> > +
> > +    'heading': function(aAccessible, flags) {
> 
> funny? 'heading', having heading without quotes I think you still can do
> this.objectUtteranceFunctions[roleString]
> 

Yup, fixed.

> @@ +171,5 @@
> > +                                                   [level.value], 1);
> > +      return [desc, name];
> > +    },
> > +
> > +    'listitem': function(aAccessible, flags) {
> 
> I wonder how you're going to deal with other listitem roles like option in
> comboboxes or rich list options used XUL listboxes or trees what group
> information makes sense for. I'd say
> this.objectUtteranceFunctions[roleString] mechanism is not flexible at all
> 

Those cases would be included in the listitem function.

> ::: accessible/src/mobile/VirtualCursorController.jsm
> @@ +57,5 @@
> > +  },
> > +
> > +  uninit: function uninit() {
> > +    this.chromeWin.document.removeEventListener('keypress', this.onkeypress);
> > +  },
> 
> maybe attach/deattach?

Yep, done. I changed all of those :)

> 
> @@ +62,5 @@
> > +
> > +  onkeypress: function onkeypress(event) {
> > +    let window = VirtualCursorController.chromeWin;
> > +    let document = (window.gBrowser) ? window.gBrowser.contentDocument :
> > +        window.BrowserApp.selectedBrowser.contentDocument;
> 
> wrong indent

Fixed, for the record - O think gjslint likes 4 space indents in those situations, also where you mentioned it above.

> 
> @@ +67,5 @@
> > +
> > +    dump('keypress ' + event.keyCode);
> > +
> > +    if (event.keyCode == event.DOM_VK_DOWN)
> > +      VirtualCursorController.nextObject(document, event.shiftKey);
> 
> maybe shift+down arrow == end key is ok for android but it looks strange for
> desktop (having lines window.gBrowser.contentDocument and window.BrowserApp.
> makes me think it's supposed to work on desktop too)
> 

the desktop support at this point is purely for testing purposes. Having the VC consume arrow keys does not make any sense for real users.

> @@ +72,5 @@
> > +    else if (event.keyCode == event.DOM_VK_UP)
> > +      VirtualCursorController.previousObject(document, event.shiftKey);
> > +    else if (event.keyCode == event.DOM_VK_ENTER)
> > +      VirtualCursorController.activateCurrent(document);
> > +    else return;
> 
> use switch instead
> 

Done.

> @@ +78,5 @@
> > +    event.preventDefault();
> > +    event.stopPropagation();
> > +  },
> > +
> > +  nextObject: function nextObject(document, last) {
> 
> a little bit strange because next does not mean last usually.
> 

Changed to forwardObject/backwardObject.

> @@ +96,5 @@
> > +      if (!virtualCursor.movePrevious(this.SimpleTraversalRule) &&
> > +          Services.appinfo.OS == 'Android') {
> > +        Cc['@mozilla.org/android/bridge;1']
> > +          .getService(Ci.nsIAndroidBridge).handleGeckoMessage(
> > +            JSON.stringify({ gecko: { type: 'ToggleChrome:Focus' } }));
> 
> please add a comment since it's not evident what android bridge is supposed
> to do with that
> 

Done.

> @@ +100,5 @@
> > +            JSON.stringify({ gecko: { type: 'ToggleChrome:Focus' } }));
> > +        virtualCursor.position = null;
> > +      }
> > +    }
> > +  },
> 
> maybe moveNext/movePrev? no action in the name might be confusing
> 

not sure what you mean here.

> @@ +111,5 @@
> > +        continue;
> > +      acc.doAction(0);
> > +      break;
> > +    } while ((acc = acc.parent) &&
> > +             acc.role != Ci.nsIAccessibleRole.ROLE_DOCUMENT);
> 
> that's a little bit weird but something similar we do for MSAA (see
> nsLinkableAccessible). How is your logic different from MSAA's one?
> 

It is similar, but here for different reasons. For example, if a user is on a link's text leaf they will hear "link", and when they activate it, they expect to jump. So we traverse up the leaf's lineage until we find an action, and activate it. This is probably not perfect in all cases, for example doing "select" on a selected list item instead of activating it, but I will extend it as we run into more stuff.

> @@ +126,5 @@
> > +      return 0;
> > +    },
> > +
> > +    preFilter: Ci.nsIAccessibleTraversalRule.PREFILTER_DEFUNCT |
> > +        Ci.nsIAccessibleTraversalRule.PREFILTER_INVISIBLE,
> 
> looks like wrong indent

Done.

> 
> @@ +132,5 @@
> > +    match: function(aAccessible) {
> > +      let rv = Ci.nsIAccessibleTraversalRule.FILTER_IGNORE;
> > +      if (aAccessible.childCount == 0) {
> > +        let ignoreRoles = [Ci.nsIAccessibleRole.ROLE_WHITESPACE,
> > +                           Ci.nsIAccessibleRole.ROLE_STATICTEXT];
> 
> the purpose to skip things like CSS generated content? Why so?

Because list bullets and empty nodes don't interest us. If there is CSS generated content that does, we should change this.

> 
> @@ +133,5 @@
> > +      let rv = Ci.nsIAccessibleTraversalRule.FILTER_IGNORE;
> > +      if (aAccessible.childCount == 0) {
> > +        let ignoreRoles = [Ci.nsIAccessibleRole.ROLE_WHITESPACE,
> > +                           Ci.nsIAccessibleRole.ROLE_STATICTEXT];
> > +        let hasName = (aAccessible.name && aAccessible.name.trim());
> 
> iirc we trim accessible name on c++ side,
> 

I am pretty sure I saw whitespace, that is why I put a trim there. I could remove it and do some more testing.

> btw, something to keep in mind: we have two different kind of empty names:
> name was not specified and name is provided as empty. In the first case you
> might want to repair name, in the second case you should leave it empty.
> 

Repair name? How? 

> ::: accessible/src/mobile/locales/en-US/chrome/MobileAccessibility.properties
> @@ +1,1 @@
> > +menubar        =       Menu bar
> 
> what about MPL header?
> 
> @@ +1,3 @@
> > +menubar        =       Menu bar
> > +scrollbar      =       Scroll bar
> > +grip           =       Grip
> 
> we need to make sure these are kept updated, maybe add a comment to Role.h?
> 

Okie dokes!

> @@ +69,5 @@
> > +objNotChecked  =       Not checked %S
> > +objExpanded    =       Expanded %s
> > +objCollapsed   =       Collapsed %s
> > +
> > +# Actions for clicked events
> 
> in general actions are not about clicked events, maybe "Accessible actions"?
> 

Feexed.

> ::: accessible/src/mobile/locales/jar.mn
> @@ +1,1 @@
> > +#filter substitution
> 
> shouldn't you have MPL header here?

Besides one jar.mn, none have it in the entire project.

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> Comment on attachment 609578 [details] [diff] [review]
> Added Javascript modules for mobile accessibility.
> 
> > DIRS += \
> >   base \
> >   html \
> >+  mobile \
> 
> we should only include this in the build where its going to be used, no?
> 

We renamed it to jsat, and it is testable on desktop. So there is no reason not to include it in all platforms.

> >diff --git a/accessible/src/mobile/Makefile.in b/accessible/src/mobile/Makefile.in
> >new file mode 100644
> >index 0000000..24a53c8
> >--- /dev/null
> >+++ b/accessible/src/mobile/Makefile.in
> >@@ -0,0 +1,49 @@
> >+# ***** BEGIN LICENSE BLOCK *****
> >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
> >+#
> >+# The contents of this file are subject to the Mozilla Public License Version
> >+# 1.1 (the "License"); you may not use this file except in compliance with
> >+# the License. You may obtain a copy of the License at
> >+# http://www.mozilla.org/MPL/
> >+#
> >+# Software distributed under the License is distributed on an "AS IS" basis,
> >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> >+# for the specific language governing rights and limitations under the
> >+# License.
> >+#
> >+# The Original Code is mozilla.org code.
> >+#
> >+# The Initial Developer of the Original Code is
> >+# The Mozilla Foundation.
> >+# Portions created by the Initial Developer are Copyright (C) 2011
> >+# the Initial Developer. All Rights Reserved.
> >+#
> >+# Contributor(s):
> >+#   Panos Astithas <[e-mail]>
> >+#
> >+# Alternatively, the contents of this file may be used under the terms of
> >+# either the GNU General Public License Version 2 or later (the "GPL"), or
> >+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> >+# in which case the provisions of the GPL or the LGPL are applicable instead
> >+# of those above. If you wish to allow use of your version of this file only
> >+# under the terms of either the GPL or the LGPL, and not to allow others to
> >+# use your version of this file under the terms of the MPL, indicate your
> >+# decision by deleting the provisions above and replace them with the notice
> >+# and other provisions required by the GPL or the LGPL. If you do not delete
> >+# the provisions above, a recipient may use your version of this file under
> >+# the terms of any one of the MPL, the GPL or the LGPL.
> >+#
> >+# ***** END LICENSE BLOCK *****
> 
> I thought we were supposed to use mpl2 in new files?
> 

Yes, done. Sorry.

> >+
> >+DEPTH     = ../../..
> >+topsrcdir = @top_srcdir@
> >+srcdir    = @srcdir@
> >+VPATH     = @srcdir@
> >+DIRS      = locales
> >+
> >+include $(DEPTH)/config/autoconf.mk
> >+
> >+include $(topsrcdir)/config/rules.mk
> >+
> >+libs::
> >+	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/accessibility
> 
> this isn't absolutely trivial, so you should probably get a build peer to
> review this.
> 

Right, I'll do that. Who is a build peer?

> >+function getAccessible(node) {
> >+  try {
> >+    return gAccRetrieval.getAccessibleFor(node)
> >+      .QueryInterface(Ci.nsIAccessible);
> 
> I thought it returned an nsIAccessible, so the qi should be a no op afaik
> 

Good to know!

> >+  /**
> >+   * The virtual cursor's position changed.
> >+   * @param {nsIAccessible} aObject the new position.
> >+   * @param {nsIAccessible[]} aNewContext the ancestry of the new position that
> >+   *    is different from the old virtual cursor position.
> 
> that's pretty confusing, do you mean the first common parent of the old nd
> new positions?
> 

Yep, I need to make that clearer.

> >+   * Text has changed, either by the user or by the system.
> >+   */
> >+  textChanged: function textChanged() {},
> 
> kind of weird it takes no arguments, and same below.

It is a stub, I added a TODO.

> 
> >+  activateCurrent: function activateCurrent(document) {
> >+    let virtualCursor = this.getVirtualCursor(document);
> >+    let acc = virtualCursor.position;
> >+    do {
> >+      if (acc.numActions == 0)
> >+        continue;
> >+      acc.doAction(0);
> >+      break;
> >+    } while ((acc = acc.parent) &&
> >+             acc.role != Ci.nsIAccessibleRole.ROLE_DOCUMENT);
> 
> I feel like you probably want to break on at least grouping controlls as
> well.
> 

How do you mean?

> >diff --git a/accessible/src/mobile/locales/Makefile.in b/accessible/src/mobile/locales/Makefile.in
> >new file mode 100644
> >index 0000000..5756cba
> >--- /dev/null
> >+++ b/accessible/src/mobile/locales/Makefile.in
> 
> I wonder if we should keep this and our other localization files
> (dom/locales/en-US/chrome/accessibility/) together.

As Axel says too.. I am a bit baffled by that directory, with the windows/mac/usnix directory that are identical, and I don't understand how one dir is chosen over another. Anyhow, I think it is worth consolodating all those into one directory, and then yes, adding the strings from here.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #9)
> > other alternatives are jslib or jstoolkit, or even just toolkit, perhaps
> > jsAT is nicer :)
> > 
> 
> I made it all lowercase, I hope that works.

ok, but it's funny since jsat makes me ask "js at what"?

> I also need to change
> MobileAccessibility to something else, maybe AccessibilityHelper?

helper is not descriptive name and it's not a helper actually, it's primary service that manages the accessibility. Perhaps jsAT?

> The actions are not necessarily invoked from the AT. For example, a user
> with residual vision could alternately press d-pad center, or directly
> interact with widgets, but they expect the same feedback. And btw, I would
> say ATs *do* need it.. Android's screen reader speaks click events it
> receives from the a11y API. With touch screens, having audible and
> alternative feedback for interactions is important.

mm, ok, sounds like a big work item. You need to file a bug on this.

(In reply to Eitan Isaacson [:eeejay] from comment #10)
> > I hope you aware of bug 727942, your code won't work properly until bug is
> > fixed ;)
> Looks like it is fixed!

nope, maybe I'll get a chance to finish it soon.

> Anyway, as Jamie said, the zoom level does not
> matter, the a11y cooridnates return whatever is rendered.

zoom level doesn't matter for Jamie :) it matters for you since you're going to map back the AT coordinates to DOM coordinates.

> > @@ +141,5 @@
> > > +  borderWidth: 1,
> > > +  borderRadius: 2,
> > > +  borderColor: 'orange',
> > > +  boxShadow: '1px 1px 1px #444'
> > > +};
> > 
> > this should be part of CSS file
> 
> Which CSS file? We are trying to be app-agnostic, so we would need to put it
> in every browser.xul.

having CSS in js is an ugly thing. I'd say you need to get feedback from toolkit/browser developers. Probably you could load CSS in js.

> > @@ +225,5 @@
> > > +      textAcc.getRangeExtents(
> > > +          aStart, aEnd, ax, ay, aw, ah,
> > > +          Ci.nsIAccessibleCoordinateType.COORDTYPE_SCREEN_RELATIVE);
> > > +      return {left: ax.value * aZoom, top: ay.value * aZoom,
> > > +              width: aw.value * aZoom, height: ah.value * aZoom};
> > 
> > a11y coordinates are returned in device pixels (which takes zoom into
> > account) but you need numbers in CSS pixels relative chrome document. Thus
> > it looks you should do something like:
> > (objXInDevPixels - chromeDocXInDevPixels) / zoomInChromeDoc (which is 1.0 I
> > believe)
> > 
> 
> This magically works. Also, don't have any real text functionality in the VC
> yet, so it might make sense to remove this for now. Also, the aZoom
> parameter is Fennec specific for now.

note, because of bug 727942 getRangeExtents() returns coordinates in CSS pixels.

> > thinking about tabSelected, shouldn't you announce something when tab is
> > changed, maybe announce current pivot? I'm thinking about if we can combine
> > tabSelected and pivotChanged methods into one
> > 
> 
> yes, something should be announced. I don't think they should be the same
> thing, but it makes sense in some presenters, like the Android one, to call
> pivotChanged from tabSelected with a full context array.

other TODO?

> > ::: accessible/src/mobile/UtteranceGenerator.jsm
> > shouldn't be these two globals be a part of UtteranceGenerator object?
> They could be, it doesn't make a difference since js modules are
> encapsulated.

just thinking of a reason having globals when they are supposed to be used within single object

> > > +  utteranceForObject: function(aAccessible, forceName) {
> > > +    let roleString = gAccRetrieval.getStringRole(aAccessible.role);
> > 
> > why to not use aAccessible.role instead?
> It keeps it more readable, imho. The mapping is not complete, so you would
> get a non continous sequence of integers. I could change it, if you think it
> makes more sense.

you should avoid unperformant things

> Weirdly enough, gjslint requires 4 space indents when wrapping lines like
> that. I fixed all of the indent issues you saw, but what you say we go with
> gjslint styling?

how are gjslint and Mozilla code stylings related?

> > > +    defaultFunc: function defaultFunc(aAccessible, flags) {
> > > +      let name = (flags & INCLUDE_NAME) ? (aAccessible.name || '').trim() : '';
> > 
> > again trim on accessible name
> > 
> 
> I don't understand :) That weird syntax is needed because the accessible
> name could be null instead of an empty string.

I meant we do trim() before accessible name is returned.

> > > +    'listitem': function(aAccessible, flags) {
> > 
> > I wonder how you're going to deal with other listitem roles like option in
> > comboboxes or rich list options used XUL listboxes or trees what group
> > information makes sense for. I'd say
> > this.objectUtteranceFunctions[roleString] mechanism is not flexible at all
> > 
> 
> Those cases would be included in the listitem function.

ok, I need to take a look I think

> > maybe shift+down arrow == end key is ok for android but it looks strange for
> > desktop (having lines window.gBrowser.contentDocument and window.BrowserApp.
> > makes me think it's supposed to work on desktop too)
> > 
> 
> the desktop support at this point is purely for testing purposes. Having the
> VC consume arrow keys does not make any sense for real users.

well, you might want to support tablet pc with keyboards (which have end key)

> > > +  nextObject: function nextObject(document, last) {
> > 
> > a little bit strange because next does not mean last usually.
> > 
> Changed to forwardObject/backwardObject.

> > maybe moveNext/movePrev? no action in the name might be confusing
> > 
> not sure what you mean here.

I'd like to see a verb in the name

> > @@ +111,5 @@
> > > +        continue;
> > > +      acc.doAction(0);
> > > +      break;
> > > +    } while ((acc = acc.parent) &&
> > > +             acc.role != Ci.nsIAccessibleRole.ROLE_DOCUMENT);
> > 
> > that's a little bit weird but something similar we do for MSAA (see
> > nsLinkableAccessible). How is your logic different from MSAA's one?
> > 
> 
> It is similar, but here for different reasons. For example, if a user is on
> a link's text leaf they will hear "link", and when they activate it, they
> expect to jump. So we traverse up the leaf's lineage until we find an
> action, and activate it. This is probably not perfect in all cases, for
> example doing "select" on a selected list item instead of activating it, but
> I will extend it as we run into more stuff.

I didn't get a difference. What I meant if you are on text node accessible inside a link accessible then you have doAction on text node and you don't traverse the tree in that case actually.

> > the purpose to skip things like CSS generated content? Why so?
> 
> Because list bullets and empty nodes don't interest us. If there is CSS
> generated content that does, we should change this.

ok, that makes you ignore list bullets and CSS generated content like
div:after{ content("hello this is my generated text" }

> > > +        let hasName = (aAccessible.name && aAccessible.name.trim());
> > 
> > iirc we trim accessible name on c++ side,
> > 
> 
> I am pretty sure I saw whitespace, that is why I put a trim there. I could
> remove it and do some more testing.

please, you shouldn't see them

> > btw, something to keep in mind: we have two different kind of empty names:
> > name was not specified and name is provided as empty. In the first case you
> > might want to repair name, in the second case you should leave it empty.
> > 
> 
> Repair name? How?

well, who knows, it's better to ask AT :) probably tag name announcement, dunno.

> > ::: accessible/src/mobile/locales/jar.mn
> > @@ +1,1 @@
> > > +#filter substitution
> > 
> > shouldn't you have MPL header here?
> 
> Besides one jar.mn, none have it in the entire project.

ok, I was always curious when they decide to put it and when no

Comment 13

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #12)
> (In reply to Eitan Isaacson [:eeejay] from comment #9)
> > I also need to change
> > MobileAccessibility to something else, maybe AccessibilityHelper?
> 
> helper is not descriptive name and it's not a helper actually, it's primary
> service that manages the accessibility. Perhaps jsAT?

Nobody besides the two of us knows what AT stands for :) If we will be calling this from chrome.js in different apps, it needs to be something universally understood. Maybe ChromeAccessibility?

Also, assistive technology is a dying term. Even my spellcheck doesn't like it :)

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #13)
> > helper is not descriptive name and it's not a helper actually, it's primary
> > service that manages the accessibility. Perhaps jsAT?
> 
> Nobody besides the two of us knows what AT stands for :) If we will be
> calling this from chrome.js in different apps, it needs to be something
> universally understood. Maybe ChromeAccessibility?

well, I cannot say ChromeAccessibility is too descriptive. If you don't like 'AT' in the name because it's not visible by user then well this object announce the changes, shows something on the screen (and supposed to do a million other things like braille display support), navigate through the page. It makes everything what AT does (except it doesn't have an icon on your desktop).

> Also, assistive technology is a dying term. Even my spellcheck doesn't like
> it :)

something else comes instead? or do they split the term into many pieces?

Comment 15

[Eitan Isaacson [:eeejay]]

Attachment: Bug 739498 - Added Javascript modules for mobile accessibility.

This still needs some dist love, specifically taking out the CSS style bits and merging the localized strings. Besides that, I think I answered almost all the feedback, might have overlooked something. Just putting this here for the record.

Comment 16

[Eitan Isaacson [:eeejay]]

Attachment: Bug 739498 - Added Javascript modules for mobile accessibility. (ver. 2)

I think I answered all the feedback. Here is round 2.

Comment 17

[Eitan Isaacson [:eeejay]]

Attachment: Bug 739498 - Added Javascript modules for mobile accessibility. (ver. 2).

Rebased this on top of current trunk.

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 613332 [details] [diff] [review]
Bug 739498 - Added Javascript modules for mobile accessibility. (ver. 2).

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

I don't feel myself that I should stop you until you polish things since the work is much in progress and I expect that interfaces and implementation are subject to change but it'd be nice to get another review.

::: accessible/src/base/Role.h
@@ +40,5 @@
>  #ifndef _role_h_
>  #define _role_h_
>  
> +/**
> + * Note: Make sure to update the localized role names when extending this list.

nit: changing the list

::: accessible/src/jsat/AccessFu.css
@@ +1,1 @@
> +#virtual-cursor-box { 

MPL header please

::: accessible/src/jsat/AccessFu.jsm
@@ +6,5 @@
> +
> +var Cc = Components.classes;
> +var Ci = Components.interfaces;
> +var Cu = Components.utils;
> +var Cr = Components.results;

any reason to not keep them const?

@@ +15,5 @@
> +
> +Cu.import('resource://gre/modules/accessibility/Presenters.jsm');
> +Cu.import('resource://gre/modules/accessibility/VirtualCursorController.jsm');
> +
> +var AccessFu = {

cool name but don't try to translate that into russian (not sure though if 'fu' is all about kungfu in english).

@@ +16,5 @@
> +Cu.import('resource://gre/modules/accessibility/Presenters.jsm');
> +Cu.import('resource://gre/modules/accessibility/VirtualCursorController.jsm');
> +
> +var AccessFu = {
> +  _pendingDocuments: {},

I'd prefer to keep internals after public methods

@@ +17,5 @@
> +Cu.import('resource://gre/modules/accessibility/VirtualCursorController.jsm');
> +
> +var AccessFu = {
> +  _pendingDocuments: {},
> +  attach: function attach(aWindow) {

please document interface methods

@@ +37,5 @@
> +      }
> +      return false;
> +    }
> +
> +    if (checkA11y())

shouldn't the caller make sure a11y is enabled, at least this is inconsistent between platforms, so if you create AccessFu on desktop then it just starts working but not on Android

@@ +41,5 @@
> +    if (checkA11y())
> +      this.startup();
> +  },
> +
> +  startup: function startup() {

it's a little bit strange that attach() check if a11y is enabled but startup just starts it. Please keep public and private method separate, you can do that by comment like "// Implementation details"

@@ +63,5 @@
> +  },
> +
> +  addPresenter: function addPresenter(presenter) {
> +    if (!this.presenters)
> +      this.presenters = [];

I'd have a filed this.presenters = [] rather than create an array on demand

@@ +66,5 @@
> +    if (!this.presenters)
> +      this.presenters = [];
> +
> +    this.presenters.push(presenter);
> +    presenter.attach(this.chromeWin);

and then perhaps you don't need two lines body method

@@ +100,5 @@
> +        let activatedAcc = getAccessible(aEvent.originalTarget);
> +        let state = {};
> +        activatedAcc.getState(state, {});
> +        if (state.value & Ci.nsIAccessibleStates.STATE_CHECKABLE)
> +          return;

it makes sense to add a comment why you do that becuase until you look at handleAccEvent (and knows some internals) it's not clear what you do here

@@ +124,5 @@
> +    if (!docAcc) {
> +      // Wait for a reorder event fired by the parent of the new doc.
> +      this._pendingDocuments[browserApp.selectedBrowser] = aCallback;
> +    } else {
> +      aCallback.apply(this, [docAcc]);

I assume you don't care whether document was loaded or not, whether it's still empty or got something

@@ +134,5 @@
> +      case 'accessible-event':
> +        let event;
> +        try {
> +          event = aSubject.QueryInterface(Ci.nsIAccessibleEvent);
> +          this.handleAccEvent(event);

protection from add-ons or stuffs like this? because 'accessible-event' observer triggers for accessible events only

@@ +153,5 @@
> +            QueryInterface(Ci.nsIAccessibleVirtualCursorChangeEvent);
> +
> +          let newContext = this.getNewContext(event.oldAccessible,
> +                                              pivot.position,
> +                                              aEvent.accessible);

if Android presenter is unique consumer of newContext (and you can't think of other applicants) then it makes sense to make AndroidPresenter to calculate newContext

@@ +157,5 @@
> +                                              aEvent.accessible);
> +          this.presenters.forEach(
> +            function(p) {
> +              p.pivotChanged(pivot.position, newContext);
> +            });

since not every presenter needs new context then I would make

@@ +178,5 @@
> +      case Ci.nsIAccessibleEvent.EVENT_REORDER:
> +        {
> +          let node = aEvent.accessible.DOMNode;
> +          let callback = this._pendingDocuments[node];
> +          if (callback && aEvent.accessible.childCount) {

aEvent.accessible.childCount shouldn't be ever 0 for reorder event in your case, otherwise that means the document was destroyed. Is that something you protect from?

@@ +206,5 @@
> +    }
> +
> +    newLineage.reverse();
> +    oldLineage.reverse();
> +    let i = 0;

if you have two indecies then you can traverse arrays without reversing, that's cheaper.

::: accessible/src/jsat/Presenters.jsm
@@ +25,5 @@
> +Presenter.prototype = {
> +  /**
> +   * The amount of padding in pixels to put around a ring.
> +   */
> +  RING_PADDING: 2,

I'm wondering what ring AndroidPresenter has

@@ +48,5 @@
> +  pivotChanged: function pivotChanged(aObject, aNewContext) {},
> +
> +  /**
> +   * An object's action has been invoked.
> +   * @param {nsIAccessible} aObject the object that has been activated.

has been invoked

@@ +104,5 @@
> +  this.stylesheet = aWindow.document.createProcessingInstruction(
> +    'xml-stylesheet', 'href="' + stylesheetURL + '" type="text/css"');
> +  aWindow.document.insertBefore(this.stylesheet, aWindow.document.firstChild);
> +
> +  // Add

add what perhaps

@@ +124,5 @@
> +  this.highlightBox.parentNode.removeChild(this.highlightBox);
> +  this.highlightBox = this.stylesheet = null;
> +};
> +
> +VisualPresenter.prototype.hide = function hide() {

please keep presenter interface implementation separate from internals

@@ +132,5 @@
> +VisualPresenter.prototype.viewportChanged = function() {
> +  if (this._currentObject)
> +    this.highlight(this._currentObject);
> +  else
> +    this.hide();

why do you need to hide it because it seems only pivotChanged can null out it and it makes sure to hide it?

@@ +170,5 @@
> +  let rv = {
> +    left: Math.round((objX.value - docX.value - this.RING_PADDING) * aZoom),
> +    top: Math.round((objY.value - docY.value - this.RING_PADDING) * aZoom),
> +    width: Math.round((objW.value + (this.RING_PADDING * 2)) * aZoom),
> +    height: Math.round((objH.value + (this.RING_PADDING * 2)) * aZoom)

still not sure how it works since you get coordinates relative tab document but draw the box relative chrome document, you do * aZoom but * aZoom should be a part of values since they are in device pixels. Anyway, if it works then I don't mind.

@@ +217,5 @@
> +AndroidPresenter.prototype.pivotChanged = function(aObject, aNewContext) {
> +  let output = [];
> +  for (let i in aNewContext)
> +    output.push.apply(output,
> +                      UtteranceGenerator.genForObject(aNewContext[i]));

actually that should produce a lot of noise in case of tabbing. I feel like you should do something smart here.

@@ +220,5 @@
> +    output.push.apply(output,
> +                      UtteranceGenerator.genForObject(aNewContext[i]));
> +
> +  output.push.apply(output,
> +      UtteranceGenerator.genForObject(aObject, true));

nit: wrong indentation

@@ +247,5 @@
> +  let context = [];
> +
> +  let parent = vcDoc.virtualCursor.position || aObject;
> +  while ((parent = parent.parent))
> +    context.push(parent);

and a funny thing that you're going to announce chrome parts when you switch between tabs (maybe your generator doesn't support those chrome elements so you don't see an issue)

@@ +259,5 @@
> +    getService(Ci.nsIAndroidBridge).
> +    handleGeckoMessage(JSON.stringify(aMessage));
> +};
> +
> +/*

nit /**

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +14,5 @@
> +const INCLUDE_CUSTOM = 0x04;
> +
> +var stringBundle = Cc['@mozilla.org/intl/stringbundle;1'].
> +  getService(Ci.nsIStringBundleService).
> +  createBundle('chrome://global/locale/AccessFu.properties');

globals should be prefixed by 'g', iirc we talked about keeping them as the object members, not sure what was decision. But curious if you keep them as globals then don't you pollute global scope by that, i.e. can you run into name collisions?

@@ +23,5 @@
> +var EXPORTED_SYMBOLS = ['UtteranceGenerator'];
> +
> +var UtteranceGenerator = {
> +  gActionMap: {
> +    jump: 'actionJumped',

can action really jump? maybe jumpAction?

@@ +38,5 @@
> +    activate: 'actionActivated',
> +    cycle: 'actionCycled'
> +  },
> +
> +  genForObject: function(aAccessible, forceName) {

forceName -> aForceName

@@ +56,5 @@
> +  genForAction: function(aObject, aActionName) {
> +    return stringBundle.GetStringFromName(this.gActionMap[aActionName]);
> +  },
> +
> +  verbosityMap: {

verbosityRoleMap?

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +57,5 @@
> +        break;
> +      case aEvent.DOM_VK_RETURN:
> +        /* It is true that desktop does not map the kp enter key to ENTER.
> +         * So for desktop we require a ctrl+return instead.
> +         */

nit: use // comments

@@ +68,5 @@
> +        return;
> +    }
> +
> +    aEvent.preventDefault();
> +    aEvent.stopPropagation();

how does it work on controls?

@@ +83,5 @@
> +
> +  moveBackward: function moveBackward(document, first) {
> +    let virtualCursor = this.getVirtualCursor(document);
> +    if (first) {
> +      virtualCursor.moveFirst(this.SimpleTraversalRule);

nit: I'd prefer to return here

@@ +105,5 @@
> +        continue;
> +      acc.doAction(0);
> +      break;
> +    } while ((acc = acc.parent) &&
> +             acc.role != Ci.nsIAccessibleRole.ROLE_DOCUMENT);

I'm still not sure I've got an answer why you can't do acc.doAction(0);

@@ +126,5 @@
> +    match: function(aAccessible) {
> +      let rv = Ci.nsIAccessibleTraversalRule.FILTER_IGNORE;
> +      if (aAccessible.childCount == 0) {
> +        let ignoreRoles = [Ci.nsIAccessibleRole.ROLE_WHITESPACE,
> +                           Ci.nsIAccessibleRole.ROLE_STATICTEXT];

not sure what's the status of CSS generated content

@@ +127,5 @@
> +      let rv = Ci.nsIAccessibleTraversalRule.FILTER_IGNORE;
> +      if (aAccessible.childCount == 0) {
> +        let ignoreRoles = [Ci.nsIAccessibleRole.ROLE_WHITESPACE,
> +                           Ci.nsIAccessibleRole.ROLE_STATICTEXT];
> +        let hasName = (aAccessible.name && aAccessible.name.trim());

iirc I told about trim already

@@ +131,5 @@
> +        let hasName = (aAccessible.name && aAccessible.name.trim());
> +        let state = {};
> +        aAccessible.getState(state, {});
> +        if ((hasName && ignoreRoles.indexOf(aAccessible.role) < 0) ||
> +            (state.value & Ci.nsIAccessibleStates.STATE_FOCUSABLE))

I would check states and then if it's not focusable then check roles. You shouldn't calcualte both if one of them can be matched only. Btw, are whitespace or static text can be focusable?

Comment 19

[Eitan Isaacson [:eeejay]]

Attachment: Bug 739498 - Added Javascript modules for mobile accessibility. (ver. 3).

Comment 20

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #18)
> Comment on attachment 613332 [details] [diff] [review]
> Bug 739498 - Added Javascript modules for mobile accessibility. (ver. 2).
> 
> Review of attachment 613332 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: accessible/src/jsat/AccessFu.css
> @@ +37,5 @@
> > +      }
> > +      return false;
> > +    }
> > +
> > +    if (checkA11y())
> 
> shouldn't the caller make sure a11y is enabled, at least this is
> inconsistent between platforms, so if you create AccessFu on desktop then it
> just starts working but not on Android
> 

If you attach AccessFu on desktop than you need to explicitly start AccessFu by calling startup(). On Android we sniff out a11y, and if there are more "supported" platforms in the future, like B2G, we will do the same for them.

> @@ +41,5 @@
> > +    if (checkA11y())
> > +      this.startup();
> > +  },
> > +
> > +  startup: function startup() {
> 
> it's a little bit strange that attach() check if a11y is enabled but startup
> just starts it. Please keep public and private method separate, you can do
> that by comment like "// Implementation details"
> 

Ok, this is obviously confusing, I renamed startup() and shutdown() to enable/disable. It is not part of attach() because eventually AccessFu should be able to be toggled on the fly as the user changes settings without a browser restart.

> @@ +134,5 @@
> > +      case 'accessible-event':
> > +        let event;
> > +        try {
> > +          event = aSubject.QueryInterface(Ci.nsIAccessibleEvent);
> > +          this.handleAccEvent(event);
> 
> protection from add-ons or stuffs like this? because 'accessible-event'
> observer triggers for accessible events only
> 

Gives a useful print when things go wrong, I think it is worth keeping.

> @@ +153,5 @@
> > +            QueryInterface(Ci.nsIAccessibleVirtualCursorChangeEvent);
> > +
> > +          let newContext = this.getNewContext(event.oldAccessible,
> > +                                              pivot.position,
> > +                                              aEvent.accessible);
> 
> if Android presenter is unique consumer of newContext (and you can't think
> of other applicants) then it makes sense to make AndroidPresenter to
> calculate newContext
> 

No, it will come up again in B2G, and maybe in braille. But perhaps this should be in the presenter meta class... OTOH, for performance reasons it makes sense to do it once and not in each presenter. I'll leave it as-is for now.

> @@ +157,5 @@
> > +                                              aEvent.accessible);
> > +          this.presenters.forEach(
> > +            function(p) {
> > +              p.pivotChanged(pivot.position, newContext);
> > +            });
> 
> since not every presenter needs new context then I would make
> 

Yes, but there might be more than one in the future.

> @@ +178,5 @@
> > +      case Ci.nsIAccessibleEvent.EVENT_REORDER:
> > +        {
> > +          let node = aEvent.accessible.DOMNode;
> > +          let callback = this._pendingDocuments[node];
> > +          if (callback && aEvent.accessible.childCount) {
> 
> aEvent.accessible.childCount shouldn't be ever 0 for reorder event in your
> case, otherwise that means the document was destroyed. Is that something you
> protect from?
> 

Yup.

> @@ +206,5 @@
> > +    }
> > +
> > +    newLineage.reverse();
> > +    oldLineage.reverse();
> > +    let i = 0;
> 
> if you have two indecies then you can traverse arrays without reversing,
> that's cheaper.
> 

I tried that, and my head almost popped. I'll give it another try.

> ::: accessible/src/jsat/Presenters.jsm
> @@ +170,5 @@
> > +  let rv = {
> > +    left: Math.round((objX.value - docX.value - this.RING_PADDING) * aZoom),
> > +    top: Math.round((objY.value - docY.value - this.RING_PADDING) * aZoom),
> > +    width: Math.round((objW.value + (this.RING_PADDING * 2)) * aZoom),
> > +    height: Math.round((objH.value + (this.RING_PADDING * 2)) * aZoom)
> 
> still not sure how it works since you get coordinates relative tab document
> but draw the box relative chrome document, you do * aZoom but * aZoom should
> be a part of values since they are in device pixels. Anyway, if it works
> then I don't mind.
> 

The aZoom is for Android where the viewport zoom varies.

> @@ +217,5 @@
> > +AndroidPresenter.prototype.pivotChanged = function(aObject, aNewContext) {
> > +  let output = [];
> > +  for (let i in aNewContext)
> > +    output.push.apply(output,
> > +                      UtteranceGenerator.genForObject(aNewContext[i]));
> 
> actually that should produce a lot of noise in case of tabbing. I feel like
> you should do something smart here.
> 

Yeah, that might be a usability issue. I'll change it when we get more input on it.

> @@ +247,5 @@
> > +  let context = [];
> > +
> > +  let parent = vcDoc.virtualCursor.position || aObject;
> > +  while ((parent = parent.parent))
> > +    context.push(parent);
> 
> and a funny thing that you're going to announce chrome parts when you switch
> between tabs (maybe your generator doesn't support those chrome elements so
> you don't see an issue)
> 

That is exactly what the generator is supposed to do.

> ::: accessible/src/jsat/UtteranceGenerator.jsm
> @@ +14,5 @@
> > +const INCLUDE_CUSTOM = 0x04;
> > +
> > +var stringBundle = Cc['@mozilla.org/intl/stringbundle;1'].
> > +  getService(Ci.nsIStringBundleService).
> > +  createBundle('chrome://global/locale/AccessFu.properties');
> 
> globals should be prefixed by 'g', iirc we talked about keeping them as the
> object members, not sure what was decision. But curious if you keep them as
> globals then don't you pollute global scope by that, i.e. can you run into
> name collisions?
> 

No, that is what I was saying about js modules, the only thing in the namespace of what imports this is in EXPORTED_SYMBOLS. I tried adding it as an object member, but it gave me a scope headache because of nested objects and the usual ambiguities of "this". I'll change to gStringBundle *giggle*.

> @@ +23,5 @@
> > +var EXPORTED_SYMBOLS = ['UtteranceGenerator'];
> > +
> > +var UtteranceGenerator = {
> > +  gActionMap: {
> > +    jump: 'actionJumped',
> 
> can action really jump? maybe jumpAction?
> 

That is a mapping from the traditional nsIAccessible action names to past tense verbs. Neither makes grammatical sense, it is just a prefix to group those actions together.

> ::: accessible/src/jsat/VirtualCursorController.jsm
> @@ +68,5 @@
> > +        return;
> > +    }
> > +
> > +    aEvent.preventDefault();
> > +    aEvent.stopPropagation();
> 
> how does it work on controls?
> 

Not well in desktop, obviously. But in Android, a popup native control typically comes up, and then it works fine. I am sure there are edge cases here.

> @@ +105,5 @@
> > +        continue;
> > +      acc.doAction(0);
> > +      break;
> > +    } while ((acc = acc.parent) &&
> > +             acc.role != Ci.nsIAccessibleRole.ROLE_DOCUMENT);
> 
> I'm still not sure I've got an answer why you can't do acc.doAction(0);
> 

Oversight, fixed.

> @@ +126,5 @@
> > +    match: function(aAccessible) {
> > +      let rv = Ci.nsIAccessibleTraversalRule.FILTER_IGNORE;
> > +      if (aAccessible.childCount == 0) {
> > +        let ignoreRoles = [Ci.nsIAccessibleRole.ROLE_WHITESPACE,
> > +                           Ci.nsIAccessibleRole.ROLE_STATICTEXT];
> 
> not sure what's the status of CSS generated content
> 

Me neither, leaving this as-is for now.

Comment 21

[Hubert Figuiere [:hub]]

Comment on attachment 613709 [details] [diff] [review]
Bug 739498 - Added Javascript modules for mobile accessibility. (ver. 3).

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

::: accessible/src/base/Role.h
@@ +42,5 @@
>  
> +/**
> + * Note: Make sure to update the localized role names when changing the list.
> + */
> +

Ideally you would indicate which file that is in, at least.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #20)

> > shouldn't the caller make sure a11y is enabled, at least this is
> > inconsistent between platforms, so if you create AccessFu on desktop then it
> > just starts working but not on Android
> > 
> 
> If you attach AccessFu on desktop than you need to explicitly start AccessFu
> by calling startup(). On Android we sniff out a11y, and if there are more
> "supported" platforms in the future, like B2G, we will do the same for them.

are you saying that I should use AccessFu differently depending on platform? i.e. on desktop I call startup(), on Android I call 'attach'? Anyway the question remains why do you think AccessFu should sniffing accessibility on some platforms and that is not something that the caller should take care of?

> > it's a little bit strange that attach() check if a11y is enabled but startup
> > just starts it. Please keep public and private method separate, you can do
> > that by comment like "// Implementation details"

> Ok, this is obviously confusing, I renamed startup() and shutdown() to
> enable/disable.

Not sure that enable/disable are clear. AccessFu.enable() makes me think I enabled some access fu thing and no idea what it could mean.

> It is not part of attach() because eventually AccessFu
> should be able to be toggled on the fly as the user changes settings without
> a browser restart.

perhaps attach/detach is enough then.

> > @@ +134,5 @
> > > +          event = aSubject.QueryInterface(Ci.nsIAccessibleEvent);
> > > +          this.handleAccEvent(event);
> > 
> > protection from add-ons or stuffs like this? because 'accessible-event'
> > observer triggers for accessible events only
> Gives a useful print when things go wrong, I think it is worth keeping.
ok, up to you

> > if Android presenter is unique consumer of newContext (and you can't think
> > of other applicants) then it makes sense to make AndroidPresenter to
> > calculate newContext
> > 
> 
> No, it will come up again in B2G, and maybe in braille. But perhaps this
> should be in the presenter meta class... OTOH, for performance reasons it
> makes sense to do it once and not in each presenter. I'll leave it as-is for
> now.

ok

> > aEvent.accessible.childCount shouldn't be ever 0 for reorder event in your
> > case, otherwise that means the document was destroyed. Is that something you
> > protect from?
> > 
> 
> Yup.

ok, btw, I don't see you answered related issue:
"I assume you don't care whether document was loaded or not, whether it's still empty or got something"

> > if you have two indecies then you can traverse arrays without reversing,
> > that's cheaper.

> I tried that, and my head almost popped. I'll give it another try.

thank you :) If you like then ping Andrzej, he likes alg stuff :)

> > still not sure how it works since you get coordinates relative tab document
> > but draw the box relative chrome document, you do * aZoom but * aZoom should
> > be a part of values since they are in device pixels. Anyway, if it works
> > then I don't mind.
> > 
> 
> The aZoom is for Android where the viewport zoom varies.

ok, you might want to compare with how we transform CSS to dev pixels (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/layout.js#162).

> > > +  let output = [];
> > > +  for (let i in aNewContext)
> > > +    output.push.apply(output,
> > > +                      UtteranceGenerator.genForObject(aNewContext[i]));
> > 
> > actually that should produce a lot of noise in case of tabbing. I feel like
> > you should do something smart here.
> > 
> 
> Yeah, that might be a usability issue. I'll change it when we get more input
> on it.

ok

> > and a funny thing that you're going to announce chrome parts when you switch
> > between tabs (maybe your generator doesn't support those chrome elements so
> > you don't see an issue)
> > 
> 
> That is exactly what the generator is supposed to do.

ok, though that's strange, since why I would need to hear hierarchy above the tab when I switch between tabs. I'd say I should be interested in where the pivot is in new tab. Say I have hierarchy:
Firefox window
  container for all UI
    fancy container for tabs
      tab#1, tab#2

and switching between tabs makes me hear "Firefox window. Container for all UI. Fancy container for tabs. tab#N".

> > ::: accessible/src/jsat/UtteranceGenerator.jsm
> > globals should be prefixed by 'g', iirc we talked about keeping them as the
> > object members, not sure what was decision. But curious if you keep them as
> > globals then don't you pollute global scope by that, i.e. can you run into
> > name collisions?
> > 
> No, that is what I was saying about js modules, the only thing in the
> namespace of what imports this is in EXPORTED_SYMBOLS. I tried adding it as
> an object member, but it gave me a scope headache because of nested objects
> and the usual ambiguities of "this". I'll change to gStringBundle *giggle*.

ok

> > can action really jump? maybe jumpAction?
> > 
> That is a mapping from the traditional nsIAccessible action names to past
> tense verbs. Neither makes grammatical sense, it is just a prefix to group
> those actions together.

yeah, I see. But still don't get why 'actionJumped' (which is funny anyway ;) ) is preferred over 'jumpAction' (which is traditional nsIAccessible action name, makes grammatical sense and just postfixed by 'Action' word to group those actions together :) )

> > > +    aEvent.preventDefault();
> > > +    aEvent.stopPropagation();
> > 
> > how does it work on controls?
> > 
> Not well in desktop, obviously. But in Android, a popup native control
> typically comes up, and then it works fine. I am sure there are edge cases
> here.

k

> > not sure what's the status of CSS generated content
> > 
> Me neither, leaving this as-is for now.

k, you might want to have XXX comment

Comment 23

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

Comment on attachment 613709 [details] [diff] [review]
Bug 739498 - Added Javascript modules for mobile accessibility. (ver. 3).

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

r=me. Since this has had careful review I did a high altitude review (~90 minutes today + some curiosity snooping on earlier reviews) mostly to check out the design. The nit questions I have don't require a new patch. I want to try this out again and debug it soon (not blocking landing).

At some point (later) I'd be curious to see toolkit/firefox/fennec peer review on jsat. That could happen via blogging the design. Oh that reminds me... gotta setup our team blog.

::: accessible/src/jsat/AccessFu.jsm
@@ +52,5 @@
> +
> +  /**
> +   * Start the special AccessFu mode, this primarily means controlling the virtual
> +   * cursor with arrow keys. Currently, on platforms other than Android this needs
> +   * to be called explicitly.

On Android (via this.addPresenter(new AndroidPresenter()) this also means utterances right?

@@ +243,5 @@
> +    return newLineage.slice(i, newLineage.length);
> +  },
> +
> +  // A hash of documents that don't yet have an accessible tree.
> +  _pendingDocuments: {}

Aside: I'm curious what is a typical size for different cases? Maybe I can easily test this myself?

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +10,5 @@
> +const Cr = Components.results;
> +
> +const INCLUDE_ROLE = 0x01;
> +const INCLUDE_NAME = 0x02;
> +const INCLUDE_CUSTOM = 0x04;

Note INCLUDE_CUSTOM is unused. (If you want to group these (I probably wouldn't) you could do: const INCLUDE = ( role: 1, name :2 ...) and access via INCLUDE.role ... )

::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +menubar        =       Menu bar

Aside: I'm curious why the values are capitalized. Are we sure we will always want these to be the first part of an utterance? Perhaps capitalization might have more important impact in speech engines for some locales?

Comment 24

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #22)
> (In reply to Eitan Isaacson [:eeejay] from comment #20)
> 
> > > shouldn't the caller make sure a11y is enabled, at least this is
> > > inconsistent between platforms, so if you create AccessFu on desktop then it
> > > just starts working but not on Android
> > > 
> > 
> > If you attach AccessFu on desktop than you need to explicitly start AccessFu
> > by calling startup(). On Android we sniff out a11y, and if there are more
> > "supported" platforms in the future, like B2G, we will do the same for them.
> 
> are you saying that I should use AccessFu differently depending on platform?
> i.e. on desktop I call startup(), on Android I call 'attach'? Anyway the
> question remains why do you think AccessFu should sniffing accessibility on
> some platforms and that is not something that the caller should take care of?
> 

The idea here was to encapsulate the sniffing in AccessFu and not require different browsers/platforms to have to do it. So you attach it once, and it gets enabled/disabled automatically depending on if a11y is toggled in the platform. That is why attach/detach is separate from enable/disable. Desktop is not a supported platform, so we need to enable and add presenters manually:

    Cu.import("resource://gre/modules/accessibility/AccessFu.jsm");
    Cu.import("resource://gre/modules/accessibility/Presenters.jsm");

    AccessFu.attach(win);
    AccessFu.addPresenter(new DummyAndroidPresenter());
    AccessFu.enable();


> > > it's a little bit strange that attach() check if a11y is enabled but startup
> > > just starts it. Please keep public and private method separate, you can do
> > > that by comment like "// Implementation details"
> 
> > Ok, this is obviously confusing, I renamed startup() and shutdown() to
> > enable/disable.
> 
> Not sure that enable/disable are clear. AccessFu.enable() makes me think I
> enabled some access fu thing and no idea what it could mean.
> 

That is exactly what it does, it enables AccessFu.

> > It is not part of attach() because eventually AccessFu
> > should be able to be toggled on the fly as the user changes settings without
> > a browser restart.
> 
> perhaps attach/detach is enough then.
> 

For supported platforms attach/detach is all they need.

> > > aEvent.accessible.childCount shouldn't be ever 0 for reorder event in your
> > > case, otherwise that means the document was destroyed. Is that something you
> > > protect from?
> > > 
> > 
> > Yup.
> 
> ok, btw, I don't see you answered related issue:
> "I assume you don't care whether document was loaded or not, whether it's
> still empty or got something"
> 

We will work that out at some point. Right now it is fine if it is empty. The VC works fine when children are added.

> > > and a funny thing that you're going to announce chrome parts when you switch
> > > between tabs (maybe your generator doesn't support those chrome elements so
> > > you don't see an issue)
> > > 
> > 
> > That is exactly what the generator is supposed to do.
> 
> ok, though that's strange, since why I would need to hear hierarchy above
> the tab when I switch between tabs. I'd say I should be interested in where
> the pivot is in new tab. Say I have hierarchy:
> Firefox window
>   container for all UI
>     fancy container for tabs
>       tab#1, tab#2
> 
> and switching between tabs makes me hear "Firefox window. Container for all
> UI. Fancy container for tabs. tab#N".
> 

You won't hear the hierarchy above the tab, for two reasons:
1. The hierarchy only goes up to the document.
2. Only items of interest have utterances generated.

So typically, you will hear: "Eitan's blog, Link, About".

> > > can action really jump? maybe jumpAction?
> > > 
> > That is a mapping from the traditional nsIAccessible action names to past
> > tense verbs. Neither makes grammatical sense, it is just a prefix to group
> > those actions together.
> 
> yeah, I see. But still don't get why 'actionJumped' (which is funny anyway
> ;) ) is preferred over 'jumpAction' (which is traditional nsIAccessible
> action name, makes grammatical sense and just postfixed by 'Action' word to
> group those actions together :) )
> 

Fine! I'll change it :)

> 
> > > not sure what's the status of CSS generated content
> > > 
> > Me neither, leaving this as-is for now.
> 
> k, you might want to have XXX comment

Will do.

Comment 25

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #23)
> Comment on attachment 613709 [details] [diff] [review]
> Bug 739498 - Added Javascript modules for mobile accessibility. (ver. 3).
> 
> Review of attachment 613709 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. Since this has had careful review I did a high altitude review (~90
> minutes today + some curiosity snooping on earlier reviews) mostly to check
> out the design. The nit questions I have don't require a new patch. I want
> to try this out again and debug it soon (not blocking landing).
> 

The visual presenter is currently broken on Android, bug 743993.

> At some point (later) I'd be curious to see toolkit/firefox/fennec peer
> review on jsat. That could happen via blogging the design. Oh that reminds
> me... gotta setup our team blog.
> 

So should we assign another reviewer here, or check it in?

> ::: accessible/src/jsat/AccessFu.jsm
> @@ +52,5 @@
> > +
> > +  /**
> > +   * Start the special AccessFu mode, this primarily means controlling the virtual
> > +   * cursor with arrow keys. Currently, on platforms other than Android this needs
> > +   * to be called explicitly.
> 
> On Android (via this.addPresenter(new AndroidPresenter()) this also means
> utterances right?
> 

Yeah, the utterance generator is mostly a utility class that AndroidPresenter uses.

> @@ +243,5 @@
> > +    return newLineage.slice(i, newLineage.length);
> > +  },
> > +
> > +  // A hash of documents that don't yet have an accessible tree.
> > +  _pendingDocuments: {}
> 
> Aside: I'm curious what is a typical size for different cases? Maybe I can
> easily test this myself?
> 

Almost always 1. It is when a user creates a new tab, the time is almost unnoticeable as to when the reorder event comes in (and the item is removed from the hash). So unless tabs are being created at a very high speed, this should never exceed one.

> ::: accessible/src/jsat/UtteranceGenerator.jsm
> @@ +10,5 @@
> > +const Cr = Components.results;
> > +
> > +const INCLUDE_ROLE = 0x01;
> > +const INCLUDE_NAME = 0x02;
> > +const INCLUDE_CUSTOM = 0x04;
> 
> Note INCLUDE_CUSTOM is unused. (If you want to group these (I probably
> wouldn't) you could do: const INCLUDE = ( role: 1, name :2 ...) and access
> via INCLUDE.role ... )
> 

I like that. I guess CUSTOM could go for now...

> ::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
> @@ +1,5 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > +# You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +menubar        =       Menu bar
> 
> Aside: I'm curious why the values are capitalized. Are we sure we will
> always want these to be the first part of an utterance? Perhaps
> capitalization might have more important impact in speech engines for some
> locales?

I forgot why this is capitalized, maybe because TalkBack capitalizes role utterances. It doesn't need to be. Probably shouldn't be...

Comment 26

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

(In reply to Eitan Isaacson [:eeejay] from comment #25)
> (In reply to David Bolter [:davidb] from comment #23)
> > At some point (later) I'd be curious to see toolkit/firefox/fennec peer
> > review on jsat. That could happen via blogging the design. Oh that reminds
> > me... gotta setup our team blog.
> > 
> 
> So should we assign another reviewer here, or check it in?

I wouldn't hold back landing for it.


> > @@ +243,5 @@
> > > +    return newLineage.slice(i, newLineage.length);
> > > +  },
> > > +
> > > +  // A hash of documents that don't yet have an accessible tree.
> > > +  _pendingDocuments: {}
> > 
> > Aside: I'm curious what is a typical size for different cases? Maybe I can
> > easily test this myself?
> > 
> 
> Almost always 1. It is when a user creates a new tab, the time is almost
> unnoticeable as to when the reorder event comes in (and the item is removed
> from the hash). So unless tabs are being created at a very high speed, this
> should never exceed one.

OK that's what I thought - thanks.

> > > +menubar        =       Menu bar
> > 
> > Aside: I'm curious why the values are capitalized. Are we sure we will
> > always want these to be the first part of an utterance? Perhaps
> > capitalization might have more important impact in speech engines for some
> > locales?
> 
> I forgot why this is capitalized, maybe because TalkBack capitalizes role
> utterances. It doesn't need to be. Probably shouldn't be...

Not a big deal (I don't think).

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #24)

> > are you saying that I should use AccessFu differently depending on platform?
> > i.e. on desktop I call startup(), on Android I call 'attach'? Anyway the
> > question remains why do you think AccessFu should sniffing accessibility on
> > some platforms and that is not something that the caller should take care of?
> > 
> 
> The idea here was to encapsulate the sniffing in AccessFu and not require
> different browsers/platforms to have to do it. So you attach it once, and it
> gets enabled/disabled automatically depending on if a11y is toggled in the
> platform.

API usage depending on platform is not good idea. I think I'm fine if AccessFu doens't get working on if platform doesn't allow it working but the way the author uses AccessFu must be the same.

> > Not sure that enable/disable are clear. AccessFu.enable() makes me think I
> > enabled some access fu thing and no idea what it could mean.
> > 
> 
> That is exactly what it does, it enables AccessFu.

the point was it's not clear what 'enable' means for AccessFu. AccessFu is fancy name but it says nearly nothing to the author and thus 'enabled AccessFu' means only 'AccessFu is enabled'. Compare to:

var ObjectName
{
  enable: function() {};
}
ObjectName.enable();

ok I just enabled ObjectName object but I don't have any clue what it means.

> > > It is not part of attach() because eventually AccessFu
> > > should be able to be toggled on the fly as the user changes settings without
> > > a browser restart.
> > 
> > perhaps attach/detach is enough then.
> > 
> 
> For supported platforms attach/detach is all they need.

I think that should be enough for unsupported platform as well.

> > > > and a funny thing that you're going to announce chrome parts when you switch
> > > > between tabs (maybe your generator doesn't support those chrome elements so
> > > > you don't see an issue)
> > > > 
> > > 
> > > That is exactly what the generator is supposed to do.
> > 
> > ok, though that's strange, since why I would need to hear hierarchy above
> > the tab when I switch between tabs. I'd say I should be interested in where
> > the pivot is in new tab. Say I have hierarchy:
> > Firefox window
> >   container for all UI
> >     fancy container for tabs
> >       tab#1, tab#2
> > 
> > and switching between tabs makes me hear "Firefox window. Container for all
> > UI. Fancy container for tabs. tab#N".
> > 
> 
> You won't hear the hierarchy above the tab, for two reasons:
> 1. The hierarchy only goes up to the document.
> 2. Only items of interest have utterances generated.

2nd is a reason I said why it should work for you. But I don't see where 1nd takes a place and it seems this item contradicts to what you said earlier "That is exactly what the generator is supposed to do."

Comment 28

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #27)
> (In reply to Eitan Isaacson [:eeejay] from comment #24)
> 
> > > are you saying that I should use AccessFu differently depending on platform?
> > > i.e. on desktop I call startup(), on Android I call 'attach'? Anyway the
> > > question remains why do you think AccessFu should sniffing accessibility on
> > > some platforms and that is not something that the caller should take care of?
> > > 
> > 
> > The idea here was to encapsulate the sniffing in AccessFu and not require
> > different browsers/platforms to have to do it. So you attach it once, and it
> > gets enabled/disabled automatically depending on if a11y is toggled in the
> > platform.
> 
> API usage depending on platform is not good idea. I think I'm fine if
> AccessFu doens't get working on if platform doesn't allow it working but the
> way the author uses AccessFu must be the same.
> 

I don't understand what you are proposing. Should sniffing not be encapsulated in AccessFu? Right now all you need to do is call attach(). Mochi tests, for example, will not depend on platform a11y being enabled, so there is a need to explicitly enable AccessFu, and you do that with the semi-public method called enable().

> > > Not sure that enable/disable are clear. AccessFu.enable() makes me think I
> > > enabled some access fu thing and no idea what it could mean.
> > > 
> > 
> > That is exactly what it does, it enables AccessFu.
> 
> the point was it's not clear what 'enable' means for AccessFu. AccessFu is
> fancy name but it says nearly nothing to the author and thus 'enabled
> AccessFu' means only 'AccessFu is enabled'. Compare to:
> 
> var ObjectName
> {
>   enable: function() {};
> }
> ObjectName.enable();
> 
> ok I just enabled ObjectName object but I don't have any clue what it means.
> 

This goes back to the highly-emotional debate about what this object is called. If there was an object called Orca, and you did Orca.start(), would you have any doubts about what that did? We just created another thing called AccessFu, which is our "brand name" for a special a11y mode in the browser.

> > > > It is not part of attach() because eventually AccessFu
> > > > should be able to be toggled on the fly as the user changes settings without
> > > > a browser restart.
> > > 
> > > perhaps attach/detach is enough then.
> > > 
> > 
> > For supported platforms attach/detach is all they need.
> 
> I think that should be enough for unsupported platform as well.
> 

So a11y sniffing should be outside the scope of this object? I don't feel too strongly about it. I thought it would be a good choice to remove the burden and complexity from the chrome scripts. If AccessFu is added to a chrome script, like it will soon be added in the Android Browser, than AccessFu needs to support sniffing on that platform, that would be the rule.

So if this were ever added to desktop Firefox, AccessFu would first need to be extended to sniff a11y on desktop platforms.

> > > > > and a funny thing that you're going to announce chrome parts when you switch
> > > > > between tabs (maybe your generator doesn't support those chrome elements so
> > > > > you don't see an issue)
> > > > > 
> > > > 
> > > > That is exactly what the generator is supposed to do.
> > > 
> > > ok, though that's strange, since why I would need to hear hierarchy above
> > > the tab when I switch between tabs. I'd say I should be interested in where
> > > the pivot is in new tab. Say I have hierarchy:
> > > Firefox window
> > >   container for all UI
> > >     fancy container for tabs
> > >       tab#1, tab#2
> > > 
> > > and switching between tabs makes me hear "Firefox window. Container for all
> > > UI. Fancy container for tabs. tab#N".
> > > 
> > 
> > You won't hear the hierarchy above the tab, for two reasons:
> > 1. The hierarchy only goes up to the document.
> > 2. Only items of interest have utterances generated.
> 
> 2nd is a reason I said why it should work for you. But I don't see where 1nd
> takes a place and it seems this item contradicts to what you said earlier
> "That is exactly what the generator is supposed to do."

Disregard point 1.

Comment 29

[Eitan Isaacson [:eeejay]]

Attachment: Bug 739498 - Added Javascript modules for mobile accessibility. (ver. 4).

Comment 30

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 614843
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=344403d56bd4
Try run started, revision 344403d56bd4. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=344403d56bd4

Comment 31

[Mozilla RelEng Bot]

Try run for 344403d56bd4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=344403d56bd4
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-344403d56bd4

Comment 32

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/071ec7e17660

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/071ec7e17660

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #28)

> > API usage depending on platform is not good idea. I think I'm fine if
> > AccessFu doens't get working on if platform doesn't allow it working but the
> > way the author uses AccessFu must be the same.
> > 
> 
> I don't understand what you are proposing. Should sniffing not be
> encapsulated in AccessFu? Right now all you need to do is call attach().
> Mochi tests, for example, will not depend on platform a11y being enabled, so
> there is a need to explicitly enable AccessFu, and you do that with the
> semi-public method called enable().

Let's say you have an object:

var NewYorkCity
{
  goToLibrary: function() { }
  goToBuildingInFrontOfMe: function() { }
}

so you wanna get into library and if you walk then you should call goToLibrary() but you drive then you should call goToBuildingInFrontOfMe (surprise but that's our API), but if you walk and call goToBuildingInFrontOfMe then you get somewhere else.

So what I mean is crossplatform API usage shouldn't depend on platform you running, otherwise there's something wrong with your crossplatform API design.

> This goes back to the highly-emotional debate about what this object is
> called. If there was an object called Orca, and you did Orca.start(), would
> you have any doubts about what that did? We just created another thing
> called AccessFu, which is our "brand name" for a special a11y mode in the
> browser.

ok, if you like it.

> So a11y sniffing should be outside the scope of this object? I don't feel
> too strongly about it. I thought it would be a good choice to remove the
> burden and complexity from the chrome scripts. If AccessFu is added to a
> chrome script, like it will soon be added in the Android Browser, than
> AccessFu needs to support sniffing on that platform, that would be the rule.
> 
> So if this were ever added to desktop Firefox, AccessFu would first need to
> be extended to sniff a11y on desktop platforms.

if you like to keep sniffing inside then it's ok but take care about API design.

> > > You won't hear the hierarchy above the tab, for two reasons:
> > > 1. The hierarchy only goes up to the document.
> > > 2. Only items of interest have utterances generated.
> > 
> > 2nd is a reason I said why it should work for you. But I don't see where 1nd
> > takes a place and it seems this item contradicts to what you said earlier
> > "That is exactly what the generator is supposed to do."
> 
> Disregard point 1.

so we have 2 only and thus the question is: do you need to crawl to top if you can stop on the tab document?

Comment 35

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #34)
> (In reply to Eitan Isaacson [:eeejay] from comment #28)
> 
> > > API usage depending on platform is not good idea. I think I'm fine if
> > > AccessFu doens't get working on if platform doesn't allow it working but the
> > > way the author uses AccessFu must be the same.
> > > 
> > 
> > I don't understand what you are proposing. Should sniffing not be
> > encapsulated in AccessFu? Right now all you need to do is call attach().
> > Mochi tests, for example, will not depend on platform a11y being enabled, so
> > there is a need to explicitly enable AccessFu, and you do that with the
> > semi-public method called enable().
> 
> Let's say you have an object:
> 
> var NewYorkCity
> {
>   goToLibrary: function() { }
>   goToBuildingInFrontOfMe: function() { }
> }
> 
> so you wanna get into library and if you walk then you should call
> goToLibrary() but you drive then you should call goToBuildingInFrontOfMe
> (surprise but that's our API), but if you walk and call
> goToBuildingInFrontOfMe then you get somewhere else.
> 
> So what I mean is crossplatform API usage shouldn't depend on platform you
> running, otherwise there's something wrong with your crossplatform API
> design.
> 

The API is consistent across platforms. There are a couple of methods (enable/disable) that a mochi test could use, for example, to directly manipulate the behaviour to ensure consistent results despite a11y being enabled or disabled on said platform.

> > > > You won't hear the hierarchy above the tab, for two reasons:
> > > > 1. The hierarchy only goes up to the document.
> > > > 2. Only items of interest have utterances generated.
> > > 
> > > 2nd is a reason I said why it should work for you. But I don't see where 1nd
> > > takes a place and it seems this item contradicts to what you said earlier
> > > "That is exactly what the generator is supposed to do."
> > 
> > Disregard point 1.
> 
> so we have 2 only and thus the question is: do you need to crawl to top if
> you can stop on the tab document?

I'm in the process of changing that code. But yes you are right. In Android it is not an issue since there is only one more level above the tab because the XUL chrome is minimal. But this will be fixed the next time i touch that code.

Comment 36

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #35)

> The API is consistent across platforms. There are a couple of methods
> (enable/disable) that a mochi test could use, for example, to directly
> manipulate the behaviour to ensure consistent results despite a11y being
> enabled or disabled on said platform.

please correct me if I'm wrong. So if I want to start AcessFu then I need to do something like:

function startAccessFu(aWindow)
{
  if (IAmOnAndroid) {
    AccessFu.attach(aWindow);
    return;
  }

  if (IAmOnDesktop) {
    AccessFun.attach(aWindow);
    AccessFu.enable();
  }
}

what is the consistent here?

Comment 37

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #36)
> (In reply to Eitan Isaacson [:eeejay] from comment #35)
> 
> > The API is consistent across platforms. There are a couple of methods
> > (enable/disable) that a mochi test could use, for example, to directly
> > manipulate the behaviour to ensure consistent results despite a11y being
> > enabled or disabled on said platform.
> 
> please correct me if I'm wrong. So if I want to start AcessFu then I need to
> do something like:
> 
> function startAccessFu(aWindow)
> {
>   if (IAmOnAndroid) {
>     AccessFu.attach(aWindow);
>     return;
>   }
> 
>   if (IAmOnDesktop) {
>     AccessFun.attach(aWindow);
>     AccessFu.enable();
>   }
> }
> 
> what is the consistent here?

I just opened bug 750528 that will hopefully make you happy.

Comment 38

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #37)

> I just opened bug 750528 that will hopefully make you happy.

it seems so, I thought about this option. thank you.