383482 - the overflow:hidden style should not "invalidate" the link with focus

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Attachment: test case

Steps to reproduce:

1. Launch Accerciser and turn on event monitoring
2. Use Tab to move from link to link in the attached test case

For the first two links, you will see events similar to:

focus(0, 0, None)
	source: [invalid | ]
	application: [application | Minefield]
object:state-changed:focused(1, 0, None)
	source: [invalid | ]
	application: [application | Minefield]

For the last two links, you will see events similar to:

focus(0, 0, None)
	source: [ | three]
	application: [application | Minefield]
object:state-changed:focused(1, 0, None)
	source: [ | three]
	application: [application | Minefield]

The reason the first two links have a role of INVALID is because the overflow:hidden style has been applied to them when they are focused.

Comment 1

[Ginn Chen]

I don't have this issue with at-poke.

I don't know what accerciser is doing.
Sometimes it works.
I'll investigate it tomorrow.

Comment 2

[Joanmarie Diggs]

Thanks Ginn!

I couldn't tell you why it's not an issue with at-poke.  As for "sometimes it works":  We're seeing that inconsistency in Orca as well.  I wonder if it's a timing thing....  In Orca what I have found is that sometimes we get a focus: event and the role has not (yet?) changed into INVALID, but the object is defunct.  Other times, it's already INVALID.

Comment 3

[Ginn Chen]

Attachment: stack

The accessible object is recreated. Here's the stack.

If you query the state of the AtkObject, you should get STATE_DEFUNCT.

Look like a website bug. I don't know what I should do in Firefox a11y module.

Comment 4

[Aaron Leventhal]

Ginn, why is there a difference between what at-poke and Accerciser report?

Websites are free to do anything on focus, so ATs need to be careful about assuming that objects are still around. However, we shouldn't fire the focus event for the accessible that's going away -- we should fire it for the new one.

Comment 5

[Ginn Chen]

The recreate happens after the link is focused.
How can we know the accessible is going away?

I noticed Accerciser called getNameCB several times, for the first 2-3 times, getNameCB returned valid value, then Shutdown is called, the node is gone.
Maybe at-poke doesn't call getNameCB so many times. Or maybe at-poke just use atkobject->name without calling getNameCB.

I think we could return the cached name and role in atkobject, when it is defunct.

Comment 6

[Ginn Chen]

I confirmed at-poke always call getNameCB before accessiblewrap is shutdown.

In nsHTMLAnchorElement.cpp
242       if (handler && aPresContext->EventStateManager()->
243                                    SetContentState(this, NS_EVENT_STATE_FOCUS)) {
244         nsCOMPtr<nsIPresShell> presShell = aPresContext->GetPresShell();
245         if (presShell) {
246           presShell->ScrollContentIntoView(this, NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE,
247                                            NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE);
248         }
249       }

nsRootAccessible::FireAccessibleFocusEvent is called from line 242
nsLinkableAccessible::Shutdown is called from line 247

Comment 7

[Aaron Leventhal]

> I think we could return the cached name and role in atkobject, when it 
> is defunct

No, please don't do that. Just make sure the focus event is fired for the new accessible object. That's why in similar cases we use FireDelayedToolkitEvent(), which fires an event after a delay for that DOM node. Because of the delay the new accessible would be there.

I see in FireAccessibleFocusEvent() we're not using delayed events. We probably should be -- but check CVS blame to see if we ever did that before:
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsRootAccessible.cpp#529
The state change event below it should then probably use FireDelayedAccessibleEvent()

Comment 8

[Aaron Leventhal]

Attachment: Ginn, does this fix it?

For some reason this fix isn't necessary on Windows, in my tests with accessible event watcher the focus events are reported correctly whether the info is gathered in process or not.

But, it doesn't seem to hurt on Windows, either. So, if it helps on ATK/AT-SPI we can do it.

Comment 9

[Ginn Chen]

state-changed:focused event is correct now, but I didn't receive any focus event after applying your patch

Comment 10

[Aaron Leventhal]

> state-changed:focused event is correct now, but I didn't receive any focus
> event after applying your patch
The fact that the state change is correct now means that this is the right direction to go, I think.

Can you debug and see why the focus event is not getting through?

Comment 11

[Ginn Chen]

Attachment: patch v2

Aaron, your patch is correct and works perfectly.
But we need fix atk code to get it work.

Any reason we should have nsDocAccessibleWrap::FireToolkitEvent?
I moved it to nsAccessibleWrap::FireAccessibleEvent.

Comment 12

[Aaron Leventhal]

Comment on attachment 268344 [details] [diff] [review]
patch v2

Ginn, Surkov has been refactoring events and he was still planning to combine those at some point later.

So I'll let him review it.

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 268344 [details] [diff] [review]
patch v2

fine with me