Closed
Bug 1200836
Opened 9 years ago
Closed 9 years ago
[AccessFu] Land on first atomic object in container quick nav modes
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: eeejay, Assigned: eeejay)
Details
Attachments
(1 file)
15.63 KB,
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Take for examples the "Lists" rule, we currently put the cursor on the whole list, and have the entire container read out. This is both slow, and not useful for the user. We should simply land on the first atomic object of that container, and allow the user to continue stepping through it at their own leisure. Another example is the "main" Landmark. When we use that, we end up reading the entire contents of the web page. In Android, at least, this makes things very sluggish as we jam up the accessibility and tts bus. This is also, obviously, not what the user wants. They simply want to navigate to the main section, and continue on their own from there.
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8655707 -
Flags: review?(yzenevich)
Comment 3•9 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #1) > Take for examples the "Lists" rule, we currently put the cursor on the whole > list, and have the entire container read out. This is both slow, and not > useful for the user. We should simply land on the first atomic object of > that container, and allow the user to continue stepping through it at their > own leisure. Is it the case in B2G? > > Another example is the "main" Landmark. When we use that, we end up reading > the entire contents of the web page. In Android, at least, this makes things > very sluggish as we jam up the accessibility and tts bus. This is also, > obviously, not what the user wants. They simply want to navigate to the main > section, and continue on their own from there.
Comment 4•9 years ago
|
||
Ah I see what you mean, it's the quick nav mode.
Comment 5•9 years ago
|
||
Comment on attachment 8655707 [details] [diff] [review] Land on first atomic object in container traversal. Review of attachment 8655707 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with comments addressed. Thanks! ::: accessible/jsat/Traversal.jsm @@ +321,5 @@ > +this.TraversalHelper = { > + _helperPivotCache: null, > + > + getHelperPivot: function TraversalHelper_getHelperPivot(aRoot) { > + if (!this._helperPivotCache) { Can we make it a lasy getter instead of the check every time. @@ +328,5 @@ > + > + let pivot = this._helperPivotCache.get(aRoot.DOMNode); > + if (!pivot) { > + pivot = Utils.AccRetrieval.createAccessiblePivot(aRoot); > + nit: no empty line @@ +343,5 @@ > + let moved = false; > + let helperPivot = this.getHelperPivot(aVirtualCursor.root); > + helperPivot.position = aVirtualCursor.position; > + > + while (!moved) { A comment would be nice @@ +356,5 @@ > + > + return moved; > + > + } else { > + return aVirtualCursor[aMethod](TraversalRules[aRule]); return aVirtualCursor[aMethod](rule); @@ +358,5 @@ > + > + } else { > + return aVirtualCursor[aMethod](TraversalRules[aRule]); > + } > + nit: no space needed. ::: accessible/tests/mochitest/jsat/test_traversal_helper.html @@ +51,5 @@ > + ok(!TraversalHelper.move(vc, 'moveNext', aRule), "reached end"); > + > + TraversalHelper.move(vc, 'moveLast', 'Simple'); > + > + walkSequence('movePrevious', aRule, aExpectedSequence.slice().reverse()); Use Array.from() instead of empty slice @@ +83,5 @@ > + ['Programming Language', 'listitem-2-1', 'listitem-3-1']); > + > + vc.position = null; > + > + while (TraversalHelper.move(vc, 'moveNext', 'List')) There are no tests here, is it just for info? Does it need to be removed?
Attachment #8655707 -
Flags: review?(yzenevich) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Yura Zenevich [:yzen] from comment #5) > Comment on attachment 8655707 [details] [diff] [review] > Land on first atomic object in container traversal. > > Review of attachment 8655707 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good with comments addressed. Thanks! > > ::: accessible/jsat/Traversal.jsm > @@ +321,5 @@ > > +this.TraversalHelper = { > > + _helperPivotCache: null, > > + > > + getHelperPivot: function TraversalHelper_getHelperPivot(aRoot) { > > + if (!this._helperPivotCache) { > > Can we make it a lasy getter instead of the check every time. > Done. > @@ +328,5 @@ > > + > > + let pivot = this._helperPivotCache.get(aRoot.DOMNode); > > + if (!pivot) { > > + pivot = Utils.AccRetrieval.createAccessiblePivot(aRoot); > > + > > nit: no empty line Done. > > @@ +343,5 @@ > > + let moved = false; > > + let helperPivot = this.getHelperPivot(aVirtualCursor.root); > > + helperPivot.position = aVirtualCursor.position; > > + > > + while (!moved) { > > A comment would be nice Added. > > @@ +356,5 @@ > > + > > + return moved; > > + > > + } else { > > + return aVirtualCursor[aMethod](TraversalRules[aRule]); > > return aVirtualCursor[aMethod](rule); > Good catch. > @@ +358,5 @@ > > + > > + } else { > > + return aVirtualCursor[aMethod](TraversalRules[aRule]); > > + } > > + > > nit: no space needed. Done. > > ::: accessible/tests/mochitest/jsat/test_traversal_helper.html > @@ +51,5 @@ > > + ok(!TraversalHelper.move(vc, 'moveNext', aRule), "reached end"); > > + > > + TraversalHelper.move(vc, 'moveLast', 'Simple'); > > + > > + walkSequence('movePrevious', aRule, aExpectedSequence.slice().reverse()); > > Use Array.from() instead of empty slice new to me! > > @@ +83,5 @@ > > + ['Programming Language', 'listitem-2-1', 'listitem-3-1']); > > + > > + vc.position = null; > > + > > + while (TraversalHelper.move(vc, 'moveNext', 'List')) > > There are no tests here, is it just for info? Does it need to be removed? Yes, removed.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → eitan
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/384ff965768a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•