757109 - [AccessFu] Handle moving from invalid virtual cursor positions

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

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.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Handle moving from invalid virtual cursor positions.

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).

Comment 2

[Marco Zehe (:MarcoZ)]

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 3

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

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.

Comment 4

[alexander :surkov (:asurkov)]

(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.

Comment 5

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

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

OK

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Bug 757109 - Handle moving from invalid virtual cursor positions.

Sorry, re-requesting review becasue I forgot to include the _isValidAcc function in the last patch. Oops!

Comment 7

[[e-mail]]

I wondered! Forgot to flag and hunt though. Will update review after lunch.

Comment 8

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

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).

Comment 9

[Eitan Isaacson [:eeejay]]

Attachment: Hundle moving from invalid virtual cursor positions. r=davidb

Comment 10

[Eitan Isaacson [:eeejay]]

Attachment: Handle moving from invalid virtual cursor positions. r=davidb

Comment 11

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

Comment on attachment 626082 [details] [diff] [review]
Handle moving from invalid virtual cursor positions. r=davidb

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

Thanks.

Comment 12

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9bb74148cee5

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9bb74148cee5