Last Comment Bug 757109 - [AccessFu] Handle moving from invalid virtual cursor positions
: [AccessFu] Handle moving from invalid virtual cursor positions
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla15
Assigned To: Eitan Isaacson [:eeejay]
:
: alexander :surkov
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-21 10:06 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-05-23 05:29 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Handle moving from invalid virtual cursor positions. (4.17 KB, patch)
2012-05-21 10:14 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review
Bug 757109 - Handle moving from invalid virtual cursor positions. (4.64 KB, patch)
2012-05-22 08:51 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review
Hundle moving from invalid virtual cursor positions. r=davidb (4.64 KB, patch)
2012-05-22 10:25 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Handle moving from invalid virtual cursor positions. r=davidb (4.64 KB, patch)
2012-05-22 10:33 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-05-21 10:06:49 PDT
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] 2012-05-21 10:14:41 PDT
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).
Comment 2 Marco Zehe (:MarcoZ) 2012-05-21 21:28:26 PDT
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] 2012-05-22 06:29:36 PDT
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 2012-05-22 06:48:37 PDT
(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] 2012-05-22 06:53:31 PDT
(In reply to alexander :surkov from comment #4)
> I think it's readable in either case.

OK
Comment 6 Eitan Isaacson [:eeejay] 2012-05-22 08:51:44 PDT
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!
Comment 7 david.bolter@gmail.com 2012-05-22 09:22:06 PDT
I wondered! Forgot to flag and hunt though. Will update review after lunch.
Comment 8 David Bolter [:davidb] 2012-05-22 10:00:36 PDT
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] 2012-05-22 10:25:48 PDT
Created attachment 626074 [details] [diff] [review]
Hundle moving from invalid virtual cursor positions. r=davidb
Comment 10 Eitan Isaacson [:eeejay] 2012-05-22 10:33:00 PDT
Created attachment 626082 [details] [diff] [review]
Handle moving from invalid virtual cursor positions. r=davidb
Comment 11 David Bolter [:davidb] 2012-05-22 10:33:45 PDT
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 13 Ed Morley [:emorley] 2012-05-23 05:29:12 PDT
https://hg.mozilla.org/mozilla-central/rev/9bb74148cee5

Note You need to log in before you can comment on or make changes to this bug.