Frédéric Wang (:fredw) Assignee
	Description • 9 years ago

After bug 1042257, I was able to make the GeckoView accessible, but as I recall the mathematical formulas were still not read correctly (only the text tokens were read, as Orca does in [1]). I suppose this is also what happens for MathML in Web pages when using Firefox for Android/B2G or MathML in Firefox OS Web apps (see [2] for some examples). So I'm opening this bug to see how we can improve MathML accessibility on mobile platforms. I'm not sure how it relates to bug 916419 and the efforts on Desktop, though.

[1] http://www.maths-informatique-jeux.com/blog/frederic/?post/2015/05/06/MathML-Accessibility#mathml_accessibility_audio_demos
[2] https://marketplace.firefox.com/search?author=BELLA+project

	Frédéric Wang (:fredw) Assignee
	Comment 1 • 9 years ago

@Raniere: Are you able to run a screen reader on flatfish? Can you tell me what you get for e.g. [2] or other Web pages with MathML content?

	Raniere Silva
	Comment 2 • 9 years ago

Frédéric,

> Are you able to run a screen reader on flatfish?

Only after I discovered that I need to enable it in the developer options, see bug 1074906.

> Can you tell me what you get for e.g. [2] or other Web pages with MathML content?

I have to swipe from left to right to Firefox OS speak the next element in the tree. For <p> it works as expected but for <math> I have to swipe for every element in the tree and Firefox OS only speak the numbers.

	Frédéric Wang (:fredw) Assignee
	Comment 3 • 9 years ago

(In reply to Raniere Silva from comment #2)
> I have to swipe from left to right to Firefox OS speak the next element in
> the tree. For <p> it works as expected but for <math> I have to swipe for
> every element in the tree and Firefox OS only speak the numbers.

Thanks, that's consistent with what I got on Android (although I'm not sure which MathML token elements are read or not).

	Frédéric Wang (:fredw) Assignee
	Comment 5 • 9 years ago

Just refreshing Yura's patch with the new names.

	Frédéric Wang (:fredw) Assignee
	Comment 8 • 9 years ago

I wanted test mtable, but Fennec crashes so I suspect bug 1178817 will need to be fixed first.

	Frédéric Wang (:fredw) Assignee
	Comment 9 • 9 years ago

Adding support for <mtable>...

	Frédéric Wang (:fredw) Assignee
	Comment 10 • 9 years ago

adding support for mfrac@linethickness & menclose

	Frédéric Wang (:fredw) Assignee
	Comment 11 • 9 years ago

Asking first feedback...

Ideally, I'd like to have something similar to VoiceOver, that is by default the whole formula is spoken with some kind of "natural" reading and it can disambiguated by exploring the formula:

http://www.dessci.com/en/reference/ies-ets/instructional_material/VoiceOver_and_Safari_short.wmv

Joanmarie is doing something similar for Orca, but navigation is not really implemented:

https://vimeo.com/132552987

The current patch for AccessFu tries to keep compatible with the current framework that Yura presented to me, so essentially formulas are read in "postfix/prefix Polish notation".

I've also made the simple TraversalRule ignore the <math> subtree so the whole formula is always read and user can easily move to the text before/after. This works well for simple formulas, but exploration will be essential later for more complex formulas. Maybe we could use "swipe down" similar to Mac's Ctrl+Shift+Apple button for what they call "interaction".

Finally, as Jamie said in another discussion the braille version will probably need more care as just serving the same text as the spoken math is not good. But neither VoiceOver nor Orca do that at the moment, so we'll sync later with them.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 13 • 9 years ago

Comment on attachment 8629936 [details] [diff] [review]
Patch V7

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

Initial comments, let me know what you think.

::: accessible/jsat/OutputGenerator.jsm
@@ +334,5 @@
>      _generateBaseOutput:
>        function _generateBaseOutput(aAccessible, aRoleStr, aState, aFlags) {
>          let output = [];
>  
> +        // Add the math 'role' implied by the parent before the actual role.

I think this should be inside 'if (aFlags & INCLUDE_DESC) {}'  and before the _addRole

@@ +426,5 @@
>        return output;
> +    },
> +
> +    // Use the same output function for all MathML scripted elements.
> +    mathmlmultiscripts: function mathmlmultiscripts(aAccessible) {

This looks almost identical to _generateBaseOutput except for the 'mathmlscripted' part. Could we somehow fold it into addMathRole maybe? That way we would not need to define all these functions.

@@ +432,5 @@
> +        this._addMathRole(output, aAccessible);
> +        this._addRole(output, 'mathmlscripted');
> +        return output;
> +    },
> +    mathmlsub: function mathmlsub() {

Nit: here and below - extra empty line between methods. Also could you break the line on args instead of after the '.'

@@ +473,5 @@
> +      // distinguish between fractions that have a bar and those that do not.
> +      // Per the MathML 3 spec, the latter happens iff the linethickness
> +      // attribute is of the form [zero-float][optional-unit].
> +      let roleStr = aRoleStr;
> +      let linethickness = Utils.getAttributes(aAccessible)['linethickness'];

Calculate roleStr first and then just call:
return this.objectOutputFunctions.defaultFunc.apply(this, [aAccessible, roleStr, aState, aFlags, aContext]);

@@ +485,5 @@
> +      return output;
> +    },
> +
> +    mathmlenclosed: function mathmlenclosed(aAccessible, aRoleStr) {
> +      let output = [];

You can just do:
let output = this.objectOutputFunctions.defaultFunc.apply(this, [aAccessible, aRoleStr, aState, aFlags, aContext]);
this._addNotations(aAccessible, output);
return output;

@@ +488,5 @@
> +    mathmlenclosed: function mathmlenclosed(aAccessible, aRoleStr) {
> +      let output = [];
> +      this._addMathRole(output, aAccessible);
> +      this._addRole(output, aRoleStr);
> +      let notations = Utils.getAttributes(aAccessible)['notation'] || 'longdiv';

Take this bloc out into _addNotation function

@@ +490,5 @@
> +      this._addMathRole(output, aAccessible);
> +      this._addRole(output, aRoleStr);
> +      let notations = Utils.getAttributes(aAccessible)['notation'] || 'longdiv';
> +      notations.split(' ').forEach(function(aNotation) {
> +        output.push({string: this._getOutputName("notation" + aNotation)});

For better readability, have - or _ after notation here and in the properties file.

::: accessible/jsat/Utils.jsm
@@ +903,5 @@
>      let getAccessibleCell = function getAccessibleCell(aAccessible) {
>        if (!aAccessible) {
>          return null;
>        }
> +      if ([Roles.CELL, Roles.COLUMNHEADER, Roles.ROWHEADER,

Nit (readability):  

[
  Roles.CELL,
  Roles.COLUMNHEADER,
  Roles.ROWHEADER,
  Roles.MATHML_CELL
]

@@ +915,5 @@
>          Logger.logException(x);
>          return null;
>        }
>      };
> +    let getHeaders = function getHeaders(aHeaderCells) {

This is a generator, * should remain

::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
@@ +339,5 @@
> +superscriptAbbr    = sup
> +underscriptAbbr    = under
> +
> +notationlongdivAbbr            = longdiv
> +notationactuarialAbbr          = actuarial

act?

	Frédéric Wang (:fredw) Assignee
	Comment 14 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #13)
> 
> I think this should be inside 'if (aFlags & INCLUDE_DESC) {}'  and before
> the _addRole

OK, that's what we initially discussed but then I'll have to add the include desc flag for all the MathML roles (although this may solve another problem I have with the <none> child of<mmultiscripts>...)

	Eitan Isaacson [:eeejay]
	Comment 15 • 9 years ago

Comment on attachment 8629936 [details] [diff] [review]
Patch V7

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

yzen beat me to it!

	Frédéric Wang (:fredw) Assignee
	Comment 16 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #13)
> @@ +426,5 @@
> >        return output;
> > +    },
> > +
> > +    // Use the same output function for all MathML scripted elements.
> > +    mathmlmultiscripts: function mathmlmultiscripts(aAccessible) {
> 
> This looks almost identical to _generateBaseOutput except for the
> 'mathmlscripted' part. Could we somehow fold it into addMathRole maybe? That
> way we would not need to define all these functions.

Yes, all what we want is to remap these roles to "mathmlscripted". Note that addMathRole describes the position inside the parent (e.g. numerator) while addRole is the actual role spoken (e.g. fraction), so I'm not sure we can reuse it. At the end, we will still have to do a conditional on aRoleStr == mathmlmmultiscripts etc What about the attempt in version 4: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1163374&attachment=8628009?

	Frédéric Wang (:fredw) Assignee
	Comment 17 • 9 years ago

> Yes, all what we want is to remap these roles to "mathmlscripted". Note that addMathRole describes the position inside the parent (e.g. numerator) while addRole is the actual role spoken (e.g. fraction), so I'm not sure we can reuse it. At the end, we will still have to do a conditional on aRoleStr == mathmlmmultiscripts etc

So I finally did that using verification on aAccessible.role ; not sure if that's better.

> I think this should be inside 'if (aFlags & INCLUDE_DESC) {}'  and before the _addRole

So the problem was that I need to add the flag INCLUDE_DESC to all MathML elements and then it starts outputing role strings for all of them, which we don't want (for example "mathml identifier" for each variable...). Again, I'm not sure how to do that without adding more conditional on the roles...

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 18 • 9 years ago

Comment on attachment 8630945 [details] [diff] [review]
Patch V8

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

Looks good, hopefully the comments below can help, flag me to take a look once you have them taken care of. Thanks!

::: accessible/jsat/OutputGenerator.jsm
@@ +200,5 @@
> +    let mathRole = Utils.getMathRole(aAccessible);
> +    if (!mathRole) {
> +      return;
> +    }
> +    this._addRole(aOutput, mathRole);

Lets actually wrap this functionality inside _addRole and get rid of _addMathRole completely

so _addRole looks like:

_addRole: function _addRole(aOutput, aAccessible, aRoleStr) {
  let mathRole = Utils.getMathRole(aAccessible);
  if (mathRole) {
    aOutput.push({string: this._getOutputName(mathRole)});
    if (Utils.isScripted(aAccessible)) {
      aOutput.push({string: this._getOutputName("mathmlscripted")});
    }
  } else {
    aOutput.push({string: this._getOutputName(aRoleStr)});
  }
}

Note the argument change here and everywhere else, including Braille definition of this funcion

@@ +209,5 @@
> +   * @param {Array} aOutput Output array.
> +   * @param {nsIAccessible} aAccessible current accessible object.
> +   */
> +  _addMencloseNotations: function _addMencloseNotations(aOutput, aAccessible) {
> +        

Nit: whitespace

@@ +215,5 @@
> +    notations.split(' ').forEach(function(aNotation) {
> +      aOutput.push({string: this._getOutputName('notation-' + aNotation)});
> +    }, this);
> +  },
> +    

Nit: whitespace

@@ +347,5 @@
>      _generateBaseOutput:
>        function _generateBaseOutput(aAccessible, aRoleStr, aState, aFlags) {
>          let output = [];
>  
> +        this._addMathRole(output, aAccessible, aRoleStr);

Won't be needed

@@ +353,4 @@
>          if (aFlags & INCLUDE_DESC) {
>            this._addState(output, aState, aRoleStr);
>            this._addType(output, aAccessible, aRoleStr);
> +          switch(aAccessible.role) {

So this very well can be a utility function in Utils.jsm: Utils.isScripted that will return a true or false.

You should then call it in _addRole (see above)

So this whole block will remain the same:

this._addRole(output, aAccessible, aRoleStr); // Just arguments change

@@ +428,5 @@
>          // We don't want to speak any table information for layout tables.
>          if (table.isProbablyForLayout()) {
>            return output;
>          }
> +        this._addMathRole(output, aAccessible, aRoleStr)

Should not be needed.

@@ +474,5 @@
> +        if (numberMatch && !parseFloat(numberMatch[0])) {
> +          roleStr += 'withoutbar';
> +        }
> +      }
> +      return this.objectOutputFunctions.defaultFunc.apply(this, [aAccessible, roleStr, aState, aFlags, aContext]);

Nit: indentation

@@ +479,5 @@
> +    },
> +
> +    mathmlenclosed: function mathmlenclosed(aAccessible, aRoleStr, aState,
> +                                            aFlags, aContext) {
> +      let output = this.objectOutputFunctions.defaultFunc.apply(this, [aAccessible, aRoleStr, aState, aFlags, aContext]);

Nit: indentation

::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
@@ +322,5 @@
> +mathmlcellAbbr               = cell
> +mathmlfractionAbbr           = frac
> +mathmlfractionwithoutbarAbbr = frac no bar
> +mathmlrootAbbr               = root
> +mathmlscriptedAbbr           = scripted

scr?

	Frédéric Wang (:fredw) Assignee
	Comment 19 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #18)
> Lets actually wrap this functionality inside _addRole and get rid of
> _addMathRole completely
> 
> so _addRole looks like:
> 
> _addRole: function _addRole(aOutput, aAccessible, aRoleStr) {
>   let mathRole = Utils.getMathRole(aAccessible);
>   if (mathRole) {
>     aOutput.push({string: this._getOutputName(mathRole)});
>     if (Utils.isScripted(aAccessible)) {
>       aOutput.push({string: this._getOutputName("mathmlscripted")});
>     }
>   } else {
>     aOutput.push({string: this._getOutputName(aRoleStr)});
>   }
> }

So this does not actually do what we want for example in

<mfrac><mfrac id="id1"><mi>x</mi><mi>y</mi></mfrac><mi id="id2">z</mi></mfrac>

"id1" should have math role "numerator" but role "mathmlfraction" and
"id2" should have math role "denominator" but its "mathmlidentifier" role should not be spoken.

but I think I can just do

_addRole: function _addRole(aOutput, aAccessible, aRoleStr) {
  aOutput.push(_getMathRole(aOutput, aAccessible, aRoleStr) ||
               {string: this._getOutputName(aRoleStr)});
 }

and move the complex logic of

1) concatenating optional math role + normal role
2) or optional math role and not the normal role
3) or optional math role + the renamed "mathmlscripted" role

into _addMathRoles.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 20 • 9 years ago

I might've then been unclear about the rules of calculating the role:
is it as follows: 

3 is clear:
mathmlscripted scripted is optional (based on the switch case you had in the patch) and is added iff it is in that group of roles. Added at the end.

What about the 1 and 2:
* is it true that if there's math role we always add it?
* So i guess the last question is, when do we add normal role and when we do not?

	Frédéric Wang (:fredw) Assignee
	Comment 21 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #20)
> What about the 1 and 2:
> * is it true that if there's math role we always add it?

Yes, we always want to read the "math" role.

> * So i guess the last question is, when do we add normal role and when we do
> not?

Currently, we have roles for 

mathmltable              = math table
mathmlcell               = cell
mathmlenclosed           = enclosed
mathmlfraction           = fraction / mathmlfractionwithoutbar = fraction without bar
mathmlroot               = root	
mathmlscripted           = scripted
mathmlsquareroot         = square root

corresponding to tables, "enclosed" expression, fractions, roots and scripts.

For token MathML elements like <mi>, <mo>, <mn> etc we probably do not want to say "mathml identifier", "mathml operator", "mathml number" etc but just the text. For grouping elements like <mrow>, <mstyle> etc, we don't want to say the role either. So for example

<msup><mi>x</mi><mrow><mo>−</mo><mn>2</mn></mrow></msup>

we just say "x base −2 superscript scripted" and not "x identifier base − operator 2 number group superscript scripted".

For other MathML roles in RoleMap.h it's not clear yet whether we want to speak the role or not. At the moment we do not do anything specific in the accessible code (e.g parse the attributes in some way) and some or even not implemented in the rendering code (elementary map elements for example). These MathML elements are actually not even in the test I added. So I'm happy if we ignore the roles for them for now.

	Frédéric Wang (:fredw) Assignee
	Comment 22 • 9 years ago

OK, what about this one?

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 23 • 9 years ago

Comment on attachment 8632021 [details] [diff] [review]
Patch V9

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

Overall looks good. Final comments, what do you think?

::: accessible/jsat/OutputGenerator.jsm
@@ +1,1 @@
> +/*-*- Mode: Javascript; indent-tabs-mode: nil; js-indent-level: 2 -*- */

We do not add editor settings here.

@@ +200,5 @@
> +   * @param {String} aRoleStr aAccessible's role string.
> +   * @return true iff aAccessible is a MathML accessible.
> +   */
> +  _addMathRoles: function _getMathRoles(aOutput, aAccessible, aRoleStr) {
> +    // First, determine whether this is actually a MathML accessible and, if so,

Can we perhaps add an attribute for all math accessibles and avoid this calculations here?

@@ +431,5 @@
>          if (aFlags & INCLUDE_DESC) {
>            this._addState(output, aState, aRoleStr);
>            this._addType(output, aAccessible, aRoleStr);
> +          if (!this._addMathRoles(output, aAccessible, aRoleStr)) {
> +            this._addRole(output, aRoleStr);

I think it's still ok to just do it all in addRole?

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 24 • 9 years ago

Comment on attachment 8629938 [details] [review]
Localization for Firefox OS

This looks good and should lend after gecko bit is merged. Forwarding to Tim for a formal sign off.

	Frédéric Wang (:fredw) Assignee
	Comment 25 • 9 years ago

mmh, I'm not sure what I did when I tested the previous patch, but the MathML support was broken because of js syntax errors. These and the conflict with bug 1179284 that recently landed are now fixed.

> We do not add editor settings here.

removed (I hoped I didn't add whitespace errors...)

> Can we perhaps add an attribute for all math accessibles and avoid this calculations here?

It's probably worth waiting to be sure about the common MathML accessible tree we want exactly and how to speak the math before doing more backend change. For non-MathML element this is just a switch on a number so it does not add much computation (and is probably even faster than the call to Utils.getMathRole that was used in initial patches).

> I think it's still ok to just do it all in addRole?

I didn't do that in the previous patch because _addMathRoles called _addRole. With a bit more of work, this is now addressed.

	Frédéric Wang (:fredw) Assignee
	Comment 26 • 9 years ago

Comment on attachment 8632109 [details] [diff] [review]
Patch V10

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

::: accessible/jsat/OutputGenerator.jsm
@@ +270,5 @@
> +      aOutput.push({string: this._getOutputName(mathRole)});
> +    }
> +    if (roleStr) {
> +      aOutput.push({string: this._getOutputName(roleStr)});
> +    }

So I think we actually want to invert mathRole & roleStr in the default utterance order.

For example on https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/a11y

<math xmlns="http://www.w3.org/1998/Math/MathML"><mroot><mi>a</mi><msqrt><mi>b</mi></msqrt></mroot></math>

is read "a base b root-index square root root" while it should probably be "a base b square root root-index root"

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 28 • 9 years ago

Comment on attachment 8632109 [details] [diff] [review]
Patch V10

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

::: accessible/jsat/OutputGenerator.jsm
@@ +201,5 @@
> +  _addMathRoles: function _addMathRoles(aOutput, aAccessible, aRoleStr) {
> +    // First, determine whether this is actually a MathML accessible and, if so,
> +    // the actual role to use (e.g. mathmlfraction).
> +    var roleStr = aRoleStr;
> +    switch(aAccessible.role) {

Ok a couple more suggestions, similar to how we have roleRuleMap, lets have another field in the output generator and call it something like:

mathmlRolesSet: new Set([/* list our math roles here */])

see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

This way we can simply check this.mathRolesSet.has(aAccessible.role) instead of if-elsing all of these roles?

Similarly i think it is ok to create mathmlScriptedRolesSet and mathmlOnlyRolesSet and only include scripted ones and mathml-only ones respectively there.

then the addRoles would be something like:

if (this.mathmlRolesSet.has(aAccessible.role)) {
  this._addMathRoles(...); // Returning true or false is not optimal design since the function only talks about adding roles nothing about its result.
} else {
  aOutput.push({string: this._getOutputName(aRoleStr)});
}

and addMathRoles would do something like

let mathRole = Utils.getMathRole(aAccessible);
if (mathRole) {
  aOutput.push({string: this._getOutputName(mathRole)});
}

if (this.mathmlOnlyRolesSet.has(aAccessible.role)) {
  return;
}
// Use unshift instead of push if you want it first here.
...

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 29 • 9 years ago

Comment on attachment 8632157 [details] [diff] [review]
Patch V11

See my last comment for the previous patch

	Frédéric Wang (:fredw) Assignee
	Comment 30 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #28)
> 
> mathmlRolesSet: new Set([/* list our math roles here */])
> 
> if (this.mathmlRolesSet.has(aAccessible.role)) {
>   this._addMathRoles(...); // Returning true or false is not optimal design
> since the function only talks about adding roles nothing about its result.
> } else {
>   aOutput.push({string: this._getOutputName(aRoleStr)});
> }
> 

Thanks, I see that this design is cleaner that the one with _addMathRoles returning a boolean so I will try that. Alternatively, perhaps we can just check whether aRoleStr starts with "mathml" (with perhaps a special case for the <math> tag).

> This way we can simply check this.mathRolesSet.has(aAccessible.role) instead
> of if-elsing all of these roles?
> 
> Similarly i think it is ok to create mathmlScriptedRolesSet and
> mathmlOnlyRolesSet and only include scripted ones and mathml-only ones
> respectively there.

I'd like to highlight again that while "switch" is semantically equivalent to if-else statements it is often implemented in a way that is much more efficient:

"Additionally, an optimized implementation may execute much faster than the alternative, because it is often implemented by using an indexed branch table. For example, deciding program flow based on a single character's value, if correctly implemented, is vastly more efficient than the alternative, reducing instruction path lengths considerably. When implemented as such, a switch statement essentially becomes a perfect hash."

(https://en.wikipedia.org/wiki/Switch_statement#Advantages_and_disadvantages)

I don't know enough about Gecko's Javascript engine to claim this applies in that case, but I expect it does :-)

So in the case of _addMathRoles, I'd prefer to keep a switch statement to determine the role string: those using aRoleStr, those using "mathmlscripted", the mathmlfraction which can have "withoutbar" and finally (the default) no role string at all. This seems cleaner to me that introducing new Set members for each of the subcase (especially if more subcases are added later...) and allows to keep the role names corresponding to each decision branch inside _addMathRoles.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 31 • 9 years ago

(In reply to Frédéric Wang (:fredw) from comment #30)
> (In reply to Yura Zenevich [:yzen] from comment #28)
> > 
> > mathmlRolesSet: new Set([/* list our math roles here */])
> > 
> > if (this.mathmlRolesSet.has(aAccessible.role)) {
> >   this._addMathRoles(...); // Returning true or false is not optimal design
> > since the function only talks about adding roles nothing about its result.
> > } else {
> >   aOutput.push({string: this._getOutputName(aRoleStr)});
> > }
> > 
> 
> Thanks, I see that this design is cleaner that the one with _addMathRoles
> returning a boolean so I will try that. Alternatively, perhaps we can just
> check whether aRoleStr starts with "mathml" (with perhaps a special case for
> the <math> tag).

This feels a bit unreliable, I think having a Set for all math roles should be fine (> 10 entries) and will give us constant time spent on a check.

> 
> > This way we can simply check this.mathRolesSet.has(aAccessible.role) instead
> > of if-elsing all of these roles?
> > 
> > Similarly i think it is ok to create mathmlScriptedRolesSet and
> > mathmlOnlyRolesSet and only include scripted ones and mathml-only ones
> > respectively there.
> 
> I'd like to highlight again that while "switch" is semantically equivalent
> to if-else statements it is often implemented in a way that is much more
> efficient:
> 
> "Additionally, an optimized implementation may execute much faster than the
> alternative, because it is often implemented by using an indexed branch
> table. For example, deciding program flow based on a single character's
> value, if correctly implemented, is vastly more efficient than the
> alternative, reducing instruction path lengths considerably. When
> implemented as such, a switch statement essentially becomes a perfect hash."
> 
> (https://en.wikipedia.org/wiki/Switch_statement#Advantages_and_disadvantages)
> 
> I don't know enough about Gecko's Javascript engine to claim this applies in
> that case, but I expect it does :-)

So I looked at some profiles of switch performance, basically switch should be used for sets of 2-10 entries, over 10, it best be a lookup (Set). So how about we use switch for specific categories (as you do), but for an overall set of all math roles, lets use a Set.

> 
> So in the case of _addMathRoles, I'd prefer to keep a switch statement to
> determine the role string: those using aRoleStr, those using
> "mathmlscripted", the mathmlfraction which can have "withoutbar" and finally
> (the default) no role string at all. This seems cleaner to me that
> introducing new Set members for each of the subcase (especially if more
> subcases are added later...) and allows to keep the role names corresponding
> to each decision branch inside _addMathRoles.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 32 • 9 years ago

FYI, here's an article: http://archive.oreilly.com/pub/a/server-administration/excerpts/even-faster-websites/writing-efficient-javascript.html#the_fastest_conditionals

	Frédéric Wang (:fredw) Assignee
	Comment 33 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #31)
> So I looked at some profiles of switch performance, basically switch should
> be used for sets of 2-10 entries, over 10, it best be a lookup (Set). So how
> about we use switch for specific categories (as you do), but for an overall
> set of all math roles, lets use a Set.
> 
Yes, that sounds good. That's the patch I was preparing.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 35 • 9 years ago

Comment on attachment 8632805 [details] [diff] [review]
Patch V12

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

looks good, final nits.

Also mathml tests seem to be failing:
The following tests failed:
4277 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for math-1 (output: ["(","x",",","y",")"]) == (expected: [{"string":"open-fence"},"(","x",",","y",{"string":"close-fence"},")"]) -     Structures begin differing at:
got[0] = (
expected[0] = [object Object]
4278 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for math-1 (output: ["(","x",",","y",")"]) == (expected: [{"string":"open-fenceAbbr"},"(","x",",","y",{"string":"close-fenceAbbr"},")"]) -     Structures begin differing at:
got[0] = (
expected[0] = [object Object]
4279 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for math-1 (output: ["(","x",",","y",")"]) == (expected: ["(",{"string":"open-fence"},"x",",","y",")",{"string":"close-fence"}]) -     Structures begin differing at:
got[1] = x
expected[1] = [object Object]
4280 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for math-1 (output: ["(","x",",","y",")"]) == (expected: ["(",{"string":"open-fenceAbbr"},"x",",","y",")",{"string":"close-fenceAbbr"}]) -     Structures begin differing at:
got[1] = x
expected[1] = [object Object]
4281 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mfrac-1 (output: [{"string":"mathmlfraction"},"a","b"]) == (expected: [{"string":"mathmlfraction"},{"string":"numerator"},"a",{"string":"denominator"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4282 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mfrac-1 (output: [{"string":"mathmlfractionAbbr"},"a","b"]) == (expected: [{"string":"mathmlfractionAbbr"},{"string":"numeratorAbbr"},"a",{"string":"denominatorAbbr"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4283 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mfrac-1 (output: ["a","b",{"string":"mathmlfraction"}]) == (expected: ["a",{"string":"numerator"},"b",{"string":"denominator"},{"string":"mathmlfraction"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4284 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mfrac-1 (output: ["a","b",{"string":"mathmlfractionAbbr"}]) == (expected: ["a",{"string":"numeratorAbbr"},"b",{"string":"denominatorAbbr"},{"string":"mathmlfractionAbbr"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4285 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mfrac-2 (output: [{"string":"mathmlfractionwithoutbar"},"a","b"]) == (expected: [{"string":"mathmlfractionwithoutbar"},{"string":"numerator"},"a",{"string":"denominator"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4286 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mfrac-2 (output: [{"string":"mathmlfractionwithoutbarAbbr"},"a","b"]) == (expected: [{"string":"mathmlfractionwithoutbarAbbr"},{"string":"numeratorAbbr"},"a",{"string":"denominatorAbbr"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4287 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mfrac-2 (output: ["a","b",{"string":"mathmlfractionwithoutbar"}]) == (expected: ["a",{"string":"numerator"},"b",{"string":"denominator"},{"string":"mathmlfractionwithoutbar"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4288 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mfrac-2 (output: ["a","b",{"string":"mathmlfractionwithoutbarAbbr"}]) == (expected: ["a",{"string":"numeratorAbbr"},"b",{"string":"denominatorAbbr"},{"string":"mathmlfractionwithoutbarAbbr"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4289 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msub-1 (output: [{"string":"mathmlscripted"},"a","b"]) == (expected: [{"string":"mathmlscripted"},{"string":"base"},"a",{"string":"subscript"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4290 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msub-1 (output: [{"string":"mathmlscriptedAbbr"},"a","b"]) == (expected: [{"string":"mathmlscriptedAbbr"},{"string":"baseAbbr"},"a",{"string":"subscriptAbbr"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4291 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msub-1 (output: ["a","b",{"string":"mathmlscripted"}]) == (expected: ["a",{"string":"base"},"b",{"string":"subscript"},{"string":"mathmlscripted"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4292 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msub-1 (output: ["a","b",{"string":"mathmlscriptedAbbr"}]) == (expected: ["a",{"string":"baseAbbr"},"b",{"string":"subscriptAbbr"},{"string":"mathmlscriptedAbbr"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4293 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msup-1 (output: [{"string":"mathmlscripted"},"a","b"]) == (expected: [{"string":"mathmlscripted"},{"string":"base"},"a",{"string":"superscript"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4294 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msup-1 (output: [{"string":"mathmlscriptedAbbr"},"a","b"]) == (expected: [{"string":"mathmlscriptedAbbr"},{"string":"baseAbbr"},"a",{"string":"superscriptAbbr"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4295 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msup-1 (output: ["a","b",{"string":"mathmlscripted"}]) == (expected: ["a",{"string":"base"},"b",{"string":"superscript"},{"string":"mathmlscripted"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4296 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msup-1 (output: ["a","b",{"string":"mathmlscriptedAbbr"}]) == (expected: ["a",{"string":"baseAbbr"},"b",{"string":"superscriptAbbr"},{"string":"mathmlscriptedAbbr"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4297 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msubsup-1 (output: [{"string":"mathmlscripted"},"a","b","c"]) == (expected: [{"string":"mathmlscripted"},{"string":"base"},"a",{"string":"subscript"},"b",{"string":"superscript"},"c"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4298 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msubsup-1 (output: [{"string":"mathmlscriptedAbbr"},"a","b","c"]) == (expected: [{"string":"mathmlscriptedAbbr"},{"string":"baseAbbr"},"a",{"string":"subscriptAbbr"},"b",{"string":"superscriptAbbr"},"c"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4299 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msubsup-1 (output: ["a","b","c",{"string":"mathmlscripted"}]) == (expected: ["a",{"string":"base"},"b",{"string":"subscript"},"c",{"string":"superscript"},{"string":"mathmlscripted"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4300 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for msubsup-1 (output: ["a","b","c",{"string":"mathmlscriptedAbbr"}]) == (expected: ["a",{"string":"baseAbbr"},"b",{"string":"subscriptAbbr"},"c",{"string":"superscriptAbbr"},{"string":"mathmlscriptedAbbr"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4301 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mmultiscripts-1 (output: [{"string":"mathmlscripted"},"a","b","c","d","e","f","g"]) == (expected: [{"string":"mathmlscripted"},{"string":"base"},"a",{"string":"subscript"},"b",{"string":"superscript"},"c",{"string":"superscript"},"d",{"string":"presubscript"},"e",{"string":"presubscript"},"f",{"string":"presuperscript"},"g"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4302 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mmultiscripts-1 (output: [{"string":"mathmlscriptedAbbr"},"a","b","c","d","e","f","g"]) == (expected: [{"string":"mathmlscriptedAbbr"},{"string":"baseAbbr"},"a",{"string":"subscriptAbbr"},"b",{"string":"superscriptAbbr"},"c",{"string":"superscriptAbbr"},"d",{"string":"presubscriptAbbr"},"e",{"string":"presubscriptAbbr"},"f",{"string":"presuperscriptAbbr"},"g"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4303 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mmultiscripts-1 (output: ["a","b","c","d","e","f","g",{"string":"mathmlscripted"}]) == (expected: ["a",{"string":"base"},"b",{"string":"subscript"},"c",{"string":"superscript"},"d",{"string":"superscript"},"e",{"string":"presubscript"},"f",{"string":"presubscript"},"g",{"string":"presuperscript"},{"string":"mathmlscripted"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4304 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mmultiscripts-1 (output: ["a","b","c","d","e","f","g",{"string":"mathmlscriptedAbbr"}]) == (expected: ["a",{"string":"baseAbbr"},"b",{"string":"subscriptAbbr"},"c",{"string":"superscriptAbbr"},"d",{"string":"superscriptAbbr"},"e",{"string":"presubscriptAbbr"},"f",{"string":"presubscriptAbbr"},"g",{"string":"presuperscriptAbbr"},{"string":"mathmlscriptedAbbr"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4305 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for munder-1 (output: [{"string":"mathmlscripted"},"a","b"]) == (expected: [{"string":"mathmlscripted"},{"string":"base"},"a",{"string":"underscript"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4306 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for munder-1 (output: [{"string":"mathmlscriptedAbbr"},"a","b"]) == (expected: [{"string":"mathmlscriptedAbbr"},{"string":"baseAbbr"},"a",{"string":"underscriptAbbr"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4307 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for munder-1 (output: ["a","b",{"string":"mathmlscripted"}]) == (expected: ["a",{"string":"base"},"b",{"string":"underscript"},{"string":"mathmlscripted"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4308 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for munder-1 (output: ["a","b",{"string":"mathmlscriptedAbbr"}]) == (expected: ["a",{"string":"baseAbbr"},"b",{"string":"underscriptAbbr"},{"string":"mathmlscriptedAbbr"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4309 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mover-1 (output: [{"string":"mathmlscripted"},"a","b"]) == (expected: [{"string":"mathmlscripted"},{"string":"base"},"a",{"string":"overscript"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4310 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mover-1 (output: [{"string":"mathmlscriptedAbbr"},"a","b"]) == (expected: [{"string":"mathmlscriptedAbbr"},{"string":"baseAbbr"},"a",{"string":"overscriptAbbr"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4311 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mover-1 (output: ["a","b",{"string":"mathmlscripted"}]) == (expected: ["a",{"string":"base"},"b",{"string":"overscript"},{"string":"mathmlscripted"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4312 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mover-1 (output: ["a","b",{"string":"mathmlscriptedAbbr"}]) == (expected: ["a",{"string":"baseAbbr"},"b",{"string":"overscriptAbbr"},{"string":"mathmlscriptedAbbr"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4313 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for munderover-1 (output: [{"string":"mathmlscripted"},"a","b","c"]) == (expected: [{"string":"mathmlscripted"},{"string":"base"},"a",{"string":"underscript"},"b",{"string":"overscript"},"c"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4314 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for munderover-1 (output: [{"string":"mathmlscriptedAbbr"},"a","b","c"]) == (expected: [{"string":"mathmlscriptedAbbr"},{"string":"baseAbbr"},"a",{"string":"underscriptAbbr"},"b",{"string":"overscriptAbbr"},"c"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4315 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for munderover-1 (output: ["a","b","c",{"string":"mathmlscripted"}]) == (expected: ["a",{"string":"base"},"b",{"string":"underscript"},"c",{"string":"overscript"},{"string":"mathmlscripted"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4316 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for munderover-1 (output: ["a","b","c",{"string":"mathmlscriptedAbbr"}]) == (expected: ["a",{"string":"baseAbbr"},"b",{"string":"underscriptAbbr"},"c",{"string":"overscriptAbbr"},{"string":"mathmlscriptedAbbr"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4317 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mroot-1 (output: [{"string":"mathmlroot"},"a","b"]) == (expected: [{"string":"mathmlroot"},{"string":"base"},"a",{"string":"root-index"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4318 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mroot-1 (output: [{"string":"mathmlrootAbbr"},"a","b"]) == (expected: [{"string":"mathmlrootAbbr"},{"string":"baseAbbr"},"a",{"string":"root-indexAbbr"},"b"]) -     Structures begin differing at:
got[1] = a
expected[1] = [object Object]
4319 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mroot-1 (output: ["a","b",{"string":"mathmlroot"}]) == (expected: ["a",{"string":"base"},"b",{"string":"root-index"},{"string":"mathmlroot"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
4320 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_output_mathml.html | Context output is correct for mroot-1 (output: ["a","b",{"string":"mathmlrootAbbr"}]) == (expected: ["a",{"string":"baseAbbr"},"b",{"string":"root-indexAbbr"},{"string":"mathmlrootAbbr"}]) -     Structures begin differing at:
got[1] = b
expected[1] = [object Object]
SUITE-END | took 54s

::: accessible/jsat/OutputGenerator.jsm
@@ +198,5 @@
> +   * @param {String} aRoleStr aAccessible's role string.
> +   */
> +  _addMathRoles: function _addMathRoles(aOutput, aAccessible, aRoleStr) {
> +    // First, determine the actual role to use (e.g. mathmlfraction).
> +    var roleStr = aRoleStr;

nit: s/var/let

@@ +225,5 @@
> +        // distinguish between fractions that have a bar and those that do not.
> +        // Per the MathML 3 spec, the latter happens iff the linethickness
> +        // attribute is of the form [zero-float][optional-unit]. In that case,
> +        // we use the string 'mathmlfractionwithoutbar'.
> +        let linethickness = Utils.getAttributes(aAccessible)['linethickness'];

nit: Utils.getAttributes(aAccessible).linethickness;

we use '' otherwise because the name has '-' in it

@@ +258,5 @@
> +   * @param {Array} aOutput Output array.
> +   * @param {nsIAccessible} aAccessible current accessible object.
> +   */
> +  _addMencloseNotations: function _addMencloseNotations(aOutput, aAccessible) {
> +    let notations = Utils.getAttributes(aAccessible)['notation'] || 'longdiv';

nit: Utils.getAttributes(aAccessible).notation

@@ +259,5 @@
> +   * @param {nsIAccessible} aAccessible current accessible object.
> +   */
> +  _addMencloseNotations: function _addMencloseNotations(aOutput, aAccessible) {
> +    let notations = Utils.getAttributes(aAccessible)['notation'] || 'longdiv';
> +    notations.split(' ').forEach(function(aNotation) {

i think you can also do this (not necessary though):

aOutput[this.outputOrder === OUTPUT_DESC_FIRST ? 'push' : 'unshift'].apply(aOutput, 
  [for (notation of notations) {string: this._getOutputName('notation-' + notation)}]);

@@ +531,5 @@
>        return output;
> +    },
> +
> +    // Use the table output functions for MathML tabular elements.
> +    mathmltable: function mathmltable(aAccessible, aRoleStr, aState, aFlags) {

nit: no need to name args , just mathmltable: function mathmltable() should be good

	Frédéric Wang (:fredw) Assignee
	Comment 36 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #35)
> Also mathml tests seem to be failing:

mmh, did you run the tests with the patches from bug 1177765 applied? :-)

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 37 • 9 years ago

Comment on attachment 8632805 [details] [diff] [review]
Patch V12

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

Ah, good catch. Applied them and everything's passing. Thanks (just those nits left)!

::: accessible/jsat/OutputGenerator.jsm
@@ +198,5 @@
> +   * @param {String} aRoleStr aAccessible's role string.
> +   */
> +  _addMathRoles: function _addMathRoles(aOutput, aAccessible, aRoleStr) {
> +    // First, determine the actual role to use (e.g. mathmlfraction).
> +    var roleStr = aRoleStr;

nit: s/var/let

@@ +225,5 @@
> +        // distinguish between fractions that have a bar and those that do not.
> +        // Per the MathML 3 spec, the latter happens iff the linethickness
> +        // attribute is of the form [zero-float][optional-unit]. In that case,
> +        // we use the string 'mathmlfractionwithoutbar'.
> +        let linethickness = Utils.getAttributes(aAccessible)['linethickness'];

nit: Utils.getAttributes(aAccessible).linethickness;

we use '' otherwise because the name has '-' in it

@@ +258,5 @@
> +   * @param {Array} aOutput Output array.
> +   * @param {nsIAccessible} aAccessible current accessible object.
> +   */
> +  _addMencloseNotations: function _addMencloseNotations(aOutput, aAccessible) {
> +    let notations = Utils.getAttributes(aAccessible)['notation'] || 'longdiv';

nit: Utils.getAttributes(aAccessible).notation

@@ +259,5 @@
> +   * @param {nsIAccessible} aAccessible current accessible object.
> +   */
> +  _addMencloseNotations: function _addMencloseNotations(aOutput, aAccessible) {
> +    let notations = Utils.getAttributes(aAccessible)['notation'] || 'longdiv';
> +    notations.split(' ').forEach(function(aNotation) {

i think you can also do this (not necessary though):

aOutput[this.outputOrder === OUTPUT_DESC_FIRST ? 'push' : 'unshift'].apply(aOutput, 
  [for (notation of notations) {string: this._getOutputName('notation-' + notation)}]);

@@ +531,5 @@
>        return output;
> +    },
> +
> +    // Use the table output functions for MathML tabular elements.
> +    mathmltable: function mathmltable(aAccessible, aRoleStr, aState, aFlags) {

nit: no need to name args , just mathmltable: function mathmltable() should be good

	Frédéric Wang (:fredw) Assignee
	Comment 38 • 9 years ago

Patch updated with remaining comments fixed.

> i think you can also do this (not necessary though):
> aOutput[this.outputOrder === OUTPUT_DESC_FIRST ? 'push' : 'unshift'].apply(aOutput, 
>  [for (notation of notations) {string: this._getOutputName('notation-' + notation)}]);

I did that one. It changes the order in which the menclose notations are listed but I think that does not matter.

	Frédéric Wang (:fredw) Assignee
	Comment 39 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a3d7cde7e39

	Pulsebot
	Comment 40 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c5b634d9bd

	Pulsebot
	Comment 41 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc081e8c6650

	Pulsebot
	Comment 42 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/95a508b38259

	Carsten Book [:Tomcat]
	Comment 43 • 9 years ago

sorry had to back this out since it was showing up in a checkin-needed push for leaks but turned out was innocent, so re-checkedin

	Carsten Book [:Tomcat]
	Comment 44 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/95a508b38259

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 45 • 9 years ago

https://github.com/mozilla-b2g/gaia/commit/28429cb87e36b49f3cb79cb885290d2dc89e4543 <- Gaia