413777 - Correct focus events absent when tabbing among links at sourceforge.net

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

1. Navigate to sourceforge.net
2. Launch Accerciser, switch to the event monitor, and monitor
   focus events for the selected application
3. Start at the top of sourceforge.net and begin tabbing from 
   link to link

Expected results:  We would get focus events for links that look like

  focus:(0, 0, None)
         source: [ | Home]
         application: [application | Minefield]
  focus:(0, 0, None)
         source: [ | Browse Software]
         application: [application | Minefield]

Actual results:  More often than not, we get focus events for objects of ROLE_INVALID that look like

    focus:(0, 0, None)
           source: [invalid | ]
           application: [application | Minefield]

Comment 1

[Joanmarie Diggs]

Setting the whiteboard to orca:immediate as this is a popular site which (I would hope) is not employing completely bogus markup. :-)  And we really cannot work around this issue.  

Thanks in advance!!

Comment 2

[Joanmarie Diggs]

I'm also seeing object of ROLE_INVALID in Gmail's standard (i.e. NOT the HTML/older) version.  Another popular site....

Comment 3

[Aaron Leventhal]

Ginn/Evan, can you look? I'm not seeing an issue on Windows. We have 2 days before freeze.

Comment 4

[Ginn Chen]

The accessible object for the link was shutdown due to a ASYNCH_HIDE event.

The event was from
=>[1] nsDocAccessible::InvalidateCacheSubtree(this = 0x86caff0, aChild = 0x9b0bee0, aChangeEventType = 6U), line 1836 in "nsDocAccessible.cpp"
  [2] nsAccessibilityService::InvalidateSubtreeFor(this = 0x8675348, aShell = 0x9b4a600, aChangeContent = 0x9b0bee0, aEvent = 6U), line 1912 in "nsAccessibilityService.cpp"
  [3] nsCSSFrameConstructor::RecreateFramesForContent(this = 0x9b4b960, aContent = 0x9b0bee0), line 11273 in "nsCSSFrameConstructor.cpp"
  [4] nsCSSFrameConstructor::RestyleElement(this = 0x9b4b960, aContent = 0x9b0bee0, aPrimaryFrame = 0x9bea2b8, aMinHint = 0), line 10061 in "nsCSSFrameConstructor.cpp"

The time line is like
nsRootAccessible::FireAccessibleFocusEvent -> FireDelayedToolkitEvent(EVENT_FOCUS,...)
nsDocAccessible::InvalidateCacheSubtree -> FireShowHideEvents -> FireDelayedAccessibleEvent
nsDocAccessible::FlushPendingEvents -> atk_focus_tracker_notify
nsDocAccessible::FlushPendingEvents -> RefreshNodes -> nsAccessibleWrap::Shutdown
getRoleCB -> got ATK_ROLE_INVALID

Comment 5

[Aaron Leventhal]

Perhaps they have a :focus rule in their style sheet.

Comment 6

[Ginn Chen]

I think it is

input:focus, a:focus {overflow: hidden}

from http://static.sourceforge.net/css/sfx.php?secure=0&20071024-1056

Any idea how to fix this problem?
I think it's not only on Linux.

Comment 7

[Ginn Chen]

BTW: nsDocAccessible::InvalidateCacheSubtree is called for EVENT_ASYNCH_SIGNIFICANT_CHANGE. (0x6)

Comment 8

[Aaron Leventhal]

Attachment: One idea -- delay creation of accessible until after style change

I'm not sure this will help, but here are some other ideas:
1. Make sure focus events come after SHOW/HIDE events in mEventsToFire if they're for the same node. This would be done in FireDelayedAccessibleEvent() where we do some similar things.
2. Find a way to detect a style change that is only an overflow change, and prevent subtree invalidation in that case. If we needed to cache something like the current style or frame, perhaps we only need to cache it for the currently focused item

Comment 9

[Aaron Leventhal]

There are too many hide/show/text-insert events being fired, even on Windows.

The EVENT_ASYNCH_SIGNIFICANT_CHANGE is really for CSS display changes that don't change visibility, not for overflow changes. So we should try not to fire anything for the EVENT_ASYNCH_SIGNIFICANT_CHANGE unless frame->GetType() changes.

I thought we tried something like this before but it didn't work. Maybe that was before we cached frame in a11y.

Comment 10

[Aaron Leventhal]

Attachment: This fixes it by only firing our LAYOUT_ASYNCH_SIGNIFICANT_CHANGE when it's truly something a11y cares about

Comment 11

[Boris Zbarsky [:bzbarsky]]

So you're OK with not firing the event if some descendant of the frame changed type?

Comment 12

[Aaron Leventhal]

Boris, is there any way we can detect all or most of those cases?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I have no idea, to be honest.  The basic assumption in layout is that if change X has to happen at some point in the tree then it's OK to just do change X at any higher point in the tree (X could be a frame reconstruct, layout, painting, whatever).  Of course doing it as low in the tree as possible is desirable for performance reasons, so we do that.  But if the same change has to happen on both a parent and a child, we just do it on the parent, again for performance reasons.

It sounds like you're interested in more fine-grained changes than layout keeps track of, so in your case the changes on the parent and child are different (though the same from layout's point of view).  So you'd want to have some separate tracking system... For example, record the entire frame tree before the frame reconstruct, and then compare it to the one after.  That's likely to be quite expensive, however.

That's the "all" answer.  The "most" answer is that this patch covers that.  So it'll usually work, and sometimes completely drop changes on the floor.

Comment 14

[Aaron Leventhal]

Can we tell when it's the simple case -- there is only 1 change and therefore it's happening at the very place the change applies?

If we could just eliminate the extra events in that case we'd solve the problem that happens with focus events, where there is a rule like:

a:focus { overflow: scroll; }

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Can we tell when it's the simple case -- there is only 1 change

Layout doesn't have that information at this point.  You'd have to compute it (at non-trivial performance penalty).

Comment 16

[Aaron Leventhal]

Marco, I just kicked off some try server builds. It makes some changes to how we deal with focus, by slightly delaying the creation of an accessible object. The layout changes and accessible tree invalidation should have taken affect by then, so it may very well fix the issue.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 300032 [details] [diff] [review]
This fixes it by only firing our LAYOUT_ASYNCH_SIGNIFICANT_CHANGE when it's truly something a11y cares about

This leads to dropped accessibility events, which I assume is undesirable.

Comment 18

[Marco Zehe (:MarcoZ)]

Aaron, these try server builds are not good at all:
1. Pages that take only a small amount of time to load, such as any mozilla.org page from within the Mozilla building, never tell Orca when the loading has finished. Thus, reading doesn't srart automatically.
2. On sourceforge.net, tab only works for the first two links, the second one being "Jump to main content". After that, tabbing again yields silence.

Comment 19

[Aaron Leventhal]

Ginn, any ideas?

Comment 20

[Aaron Leventhal]

Attachment: New idea -- filter out extra events in a11y module itself. This one keeps it simple by only worrying about superfluous invalidations on the focused item itself.

We should take extra time to make sure these came from an ASYNCH_SIGNIFICANT_CHANGE.

Comment 21

[Aaron Leventhal]

Attachment: For focused accessible, don't listen to layout invalidations if the visibility and frame type don't change

Comment 22

[Aaron Leventhal]

Please ignore the extra newline I accidentally added in nsRootAccessible. That's fixed.

Comment 23

[Aaron Leventhal]

New test builds with the proposed fix:
https://build.mozilla.org/tryserver-builds/2008-02-03_14:[e-mail]-AvoidExtraFocusInvalidations413777/

Comment 24

[Ginn Chen]

Looks good to me.
Nit:
+nsIAtom *nsDocAccessible::gLastFocusedFrameType = 0;

+        nsIAtom *newFrameType = (focusFrame && focusFrame->GetStyleVisibility()->IsVisible()) ? focusFrame->GetType() : 0;

nsnull is better.

Comment 25

[Joanmarie Diggs]

The test build seems to fix the problem.  Thanks!

Comment 26

[alexander :surkov (:asurkov)]

checked in with Ginn's nit

Comment 27

[alexander :surkov (:asurkov)]

in-testsuite+ per bug 591163

Comment 28

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381