Closed
Bug 391847
Opened 15 years ago
Closed 15 years ago
Too many visibility notifications being fired from layout
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: aaronlev, Assigned: aaronlev)
References
(Blocks 1 open bug, )
Details
(Keywords: access, regression)
Attachments
(2 files)
16.65 KB,
patch
|
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
17.30 KB,
patch
|
ginnchen+exoracle
:
review+
bzbarsky
:
approval1.9+
|
Details | Diff | Splinter Review |
1) Go to http://www.mozilla.org/access/dhtml/alert 2) Active "Show (via visibility style), no focus In Firefox 2 we only received 1 notification for the root visibility change, from nsFrameManager::ReResolveStyleContext(). Now we are getting a notification from every child frame as well.
Flags: blocking1.9?
![]() |
||
Comment 1•15 years ago
|
||
So when did this regress?
Assignee | ||
Comment 2•15 years ago
|
||
Don't know yet, I'll try to find out when I have time.
Assignee | ||
Comment 3•15 years ago
|
||
Works 10-05-06 Broken 10-06-06
Assignee | ||
Comment 4•15 years ago
|
||
Hmm that looks like my own checkin -- bug 354745
![]() |
||
Comment 5•15 years ago
|
||
Right. That would make sense. That checkin would have exactly this effect.
Assignee | ||
Comment 6•15 years ago
|
||
Any idea how I can fix bug 354745 but only get notifications for the root of each change? If a style change affects n subtrees I just want n notifcations. That bug was "When frames change visibility or display indirectly, e.g. via an ancestor class/attribute change which causes computed style to change, the accessibility show/hide/reorder events are not getting fired."
Summary: To many visibility notifications being fired from layout → Too many visibility notifications being fired from layout
![]() |
||
Comment 7•15 years ago
|
||
What do you mean by "root of each change"? For example if node A has nodes B, C as kids, and B has D, E as kids and single mutation causes a visibility change on A and E, what notifications do you expect to get?
Assignee | ||
Comment 8•15 years ago
|
||
Like this? Changes on A and E A | \ B C |\ D E If possible, I just want the notification on A. That accessible whole subtree will be invalidated, so the notification on E is not necessary. But certainly, if the whole subtree A becomes visible, I don't want notifactions on A, B, C, D and E.
![]() |
||
Comment 9•15 years ago
|
||
Then you'll have to do your own coalescing somewhere, I think... The style system doesn't coalesce things like that.
![]() |
||
Comment 10•15 years ago
|
||
Though maybe we could change nsStyleChangeList::AppendChange to do more coalescing... It'd be pretty risky, though. Not 1.9 material.
Assignee | ||
Comment 11•15 years ago
|
||
FireDelayedAccessibleEvent() already has code to coalesce duplicate a11y events, so it was convenient to provide a new option for what it considered a duplicate event. For mutation events we will throw away something with the same type of event in an ancestor. We coallesce the events and then fire them in FlushPendingEvents(). I think this is a very good solution.
Attachment #276537 -
Flags: superreview?(bzbarsky)
Attachment #276537 -
Flags: review?(bzbarsky)
![]() |
||
Comment 12•15 years ago
|
||
Comment on attachment 276537 [details] [diff] [review] 1) Only fire notifications for primary frame, 2) Coalesce in FireDelayedAccessibleEvent() 3) Delay subtree shutdown (RefreshNodes) until after hide event, 4) Minor touchups >Index: accessible/src/base/nsAccessibilityUtils.cpp >+nsAccUtils::IsAncestorOf(nsIDOMNode *aPossibleAncestorNode, >+ nsIDOMNode *aPossibleDescendantNode) So X is not an ancestor of itself as far as this function is concerned, right? >+ aPossibleDescendantNode = parentNode; Then you lose your ref... that's bad. Don't do that. Stash it in a comptr, please. >Index: accessible/src/base/nsDocAccessible.cpp > FireDelayedToolkitEvent(nsIAccessibleEvent::EVENT_SELECTION_WITHIN, >- multiSelectDOMNode, nsnull, PR_TRUE); >+ multiSelectDOMNode, nsnull, eRemoveDupes); So this changes from allow to remove? >- if (!isAsynch) { >- // Calculate "is from user input" while we still synchronous and have the info >- nsAccEvent::PrepareForEvent(childNode); >- } Why is that no longer needed? sr=bzbarsky, but this needs module peer review.
Attachment #276537 -
Flags: superreview?(bzbarsky)
Attachment #276537 -
Flags: superreview+
Attachment #276537 -
Flags: review?(bzbarsky)
Attachment #276537 -
Flags: review?
Assignee | ||
Comment 13•15 years ago
|
||
(In reply to comment #12) > (From update of attachment 276537 [details] [diff] [review]) > >Index: accessible/src/base/nsAccessibilityUtils.cpp > >+nsAccUtils::IsAncestorOf(nsIDOMNode *aPossibleAncestorNode, > >+ nsIDOMNode *aPossibleDescendantNode) > > So X is not an ancestor of itself as far as this function is concerned, right? Right. Otherwise it'd be harder to name and we'd be checking if it's the same node twice. > >- if (!isAsynch) { > >- // Calculate "is from user input" while we still synchronous and have the info > >- nsAccEvent::PrepareForEvent(childNode); > >- } > Why is that no longer needed? That's actually done in FireAccessibleEvent() anyway. When I fixed bug 390692 I added that improvement while developing the patch but neglected to remove the extra caller. Will fix the other things.
Assignee | ||
Comment 14•15 years ago
|
||
Boris, do I need another r= for the small layout change?
Assignee | ||
Updated•15 years ago
|
Attachment #276537 -
Flags: review?
Assignee | ||
Comment 15•15 years ago
|
||
Attachment #276574 -
Flags: review?(ginn.chen)
![]() |
||
Comment 16•15 years ago
|
||
No, the layout change is good to go.
Comment 17•15 years ago
|
||
Comment on attachment 276574 [details] [diff] [review] Address bz's comments I'm not sure I understand the relation of !aFrame->GetNextContinuation() and "Primary frames" We only do it for last frame, so that we only do it once for one accessible object ?
Attachment #276574 -
Flags: review?(ginn.chen) → review+
Assignee | ||
Comment 18•15 years ago
|
||
Actually, we only want to do it for the *first* frame, so that we only do it once per DOM node.
Assignee | ||
Updated•15 years ago
|
Attachment #276574 -
Flags: approval1.9?
Comment 19•15 years ago
|
||
(In reply to comment #18) > Actually, we only want to do it for the *first* frame, so that we only do it > once per DOM node. > but !aFrame->GetNextContinuation() means *last* frame, right?
Assignee | ||
Comment 20•15 years ago
|
||
Right, that's true -- it actually still works. It's fired once per node. Although, it would be clearer if there was a good test for whether it's the primary frame.
![]() |
||
Comment 21•15 years ago
|
||
Check GetPrevContinuation instead?
Assignee | ||
Comment 22•15 years ago
|
||
Okay, I'll change it to GetPrevContinuation(). A new patch should not necessary, but I'll test it before checking in.
![]() |
||
Comment 23•15 years ago
|
||
Comment on attachment 276574 [details] [diff] [review] Address bz's comments Looks good. a=bzbarsky with the GetPrevContinuation change.
Attachment #276574 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Flags: blocking1.9?
You need to log in
before you can comment on or make changes to this bug.
Description
•