986054 - Propagate name change events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jonathan Wei [:jwei]]

Proper name change event firing is required for the ATK name cache to be up to date.  For accessibles where the name is determined through its subtree (e.g. a XUL richlistitem), we need to fire a name change event if a child accessible's name changes.

Comment 1

[Jonathan Wei [:jwei]]

Attachment: Propagate name change events if necessary

Name change events are fired for an accessible when text changes for a child element it's dependent on.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 8394350 [details] [diff] [review]
Propagate name change events if necessary

Review of attachment 8394350 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/Accessible.cpp
@@ +1149,5 @@
> +  uint32_t type = aEvent->GetEventType();
> +
> +  if ((type == nsIAccessibleEvent::EVENT_NAME_CHANGE ||
> +       type == nsIAccessibleEvent::EVENT_TEXT_REMOVED ||
> +       type == nsIAccessibleEvent::EVENT_TEXT_INSERTED) &&

it's rather event processing, more suitable place should be EventQueue::ProcessEventQueue

btw, what about tree mutations? we should care right?

@@ +1153,5 @@
> +       type == nsIAccessibleEvent::EVENT_TEXT_INSERTED) &&
> +      mPropagateNameChange) {
> +    Accessible* parent = mParent;
> +    while (parent && nsTextEquivUtils::GetRoleRule(parent->Role()) !=
> +           eNameFromSubtreeRule)

maybe nicer to have HasNameRule(accessible, nameRule) or something similar

@@ +2650,5 @@
>    if (!nsAccUtils::IsEmbeddedObject(aChild))
>      SetChildrenFlag(eMixedChildren);
>  
> +  aChild->BindToParent(this, aIndex,
> +    nsTextEquivUtils::GetRoleRule(Role()) & eNameFromSubtreeIfReqRule);

it's really unfortunate to call Role() (it's virtual but depends on whether accessible is bound to the tree or not). Maybe that map should depend on accessible type rather on role but that'd be a change. Curious, if Trev has ideas.

::: accessible/src/generic/Accessible.h
@@ +976,5 @@
>     * Non-null indicates author-supplied role; possibly state & value as well
>     */
>    nsRoleMapEntry* mRoleMapEntry;
>  
> +  bool mPropagateNameChange;

I'd like to have flags member like mContextFlags and it should be bundled with other flags.

Comment 3

[Jonathan Wei [:jwei]]

Attachment: Propagate name change events if necessary v2

Made suggested changes.

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8394866 [details] [diff] [review]
Propagate name change events if necessary v2

Review of attachment 8394866 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/EventQueue.cpp
@@ +525,5 @@
> +          target->ShouldPropagateNameChange()) {
> +        Accessible* parent = target->Parent();
> +        while (parent &&
> +               !nsTextEquivUtils::HasNameRule(parent, eNameFromSubtreeRule))
> +          parent = parent->Parent();

we don't need to cross DOM document boundaries or do we care some iframe-based widgets?

@@ +527,5 @@
> +        while (parent &&
> +               !nsTextEquivUtils::HasNameRule(parent, eNameFromSubtreeRule))
> +          parent = parent->Parent();
> +
> +        if (parent && mDocument)

how mDocument can be null here (see if condition below)

::: accessible/src/base/nsTextEquivUtils.cpp
@@ +27,5 @@
> +bool
> +nsTextEquivUtils::HasNameRule(Accessible* aAccessible, ETextEquivRule aRule)
> +{
> +  if (!aAccessible)
> +    return false;

all callers can guarantee aAccessible is never null, you don't need to double check that (the method usage is quite restricted so we can do this)

@@ +29,5 @@
> +{
> +  if (!aAccessible)
> +    return false;
> +
> +  return GetRoleRule(aAccessible->Role()) & aRule;

would we benefit from inlining it?

::: accessible/src/generic/Accessible.cpp
@@ +2539,5 @@
>    }
>  
>    mParent = aParent;
>    mIndexInParent = aIndexInParent;
> +  SetPropagateNameChange(aPropagateNameChange);

you don't need to pass aPropagateNameChange since you can calculate it right here, right?

::: accessible/src/generic/Accessible.h
@@ +788,5 @@
> +  /**
> +   * Return true if the accessible should propagate a name change event upwards.
> +   */
> +  bool ShouldPropagateNameChange() const
> +    { return mContextFlags & ePropagateNameChange; }

the name is rather confusing because the flag is about name dependency rather than about eventing (eventing is rather consequence of the name dependency). Would 'HasNameDependentParent' be more descriptive?

@@ +793,5 @@
> +
> +  /**
> +   * Set name change propagation flag.
> +   */
> +  void SetPropagateNameChange(bool aPropagateNameChange);

no need to have this method (esp public method), it's nicer to inline its body into methods where it called

@@ +968,5 @@
>    nsTArray<nsRefPtr<Accessible> > mChildren;
>    int32_t mIndexInParent;
>  
>    static const uint8_t kChildrenFlagsBits = 2;
>    static const uint8_t kStateFlagsBits = 5;

pls fix StateFlags while you are here

Comment 5

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #4)
> we don't need to cross DOM document boundaries or do we care some
> iframe-based widgets?

You mean stopping when we reach a document?  Yeah, that should be fine - they don't calculate their name from their subtree anyway.

> how mDocument can be null here (see if condition below)

Probably makes sense to move it to the beginning of the loop, then.  I'll do that and remove the null check.

> all callers can guarantee aAccessible is never null, you don't need to
> double check that (the method usage is quite restricted so we can do this)

Will do.

> would we benefit from inlining it?

Shouldn't hurt.  I'll do that.

> you don't need to pass aPropagateNameChange since you can calculate it right
> here, right?

Sure, I'll remove the argument.

> the name is rather confusing because the flag is about name dependency
> rather than about eventing (eventing is rather consequence of the name
> dependency). Would 'HasNameDependentParent' be more descriptive?

Yup, it makes more sense.  I'll change it to that.

> no need to have this method (esp public method), it's nicer to inline its
> body into methods where it called

Okay.

> pls fix StateFlags while you are here

IIRC there's a problem with some accessibility tests when the eIgnoreDOMUIEvent flag is used, but I'll double check.

Comment 6

[Jonathan Wei [:jwei]]

Turning on eIgnoreDOMUIEvent causes the jsat/test_content_integration.html test to hang.  I guess it would make sense to deal with this in another bug.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Jonathan Wei [:jwei] from comment #5)
> (In reply to alexander :surkov from comment #4)
> > we don't need to cross DOM document boundaries or do we care some
> > iframe-based widgets?
> 
> You mean stopping when we reach a document?  Yeah, that should be fine -
> they don't calculate their name from their subtree anyway.

yep. at least I'm pretty sure we don't need that in most times. So we can save some 'ticks' :)


(In reply to Jonathan Wei [:jwei] from comment #6)
> Turning on eIgnoreDOMUIEvent causes the jsat/test_content_integration.html
> test to hang.  I guess it would make sense to deal with this in another bug.

ok, please file a bug

Comment 8

[Jonathan Wei [:jwei]]

Attachment: Propagate name change events if necessary v3

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 8396517 [details] [diff] [review]
Propagate name change events if necessary v3

Review of attachment 8396517 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/EventQueue.cpp
@@ +467,5 @@
>  #endif
>  
>    for (uint32_t idx = 0; idx < eventCount; idx++) {
> +    if (!mDocument)
> +      return;

I didn't catch why do you move it?

@@ +532,5 @@
> +          parent = parent->Parent();
> +
> +        if (parent && parent != mDocument)
> +          mDocument->FireDelayedEvent(nsIAccessibleEvent::EVENT_NAME_CHANGE,
> +                                      parent);

it makes sense to comment that we don't support nested name changes.

wouldn't it make more sense to fire name change event when one of those events are added into the queue? that would let us to keep events order.

::: accessible/src/generic/Accessible.cpp
@@ +2533,5 @@
> +
> +  if (nsTextEquivUtils::HasNameRule(mParent, eNameFromSubtreeIfReqRule))
> +    mContextFlags |= eHasNameDependentParent;
> +  else
> +    mContextFlags &= ~eHasNameDependentParent;

Even if it's not possible to have initialized mContextFlags because BindToParent is called only once but that should be ok if we change that in the future

::: accessible/src/generic/Accessible.h
@@ +870,5 @@
> +  /**
> +   * Flags used for contextual information about the accessible.
> +   */
> +  enum ContextFlags {
> +    eHasNameDependentParent = 1 << 0, // Parent's name depends on our name.

'on this accessible' maybe?

Comment 10

[Trevor Saunders (:tbsaunde)]

> @@ +532,5 @@
> > +          parent = parent->Parent();
> > +
> > +        if (parent && parent != mDocument)
> > +          mDocument->FireDelayedEvent(nsIAccessibleEvent::EVENT_NAME_CHANGE,
> > +                                      parent);
> 
> it makes sense to comment that we don't support nested name changes.
> 
> wouldn't it make more sense to fire name change event when one of those
> events are added into the queue? that would let us to keep events order.

well, typical way we do this is with just directly calling nsEventShell::FireEvent right?

> ::: accessible/src/generic/Accessible.h
> @@ +870,5 @@
> > +  /**
> > +   * Flags used for contextual information about the accessible.
> > +   */
> > +  enum ContextFlags {
> > +    eHasNameDependentParent = 1 << 0, // Parent's name depends on our name.
> 
> 'on this accessible' maybe?

why aren't we just doing mParentUsesName : 1; or the like instead of the enum business?

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8396517 [details] [diff] [review]
Propagate name change events if necessary v3

># HG changeset patch
># Parent c69c55582faa7c1985ecffe8620691e7fbcf2ae2
># User Jonathan Wei <[e-mail]>
>Bug 986054 - Propagate name change events
>
>diff --git a/accessible/src/base/EventQueue.cpp b/accessible/src/base/EventQueue.cpp
>--- a/accessible/src/base/EventQueue.cpp
>+++ b/accessible/src/base/EventQueue.cpp
>@@ -4,16 +4,17 @@
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "EventQueue.h"
> 
> #include "Accessible-inl.h"
> #include "nsEventShell.h"
> #include "DocAccessible.h"
> #include "nsAccessibilityService.h"
>+#include "nsTextEquivUtils.h"
> #ifdef A11Y_LOG
> #include "Logging.h"
> #endif
> 
> using namespace mozilla;
> using namespace mozilla::a11y;
> 
> // Defines the number of selection add/remove events in the queue when they
>@@ -461,16 +462,19 @@ EventQueue::ProcessEventQueue()
>   if (eventCount > 0 && logging::IsEnabled(logging::eEvents)) {
>     logging::MsgBegin("EVENTS", "events processing");
>     logging::Address("document", mDocument);
>     logging::MsgEnd();
>   }
> #endif
> 
>   for (uint32_t idx = 0; idx < eventCount; idx++) {
>+    if (!mDocument)
>+      return;
>+
>     AccEvent* event = events[idx];
>     if (event->mEventRule != AccEvent::eDoNotEmit) {
>       Accessible* target = event->GetAccessible();
>       if (!target || target->IsDefunct())
>         continue;
> 
>       // Dispatch the focus event if target is still focused.
>       if (event->mEventType == nsIAccessibleEvent::EVENT_FOCUS) {
>@@ -510,17 +514,30 @@ EventQueue::ProcessEventQueue()
>       nsEventShell::FireEvent(event);
> 
>       // Fire text change events.
>       AccMutationEvent* mutationEvent = downcast_accEvent(event);
>       if (mutationEvent) {
>         if (mutationEvent->mTextChangeEvent)
>           nsEventShell::FireEvent(mutationEvent->mTextChangeEvent);
>       }
>+
>+      // Propagate name change event if necessary.
>+      if ((event->mEventType == nsIAccessibleEvent::EVENT_NAME_CHANGE ||
>+           event->mEventType == nsIAccessibleEvent::EVENT_TEXT_REMOVED ||
>+           event->mEventType == nsIAccessibleEvent::EVENT_TEXT_INSERTED ||
>+           event->mEventType == nsIAccessibleEvent::EVENT_REORDER) &&

why do we care about tree changes? if they cause a text change then we'll get an event for that no? so doesn't this give us dup events (that I guess we coalesce)

>+          target->HasNameDependentParent()) {
>+        Accessible* parent = target->Parent();
>+        while (parent && parent != mDocument &&
>+               !nsTextEquivUtils::HasNameRule(parent, eNameFromSubtreeRule))
>+          parent = parent->Parent();

the name rule check seems reversed (but I guess by the tests passing it somehow isn't) ?  It seems like you want to fire an event on each accessible until you get to a parent who doesn't use name from subtree.

>+
>+        if (parent && parent != mDocument)

what is point of != mDocument check?

Comment 12

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #9)

> I didn't catch why do you move it?

I moved it since I was using mDocument->FireDelayedEvent but...

> it makes sense to comment that we don't support nested name changes.
> 
> wouldn't it make more sense to fire name change event when one of those
> events are added into the queue? that would let us to keep events order.

...I guess it won't be needed after I move the logic into where the events are being added to the queue (PushEvent).

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)

> > wouldn't it make more sense to fire name change event when one of those
> > events are added into the queue? that would let us to keep events order.
> 
> well, typical way we do this is with just directly calling
> nsEventShell::FireEvent right?

if we called FireEvent in that place then we would fire dupe events

> why aren't we just doing mParentUsesName : 1; or the like instead of the
> enum business?

I want to be more flexible sine periodically we get back to context info propagation idea

(In reply to Trevor Saunders (:tbsaunde) from comment #11)

> why do we care about tree changes? if they cause a text change then we'll
> get an event for that no?

<div role="button">btn</div>

div.textContent = "hello" - show/hide/reorder/textins/textrem events
div.firstChild.data = "opaopa" - textins/textrem events

so text inserted/removed should describe most of cases. I guess we can ignore reorder events for now until we have more data where text events don't work.

> so doesn't this give us dup events (that I guess
> we coalesce)

we'd need coalesce anyway in case of multiple changes

> >+          target->HasNameDependentParent()) {
> >+        Accessible* parent = target->Parent();
> >+        while (parent && parent != mDocument &&
> >+               !nsTextEquivUtils::HasNameRule(parent, eNameFromSubtreeRule))
> >+          parent = parent->Parent();
> 
> the name rule check seems reversed (but I guess by the tests passing it
> somehow isn't) ?  It seems like you want to fire an event on each accessible
> until you get to a parent who doesn't use name from subtree.

yes but that's a rare case and we can ignore that until we have more data

Comment 14

[Jonathan Wei [:jwei]]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> the name rule check seems reversed (but I guess by the tests passing it
> somehow isn't) ?  It seems like you want to fire an event on each accessible
> until you get to a parent who doesn't use name from subtree.

Unless I'm mistaken, the idea is that if our accessible's name changes, then we want to trigger a name change on our direct parent if it derives its name from its subtree.  In the nsTextEquivUtils::GetNameFromSubtree logic, it only recurses further on its children if the child has the eNameFromSubtreeRule or has eNameFromSubtreeIfReqRule explicitly set.  Since the accessibles with eNameFromSubtreeIfReqRule don't derive their own names from their children, we don't need to fire a name change event on them, but only the eNameFromSubtreeRule accessibles.  The firing of a name change event on the parent that needs the notification will go through the same logic until we traverse up to the document, at which point we stop.

> what is point of != mDocument check?

We want to stop when we hit the document, since we're not crossing DOM documents as mentioned in comment #4.

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> 
> > > wouldn't it make more sense to fire name change event when one of those
> > > events are added into the queue? that would let us to keep events order.
> > 
> > well, typical way we do this is with just directly calling
> > nsEventShell::FireEvent right?
> 
> if we called FireEvent in that place then we would fire dupe events

how? we already do stuff like that so..

> > why aren't we just doing mParentUsesName : 1; or the like instead of the
> > enum business?
> 
> I want to be more flexible sine periodically we get back to context info
> propagation idea

if there's more context we can always add more fields so I don't see why that matters.

> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> 
> > why do we care about tree changes? if they cause a text change then we'll
> > get an event for that no?
> 
> <div role="button">btn</div>
> 
> div.textContent = "hello" - show/hide/reorder/textins/textrem events
> div.firstChild.data = "opaopa" - textins/textrem events
> 
> so text inserted/removed should describe most of cases. I guess we can
> ignore reorder events for now until we have more data where text events
> don't work.

by reorder are you including show / hide? I'm not sure how just text change couldn't work since fireing show / hide causes text change if needed.

> > so doesn't this give us dup events (that I guess
> > we coalesce)
> 
> we'd need coalesce anyway in case of multiple changes

Well, arguably it changed multiple times, but whatever

> > >+          target->HasNameDependentParent()) {
> > >+        Accessible* parent = target->Parent();
> > >+        while (parent && parent != mDocument &&
> > >+               !nsTextEquivUtils::HasNameRule(parent, eNameFromSubtreeRule))
> > >+          parent = parent->Parent();
> > 
> > the name rule check seems reversed (but I guess by the tests passing it
> > somehow isn't) ?  It seems like you want to fire an event on each accessible
> > until you get to a parent who doesn't use name from subtree.
> 
> yes but that's a rare case and we can ignore that until we have more data

I don't care about the rare case I do want to understand why the logic seems reversed.

(In reply to Jonathan Wei [:jwei] from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > the name rule check seems reversed (but I guess by the tests passing it
> > somehow isn't) ?  It seems like you want to fire an event on each accessible
> > until you get to a parent who doesn't use name from subtree.
> 
> Unless I'm mistaken, the idea is that if our accessible's name changes, then
> we want to trigger a name change on our direct parent if it derives its name
> from its subtree.  In the nsTextEquivUtils::GetNameFromSubtree logic, it
> only recurses further on its children if the child has the
> eNameFromSubtreeRule or has eNameFromSubtreeIfReqRule explicitly set.  Since
> the accessibles with eNameFromSubtreeIfReqRule don't derive their own names
> from their children, we don't need to fire a name change event on them, but
> only the eNameFromSubtreeRule accessibles.  The firing of a name change
> event on the parent that needs the notification will go through the same
> logic until we traverse up to the document, at which point we stop.

What I don't get is that once you get to a parent whose name doesn't depend on its children I don't see how any parent accessibles can care about the name change so it seems to me if you find an accesible whose name doesn't depend on its children you can break out of the loop.

> > what is point of != mDocument check?
> 
> We want to stop when we hit the document, since we're not crossing DOM
> documents as mentioned in comment #4.

no, I mean the check before   firing the event, not the one to break out of the loop.

Comment 16

[Jonathan Wei [:jwei]]

(In reply to Trevor Saunders (:tbsaunde) from comment #15)
> What I don't get is that once you get to a parent whose name doesn't depend
> on its children I don't see how any parent accessibles can care about the
> name change so it seems to me if you find an accesible whose name doesn't
> depend on its children you can break out of the loop.

Yeah, the logic's just plain wrong.  It should be go up the tree if the parent is explicitly eNameFromSubtreeIfReqRule but not eNameFromSubtreeRule.  When we stop, only fire the additional event if the element has eNameFromSubtreeRule.  Thanks - not sure why I thought it was correct, before.

> no, I mean the check before   firing the event, not the one to break out of
> the loop.

Yup, this shouldn't be necessary anymore.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #15)

> > > well, typical way we do this is with just directly calling
> > > nsEventShell::FireEvent right?
> > 
> > if we called FireEvent in that place then we would fire dupe events
> 
> how? we already do stuff like that so..

<div>hello</div> div.setTextContent = "buy" will result in text insertion and text removal events. If we do FireEvent there two name change will be fired

Comment 18

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #17)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> 
> > > > well, typical way we do this is with just directly calling
> > > > nsEventShell::FireEvent right?
> > > 
> > > if we called FireEvent in that place then we would fire dupe events
> > 
> > how? we already do stuff like that so..
> 
> <div>hello</div> div.setTextContent = "buy" will result in text insertion
> and text removal events. If we do FireEvent there two name change will be
> fired

oh blerg

Comment 19

[Jonathan Wei [:jwei]]

Attachment: Propagate name change events if necessary v4

Fixed tree traversal logic and added nested test case.

It seems that many of the text remove/inserted events bypass the event queue using nsEventShell::FireEvent, since after removing the check for EVENT_REORDER, the check in EventQueue (either ProcessEventQueue or PushEvent) is never reached.  Moving the logic back into Accessible::HandleAccEvent allows us to handle the events.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Jonathan Wei [:jwei] from comment #16)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> > What I don't get is that once you get to a parent whose name doesn't depend
> > on its children I don't see how any parent accessibles can care about the
> > name change so it seems to me if you find an accesible whose name doesn't
> > depend on its children you can break out of the loop.
> 
> Yeah, the logic's just plain wrong.  It should be go up the tree if the
> parent is explicitly eNameFromSubtreeIfReqRule but not eNameFromSubtreeRule.
> When we stop, only fire the additional event if the element has
> eNameFromSubtreeRule.  Thanks - not sure why I thought it was correct,
> before.

I don't follow. If the event target has a parent with name dependancy, then the first accessible in parent chain having eNmaeFromSubtree rule is that parent. Why is that wrong?

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 8396672 [details] [diff] [review]
Propagate name change events if necessary v4

Review of attachment 8396672 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/Accessible.cpp
@@ +1162,5 @@
> +      parent = parent->mParent;
> +
> +    if (parent && nsTextEquivUtils::HasNameRule(parent, eNameFromSubtreeRule))
> +      mDoc->FireDelayedEvent(nsIAccessibleEvent::EVENT_NAME_CHANGE, parent);
> +  }

why don't you want to put the logic into EventQueue::PushEvent?

Comment 22

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #20)
> I don't follow. If the event target has a parent with name dependancy, then
> the first accessible in parent chain having eNmaeFromSubtree rule is that
> parent. Why is that wrong?

The previous logic just kept going up the tree until it found an accessible with eNameFromSubtreeRule.  This is incorrect, since the flag is set on eNameFromSubtreeIfReqRule, as we need to deal with the case that there's an accessible with eNameFromSubtreeRule -> eNameFromSubtreeIfReqRule x large number -> eNameFromValue.

> why don't you want to put the logic into EventQueue::PushEvent?

See comment #19.

Comment 23

[Trevor Saunders (:tbsaunde)]

> It seems that many of the text remove/inserted events bypass the event queue
> using nsEventShell::FireEvent, since after removing the check for
> EVENT_REORDER, the check in EventQueue (either ProcessEventQueue or
> PushEvent) is never reached.  Moving the logic back into
> Accessible::HandleAccEvent allows us to handle the events.

oh, yeah, see EventQueue::CreateTextChangeEventFor maybe we should explicitly put those in the queue or something...

Comment 24

[alexander :surkov (:asurkov)]

(In reply to Jonathan Wei [:jwei] from comment #22)
> (In reply to alexander :surkov from comment #20)
> > I don't follow. If the event target has a parent with name dependancy, then
> > the first accessible in parent chain having eNmaeFromSubtree rule is that
> > parent. Why is that wrong?
> 
> The previous logic just kept going up the tree until it found an accessible
> with eNameFromSubtreeRule.  This is incorrect, since the flag is set on
> eNameFromSubtreeIfReqRule, as we need to deal with the case that there's an
> accessible with eNameFromSubtreeRule -> eNameFromSubtreeIfReqRule x large
> number -> eNameFromValue.

If we propagated eNameFromSubtreeRule flag to kids and then traversed up to that parent then would it work? 

> > why don't you want to put the logic into EventQueue::PushEvent?
> 
> See comment #19.

if you handled show/hide + textinser/del then it would work in all cases. You could reduce processing by handling reorder event by special way

Comment 25

[Jonathan Wei [:jwei]]

Attachment: Propagate name change events if necessary v5

Logic was moved back to EventQueue::PushEvent.

The HasDependentParent flag is now propagated to child accessibles in two cases: 1. The parent has eNameFromSubtreeRule, or 2. The parent has the HasDependentParent flag and also has eNameFromSubtreeIfReqRule explicitly.  Consequently, the code walking up the tree has been simplified, and just needs to find the first accessible with eNameFromSubtreeRule.

Comment 26

[alexander :surkov (:asurkov)]

Comment on attachment 8397338 [details] [diff] [review]
Propagate name change events if necessary v5

Review of attachment 8397338 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/EventQueue.cpp
@@ +44,5 @@
> +       aEvent->mEventType == nsIAccessibleEvent::EVENT_TEXT_REMOVED ||
> +       aEvent->mEventType == nsIAccessibleEvent::EVENT_TEXT_INSERTED ||
> +       aEvent->mEventType == nsIAccessibleEvent::EVENT_SHOW ||
> +       aEvent->mEventType == nsIAccessibleEvent::EVENT_HIDE) &&
> +      target->HasNameDependentParent()) {

it's not really necessary condition for name change event, for example,

<button aria-label="hello"></button>
button.textContent = "oho-ho!" shouldn't result into event

1) if we have textins/remove/show/hide then
1.1) if we have an accessible having name dependant parent then we should take a parent and if its rule is subtree or subtreerec then check its name, if name is not empty and not from subtree then ignore it
2) if it's namechange event then look for parent having fromsubtree rule

sounds more or less right?

@@ +52,5 @@
> +      parent = parent->Parent();
> +
> +    if (parent)
> +      mDocument->FireDelayedEvent(nsIAccessibleEvent::EVENT_NAME_CHANGE,
> +                                  parent);

you can use PushEvent directly, right?

also you need to coalesce events

::: accessible/src/generic/Accessible.cpp
@@ +2532,5 @@
>    mIndexInParent = aIndexInParent;
> +
> +  if (nsTextEquivUtils::HasNameRule(mParent, eNameFromSubtreeRule) ||
> +      (mParent->HasNameDependentParent() &&
> +       nsTextEquivUtils::HasNameRule(mParent, eNameFromSubtreeIfReqRule)))

if we complicate logic in EventQueue then we could simplify this one. Can we oversimplify it to mParent->HasNameDependentParent() || HasNameRule(eNameFromSubtree)?

it's critical part and it should be light as possible

Comment 27

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #26)
> it's not really necessary condition for name change event, for example,
> 
> <button aria-label="hello"></button>
> button.textContent = "oho-ho!" shouldn't result into event
> 
> 1) if we have textins/remove/show/hide then
> 1.1) if we have an accessible having name dependant parent then we should
> take a parent and if its rule is subtree or subtreerec then check its name,
> if name is not empty and not from subtree then ignore it
> 2) if it's namechange event then look for parent having fromsubtree rule
> 
> sounds more or less right?

I forgot about the ARIA attributes overriding any calculated name, so I'll need to add logic to handle that.  However, I'm not sure the above rules are sufficient.  If we have the following structure:

<li aria-label='name'>
  <button id='button'></button>
</li>

Then if we call document.getElementById('button').textContent = 'New name';
we should fire a name change event for the button, but not the list item since it has a name through the aria-label (which has a higher priority in the text alternative calculation).  

I'll have to see what's the most efficient way to add this detection since there's already enough extra logic being added to PushEvent and I don't want it getting too much more bloated.

> you can use PushEvent directly, right?
> 
> also you need to coalesce events

Yes, I can create an nsRefPtr<AccEvent> and pass it to PushEvent to save a few stack frames.
The events are coalesced in PushEvent already - there's a call to CoalesceEvents() above the added logic.

> if we complicate logic in EventQueue then we could simplify this one. Can we
> oversimplify it to mParent->HasNameDependentParent() ||
> HasNameRule(eNameFromSubtree)?
> 
> it's critical part and it should be light as possible

Yup, that's fine.  It just means that there's no longer a guarantee that there's actually a parent accessible whose name depends on the current accessible.

Comment 28

[Jonathan Wei [:jwei]]

Attachment: Propagate name change events if necessary v6

After some discussion, it was decided that only the specific case that spun off this bug (bug 743568 comment #32) would be handled for now.

Accessibles of role RICH_OPTION (richlistitems) and their children will have name change events fired if they derive their name from their subtrees and their name is changed as a result of a change in their subtree.

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 8398102 [details] [diff] [review]
Propagate name change events if necessary v6

Review of attachment 8398102 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/EventQueue.cpp
@@ +52,5 @@
> +           nsTextEquivUtils::HasNameRule(parent, eNameFromSubtreeIfReqRule) &&
> +           !nsTextEquivUtils::HasNameRule(parent, eNameFromSubtreeRule))
> +      parent = parent->Parent();
> +
> +    if (parent && nsTextEquivUtils::HasNameRule(parent, eNameFromSubtreeRule)) {

it's good to comment what we look here for and in what cases the logic doesn't work

would it be good to move the logic under while to not check eNameFromSubtree twice?

@@ +59,5 @@
> +
> +      if (nameFlag == eNameFromSubtree) {
> +        nsRefPtr<AccEvent> parentEvent =
> +          new AccEvent(nsIAccessibleEvent::EVENT_NAME_CHANGE, parent);
> +        PushEvent(parentEvent);

CoalesceEvents() is missed?

::: accessible/src/generic/Accessible.cpp
@@ +2532,5 @@
>    mIndexInParent = aIndexInParent;
> +
> +  // Note: this is currently only used for richlistitems and their children.
> +  if (mParent->HasNameDependentParent() ||
> +      mParent->Role() == roles::RICH_OPTION)

it's good to use type flags

Comment 30

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #29)
> it's good to comment what we look here for and in what cases the logic
> doesn't work

I forgot to add comments here - I'll fix that.

> would it be good to move the logic under while to not check eNameFromSubtree
> twice?

Yeah.  I'll refactor that loop and the following logic.

> CoalesceEvents() is missed?

CoalesceEvents() is called right before this, and we check that the event hasn't been coalesced (mEventRule != eDoNotEmit).  Calling PushEvent on our newly created event will also go through CoalesceEvents(), so we won't have duplicate events.

> it's good to use type flags

Okay, adding an eXULListItemType.

Comment 31

[Jonathan Wei [:jwei]]

Attachment: Propagate name change events if necessary v7

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 8398572 [details] [diff] [review]
Propagate name change events if necessary v7

Review of attachment 8398572 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/EventQueue.cpp
@@ +37,5 @@
>  
>    // Filter events.
>    CoalesceEvents();
>  
> +  // Propagate name change event if necessary - event hasn't been coalesced,

not really propagate name change event, rather something like 'fire name change event on parent if its name was calculated from subtree and the subtree was changed', not 100% correct but less ambiguous than propogate

@@ +59,5 @@
> +        nsAutoString name;
> +        ENameValueFlag nameFlag = parent->Name(name);
> +        // If name is obtained from subtree, fire name change event.
> +        if (nameFlag == eNameFromSubtree) {
> +          nsRefPtr<AccEvent> parentEvent =

nit: nameChangeEvent?

Comment 33

[Jonathan Wei [:jwei]]

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c90ddbdf1e2c

Comment 34

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/c90ddbdf1e2c