760972 - [AccessFu] After fix for bug 758884, inconsistent behavior when a skip link is encountered

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Open Fennec with AccessFu enabled, and go to my blog.
2. Start navigating to the right. The following sequence is encountered:

Right: First heading.
Right: Tagline "Musings...."
Right: Graphic.
Right: Skip link.
Right: Nothing. Expected next item.
Right: Skip link again.
Right: List item 1 of 3 "Home" link.
Right: Verbose output with document title and list item 1 of 3 again.
Right: Again list item 1 of 3.
Right: Finally list item 2 of 3 "About" link.

From then on, navigation continues normally.

The unexpected behavior here is that the skip link is repeated, the home link is repeated, once along with speaking the document title again.

Expected behavour is that navigation takes place without elements repeating.

This is the case on both ICS and Gingerbread.

Comment 1

[Eitan Isaacson [:eeejay]]

This is partially related to bug 759875, and other parts should work better with the new moveNext implementation that landed, but is not taken advantage of yet with focus.

Comment 2

[Marco Zehe (:MarcoZ)]

Attachment: Revert actually focusing elements the virtual cursor moves to

As we discussed on Skype yesterday, this reverts the focusing of each element the virtual cursor moves to. Is this enough, or do we need to move this somewhere else to make sure things get properly activated/scrolled to when simulating mouse clicks?

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 630100 [details] [diff] [review]
Revert actually focusing elements the virtual cursor moves to

Cancelling review request since this patch is incomplete. Will wait for eeejay's patch.

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Remove focus-follows-cursor behavior and add cursor-follows-focus.

Marco and I discussed this. In an ideal world cursor would follow the focus and the focus would change with the cursor. This is cyclical, and needs to be done right. But more importantly changing the focus with the cursor has the potential of changing the DOM with the cursor - a complicated issue, as Marco's skip to content link shows.

So I am proposing, for the time being, to only do cursor follows focus. Since this is (a) safer and (b) more directly benificial for the user.

This patch removes the focus-follows-cursor behavior and robustifies the cursor-follows-focus feature.

Comment 5

[Eitan Isaacson [:eeejay]]

Doing a try build for easier testing.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 630283 [details] [diff] [review]
Remove focus-follows-cursor behavior and add cursor-follows-focus.

Review of attachment 630283 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the patch. r- for failing at PTO.

::: accessible/src/jsat/AccessFu.jsm
@@ -233,5 @@
> -            sel.collapse(position.DOMNode, 0);
> -            Cc["@mozilla.org/focus-manager;1"]
> -              .getService(Ci.nsIFocusManager).moveFocus(
> -                doc.defaultView, null, Ci.nsIFocusManager.MOVEFOCUS_CARET, 0);
> -          }

Note (somewhere in the back of your mind) for windows (if we polish accessfu there), we'll want the system caret to track the VC I think... for screen mags.

@@ +348,5 @@
> +        let acc = aEvent.accessible;
> +        let doc = aEvent.accessibleDocument;
> +        if (acc.role != Ci.nsIAccessibleRole.ROLE_DOCUMENT &&
> +            doc.role != Ci.nsIAccessibleRole.ROLE_CHROME_WINDOW)
> +          VirtualCursorController.moveCursorToObject(acc);

You don't really need the doc variable but your call. What happens for iframe docs? Will this potentially prevent the VC moving to a focusable iframe doc? Not sure we need to worry about it (now).

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +57,5 @@
> +            // Don't move forward if caret is not at end of entry.
> +            // XXX: Fix for rtl
> +            return;
> +          else
> +            target.blur();

I'm not a huge fan of focus going to the doc (if that is what happens) but okay for now.

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 630283 [details] [diff] [review]
Remove focus-follows-cursor behavior and add cursor-follows-focus.

>+      case Ci.nsIAccessibleEvent.EVENT_FOCUS:
>+      {
>+        let acc = aEvent.accessible;
>+        let doc = aEvent.accessibleDocument;
>+        if (acc.role != Ci.nsIAccessibleRole.ROLE_DOCUMENT &&
>+            doc.role != Ci.nsIAccessibleRole.ROLE_CHROME_WINDOW)
>+          VirtualCursorController.moveCursorToObject(acc);
>+        break;
>+      }

Very nice! This takes care of not losing position when a textbox gets a blur.

Comment 8

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3f4e6f14fd28

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3f4e6f14fd28

Comment 10

[Marco Zehe (:MarcoZ)]

Verified in Nightly/Android 2012-06-09.

Comment 11

[Marco Zehe (:MarcoZ)]

Comment on attachment 630283 [details] [diff] [review]
Remove focus-follows-cursor behavior and add cursor-follows-focus.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 758884, 753076
User impact if declined: Inconsistent navigation on first accessible Android release, esp when it comes to skip links like on my blog, which is a very commonly used accessibility technique.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): No risk, simply reverts part of earlier patches which turned out not to be needed
String or UUID changes made by this patch: None.

Comment 12

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/68f36beee497

Comment 13

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 630283
	Branch: mozilla-central => try
Patch 630283 could not be applied to mozilla-central.
patching file accessible/src/jsat/AccessFu.jsm
Hunk #1 FAILED at 216
Hunk #2 FAILED at 350
2 out of 2 hunks FAILED -- saving rejects to file accessible/src/jsat/AccessFu.jsm.rej
patching file accessible/src/jsat/VirtualCursorController.jsm
Hunk #1 FAILED at 46
Hunk #2 FAILED at 106
2 out of 2 hunks FAILED -- saving rejects to file accessible/src/jsat/VirtualCursorController.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.