Too many visibility notifications being fired from layout

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

(Blocks: 1 bug, {access, regression})

Trunk
access, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
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?
(Assignee)

Comment 2

11 years ago
Don't know yet, I'll try to find out when I have time.
(Assignee)

Comment 3

11 years ago
Works 10-05-06
Broken 10-06-06
(Assignee)

Comment 4

11 years ago
Hmm that looks like my own checkin -- bug 354745
Right.  That would make sense.  That checkin would have exactly this effect.
(Assignee)

Comment 6

11 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
(Assignee)

Updated

11 years ago
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?
(Assignee)

Comment 8

11 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.
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.
(Assignee)

Comment 11

11 years ago
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.
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?
(Assignee)

Comment 13

11 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

11 years ago
Boris, do I need another r= for the small layout change?
(Assignee)

Updated

11 years ago
Attachment #276537 - Flags: review?
(Assignee)

Comment 15

11 years ago
Created attachment 276574 [details] [diff] [review]
Address bz's comments
Attachment #276574 - Flags: review?(ginn.chen)
No, the layout change is good to go.

Comment 17

11 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

11 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

11 years ago
Attachment #276574 - Flags: approval1.9?

Comment 19

11 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

11 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.
Check GetPrevContinuation instead?
(Assignee)

Comment 22

11 years ago
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+
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.