Closed
Bug 413777
Opened 17 years ago
Closed 17 years ago
Correct focus events absent when tabbing among links at sourceforge.net
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jdiggs, Assigned: aaronlev)
References
(Blocks 1 open bug, )
Details
(Keywords: access, Whiteboard: orca:immediate)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
ginnchen+exoracle
:
review+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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]
Reporter | ||
Comment 1•17 years ago
|
||
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!!
Whiteboard: orca:immediate
Reporter | ||
Comment 2•17 years ago
|
||
I'm also seeing object of ROLE_INVALID in Gmail's standard (i.e. NOT the HTML/older) version. Another popular site....
Assignee | ||
Comment 3•17 years ago
|
||
Ginn/Evan, can you look? I'm not seeing an issue on Windows. We have 2 days before freeze.
Assignee: aaronleventhal → 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
Assignee | ||
Comment 5•17 years ago
|
||
Perhaps they have a :focus rule in their style sheet.
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.
BTW: nsDocAccessible::InvalidateCacheSubtree is called for EVENT_ASYNCH_SIGNIFICANT_CHANGE. (0x6)
Assignee | ||
Comment 8•17 years ago
|
||
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
Assignee | ||
Comment 9•17 years ago
|
||
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.
Assignee | ||
Comment 10•17 years ago
|
||
Assignee: ginn.chen → aaronleventhal
Attachment #300010 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #300032 -
Flags: superreview?(bzbarsky)
Attachment #300032 -
Flags: review?(bzbarsky)
Comment 11•17 years ago
|
||
So you're OK with not firing the event if some descendant of the frame changed type?
Assignee | ||
Comment 12•17 years ago
|
||
Boris, is there any way we can detect all or most of those cases?
Comment 13•17 years ago
|
||
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.
Assignee | ||
Comment 14•17 years ago
|
||
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•17 years ago
|
||
> 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).
Assignee | ||
Comment 16•17 years ago
|
||
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•17 years ago
|
||
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.
Attachment #300032 -
Flags: superreview?(bzbarsky)
Attachment #300032 -
Flags: superreview-
Attachment #300032 -
Flags: review?(bzbarsky)
Attachment #300032 -
Flags: review-
Comment 18•17 years ago
|
||
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.
Assignee | ||
Comment 19•17 years ago
|
||
Ginn, any ideas?
Assignee: aaronleventhal → ginn.chen
Status: ASSIGNED → NEW
Assignee | ||
Comment 20•17 years ago
|
||
We should take extra time to make sure these came from an ASYNCH_SIGNIFICANT_CHANGE.
Attachment #300032 -
Attachment is obsolete: true
Assignee | ||
Comment 21•17 years ago
|
||
Assignee: ginn.chen → aaronleventhal
Attachment #301146 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #301171 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 22•17 years ago
|
||
Please ignore the extra newline I accidentally added in nsRootAccessible. That's fixed.
Assignee | ||
Comment 23•17 years ago
|
||
New test builds with the proposed fix:
https://build.mozilla.org/tryserver-builds/2008-02-03_14:10-aaronleventhal@moonset.net-AvoidExtraFocusInvalidations413777/
Comment 24•17 years ago
|
||
Looks good to me.
Nit:
+nsIAtom *nsDocAccessible::gLastFocusedFrameType = 0;
+ nsIAtom *newFrameType = (focusFrame && focusFrame->GetStyleVisibility()->IsVisible()) ? focusFrame->GetType() : 0;
nsnull is better.
Attachment #301171 -
Flags: review+
Assignee | ||
Updated•17 years ago
|
Attachment #301171 -
Flags: review?(surkov.alexander) → approval1.9?
Reporter | ||
Comment 25•17 years ago
|
||
The test build seems to fix the problem. Thanks!
Updated•17 years ago
|
Attachment #301171 -
Flags: approval1.9? → approval1.9+
Comment 26•17 years ago
|
||
checked in with Ginn's nit
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Flags: in-testsuite?
Comment 28•6 years ago
|
||
Keywords: sec508
You need to log in
before you can comment on or make changes to this bug.
Description
•