915458 - [AccessFu] When traversing a label that nests a radio/checkbox we should land only on the label

Status: RESOLVED FIXED

Description

[Eitan Isaacson [:eeejay]]

This reduces the clutter. Nesting checkbox labels are functionally similar to the checkbox itself.

Comment 1

[Eitan Isaacson [:eeejay]]

Created attachment 803369 [details] [diff] [review]
Land on nesting labels if they label a checkbox or a radio button

Just a wip for now. It still suffers from false positives.

Comment 2

[Eitan Isaacson [:eeejay]]

For this kind of markup: <label><input type="checkbox">Enable</label>

The expected behavior would be - 
The user swipes to it: The vc should land on the label.
The user explore-by-touches to it: The vc should land on the label.

The expected output would be -
Visually, it would be the label bounds.
Speech/braille, it would be the embedded checkbox.

When activated -
The embedded checkbox is toggled.

Comment 3

[Eitan Isaacson [:eeejay]]

Created attachment 807526 [details] [diff] [review]
Land on nesting labels instead of their children, and present them correctly.

Comment 4

[Eitan Isaacson [:eeejay]]

Created attachment 808762 [details] [diff] [review]
Land on nesting labels instead of their children, and present them correctly.

Comment 5

[Yura Zenevich [:yzen]]

Comment on attachment 808762 [details] [diff] [review]
Land on nesting labels instead of their children, and present them correctly.

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

Looks good, just a couple of changes, the patch also needs to be updated to the mc tip.

::: accessible/src/jsat/TraversalRules.jsm
@@ +54,5 @@
>  Cu.import('resource://gre/modules/XPCOMUtils.jsm');
>  
>  let gSkipEmptyImages = new PrefCache('accessibility.accessfu.skip_empty_images');
>  
>  function BaseTraversalRule(aRoles, aMatchFunc) {

If you provide a default value for aMatchFunc that simply returns FILTER_MATCH, you can keep/simplify the existing logic around _matchFunc and avoid creating _matchFuncInternal.

@@ +60,1 @@
>    this._matchRoles = aRoles;

As far as I can tell, _matchRoles is used only in getMatchRoles which is not used anywhere in the code. Is it still necessary?

@@ +89,5 @@
> +      // It is a bigger touch target, and it reduces clutter.
> +      if (role == ROLE_LABEL && !(matchResult & FILTER_IGNORE_SUBTREE)) {
> +        let control = Utils.getEmbeddedControl(aAccessible);
> +        if (control && this._explicitMatchRoles.has(control.role)) {
> +           matchResult = this._matchFunc(control) | FILTER_IGNORE_SUBTREE;

Nit: Extra space in indentation.

@@ +97,5 @@
> +      return matchResult;
> +    },
> +
> +    _matchFunc: function BaseTraversalRule__matchFunc(aAccessible) {
> +      if (this._matchFuncInternal)

Nit: if {}

::: accessible/src/jsat/Utils.jsm
@@ +294,5 @@
>      // Looking up a role that would match a landmark.
>      return this.matchAttributeValue(roles, landmarks);
> +  },
> +
> +  getEmbeddedControl: function getImbeddedControl(aLabel) {

Typo: getImbeddedControl

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #5)
> Comment on attachment 808762 [details] [diff] [review]
> Land on nesting labels instead of their children, and present them correctly.
> 
> Review of attachment 808762 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just a couple of changes, the patch also needs to be updated to
> the mc tip.
> 
> ::: accessible/src/jsat/TraversalRules.jsm
> @@ +54,5 @@
> >  Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> >  
> >  let gSkipEmptyImages = new PrefCache('accessibility.accessfu.skip_empty_images');
> >  
> >  function BaseTraversalRule(aRoles, aMatchFunc) {
> 
> If you provide a default value for aMatchFunc that simply returns
> FILTER_MATCH, you can keep/simplify the existing logic around _matchFunc and
> avoid creating _matchFuncInternal.
> 

Good idea!

> @@ +60,1 @@
> >    this._matchRoles = aRoles;
> 
> As far as I can tell, _matchRoles is used only in getMatchRoles which is not
> used anywhere in the code. Is it still necessary?
> 

Yeah, it is part of the nsIAccessibleTraversalRule interface.

Comment 7

[Eitan Isaacson [:eeejay]]

Created attachment 814955 [details] [diff] [review]
Land on nesting labels instead of their children, and present them correctly.

Comment 8

[Eitan Isaacson [:eeejay]]

Created attachment 815992 [details] [diff] [review]
Land on nesting labels instead of their children, and present them correctly.

Integrated tests into test_utterance_order.html

Comment 9

[Yura Zenevich [:yzen]]

Comment on attachment 815992 [details] [diff] [review]
Land on nesting labels instead of their children, and present them correctly.

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

Looks great! r=me with minor nits/comments.

::: accessible/src/jsat/TraversalRules.jsm
@@ +56,5 @@
>  Cu.import('resource://gre/modules/XPCOMUtils.jsm');
>  
>  let gSkipEmptyImages = new PrefCache('accessibility.accessfu.skip_empty_images');
>  
>  function BaseTraversalRule(aRoles, aMatchFunc) {

Just a nit (feel free to leave it as is):

I actually meant something like this:

function BaseTraversalRule(aRoles,
  aMatchFunc = function matchFunc(acc) {
    return FILTER_MATCH;
  }) {
  ...
  this._matchFunc = aMatchFunc;
}

@@ +84,5 @@
>            FILTER_MATCH  | FILTER_IGNORE_SUBTREE : FILTER_IGNORE;
>        }
>  
> +      let matchResult = this._explicitMatchRoles.has(role) ?
> +            this._matchFunc(aAccessible) : FILTER_IGNORE;

NIT: indentation: 4 extra spaces

::: accessible/src/jsat/Utils.jsm
@@ +295,5 @@
>      return this.matchAttributeValue(roles, landmarks);
> +  },
> +
> +  getEmbeddedControl: function getEmbeddedControl(aLabel) {
> +    let relation = aLabel.getRelationByType(RELATION_LABEL_FOR);

I managed to get here (when playing with explore by touch on the emulator)
[AccessFu] ERROR Error handing accessible event:
 aLabel is null
  getEmbeddedControl@resource://gre/modules/accessibility/Utils.jsm:299
  PivotContext@resource://gre/modules/accessibility/Utils.jsm:452
  Presentation_pivotChanged@resource://gre/modules/accessibility/Presentation.jsm:562
  handleAccEvent@resource://gre/modules/accessibility/EventManager.jsm:173
  observe@resource://gre/modules/accessibility/EventManager.jsm:587
  moveCursor@chrome://global/content/accessibility/content-script.js:85

Perhaps it makes sense to catch this earlier or I can open a different bug for this.

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #9)
> Comment on attachment 815992 [details] [diff] [review]
> Land on nesting labels instead of their children, and present them correctly.
> 
> Review of attachment 815992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great! r=me with minor nits/comments.
> 
> ::: accessible/src/jsat/TraversalRules.jsm
> @@ +56,5 @@
> >  Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> >  
> >  let gSkipEmptyImages = new PrefCache('accessibility.accessfu.skip_empty_images');
> >  
> >  function BaseTraversalRule(aRoles, aMatchFunc) {
> 
> Just a nit (feel free to leave it as is):
> 
> I actually meant something like this:
> 
> function BaseTraversalRule(aRoles,
>   aMatchFunc = function matchFunc(acc) {
>     return FILTER_MATCH;
>   }) {
>   ...
>   this._matchFunc = aMatchFunc;
> }
> 

I love default args, but that is a bit too much for me in a function prototype :)

> @@ +84,5 @@
> >            FILTER_MATCH  | FILTER_IGNORE_SUBTREE : FILTER_IGNORE;
> >        }
> >  
> > +      let matchResult = this._explicitMatchRoles.has(role) ?
> > +            this._matchFunc(aAccessible) : FILTER_IGNORE;
> 
> NIT: indentation: 4 extra spaces
> 

Fixed.

> ::: accessible/src/jsat/Utils.jsm
> @@ +295,5 @@
> >      return this.matchAttributeValue(roles, landmarks);
> > +  },
> > +
> > +  getEmbeddedControl: function getEmbeddedControl(aLabel) {
> > +    let relation = aLabel.getRelationByType(RELATION_LABEL_FOR);
> 
> I managed to get here (when playing with explore by touch on the emulator)
> [AccessFu] ERROR Error handing accessible event:
>  aLabel is null
>   getEmbeddedControl@resource://gre/modules/accessibility/Utils.jsm:299
>   PivotContext@resource://gre/modules/accessibility/Utils.jsm:452
>  
> Presentation_pivotChanged@resource://gre/modules/accessibility/Presentation.
> jsm:562
>   handleAccEvent@resource://gre/modules/accessibility/EventManager.jsm:173
>   observe@resource://gre/modules/accessibility/EventManager.jsm:587
>   moveCursor@chrome://global/content/accessibility/content-script.js:85
> 
> Perhaps it makes sense to catch this earlier or I can open a different bug
> for this.

Yeah, I added a null-check there. But I think it is a bigger problem.

Comment 11

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e39971f3bb3

Comment 12

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/9e39971f3bb3