[AccessFu] Handle moving from invalid virtual cursor positions

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

Trunk
mozilla15
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 625676 [details] [diff] [review]
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).
Attachment #625676 - Flags: review?(dbolter)

Comment 2

5 years ago
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+

Comment 4

5 years ago
(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
(Assignee)

Comment 6

5 years ago
Created attachment 626038 [details] [diff] [review]
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!
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+
(Assignee)

Comment 9

5 years ago
Created attachment 626074 [details] [diff] [review]
Hundle moving from invalid virtual cursor positions. r=davidb
Attachment #626038 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 626082 [details] [diff] [review]
Handle moving from invalid virtual cursor positions. r=davidb
Attachment #626074 - 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.
(Assignee)

Comment 12

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.