758884 - [AccessFu] Regression: virtual cursor gets stuck on banner graphic of Marco's blog

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

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]]

Is this with the virtual d-pad or a hardware keyboard?

Comment 2

[Marco Zehe (:MarcoZ)]

This is with a hardware controller.

Comment 3

[Marco Zehe (:MarcoZ)]

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

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]]

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]]

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]]

Attachment: (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]]

Attachment: (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)]

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] (NeedInfo me for attention)]

Requesting Alexander's feedback on comment 8 :)

Comment 11

[alexander :surkov (:asurkov)]

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

(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 (:asurkov)]

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

(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 (:asurkov)]

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

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

Attachment: 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.

Comment 18

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d3092b482b0f
http://hg.mozilla.org/integration/mozilla-inbound/rev/b7728496df08

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d3092b482b0f
https://hg.mozilla.org/mozilla-central/rev/b7728496df08