472662 - no reorder event for most display property & DOM changes

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[alexander :surkov (:asurkov)]

There is no reorder event when html:link display property is changed from 'none' to 'inline'.

Example is https://bugzilla.mozilla.org/attachment.cgi?id=240526 (toggle display:none for link #1 button).

Comment 1

[alexander :surkov (:asurkov)]

the check

 +  if (childAccessible) {
+     fire reorder event

was introduced in bug 434857 (see bug 434857 comment #20).

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[Aaron Leventhal]

Surkov, why do you think we have the |if (childAccessible)| condition?

Also, what is gained by removing it? I need more of a description as to what is going on :P .... my head is not as much in the code these days.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Surkov, why do you think we have the |if (childAccessible)| condition?
> 
> Also, what is gained by removing it? I need more of a description as to what is
> going on :P .... my head is not as much in the code these days.

Aaron, no idea actually. Reorder event has been introduced in bug 392243 where you added this condition without any clarification. It was removed in bug 390692 in huge patch where I can't track the existence of similar condition. I assumed it prevents reorder events when there are DOM changes and there are not accessible tree changes but my mochitests shows all is ok.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

with additional mochitests to test more reorder events. Aaron if you can see cases I missed then I'm glad to add them.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 356159 [details] [diff] [review]
patch2

let me think about tests a bit more

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #4)
 I
> assumed it prevents reorder events when there are DOM changes and there are not
> accessible tree changes but my mochitests shows all is ok.

That's right. I have not idea how to fix this bug at the moment because when we fire delayed reorder we don't know if appended/changed DOM node is accessible. Also it's not easy to move reorder event firing when we process delayed events because of coalesce events from queue.

Comment 8

[alexander :surkov (:asurkov)]

I will get back to this once we get mochitests for this (bug 469985).

Comment 9

[Aaron Leventhal]

Can we fix the summary of this bug so it describes all the broken cases?
The current summary makes it sound a lot less important than it is.

What if a major website, say Yahoo! is changing the display property onload. Doesn't that mean that a screen reader which needs REORDER events might present the wrong page content in their virtual buffer? That is what is happening in at least one major screen reader right now, and this bug could be the reason.

I'm wondering if this should be blocking 1.9.1.

Comment 10

[Aaron Leventhal]

I see, so the problem with firing the reorder event only |if (childAccessible)| is that layout hasn't updated yet, and we don't know if there will be a childAccessible yet.

So if we fire it on a delay it is better. 

With patch 2, we may get a few extra REORDER events, however. With patch2, there could be an extra event if there was no childAccessible, and there still isn't a childAccessible after the delay. I'm not sure if that matters, but it would be good to think of a solution for that.

Perhaps a reorder event could track the number of children in the container. If the number is 0 and there are still no children, then FlushPendingEvents wouldn't fire it.

Comment 11

[Aaron Leventhal]

> Perhaps a reorder event could track the number of children in the container. 
Or, just whether there are any children at all.

Comment 12

[Aaron Leventhal]

How about this.

First, when firing delayed reorder event:
reorderEvent->SetWasThereAChildAccessible(childAccessible != nsnull);

Second:
In nsAcceEvent::ApplyEventRules(), if two reorder events are combined:
eventWeKeep->SetWasThereAchildAccessible(wasChildAccessibleInEventWeRemove);

Finally:
In FlushPendingEvents()
if (!wasThereAChildAccessible && !isThereAChildAccessible)
  continue; // Don't fire this one

Comment 13

[Aaron Leventhal]

Changes:
First, when firing delayed reorder event:
reorderEvent->SetMustFire(childAccessible != nsnull);

Second:
In nsAcceEvent::ApplyEventRules(), if two reorder events are combined:
eventWeKeep->SetMustFire(PR_TRUE);

Finally:
In FlushPendingEvents()
if (!requiredToFire && !isThereAChildAccessible)
  continue; // Don't fire this one, nothing has changed

--------------

This takes a balanced approach between code simplicity and removing most of the extra events we don't need. There will be some extra REORDER events when >1 children are changed and there are still no accessibles for those children, but it's not a correctness or perf problem to have it happen once in those cases. 

This is better than a complicated solution to determine when the multi-child change situation should result in no reorder event. By complicated, I mean that reorder events would need to cache which DOM nodes were changed and whether they had accessibles. Lists would need to be combined when REORDER events were combined, and then FlushPendingEvents() would need to look to see if there were still no accessibles created for those.

I like the simple solution best. We can change later if necessary, but I don't see why it would be.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: wip

Comment 15

[Aaron Leventhal]

This is a lot of code. Is this the simple solution?

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> This is a lot of code. Is this the simple solution?

Simpler I can't see :). We shouldn't fire excess reorder events and should fire them for show events. But I didn't tests yet. I guess it's very similar to you proposed in comment 13, excepting isThereAChildAccessible is placed on more complex check HasAccessibleInReasonSubtree.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 18

[Marco Zehe (:MarcoZ)]

Comment on attachment 359018 [details] [diff] [review]
patch

A few nits/questions:
+  // Do not emit descedant event if this event is unconditional.

"descedant" should be "descendant" (missing n). You have this in several places in both comment and variable names.

+   * Return true if changed DOM node has accessible is its tree.

"...in its tree".

+      <span id="child1"></span>

Did you mean to put role="listitem" here like you have for the other spans in this container?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)

> +      <span id="child1"></span>
> 
> Did you mean to put role="listitem" here like you have for the other spans in
> this container?

No, this is intended. I need non accessible node to check we don't fire any events on change.

Comment 20

[[e-mail]]

This patch looks OK but then again I don't really understand a11y code... was there a particular reason you requested my sr?

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> This patch looks OK but then again I don't really understand a11y code... was
> there a particular reason you requested my sr?

No special reason actually. Just when patch is getting complex then I ask for superreview usually. It makes me to feel safe.

Comment 22

[[e-mail]]

Comment on attachment 359018 [details] [diff] [review]
patch

>+  return accessible ? PR_TRUE : nsAccUtils::HasAccessibleChildren(mReasonNode);
Well, I don't really understand what's going on here so if you don't mind I'll just refuse to review but I'll just note that you could write this as
return accessible || nsAccUtils::HasAccessibleChildren(mReasonNode);

>+  PRBool isUnconditionalEvent = childAccessible ?
>+    PR_TRUE : (aChild ? nsAccUtils::HasAccessibleChildren(childNode) : PR_FALSE);
And this could be written
PRBool isUnconditionalEvent = childAccessible ||
    (aChild && nsAccUtils::HasAccessibleChildren(childNode);

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)

> Well, I don't really understand what's going on here so if you don't mind I'll
> just refuse to review

Sure, I don't mind. Thanks for comments.

Comment 24

[Marco Zehe (:MarcoZ)]

Comment on attachment 359018 [details] [diff] [review]
patch

Aside from the two spelling errors I found, looks and works fine! I agree with Neil's suggestions to change those nested : ? constructs.

With that fixed, r=me.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> (From update of attachment 359018 [details] [diff] [review])
> Aside from the two spelling errors I found, looks and works fine! I agree with
> Neil's suggestions to change those nested : ? constructs.

I fixed these locally already.

Comment 26

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 359018 [details] [diff] [review]
patch

Impressive.

>+  nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(presShell));
>+  nsAccessibleTreeWalker walker(weakShell, aNode, PR_FALSE);

I don't (yet) know why the tree walker needs a weakly referenced shell but I'll take your word for it. Are we worried about cyclic references here?

>+/* static */
>+void
>+nsAccEvent::DupeReorderEvents(nsAccEvent *aAccEvent1,

Since this method does something... can we make the name a verb? (If 'Dupe' is a verb here it is incorrect, meaning "to fool someone" or to "duplicate")

How about ProcessDupeROEvents?

>+  // Do not emit event2 if event1 is not valid, otherwise do not emit event1.

Nit:
Should this be "Do not emit event2 if event1 is valid, otherwise do not emit event1" ?

>+  if (reorderEvent1->HasAccessibleInReasonSubtree())
>+    aAccEvent2->mEventRule = nsAccEvent::eDoNotEmit;
>+  else
>+    aAccEvent1->mEventRule = nsAccEvent::eDoNotEmit;
>+}
>+
>+void
>+nsAccEvent::CoalesceReorderEvents(nsAccEvent *aAccEvent,
>+                                  nsAccEvent *aDescedantAccEvent)
>+{
>+  // Do not emit descedant event if this event is unconditional.

nit: do a find and replace descedant -> descendant


>+PRBool
>+nsAccReorderEvent::HasAccessibleInReasonSubtree()
>+{
>+  if (!mReasonNode)
>+    return PR_FALSE;
>+
>+  nsCOMPtr<nsIAccessible> accessible;
>+  nsAccessNode::GetAccService()->GetAccessibleFor(mReasonNode,
>+                                                  getter_AddRefs(accessible));
>+
>+  return accessible ? PR_TRUE : nsAccUtils::HasAccessibleChildren(mReasonNode);
>+}
>+
>+  /**
>+   * Do not emit one of two given reorder events fired for the same DOM node.
>+   */
>+  static void DupeReorderEvents(nsAccEvent *aAccEvent1,
>+                                nsAccEvent *aAccEvent2);

CoalesceReorderEventsFromSameSource

>+
>+  /**
>+   * Do not emit one of two given reorder events fired for DOM nodes in the case
>+   * when one DOM node is in parent chain of second one.
>+   */
>+  static void CoalesceReorderEvents(nsAccEvent *aAccEvent,
>+                                    nsAccEvent *aDescedantAccEvent);

CoalesceReorderEventsFromSameTree

>+private:
>+  PRBool mUnconditionalEvent;
>+  nsCOMPtr<nsIDOMNode> mReasonNode;

What is the meaning of "Reason" in mReasonNode?

>+      else if (eventType == nsIAccessibleEvent::EVENT_REORDER) {
>+        nsAccReorderEvent* reorderEvent = nsnull;
>+        CallQueryInterface(accessibleEvent, &reorderEvent);
>+        if (reorderEvent->IsUnconditionalEvent() ||
>+            reorderEvent->HasAccessibleInReasonSubtree()) {
>+          nsAccEvent::PrepareForEvent(accessibleEvent);
>+          FireAccessibleEvent(accessibleEvent);

Why do we check HasAccessibleInReasonSubtree when we've already checked for target accessible and accessible in subtree (captured in unconditional variable) before we put it on the queue?

I need to understand the need for HasAccessibleInReasonSubtree.

>+  // Fire an event so the MSAA clients know the children have changed. Also
>+  // the event is used internally by MSAA part.

maybe "internally by nsIAccessible* layer"?

+ Neil's nits.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #26)

> >+  nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(presShell));
> >+  nsAccessibleTreeWalker walker(weakShell, aNode, PR_FALSE);
> 
> I don't (yet) know why the tree walker needs a weakly referenced shell but I'll
> take your word for it.

I never looked seriously at a11y tree walker implementation. Do you concern why it needs shell or weakly referenced shell?

> Are we worried about cyclic references here?

I don't think so. A11y tree walker is created on stack.

> >+  // Do not emit event2 if event1 is not valid, otherwise do not emit event1.
> 
> Nit:
> Should this be "Do not emit event2 if event1 is valid, otherwise do not emit
> event1" ?

yes.
 
> What is the meaning of "Reason" in mReasonNode?

I didn't find good word for this. I say reason node is node that was changed (shown or hidden) and the change of this node is a reason of the fact we fire reorder event.

> 
> >+      else if (eventType == nsIAccessibleEvent::EVENT_REORDER) {
> >+        nsAccReorderEvent* reorderEvent = nsnull;
> >+        CallQueryInterface(accessibleEvent, &reorderEvent);
> >+        if (reorderEvent->IsUnconditionalEvent() ||
> >+            reorderEvent->HasAccessibleInReasonSubtree()) {
> >+          nsAccEvent::PrepareForEvent(accessibleEvent);
> >+          FireAccessibleEvent(accessibleEvent);
> 
> Why do we check HasAccessibleInReasonSubtree when we've already checked for
> target accessible and accessible in subtree (captured in unconditional
> variable) before we put it on the queue?

initially we call HasAccessibleInReasonSubtree() only if there is accessible, after timeout we call HasAccessibleInReasonSubtree only if there wasn't accessible.

> 
> I need to understand the need for HasAccessibleInReasonSubtree.

Ok, refer to examples in mochitests. So how it should be (but possibly how it isn't in the patch).

1) We need to prepare all checks after timeout for show events because frame may be not created at initial point
2) We need to prepare all checks initially for hide events because accessible isn't destroyed yet.

<div role="bla"><span id="1"></span><div>

1) Clone and append @id=1 I shouldn't get any events - we ensure after timeout @id=1 isn't accessible and doens't contain any accessible children.
2) Remove @id="1" shouldn't fire any events - we ensure initially there is no accessible and there is no accessible children.

<div role="bla"><span id="2"><span role="bla"></span></span></div>
1) clone and add @id="2", we should get reorder event because child of @id=2 is accessible (HasAccessibleInReasonSubtree - check after timeout)
2) remove id=2 - we should get reorder event because child of id=2 is accessible (again :) ) (
check HasAccessibleInReasonSubtree initially)

> >+  // Fire an event so the MSAA clients know the children have changed. Also
> >+  // the event is used internally by MSAA part.
> 
> maybe "internally by nsIAccessible* layer"?

why you don't like internally by MSAA part, I mean we should save word "MSAA" here, right?
> + Neil's nits.

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch2

Marco, Neil and David comments

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 359069 [details] [diff] [review]
patch2

Ok, David, requesting review. I'm not sure my patch code is optimal (there is no excess checks). So if you like then you can get back to review when I'll clear this point in my mind.

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 359069 [details] [diff] [review]
patch2

Thanks for the changes -- it would be nice to get Aaron's signoff.

BTW I'm OK with "reason" now, the alternatives aren't much better ("trigger" etc).

Nit:

>+      else if (eventType == nsIAccessibleEvent::EVENT_REORDER) {
>+        nsAccReorderEvent* reorderEvent = nsnull;
>+        CallQueryInterface(accessibleEvent, &reorderEvent);
>+        if (reorderEvent->IsUnconditionalEvent() ||
>+            reorderEvent->HasAccessibleInReasonSubtree()) {

Maybe put a comment here as to why we re-check for an accessible (see below)

<snip>

>+  // Create and put reorder event into queue processed after timeout. If
>+  // the target node is accessible or contains accessible children then mark
>+  // reorder event as unconditional. If after timeout reorder event is
>+  // unconditional or target node is accessible or has accessible children then
>+  // reorder event will be fired.

Maybe move the last part of the comment to where we process the reorder event off the queue? (see above).

r=me

Comment 31

[alexander :surkov (:asurkov)]

Attachment: patch3

I looked today the logic seems to be correct. I improved the comments for reorder event. What do you think?

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 359209 [details] [diff] [review]
patch3

>+      else if (eventType == nsIAccessibleEvent::EVENT_REORDER) {
>+        // Fire reorder event if it's unconditional (see InvalidateCacheSubtree
>+        // method) or if changed node (that is the reason of this reorder event)
>+        // is accessible or has accessible children.
>+        nsAccReorderEvent* reorderEvent = nsnull;

Thanks.

>+  // We need to fire reorder event for accessible parent of the changed node if
>+  // the changed node is accessible or has accessible children. In this case
>+  // we fire delayed unconditional reorder event which means it will be fired
>+  // after timeout in any case (of course if it won't be coalesced from event
>+  // queue). But at this point in the case of show events accessible object may
>+  // be not created for generally accessible changed node (because its frame may
>+  // be not constructed yet). Therefore we can to check whether the changed node
>+  // is accessible or has accessible children after timeout only. In the case we
>+  // fire conditional reorder event.

Thanks. It is good as is, or you can tweak it:

=begin=
We need to fire a delayed reorder event for the accessible parent of the changed node. We fire an unconditional reorder event if the changed node or one of its children is already accessible. In the case of show events, the accessible object might not be created yet for an otherwise accessible changed node (because its frame might not be constructed yet). In this case we fire a conditional reorder event, so that we will later check whether the changed node is accessible or has accessible children. Filtering/coalescing of these events happens during the queue flush.
=end=

r=me

Comment 33

[Aaron Leventhal]

Comment on attachment 359209 [details] [diff] [review]
patch3

Since we've talked about it a lot on IRC, and Neil, Marco and David looked at it, I'm going to pass on the review.

Comment 34

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/02577adddd3e plus
http://hg.mozilla.org/mozilla-central/rev/2b33a85857db for last David's comment.

Comment 35

[Ginn Chen]

+  nsCOMPtr<nsIAccessibleEvent> reorderEvent =
+    new nsAccReorderEvent(containerAccessible, isAsynch,
+                          isUnconditionalEvent,
+                          aChild ? childNode : nsnull);

should be 
+                          aChild ? childNode.get() : nsnull);

Comment 36

[alexander :surkov (:asurkov)]

Ginn, checked in - http://hg.mozilla.org/mozilla-central/rev/212ac2e73103

Comment 37

[Peter Van der Beken [:peterv]]

The a11y tests started leaking after the checkin for this bug. Could you fix or back out?

Comment 38

[Marco Zehe (:MarcoZ)]

Attachment: Fixes leaks

Peterv, is this what you had in mind from our talk on IRC?

Comment 39

[Marco Zehe (:MarcoZ)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/44e779f655f8

Comment 40

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090210 Minefield/3.2a1pre. Example: When changing a bug's status to "Resolved", the resolution field is now automatically being picked up by JAWS 10, where it didn't do that a few days ago.

Comment 41

[Marco Zehe (:MarcoZ)]

This proves to be a big improvement for screen readers in dynamic web apps and other in-place page updates. Requesting blocking to make sure we get this into 3.1. Surkov is preparing a port of the patch.

Comment 42

[alexander :surkov (:asurkov)]

Attachment: patch1.9.1

includes patch3 and fixes leaks (ported to 1.9.1 cleanly, without any hunk), as well imports mochitests changes from trunk needed for successful work.

Comment 43

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/40317cfae98b

Comment 44

[Steve England [:stevee]]

fixed1.9.1 keyword?

Comment 45

[jag (Peter Annema)]

This caused build bustage on the Solaris Firefox-Ports tinderboxen:

"/export/home/x86slave/xslave/mozilla-1.9.1-solaris-x86/build/accessible/src/base/nsDocAccessible.cpp", line 2077: Error: Ambiguous "?:" expression, second operand of type "nsCOMPtr<nsIDOMNode>" and third operand of type "int" can be converted to one another.
gmake[6]: *** [nsDocAccessible.o] Error 1


Fix is easy:

 +  nsCOMPtr<nsIAccessibleEvent> reorderEvent =
 +    new nsAccReorderEvent(containerAccessible, isAsynch,
 +                          isUnconditionalEvent,
 +                          aChild ? childNode : nsnull);

Change childNode to childNode.get().

Comment 46

[Ginn Chen]

It is http://hg.mozilla.org/mozilla-central/rev/212ac2e73103
It is missed in 1.9.1 branch.

Comment 47

[Ginn Chen]

Bustage fix pushed to 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e36b32238767

Comment 48

[alexander :surkov (:asurkov)]

(In reply to comment #47)
> Bustage fix pushed to 1.9.1
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e36b32238767

thanks

Comment 49

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090222 Shiretoko/3.1b3pre using my Bugzilla "Change Status" combobox steps. Thanks!