The default bug view has changed. See this FAQ.

[AccessFu] Filter out whitespace text leaves in navigation

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

(Depends on: 1 bug)

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
They got reintroduced with that whole trim() discussion we had...
(Assignee)

Comment 1

5 years ago
Created attachment 617701 [details] [diff] [review]
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.
Attachment #617701 - Flags: review?(surkov.alexander)

Comment 2

5 years ago
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

5 years ago
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
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 748405

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 619692 [details] [diff] [review]
Filter out whitespace text leaves.

A revision with a comment that refers to bug 748405.
Attachment #617701 - Attachment is obsolete: true
Attachment #617701 - Flags: review?(surkov.alexander)
Attachment #619692 - Flags: review?(surkov.alexander)

Comment 8

5 years ago
(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

5 years ago
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
Attachment #619692 - Flags: review?(surkov.alexander)
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 620022 [details] [diff] [review]
Filter out whitespace text leaves.

Simplified. So we could land this already.
Attachment #619692 - Attachment is obsolete: true
Attachment #620022 - Flags: review?(surkov.alexander)

Comment 13

5 years ago
(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

5 years ago
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;
Attachment #620022 - Flags: review?(surkov.alexander) → review+

Updated

5 years ago
Assignee: nobody → eitan
(Assignee)

Comment 15

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/7b46c7fc91a4
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/7b46c7fc91a4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.