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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

(Blocks 1 open bug, )

Details

(Keywords: access, regression)

Attachments

(2 files)

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?
So when did this regress?
Don't know yet, I'll try to find out when I have time.
Works 10-05-06
Broken 10-06-06
Hmm that looks like my own checkin -- bug 354745
Right.  That would make sense.  That checkin would have exactly this effect.
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
Blocks: 391723
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?
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.
Then you'll have to do your own coalescing somewhere, I think... The style system doesn't coalesce things like that.
Though maybe we could change nsStyleChangeList::AppendChange to do more coalescing... It'd be pretty risky, though.  Not 1.9 material.
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 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?
(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.
Boris, do I need another r= for the small layout change?
Attachment #276537 - Flags: review?
Attachment #276574 - Flags: review?(ginn.chen)
No, the layout change is good to go.
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+
Actually, we only want to do it for the *first* frame, so that we only do it once per DOM node.
Attachment #276574 - Flags: approval1.9?
(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?
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.
Check GetPrevContinuation instead?
Okay, I'll change it to GetPrevContinuation(). A new patch should not necessary, but I'll test it before checking in.
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+
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.