Last Comment Bug 760972 - [AccessFu] After fix for bug 758884, inconsistent behavior when a skip link is encountered
: [AccessFu] After fix for bug 758884, inconsistent behavior when a skip link i...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla16
Assigned To: Eitan Isaacson [:eeejay]
:
: alexander :surkov
Mentors:
http://www.marcozehe.de
Depends on:
Blocks: 758884
  Show dependency treegraph
 
Reported: 2012-06-03 04:23 PDT by Marco Zehe (:MarcoZ)
Modified: 2012-06-23 10:03 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Revert actually focusing elements the virtual cursor moves to (1.55 KB, patch)
2012-06-05 01:10 PDT, Marco Zehe (:MarcoZ)
no flags Details | Diff | Splinter Review
Remove focus-follows-cursor behavior and add cursor-follows-focus. (6.10 KB, patch)
2012-06-05 12:47 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
mzehe: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) 2012-06-03 04:23:18 PDT
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] 2012-06-03 07:00:42 PDT
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) 2012-06-05 01:10:18 PDT
Created attachment 630100 [details] [diff] [review]
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) 2012-06-05 12:17:37 PDT
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] 2012-06-05 12:47:28 PDT
Created attachment 630283 [details] [diff] [review]
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] 2012-06-05 12:51:05 PDT
Doing a try build for easier testing.
Comment 6 David Bolter [:davidb] 2012-06-05 13:01:40 PDT
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) 2012-06-06 07:28:11 PDT
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] 2012-06-06 10:04:15 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/3f4e6f14fd28
Comment 9 Ed Morley [:emorley] 2012-06-07 05:53:14 PDT
https://hg.mozilla.org/mozilla-central/rev/3f4e6f14fd28
Comment 10 Marco Zehe (:MarcoZ) 2012-06-09 07:38:20 PDT
Verified in Nightly/Android 2012-06-09.
Comment 11 Marco Zehe (:MarcoZ) 2012-06-09 07:42:42 PDT
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] 2012-06-11 15:01:22 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/68f36beee497
Comment 13 Mozilla RelEng Bot 2012-06-21 01:06:16 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.