Last Comment Bug 758884 - [AccessFu] Regression: virtual cursor gets stuck on banner graphic of Marco's blog
: [AccessFu] Regression: virtual cursor gets stuck on banner graphic of Marco's...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All Android
: -- major (vote)
: mozilla15
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
http://www.marcozehe.de
Depends on: 760972
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-26 08:06 PDT by Marco Zehe (:MarcoZ) on PTO until August 15
Modified: 2012-06-03 04:23 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(part 1/2) Present virtual cursor before modifying focus. (2.10 KB, patch)
2012-05-29 19:22 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review
(part 2/2) Check if position is still a descendant of pivot's root before moving in relation to it. (1.69 KB, patch)
2012-05-29 19:26 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review
Bug 758884 - (part 2/2) Check if position is still a descendant of pivot's root before moving in relation to it. (2.20 KB, patch)
2012-05-30 13:37 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) on PTO until August 15 2012-05-26 08:06:03 PDT
STR:
1. With the May 26, 2012 build of Android nightly, Talkback enabled, go to http://www.marcozehe.de
2. Start navigating with left and right d-pad or controller.

Result: Once v cursor lands on the graphic following the blog tag line, third or fourth element in all, v cursor gets stuck and Talkback becomes very unresponsive, even after closing nightly.

This is a very recent regression.
Comment 1 Eitan Isaacson [:eeejay] 2012-05-27 10:51:28 PDT
Is this with the virtual d-pad or a hardware keyboard?
Comment 2 Marco Zehe (:MarcoZ) on PTO until August 15 2012-05-27 11:45:35 PDT
This is with a hardware controller.
Comment 3 Marco Zehe (:MarcoZ) on PTO until August 15 2012-05-29 08:58:48 PDT
Just reconfirmed this in the latest nightly build (May 29). On www.marcozehe.de, the virtual cursor gets stuck on the graphic element following the "Musings ..." text, which is the third item on the page the pivot navigates to.

The *only* thing that works is navigating upwards to the native UI and loading a different page. Getting stuck on that graphic is 100% reproducible for me.
Comment 4 Marco Zehe (:MarcoZ) on PTO until August 15 2012-05-29 09:00:15 PDT
The "Talkback almost unusable" part didn't occur again, I guess this was a glitch that was remedeed by a proper reboot.
Comment 5 Eitan Isaacson [:eeejay] 2012-05-29 09:43:33 PDT
I see this error in adb logcat:

05-29 09:41:32.751  2507  2517 I Gecko   : [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAccessible.parent]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/accessibility/Presenters.jsm :: <TOP_LEVEL> :: line 367"  data: no]

Coming up with a fix soon.
Comment 6 Eitan Isaacson [:eeejay] 2012-05-29 19:19:54 PDT
The error is due to the tree mutations that happen when the CSS :focus selector is triggered when we focus on the link under the cursor. In other words, once we focus the pivot position it becomes invalid. Schrodinger's cat indeed! This is the cost of modifying the model from the view in an MVC design. I still think it is worth changing the focus with the virtual cursor.
Comment 7 Eitan Isaacson [:eeejay] 2012-05-29 19:22:01 PDT
Created attachment 628187 [details] [diff] [review]
(part 1/2) Present virtual cursor before modifying focus.

First present the cursor's position, and only then modify the focus in the document.
Comment 8 Eitan Isaacson [:eeejay] 2012-05-29 19:26:05 PDT
Created attachment 628188 [details] [diff] [review]
(part 2/2) Check if position is still a descendant of pivot's root before moving in relation to it.

IsInDocument seems to be the wrong test. It is hard to reproduce, but sometimes the accessibles are in the doc's accessible map, but are detached from the heirarchy. I don't understand the accessible lifecycle well enough, but checking if the position is a descendant of the pivot root seems to be more correct anyway.
Comment 9 Marco Zehe (:MarcoZ) on PTO until August 15 2012-05-29 21:23:20 PDT
Oh, it's the skip link! It is being brought into the visible view once it gets focus. Previously, it is off-screen.
Comment 10 David Bolter [:davidb] 2012-05-30 08:49:58 PDT
Requesting Alexander's feedback on comment 8 :)
Comment 11 alexander :surkov 2012-05-30 09:04:42 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #8)
> Created attachment 628188 [details] [diff] [review]
> (part 2/2) Check if position is still a descendant of pivot's root before
> moving in relation to it.
> 
> IsInDocument seems to be the wrong test. It is hard to reproduce, but
> sometimes the accessibles are in the doc's accessible map, but are detached
> from the heirarchy. I don't understand the accessible lifecycle well enough,
> but checking if the position is a descendant of the pivot root seems to be
> more correct anyway.

if you workaround IsInDocument issue and don't have reliable steps to reproduce then add an assertion
Comment 12 Eitan Isaacson [:eeejay] 2012-05-30 09:48:13 PDT
(In reply to alexander :surkov from comment #11)
> (In reply to Eitan Isaacson [:eeejay] from comment #8)
> > Created attachment 628188 [details] [diff] [review]
> > (part 2/2) Check if position is still a descendant of pivot's root before
> > moving in relation to it.
> > 
> > IsInDocument seems to be the wrong test. It is hard to reproduce, but
> > sometimes the accessibles are in the doc's accessible map, but are detached
> > from the heirarchy. I don't understand the accessible lifecycle well enough,
> > but checking if the position is a descendant of the pivot root seems to be
> > more correct anyway.
> 
> if you workaround IsInDocument issue and don't have reliable steps to
> reproduce then add an assertion

A parallel discussion is happening in bug 759618. I think I will modify IsRootDescendant here, to traverse up the tree and if it is a document do IsInDocument.

How do you suggest asserting? First traverse up the tree and then compare with IsInDocument? Keep the assertion there until we find a solution and could drop up-tree traversal in document roots?
Comment 13 alexander :surkov 2012-05-30 09:58:49 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #12)

> A parallel discussion is happening in bug 759618. 

there IsRootDescendant makes sense (because you pass arbitrary anchor), here it's used to workaround bug.

> I think I will modify
> IsRootDescendant here, to traverse up the tree and if it is a document do
> IsInDocument.

not sure how you're going to connect IsInDocument and IsRootDescendant, anyway, I guess I'll see a patch.

> How do you suggest asserting? First traverse up the tree and then compare
> with IsInDocument? 

yes, compare results of IsInDocument and IsRootDescendant, if different then assert

>Keep the assertion there until we find a solution and
> could drop up-tree traversal in document roots?

keep assertion to find a bug, when you see an assertion then you can attach a debugger and see what happens. If we fix it then let's get back IsInDocument() check or whatever appropriate.
Comment 14 Eitan Isaacson [:eeejay] 2012-05-30 10:00:42 PDT
(In reply to alexander :surkov from comment #13)
> (In reply to Eitan Isaacson [:eeejay] from comment #12)
> >Keep the assertion there until we find a solution and
> > could drop up-tree traversal in document roots?
> 
> keep assertion to find a bug, when you see an assertion then you can attach
> a debugger and see what happens. If we fix it then let's get back
> IsInDocument() check or whatever appropriate.

So you are suggesting I do this locally, not propose a patch with an assertion...
Comment 15 alexander :surkov 2012-05-30 10:13:18 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #14)

> So you are suggesting I do this locally, not propose a patch with an
> assertion...

no, I suggest a patch with assertion. Anybody can debug it.
Comment 16 Eitan Isaacson [:eeejay] 2012-05-30 13:34:37 PDT
(In reply to alexander :surkov from comment #15)
> (In reply to Eitan Isaacson [:eeejay] from comment #14)
> 
> > So you are suggesting I do this locally, not propose a patch with an
> > assertion...
> 
> no, I suggest a patch with assertion. Anybody can debug it.

I managed to reliably reproduce, and I filed bug 759875. So I am just putting in a to-do comment to use a more optimized path once that bug is fixed.
Comment 17 Eitan Isaacson [:eeejay] 2012-05-30 13:37:12 PDT
Created attachment 628453 [details] [diff] [review]
Bug 758884 - (part 2/2) Check if position is still a descendant of pivot's root before moving in relation to it.

Just added a comment about a future optimization via IsInDocument.

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