1293380 - Don't fire show events for child nodes after they're moved to new parent

Status: RESOLVED INVALID

(Core :: Disability Access APIs, defect)

Description

[Michael Li]

In e10s, the moved child node will be cached when the new parent gets shown anyway, so firing an extra show event for the child node will cause "ID already in use" assertions.

Comment 1

[Michael Li]

Attachment: Bug 1293380: Don't fire show events for child nodes after they're moved to new parent

Review commit: https://reviewboard.mozilla.org/r/70100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70100/

Comment 2

[Michael Li]

Comment on attachment 8779034 [details]
Bug 1293380: Don't fire show events for child nodes after they're moved to new parent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70100/diff/1-2/

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8779034 [details]
Bug 1293380: Don't fire show events for child nodes after they're moved to new parent

https://reviewboard.mozilla.org/r/70100/#review67338

::: accessible/generic/Accessible.cpp:2184
(Diff revision 2)
>      mChildren[idx]->mStateFlags |= eGroupInfoDirty;
>      mChildren[idx]->mInt.mIndexOfEmbeddedChild = -1;
>    }
>  
> -  if (eventTree) {
> +  // Only add a show event if it's a direct child of the root
> +  if (eventTree && aChild->Document() == aChild->Parent()) {

would you please explain how it works and why under-the-root changes are special. I would say that EventTree should make all coalescence.

Comment 4

[Michael Li]

Comment on attachment 8779034 [details]
Bug 1293380: Don't fire show events for child nodes after they're moved to new parent

https://reviewboard.mozilla.org/r/70100/#review67338

> would you please explain how it works and why under-the-root changes are special. I would say that EventTree should make all coalescence.

My thinking was that if the target is a grandchild, then it's easier to not have the EventTree process its show event at all than to have EventTree do coalesce the event. And if the target is moved to right under the root, the target needs to be shown under the root or else the target won't be cached by the parent process; it's like a "new" node.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 8779034 [details]
Bug 1293380: Don't fire show events for child nodes after they're moved to new parent

let me know if the patch still make sense in the light of the approach we take, canceling review for now

Comment 6

[Jim Mathies [:jimm]]

Trevor, is this something we should block on for a11y+e10s rollout?

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to Jim Mathies [:jimm] from comment #6)
> Trevor, is this something we should block on for a11y+e10s rollout?

no, the only reason it matters is fixing BindChildDoc / PDocAccessible crashes.

Comment 8

[Michael Li]

Closing since we didn't go with this approach.