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)
Tracking
()
VERIFIED
FIXED
mozilla16
Tracking | Status | |
---|---|---|
firefox15 | --- | fixed |
People
(Reporter: MarcoZ, Assigned: eeejay)
References
()
Details
Attachments
(1 file, 1 obsolete file)
6.10 KB,
patch
|
davidb
:
review+
MarcoZ
:
feedback+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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.
Reporter | ||
Comment 2•13 years ago
|
||
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)
Reporter | ||
Comment 3•13 years ago
|
||
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)
Assignee | ||
Comment 4•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Assignee: nobody → eitan
Target Milestone: --- → mozilla15
Assignee | ||
Updated•13 years ago
|
Attachment #630283 -
Flags: feedback?(marco.zehe)
Comment 6•13 years ago
|
||
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+
Reporter | ||
Updated•13 years ago
|
Attachment #630283 -
Flags: feedback?(marco.zehe) → feedback+
Reporter | ||
Comment 7•13 years ago
|
||
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.
Assignee | ||
Comment 8•13 years ago
|
||
Comment 9•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla15 → mozilla16
Reporter | ||
Comment 10•12 years ago
|
||
Verified in Nightly/Android 2012-06-09.
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 11•12 years ago
|
||
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?
Updated•12 years ago
|
Attachment #630283 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 12•12 years ago
|
||
Updated•12 years ago
|
Whiteboard: [autoland-try] → [autoland-in-queue]
Comment 13•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: [autoland-in-queue]
Updated•12 years ago
|
status-firefox15:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•