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)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: eeejay, Assigned: eeejay)
References
Details
Attachments
(1 file, 4 obsolete files)
15.20 KB,
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
This reduces the clutter. Nesting checkbox labels are functionally similar to the checkbox itself.
Assignee | ||
Comment 1•12 years ago
|
||
Just a wip for now. It still suffers from false positives.
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #807526 -
Flags: review?(yura.zenevich)
Assignee | ||
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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)
Assignee | ||
Comment 6•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #808762 -
Attachment is obsolete: true
Attachment #814955 -
Flags: review?(yura.zenevich)
Assignee | ||
Comment 8•12 years ago
|
||
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 9•12 years ago
|
||
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+
Assignee | ||
Comment 10•12 years ago
|
||
(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.
Assignee | ||
Comment 11•12 years ago
|
||
Assignee: nobody → eitan
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.
Description
•