Closed Bug 760972 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: Disability Access APIs, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla16
Tracking Status
firefox15 --- fixed

People

(Reporter: MarcoZ, Assigned: eeejay)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 758884
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.
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?
Attachment #630100 - Flags: review?(eitan)
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.
Attachment #630100 - Attachment is obsolete: true
Attachment #630100 - Flags: review?(eitan)
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.
Attachment #630283 - Flags: review?(dbolter)
Assignee: nobody → eitan
Target Milestone: --- → mozilla15
Attachment #630283 - Flags: feedback?(marco.zehe)
Doing a try build for easier testing.
Whiteboard: [autoland-try]
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.
Attachment #630283 - Flags: review?(dbolter) → review+
Attachment #630283 - Flags: feedback?(marco.zehe) → feedback+
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla15 → mozilla16
Verified in Nightly/Android 2012-06-09.
Status: RESOLVED → VERIFIED
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.
Attachment #630283 - Flags: approval-mozilla-aurora?
Attachment #630283 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [autoland-try] → [autoland-in-queue]
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.
Whiteboard: [autoland-in-queue]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: