391847 - Too many visibility notifications being fired from layout

Status: RESOLVED FIXED

Description

[Aaron Leventhal]

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.

Comment 1

[Boris Zbarsky [:bz] (no decent commit message means r-)]

So when did this regress?

Comment 2

[Aaron Leventhal]

Don't know yet, I'll try to find out when I have time.

Comment 3

[Aaron Leventhal]

Works 10-05-06
Broken 10-06-06

Comment 4

[Aaron Leventhal]

Hmm that looks like my own checkin -- bug 354745

Comment 5

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Right.  That would make sense.  That checkin would have exactly this effect.

Comment 6

[Aaron Leventhal]

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."

Comment 7

[Boris Zbarsky [:bz] (no decent commit message means r-)]

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?

Comment 8

[Aaron Leventhal]

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

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Then you'll have to do your own coalescing somewhere, I think... The style system doesn't coalesce things like that.

Comment 10

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Though maybe we could change nsStyleChangeList::AppendChange to do more coalescing... It'd be pretty risky, though.  Not 1.9 material.

Comment 11

[Aaron Leventhal]

Created 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

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.

Comment 12

[Boris Zbarsky [:bz] (no decent commit message means r-)]

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.

Comment 13

[Aaron Leventhal]

(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.

Comment 14

[Aaron Leventhal]

Boris, do I need another r= for the small layout change?

Comment 15

[Aaron Leventhal]

Created attachment 276574 [details] [diff] [review]
Address bz's comments

Comment 16

[Boris Zbarsky [:bz] (no decent commit message means r-)]

No, the layout change is good to go.

Comment 17

[Ginn Chen]

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 ?

Comment 18

[Aaron Leventhal]

Actually, we only want to do it for the *first* frame, so that we only do it once per DOM node.

Comment 19

[Ginn Chen]

(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?

Comment 20

[Aaron Leventhal]

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

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Check GetPrevContinuation instead?

Comment 22

[Aaron Leventhal]

Okay, I'll change it to GetPrevContinuation(). A new patch should not necessary, but I'll test it before checking in.

Comment 23

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Comment on attachment 276574 [details] [diff] [review]
Address bz's comments

Looks good.  a=bzbarsky with the GetPrevContinuation change.