747272 - [AccessFu] Filter out whitespace text leaves in navigation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

They got reintroduced with that whole trim() discussion we had...

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Filter out whitespace text leaves.

So it turns out that there is such a thing as whitespace only-text leaves, if those whitespaces get rendered. This website has some:
http://www.israelilaundry.org

I looked at nsAccessibilityService.cpp and GetOrCreateAccessible does not distinguish between nodes that have only whitespace or not. So this is necessary in our filter. I don't remember anymore at which stage of the previous reviews Alex had me take it out, but it is necessary.

In this patch, I also took the liberty of refactoring SimpleTraversalRule and putting the matchRoles property to better use.

Comment 2

[alexander :surkov (:asurkov)]

iirc we talked about trim for accessible names what is not necessary because we do that on our side but of course we create accessible objects for rendered whitespace nodes

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 617701 [details] [diff] [review]
Filter out whitespace text leaves.

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

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +116,2 @@
>  
> +function SimpleTraversalRule() {

why do you change from object to constructor?

@@ +118,5 @@
> +  this._matchRoles = [];
> +  for (var i=0; i< Ci.nsIAccessibleRole.ROLE_LAST_ENTRY; i++) {
> +    // TODO: Find a better solution for ROLE_STATICTEXT.
> +    // Right now it helps filter list bullets, but it is also used
> +    // in CSS generated content.

perhaps: It allows to filter list bullets but the same time it filters CSS generated content too as unwanted side effect

@@ +122,5 @@
> +    // in CSS generated content.
> +    if (i != Ci.nsIAccessibleRole.ROLE_WHITESPACE &&
> +        i != Ci.nsIAccessibleRole.ROLE_STATICTEXT)
> +      this._matchRoles.push(i);
> +  }

I think I like ignoreRoles approach more than this one

@@ +140,4 @@
>          let state = {};
>          aAccessible.getState(state, {});
>          if ((state.value & Ci.nsIAccessibleStates.STATE_FOCUSABLE) ||
> +          (aAccessible.name && aAccessible.name.trim()))

if you really get whitespace name then file a bug and provide a test case please

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #3)
> if you really get whitespace name then file a bug and provide a test case
> please

Is that considered a bug? I didn't think it was.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #4)
> (In reply to alexander :surkov from comment #3)
> > if you really get whitespace name then file a bug and provide a test case
> > please
> 
> Is that considered a bug? I didn't think it was.

at least it doesn't make sense when you get a name for whitespace accessible.

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #3)
> Comment on attachment 617701 [details] [diff] [review]
> Filter out whitespace text leaves.
> 
> Review of attachment 617701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/VirtualCursorController.jsm
> @@ +116,2 @@
> >  
> > +function SimpleTraversalRule() {
> 
> why do you change from object to constructor?
> 
> @@ +118,5 @@
> > +  this._matchRoles = [];
> > +  for (var i=0; i< Ci.nsIAccessibleRole.ROLE_LAST_ENTRY; i++) {
> > +    // TODO: Find a better solution for ROLE_STATICTEXT.
> > +    // Right now it helps filter list bullets, but it is also used
> > +    // in CSS generated content.
> 
> perhaps: It allows to filter list bullets but the same time it filters CSS
> generated content too as unwanted side effect
> 

Because we now have an op in the constructor :) we pre-build a list of roles.

> @@ +122,5 @@
> > +    // in CSS generated content.
> > +    if (i != Ci.nsIAccessibleRole.ROLE_WHITESPACE &&
> > +        i != Ci.nsIAccessibleRole.ROLE_STATICTEXT)
> > +      this._matchRoles.push(i);
> > +  }
> 
> I think I like ignoreRoles approach more than this one
> 

We will need matchRoles in the future.

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Filter out whitespace text leaves.

A revision with a comment that refers to bug 748405.

Comment 8

[alexander :surkov (:asurkov)]

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

> > > +function SimpleTraversalRule() {
> > 
> > why do you change from object to constructor?
> Because we now have an op in the constructor :) we pre-build a list of roles.

ok

> > > +    // TODO: Find a better solution for ROLE_STATICTEXT.
> > > +    // Right now it helps filter list bullets, but it is also used
> > > +    // in CSS generated content.
> > 
> > perhaps: It allows to filter list bullets but the same time it filters CSS
> > generated content too as unwanted side effect
what about comment wording?

> > I think I like ignoreRoles approach more than this one
> We will need matchRoles in the future.

I trust you but next time please do a change when you need it (or do it right before when you need it), I can't read your mind :)

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 619692 [details] [diff] [review]
Filter out whitespace text leaves.

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

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +20,5 @@
>  var VirtualCursorController = {
>    attach: function attach(aWindow) {
>      this.chromeWin = aWindow;
>      this.chromeWin.document.addEventListener('keypress', this.onkeypress, true);
> +    this.simpleTraversalRule = new SimpleTraversalRule();

will you have other traversal rules as members, if not then it makes sense to keep it shorter like this.traversalRule

@@ +116,4 @@
>  
> +function SimpleTraversalRule() {
> +  this._matchRoles = [];
> +  for (var i=0; i< Ci.nsIAccessibleRole.ROLE_LAST_ENTRY; i++) {

spaces around operator '=' and before '<'

@@ +122,5 @@
> +    // in CSS generated content.
> +    if (i != Ci.nsIAccessibleRole.ROLE_WHITESPACE &&
> +        i != Ci.nsIAccessibleRole.ROLE_STATICTEXT)
> +      this._matchRoles.push(i);
> +  }

that roles iteration is not optimal (and taking into account we don't have last_entry anymore) is not possible. Can you give me an idea of future changes that make ignoreRoles (match function) not suitable?

@@ +133,1 @@
>      },

if you do that then you should do a proper indentation

@@ +144,2 @@
>          if ((state.value & Ci.nsIAccessibleStates.STATE_FOCUSABLE) ||
> +          (aAccessible.name && aAccessible.name.trim()))

wrong indentation again, don't calculate name twice, it's heavy

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #8)
> (In reply to Eitan Isaacson [:eeejay] from comment #6)
> > > I think I like ignoreRoles approach more than this one
> > We will need matchRoles in the future.
> 
> I trust you but next time please do a change when you need it (or do it
> right before when you need it), I can't read your mind :)

I didn't know about the future until it happened :) I am rapidly developing and touching code here. So when a patch gets debated in bugzilla for a couple of weeks, it becomes obsolete.

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #9)
> that roles iteration is not optimal (and taking into account we don't have
> last_entry anymore) is not possible. Can you give me an idea of future
> changes that make ignoreRoles (match function) not suitable?
> 

The FOCUSABLE check is going to go away, and instead we are going to filter on a per-role basis via matchRoles.
ignoreRoles was a hack, matchRoles is how the API was designed to be used. If we could not retrieve the role in each match function call than it is an improvement.

Comment 12

[Eitan Isaacson [:eeejay]]

Attachment: Filter out whitespace text leaves.

Simplified. So we could land this already.

Comment 13

[alexander :surkov (:asurkov)]

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

> > I trust you but next time please do a change when you need it (or do it
> > right before when you need it), I can't read your mind :)
> 
> I didn't know about the future until it happened :) I am rapidly developing
> and touching code here. So when a patch gets debated in bugzilla for a
> couple of weeks, it becomes obsolete.

perhaps you need to have better planing but it seems so it wasn't obsolete yet couple days ago (comment #6) :)

(In reply to Eitan Isaacson [:eeejay] from comment #11)
> The FOCUSABLE check is going to go away, and instead we are going to filter
> on a per-role basis via matchRoles.
> ignoreRoles was a hack, matchRoles is how the API was designed to be used.

matchRoles array is not good approach when you need to match all roles expect some because it requires you traverse 100 items array for each accessible.

so match() function (where ignoreRoles was used) is much more flexible and can be performant.

> If we could not retrieve the role in each match function call than it is an
> improvement.

if match() function needs to match roles then nothing is left to you and matchRoles array approach doesn't save you from this.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 620022 [details] [diff] [review]
Filter out whitespace text leaves.

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

r=me with comments fixed

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +125,5 @@
>        let rv = Ci.nsIAccessibleTraversalRule.FILTER_IGNORE;
>        if (aAccessible.childCount == 0) {
>          // TODO: Find a better solution for ROLE_STATICTEXT.
> +        // It allows to filter list bullets but the same time it
> +        // filters CSS generated content too as unwanted side effect

dot in the end

@@ +131,5 @@
>                             Ci.nsIAccessibleRole.ROLE_STATICTEXT];
>          let state = {};
>          aAccessible.getState(state, {});
> +        let name = accessible.name;
> +        let hasName = name && name.strip();

state and name may be heavy, especially as part of tree traversal, I'd recommend to use nested ifs like

if (childCOunt != 0)
  return IGNORE; // please comment it

if (ignoredRoles.indexOf < 0) {
  let name = accessible.name;
  if (name && name.strip())
    return MATCH;
}

let state = {};
if (state.value & Ci)
  return MATCH;

return IGNORE;

Comment 15

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7b46c7fc91a4

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7b46c7fc91a4