[AccessFu] Land on first atomic object in container quick nav modes

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

No description provided.
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.
(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.
Ah I see what you mean, it's the quick nav mode.
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+
(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: nobody → eitan
https://hg.mozilla.org/mozilla-central/rev/384ff965768a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.