Closed Bug 757109 Opened 9 years ago Closed 9 years ago

[AccessFu] Handle moving from invalid virtual cursor positions

Categories

(Core :: Disability Access APIs, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: eeejay, Assigned: eeejay)

Details

Attachments

(1 file, 3 obsolete files)

For example, by activating an item under the virtual cursor, a user could make that item go away, like a close button on a dialog. Or other tree mutations.

If we attempt to move the virtual cursor, it tries to move in relation to an invalid position. We added an exception that is thrown in this case in bug 753093. Now AccessFu needs to do the right thing.
If there is a focused node in the document, set the cursor on that. If not, set the position to null, the next move will start at the begining of the doc.

We will probably need to revisit this one day and do something more sophisticated, like move in relation to the next surviving sibling (or aunt/uncle).
Attachment #625676 - Flags: review?(dbolter)
Comment on attachment 625676 [details] [diff] [review]
Handle moving from invalid virtual cursor positions.

This will put AccessFu on par with screen reader/browser combos on Windows where, if no focused item is there and the last focused/dealt with accessible goes away, the next tab etc. will put focus on the document, or on the first focusable item. I think it's good to have this to not leave users hanging in limbo land.
Comment on attachment 625676 [details] [diff] [review]
Handle moving from invalid virtual cursor positions.

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

r=me

::: accessible/src/jsat/AccessFu.jsm
@@ +202,5 @@
>            let position = pivot.position;
>            let doc = aEvent.DOMNode;
>  
> +          if (doc instanceof Ci.nsIDOMDocument &&
> +              position && position.DOMNode) {

Aside: micro benchmarking seems to indicate instanceof is the slowest op here (so I'd normally check that condition last) - yet I slap my hand for premature optimization over readability.
Attachment #625676 - Flags: review?(dbolter) → review+
(In reply to David Bolter [:davidb] from comment #3)

> > +          if (doc instanceof Ci.nsIDOMDocument &&
> > +              position && position.DOMNode) {
> 
> Aside: micro benchmarking seems to indicate instanceof is the slowest op
> here (so I'd normally check that condition last) - yet I slap my hand for
> premature optimization over readability.

I think it's readable in either case.
(In reply to alexander :surkov from comment #4)
> I think it's readable in either case.

OK
Sorry, re-requesting review becasue I forgot to include the _isValidAcc function in the last patch. Oops!
Attachment #625676 - Attachment is obsolete: true
Attachment #626038 - Flags: review?(dbolter)
I wondered! Forgot to flag and hunt though. Will update review after lunch.
Comment on attachment 626038 [details] [diff] [review]
Bug 757109 - Handle moving from invalid virtual cursor positions.

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

ok.

::: accessible/src/jsat/Presenters.jsm
@@ +397,5 @@
> +    try {
> +      let extstate = {};
> +      aAccessible.getState({}, extstate);
> +      return !(aAccessible.value & Ci.nsIAccessibleStates.EXT_STATE_DEFUNCT);
> +    } catch (x) {

I'd be tempted to write an _isDefunct instead (valid is more of a loaded term).
Attachment #626038 - Flags: review?(dbolter) → review+
Attachment #626038 - Attachment is obsolete: true
Comment on attachment 626082 [details] [diff] [review]
Handle moving from invalid virtual cursor positions. r=davidb

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

Thanks.
http://hg.mozilla.org/integration/mozilla-inbound/rev/9bb74148cee5
Assignee: nobody → eitan
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/9bb74148cee5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.