Closed Bug 915458 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file, 4 obsolete files)

This reduces the clutter. Nesting checkbox labels are functionally similar to the checkbox itself.
Just a wip for now. It still suffers from false positives.
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.
Attachment #803369 - Attachment is obsolete: true
Attachment #807526 - Attachment is obsolete: true
Attachment #807526 - Flags: review?(yura.zenevich)
Attachment #808762 - Flags: review?(yura.zenevich)
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
Attachment #808762 - Flags: review?(yura.zenevich)
(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.
Attachment #808762 - Attachment is obsolete: true
Attachment #814955 - Flags: review?(yura.zenevich)
Integrated tests into test_utterance_order.html
Attachment #814955 - Attachment is obsolete: true
Attachment #814955 - Flags: review?(yura.zenevich)
Attachment #815992 - Flags: review?(yura.zenevich)
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.
Attachment #815992 - Flags: review?(yura.zenevich) → review+
(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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: