Closed
Bug 747272
Opened 12 years ago
Closed 12 years ago
[AccessFu] Filter out whitespace text leaves in navigation
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: eeejay, Assigned: eeejay)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
1.64 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
They got reintroduced with that whole trim() discussion we had...
Assignee | ||
Comment 1•12 years ago
|
||
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•12 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•12 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•12 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.
Comment 5•12 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•12 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•12 years ago
|
||
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•12 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•12 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•12 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•12 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•12 years ago
|
||
Simplified. So we could land this already.
Attachment #619692 -
Attachment is obsolete: true
Attachment #620022 -
Flags: review?(surkov.alexander)
Comment 13•12 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•12 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•12 years ago
|
Assignee: nobody → eitan
Assignee | ||
Comment 15•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/7b46c7fc91a4
Target Milestone: --- → mozilla15
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7b46c7fc91a4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•