Closed Bug 1003491 Opened 6 years ago Closed 6 years ago

[AccessFu] Change traversal rule to land on the parent of simple subtrees

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: eeejay, Assigned: eeejay)

Details

Attachments

(1 file)

A simple subtree either has a single child lineage or one generation of children.
Comment on attachment 8414806 [details] [diff] [review]
Make traversal rule land on bigger items that have simple subtrees.

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

Nice. A couple of nits and it works with my patch for status bar quite well.

::: accessible/src/jsat/OutputGenerator.jsm
@@ +598,5 @@
>      rowheader: function rowheader() {
>        return this.objectOutputFunctions.cell.apply(this, arguments);
> +    },
> +
> +    statictext: function statictext(aAccessible, aRoleStr, aState, aFlags) {

Nit: aRoleStr, aState, aFlags are unused arguments.

@@ +608,2 @@
>      }
> +

Nit: whitespace.

::: accessible/src/jsat/TraversalRules.jsm
@@ +33,5 @@
>    this._matchRoles = aRoles;
>    if (aRoles.indexOf(Roles.LABEL) < 0) {
>      this._matchRoles.push(Roles.LABEL);
>    }
> +  this._matchFunc = aMatchFunc || function () { return Filters.MATCH; };

Nit: extra space - function()

@@ +102,5 @@
>  var gSimpleMatchFunc = function gSimpleMatchFunc(aAccessible) {
> +  // An object is simple, if it either has a single child lineage,
> +  // or has a flat subtree.
> +  function isSimpleObject (acc) {
> +    function isSingleLineage() {

I would not define isSingleLineage and isFlatSubtree every time we call isSimpleObject. Lets have them outside of its scope.

@@ +140,2 @@
>        // Ignore prefix static text in list items. They are typically bullets or numbers.
> +      if (Utils.isListItemDecorator(aAccessible)) {

Since you are using "? :" pattern on line 136 lets use it here as well.
Attachment #8414806 - Flags: review?(yzenevich) → review+
Comment on attachment 8414806 [details] [diff] [review]
Make traversal rule land on bigger items that have simple subtrees.

Nice! :-)
(In reply to Yura Zenevich [:yzen] from comment #2)
> ::: accessible/src/jsat/OutputGenerator.jsm
> @@ +598,5 @@
> >      rowheader: function rowheader() {
> >        return this.objectOutputFunctions.cell.apply(this, arguments);
> > +    },
> > +
> > +    statictext: function statictext(aAccessible, aRoleStr, aState, aFlags) {
> 
> Nit: aRoleStr, aState, aFlags are unused arguments.

Generally, I would like us to be more strict about preserving the function prototype no matter what argument is used or not. It is one of the pitfalls of dynamic languages that you start obfuscating things because you could.

Removing the arguments in the patch I am about to land because most of the file conforms to what you suggest, and it just stands out...
https://hg.mozilla.org/mozilla-central/rev/8c3758dd1ba8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.