1198974 - fix show / hide events for visibility changes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

Visibility is one thing, but there may be others that cause content that is above existing accessibles to become accessible and then the existing accesibles are reparented.  That's not ok because it means there's no way to have show and hide events that either remove a subtree or add a new subtree.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: make DocAccessible::ContentInserted bail early if there's nothing to do

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: call ContentRemoved on inserted nodes before processing the insertions

This means we fire hide events for the accessibles that may exist for children
of the inserted nodes before creating accessibles for the new content.  Which
is important to make sure we don't reparent those existing accessibles to the
new content.

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: collect the list of inserted nodes in DocAccessible::ContentInserted

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8653112 [details] [diff] [review]
call ContentRemoved on inserted nodes before processing the insertions

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

::: accessible/generic/DocAccessible.cpp
@@ +1314,5 @@
> +      // accessibles.  It would probably be more efficient to go through the
> +      // containers children and see which are children of the inserted
> +      // content, but that would take more work than calling ContentRemoved on
> +      // each inserted node.
> +      ContentRemoved(container, node);

I considered as an ugly thing that the layout quite often makes a pair of insertion and removal notifications for a single removal or insertion event, but here you suggest to make this mandatory in a11y (also it will dupe some layout calls).

When we chatted yesterday about this stuff I've got impression that you wanted to hack layout part to add an extra call, which is probably fine, if possible. Anyway I would love to see a solution that less heavy and more elegant. Can you think of it please?

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> Comment on attachment 8653112 [details] [diff] [review]
> call ContentRemoved on inserted nodes before processing the insertions
> 
> Review of attachment 8653112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/DocAccessible.cpp
> @@ +1314,5 @@
> > +      // accessibles.  It would probably be more efficient to go through the
> > +      // containers children and see which are children of the inserted
> > +      // content, but that would take more work than calling ContentRemoved on
> > +      // each inserted node.
> > +      ContentRemoved(container, node);
> 
> I considered as an ugly thing that the layout quite often makes a pair of
> insertion and removal notifications for a single removal or insertion event,
> but here you suggest to make this mandatory in a11y (also it will dupe some
> layout calls).

I'm not sure I follow this

> When we chatted yesterday about this stuff I've got impression that you
> wanted to hack layout part to add an extra call, which is probably fine, if

It might be possible to change layout somehow, but it sounds like its more complicated.

> possible. Anyway I would love to see a solution that less heavy and more
> elegant. Can you think of it please?

I'd really rather not spend more time on this, but if you want you are free to try.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to alexander :surkov from comment #4)
> > Comment on attachment 8653112 [details] [diff] [review]
> > call ContentRemoved on inserted nodes before processing the insertions
> > 
> > Review of attachment 8653112 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/generic/DocAccessible.cpp
> > @@ +1314,5 @@
> > > +      // accessibles.  It would probably be more efficient to go through the
> > > +      // containers children and see which are children of the inserted
> > > +      // content, but that would take more work than calling ContentRemoved on
> > > +      // each inserted node.
> > > +      ContentRemoved(container, node);
> > 
> > I considered as an ugly thing that the layout quite often makes a pair of
> > insertion and removal notifications for a single removal or insertion event,
> > but here you suggest to make this mandatory in a11y (also it will dupe some
> > layout calls).
> 
> I'm not sure I follow this

The point is it's not performant to run through the subtree of a shown element to destroy its previous subtree. Iirc sometimes we do this now, because layout triggers pair of ContentRemoved/ContentInserted calls for a shown element. That means in some cases we will try to destroy the subtree twice if your patch is taken. It's the ugly part I would like to get rethought.

> > When we chatted yesterday about this stuff I've got impression that you
> > wanted to hack layout part to add an extra call, which is probably fine, if
> 
> It might be possible to change layout somehow, but it sounds like its more
> complicated.

agree, but may be more accurate and performant.

> > possible. Anyway I would love to see a solution that less heavy and more
> > elegant. Can you think of it please?
> 
> I'd really rather not spend more time on this, but if you want you are free
> to try.

In general I'm convinced that hide event is a must in your test case (but I certainly can live with that, since it shouldn't regress anyone), so I'm not tempted to jump working on it. Can we outline at least the approaches to fix the bug, including the one without hide event?

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > (In reply to alexander :surkov from comment #4)
> > > Comment on attachment 8653112 [details] [diff] [review]
> > > call ContentRemoved on inserted nodes before processing the insertions
> > > 
> > > Review of attachment 8653112 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: accessible/generic/DocAccessible.cpp
> > > @@ +1314,5 @@
> > > > +      // accessibles.  It would probably be more efficient to go through the
> > > > +      // containers children and see which are children of the inserted
> > > > +      // content, but that would take more work than calling ContentRemoved on
> > > > +      // each inserted node.
> > > > +      ContentRemoved(container, node);
> > > 
> > > I considered as an ugly thing that the layout quite often makes a pair of
> > > insertion and removal notifications for a single removal or insertion event,
> > > but here you suggest to make this mandatory in a11y (also it will dupe some
> > > layout calls).
> > 
> > I'm not sure I follow this
> 
> The point is it's not performant to run through the subtree of a shown
> element to destroy its previous subtree. Iirc sometimes we do this now,
> because layout triggers pair of ContentRemoved/ContentInserted calls for a

sure its not super fast, but its basically necessary so

> shown element. That means in some cases we will try to destroy the subtree
> twice if your patch is taken. It's the ugly part I would like to get
> rethought.

in all the cases where things work nicely the second time you only go through the direct children not the whole subtree which shouldn't be too bad.

> > > When we chatted yesterday about this stuff I've got impression that you
> > > wanted to hack layout part to add an extra call, which is probably fine, if
> > 
> > It might be possible to change layout somehow, but it sounds like its more
> > complicated.
> 
> agree, but may be more accurate and performant.

faster sure, but I don't think it can be any more correct (but it could be less correct if it still misses some things)

> > > possible. Anyway I would love to see a solution that less heavy and more
> > > elegant. Can you think of it please?
> > 
> > I'd really rather not spend more time on this, but if you want you are free
> > to try.
> 
> In general I'm convinced that hide event is a must in your test case (but I
> certainly can live with that, since it shouldn't regress anyone), so I'm not
> tempted to jump working on it. Can we outline at least the approaches to fix
> the bug, including the one without hide event?

I don't think reasonable ones exist.  but if your looking to do a whole lot of work I suppose you could figure out how to tell the parent process about tree changes where some of the shown subtree already exists somewhere else.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #0)
> Visibility is one thing, but there may be others that cause content that is
> above existing accessibles to become accessible and then the existing
> accesibles are reparented.  That's not ok because it means there's no way to
> have show and hide events that either remove a subtree or add a new subtree.

I recall you had an example of it. Can you share it here pls? If you do simply

<div id="container">
  <div style="visibility:hidden" id="middle">
    <div style="visibility:visible" id="child">
    </div>
  </div>
</div>

then you don't get an adoption because "container" invalidates its children and "child" div gets unattached before it reattached to new accessible created for "middle" div. Something more complicated has to take a place here.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #0)
> > Visibility is one thing, but there may be others that cause content that is
> > above existing accessibles to become accessible and then the existing
> > accesibles are reparented.  That's not ok because it means there's no way to
> > have show and hide events that either remove a subtree or add a new subtree.
> 
> I recall you had an example of it. Can you share it here pls? If you do

I would guess the example I had was the test

> simply
> 
> <div id="container">
>   <div style="visibility:hidden" id="middle">
>     <div style="visibility:visible" id="child">
>     </div>
>   </div>
> </div>
> 
> then you don't get an adoption because "container" invalidates its children
> and "child" div gets unattached before it reattached to new accessible
> created for "middle" div. Something more complicated has to take a place
> here.

do you mean to say the test passes without any changes now?  I'm pretty sure it didn't used to do that.  I'm also confused why you talk about adoption when the issue is firing events.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> (In reply to alexander :surkov from comment #8)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #0)
> > > Visibility is one thing, but there may be others that cause content that is
> > > above existing accessibles to become accessible and then the existing
> > > accesibles are reparented.  That's not ok because it means there's no way to
> > > have show and hide events that either remove a subtree or add a new subtree.
> > 
> > I recall you had an example of it. Can you share it here pls? If you do
> 
> I would guess the example I had was the test

which test?

> do you mean to say the test passes without any changes now? I'm pretty sure
> it didn't used to do that.

it seems I'm not aware of any tests problems :)

> I'm also confused why you talk about adoption
> when the issue is firing events.

I guess I forgot what the bug's problem was. So there's no any problems with adoption like assertion stuff, and all we need is to fire hide/show events for an accessible that was moved in the tree, correct?

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > (In reply to alexander :surkov from comment #8)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #0)
> > > > Visibility is one thing, but there may be others that cause content that is
> > > > above existing accessibles to become accessible and then the existing
> > > > accesibles are reparented.  That's not ok because it means there's no way to
> > > > have show and hide events that either remove a subtree or add a new subtree.
> > > 
> > > I recall you had an example of it. Can you share it here pls? If you do
> > 
> > I would guess the example I had was the test
> 
> which test?

the one in the second patch?

> > I'm also confused why you talk about adoption
> > when the issue is firing events.
> 
> I guess I forgot what the bug's problem was. So there's no any problems with
> adoption like assertion stuff, and all we need is to fire hide/show events
> for an accessible that was moved in the tree, correct?

I believe so.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: fix show / hide events for visibility changes, try: -u mochitest-o

Comment 13

[Marco Zehe (:MarcoZ)]

> void
> DocAccessible::UpdateTreeOnInsertion(Accessible* aContainer)
> {
>+  nsTArray<Accessible*> survivers(aContainer->ContentChildCount());
Nit: Survivors, o instead of e.

Comment 14

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8686222 [details] [diff] [review]
fix show / hide events for visibility changes,  try: -u mochitest-o

If you want to do this, and I guess the idea of collecting the removed subtrees in UpdateTreeOnInsertion is ok a better way would be to get rid of InvalidateChildren and have something like EnsureChildren that updates the set of children in place.  Then knowing what is removed and reinserted would be trivial, and you could get rid of all the special stuff in this patch to get the order right.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> Comment on attachment 8686222 [details] [diff] [review]
> fix show / hide events for visibility changes,  try: -u mochitest-o
> 
> If you want to do this, and I guess the idea of collecting the removed
> subtrees in UpdateTreeOnInsertion is ok a better way would be to get rid of
> InvalidateChildren and have something like EnsureChildren that updates the
> set of children in place.  Then knowing what is removed and reinserted would
> be trivial, and you could get rid of all the special stuff in this patch to
> get the order right.

I guess it's ok to encapsulate the logic from the patch into InvdalidateChildren/EnsureChildren pair. Iirc we had UpdateChildren that coupled these two in the past. Also it'd be nice to sort out InvalidateChildren/EnsureChildren, but it may not trivial. Would you like to give it a try?

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #15)
> (In reply to Trevor Saunders (:tbsaunde) from comment #14)
> > Comment on attachment 8686222 [details] [diff] [review]
> > fix show / hide events for visibility changes,  try: -u mochitest-o
> > 
> > If you want to do this, and I guess the idea of collecting the removed
> > subtrees in UpdateTreeOnInsertion is ok a better way would be to get rid of
> > InvalidateChildren and have something like EnsureChildren that updates the
> > set of children in place.  Then knowing what is removed and reinserted would
> > be trivial, and you could get rid of all the special stuff in this patch to
> > get the order right.
> 
> I guess it's ok to encapsulate the logic from the patch into
> InvdalidateChildren/EnsureChildren pair. Iirc we had UpdateChildren that
> coupled these two in the past. Also it'd be nice to sort out
> InvalidateChildren/EnsureChildren, but it may not trivial. Would you like to
> give it a try?

I looked some to convince some it really is pretty simple, but I'd really rather you worked on it.

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 8653110 [details] [diff] [review]
make DocAccessible::ContentInserted bail early if there's nothing to do

cancelling for now, since I keep getting emails

Comment 18

[alexander :surkov (:asurkov)]

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

> I looked some to convince some it really is pretty simple, but I'd really
> rather you worked on it.

you have to do something about InvalidateChildren() in Shutdown(), the code can be duped into Shutdown(), which is probably ok, but anyway it is something that should be sorted out.

One thing about making UpdateChildren() to fire events is that the mutation events are commonly handled by nsDocAccessible now. I guess that's why I didn't proceed this way from the beginning. I assume it be can named as UpdateChildrenNFireEventsIfNeeded to make that explicit, but it seems a bit ugly.

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> (In reply to Trevor Saunders (:tbsaunde) from comment #16)
> 
> > I looked some to convince some it really is pretty simple, but I'd really
> > rather you worked on it.
> 
> you have to do something about InvalidateChildren() in Shutdown(), the code
> can be duped into Shutdown(), which is probably ok, but anyway it is
> something that should be sorted out.

I guess, but probably a lot of that should be unnecessary since the whole subtree is going away.

> One thing about making UpdateChildren() to fire events is that the mutation
> events are commonly handled by nsDocAccessible now. I guess that's why I
> didn't proceed this way from the beginning. I assume it be can named as
> UpdateChildrenNFireEventsIfNeeded to make that explicit, but it seems a bit
> ugly.

I have no idea what you are refering to here, but UpdateTreeInternal already deals with events so I don't see why making it deal with more is an issue.

Comment 20

[Trevor Saunders (:tbsaunde)]

Ok, so I spent a fair bit of time trying to get rid of InvalidateChildren(), and made a bit of progress which might be worth committing on its own.  However I discovered that given this edge case of UpdateTreeOnInsertion() removing kids we actually need InvalidateChildren() or something like it.  We can try to remove children as we cache the new ones, but that gets kind of complicated to sort out what needs to happen.  So I'm thinking we should just go back to the original patches unless there's a provable performance issue with them.

Comment 21

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8686222 [details] [diff] [review]
fix show / hide events for visibility changes,  try: -u mochitest-o

Ok, fighting with tree update stuff has reminded me how complicated this is already.  So I don't think we should make it even more complicated and bleed that into event dispatch code.

Comment 22

[alexander :surkov (:asurkov)]

tree update is indeed complicated but tree rebuilding just in case doesn't make things easier, and it may serve bad for performance as I said. Taking into account that Firefox accessibility is already slower than its competitors, I wouldn't say it's a wise decision to take original patches without any performance testing, proving that perf regression is not noticeable.

Comment 23

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #22)
> tree update is indeed complicated but tree rebuilding just in case doesn't
> make things easier, and it may serve bad for performance as I said. Taking

I think it is pretty clearly true that it does make things simpler, at least more than any other proposed solution.

> into account that Firefox accessibility is already slower than its

evidence? and evidence tree update is what is slow?

> competitors, I wouldn't say it's a wise decision to take original patches
> without any performance testing, proving that perf regression is not
> noticeable.

I doubt it will, it should only effect this case we are dealing with here that accessibles need to be removed.

I'm willing to see what talos says, but I don't want to spend much more time trying to come up with other solutions in part because I'm not sure they exist.  If you want to try to come up with something that decreases complexity fine, but we need to get this fixed soon.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #23)
> (In reply to alexander :surkov from comment #22)
> > tree update is indeed complicated but tree rebuilding just in case doesn't
> > make things easier, and it may serve bad for performance as I said. Taking
> 
> I think it is pretty clearly true that it does make things simpler, at least
> more than any other proposed solution.

just imagine for a sec you if you were inlined RemoveContent for each insertion, how could it make things clearer. You inject and run a bunch of complicated code just in case.

> > into account that Firefox accessibility is already slower than its
> 
> evidence? and evidence tree update is what is slow?

For each inserted node, you suggest to run through all children of its container. It's basically the known bottleneck, that's why we want to get rid of InvalidateChildren. So I would be really surprised if your approach doesn't make us slower.

> I'm willing to see what talos says, but I don't want to spend much more time
> trying to come up with other solutions in part because I'm not sure they
> exist.  If you want to try to come up with something that decreases
> complexity fine, but we need to get this fixed soon.

I'm not sure I have better ideas at this time. We may take my patch for now and figure out better solution later. I'd say it much depends on how this bug is urgent.

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #24)
> (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> > (In reply to alexander :surkov from comment #22)
> > > tree update is indeed complicated but tree rebuilding just in case doesn't
> > > make things easier, and it may serve bad for performance as I said. Taking
> > 
> > I think it is pretty clearly true that it does make things simpler, at least
> > more than any other proposed solution.
> 
> just imagine for a sec you if you were inlined RemoveContent for each
> insertion, how could it make things clearer. You inject and run a bunch of
> complicated code just in case.

remove anything that might have been there, and then build up what should be there is a lot simpler than kind of remove things and then rebuild the tree moving things around as necessary.

> > > into account that Firefox accessibility is already slower than its
> > 
> > evidence? and evidence tree update is what is slow?
> 
> For each inserted node, you suggest to run through all children of its
> container. It's basically the known bottleneck, that's why we want to get
> rid of InvalidateChildren. So I would be really surprised if your approach
> doesn't make us slower.

calling ContentRemoved() on each node is rather stupid, but it shouldn't be hard to do something better than call UpdateTreeOnRemoval a bunch of times.

> 
> > I'm willing to see what talos says, but I don't want to spend much more time
> > trying to come up with other solutions in part because I'm not sure they
> > exist.  If you want to try to come up with something that decreases
> > complexity fine, but we need to get this fixed soon.
> 
> I'm not sure I have better ideas at this time. We may take my patch for now
> and figure out better solution later. I'd say it much depends on how this
> bug is urgent.

I think its pretty important, but  I still don't think adding complexity is ok, especially when it makes really fixing things harder, and you are going to have to do something like remove the existing accessibles in the end anyway.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #25)
> remove anything that might have been there, and then build up what should be
> there is a lot simpler than kind of remove things and then rebuild the tree
> moving things around as necessary.

my patch doesn't really remove anything. It just checks if an accessible element was reparented, and if it was, then fire events. It should not be necessary to force accessible recreation for something that was moved in the tree.

If we cannot agree on approach how to workaround the problem, then we might want to try to figure out root problem. Do you have ideas why some nodes are getting reparented before we were notified about their removal and/or insertion? If we had got rid of InvalidateChildren and replaced it on insertions then would it lead to that sometimes we have incorrect tree?

Comment 27

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #26)
> (In reply to Trevor Saunders (:tbsaunde) from comment #25)
> > remove anything that might have been there, and then build up what should be
> > there is a lot simpler than kind of remove things and then rebuild the tree
> > moving things around as necessary.
> 
> my patch doesn't really remove anything. It just checks if an accessible
> element was reparented, and if it was, then fire events. It should not be

kind of remove things is what we already do in Invalidatechildren().

> necessary to force accessible recreation for something that was moved in the
> tree.

accept that it has correctness isues like invalidating the group info / name dependant parent / aria hidden caching stuff? and the asserts we have toprevent it in other cases.

> 
> If we cannot agree on approach how to workaround the problem, then we might
> want to try to figure out root problem. Do you have ideas why some nodes are
> getting reparented before we were notified about their removal and/or

well, in the test case in my third patch some nodes are becoming visible so layout doesn't really consider them to be removed enough to call nsAccessibilityService::ContentRemoved().  Besides which that would be even worse performance wise than what I did originally.

> insertion? If we had got rid of InvalidateChildren and replaced it on
> insertions then would it lead to that sometimes we have incorrect tree?

Well, it would depend how you did that, but I thought about it for a while recently when I tried to get rid of InvalidateChildren() and I couldn't come up with a way of not having dupplicate things in the set of children that I liked.

basically we have an iterator that gives us the ordered list we want, and we have a list that contains some things the iterator didn't and is missing some the iterator gave us, and somehow we need to somehow make those match up.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #27)
> (In reply to alexander :surkov from comment #26)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #25)
> > > remove anything that might have been there, and then build up what should be
> > > there is a lot simpler than kind of remove things and then rebuild the tree
> > > moving things around as necessary.
> > 
> > my patch doesn't really remove anything. It just checks if an accessible
> > element was reparented, and if it was, then fire events. It should not be
> 
> kind of remove things is what we already do in Invalidatechildren().

I referred to accessible object recreation

> > necessary to force accessible recreation for something that was moved in the
> > tree.
> 
> accept that it has correctness isues like invalidating the group info / name
> dependant parent / aria hidden caching stuff?

so an accessible moves from the container to its just created subtree. Group info should be fine for both container (because of UpdateTreeOnInsertion part) and a subtree root (because subtree was just created and because of UnbindFromParent part on accessible itself). Name dependant parent and aria-hidden should be also ok, because of BindToParent part, when an accessible is inserted under new tree.

> and the asserts we have
> toprevent it in other cases.

can you describe it more? I'm not sure I follow it.

Comment 29

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #28)
> (In reply to Trevor Saunders (:tbsaunde) from comment #27)
> > (In reply to alexander :surkov from comment #26)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #25)
> > > > remove anything that might have been there, and then build up what should be
> > > > there is a lot simpler than kind of remove things and then rebuild the tree
> > > > moving things around as necessary.
> > > 
> > > my patch doesn't really remove anything. It just checks if an accessible
> > > element was reparented, and if it was, then fire events. It should not be
> > 
> > kind of remove things is what we already do in Invalidatechildren().
> 
> I referred to accessible object recreation

I have no idea how that ties in with the quoted context.

> > > necessary to force accessible recreation for something that was moved in the
> > > tree.
> > 
> > accept that it has correctness isues like invalidating the group info / name
> > dependant parent / aria hidden caching stuff?
> 
> so an accessible moves from the container to its just created subtree. Group
> info should be fine for both container (because of UpdateTreeOnInsertion
> part) and a subtree root (because subtree was just created and because of
> UnbindFromParent part on accessible itself). Name dependant parent and
> aria-hidden should be also ok, because of BindToParent part, when an
> accessible is inserted under new tree.

and what about the name dependent parent stuff?

> > and the asserts we have
> > toprevent it in other cases.
> 
> can you describe it more? I'm not sure I follow it.

I'm not sure what you want described.  I'm just saying here that allowing  this is inconsistant with http://mxr.mozilla.org/mozilla-central/source/accessible/generic/Accessible.cpp#1957 and only works because InvalidateChildren() is wied.

Comment 30

[alexander :surkov (:asurkov)]

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

> > so an accessible moves from the container to its just created subtree. Group
> > info should be fine for both container (because of UpdateTreeOnInsertion
> > part) and a subtree root (because subtree was just created and because of
> > UnbindFromParent part on accessible itself). Name dependant parent and
> > aria-hidden should be also ok, because of BindToParent part, when an
> > accessible is inserted under new tree.
> 
> and what about the name dependent parent stuff?

same, see above

> 
> > > and the asserts we have
> > > toprevent it in other cases.
> > 
> > can you describe it more? I'm not sure I follow it.
> 
> I'm not sure what you want described.  I'm just saying here that allowing 
> this is inconsistant with
> http://mxr.mozilla.org/mozilla-central/source/accessible/generic/Accessible.
> cpp#1957 and only works because InvalidateChildren() is wied.

right, we don't hit it now because of InvalidateChildren(), but after it, I think we should legalize adoption to not waste resources.

Comment 31

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #30)
> (In reply to Trevor Saunders (:tbsaunde) from comment #29)
> 
> > > so an accessible moves from the container to its just created subtree. Group
> > > info should be fine for both container (because of UpdateTreeOnInsertion
> > > part) and a subtree root (because subtree was just created and because of
> > > UnbindFromParent part on accessible itself). Name dependant parent and
> > > aria-hidden should be also ok, because of BindToParent part, when an
> > > accessible is inserted under new tree.
> > 
> > and what about the name dependent parent stuff?
> 
> same, see above
> 
> > 
> > > > and the asserts we have
> > > > toprevent it in other cases.
> > > 
> > > can you describe it more? I'm not sure I follow it.
> > 
> > I'm not sure what you want described.  I'm just saying here that allowing 
> > this is inconsistant with
> > http://mxr.mozilla.org/mozilla-central/source/accessible/generic/Accessible.
> > cpp#1957 and only works because InvalidateChildren() is wied.
> 
> right, we don't hit it now because of InvalidateChildren(), but after it, I
> think we should legalize adoption to not waste resources.

maybe, if someone can demonstrate a case where the complexity is worth it.

anyway that's not today's world, and as I said before I don't think you can even remove  InvalidateChildren() without fixing this issue.

Comment 32

[alexander :surkov (:asurkov)]

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

> > right, we don't hit it now because of InvalidateChildren(), but after it, I
> > think we should legalize adoption to not waste resources.
> 
> maybe, if someone can demonstrate a case where the complexity is worth it.

I'm not sure what scenarios you refer to

> anyway that's not today's world, and as I said before I don't think you can
> even remove  InvalidateChildren() without fixing this issue.

that's probably true, but we don't need to introduce large changes for adoption I think, it should be a subtree flags update plus events. anyway it's worth to try and see.

Comment 33

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #32)
> (In reply to Trevor Saunders (:tbsaunde) from comment #31)
> 
> > > right, we don't hit it now because of InvalidateChildren(), but after it, I
> > > think we should legalize adoption to not waste resources.
> > 
> > maybe, if someone can demonstrate a case where the complexity is worth it.
> 
> I'm not sure what scenarios you refer to

I don't understand that as a response, your suggesting I know of some case when I'm suggesting none exist.

> > anyway that's not today's world, and as I said before I don't think you can
> > even remove  InvalidateChildren() without fixing this issue.
> 
> that's probably true, but we don't need to introduce large changes for
> adoption I think, it should be a subtree flags update plus events. anyway
> it's worth to try and see.

maybe, if you want to try it quickly I guess that's ok, but I'll be pretty suprised if the result doesn't scare me.

Comment 34

[alexander :surkov (:asurkov)]

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

> maybe, if you want to try it quickly I guess that's ok, but I'll be pretty
> suprised if the result doesn't scare me.

I don't think there are quick-t-try approaches, but basically we don't have much alternatives.

Comment 35

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

Trevor I'm still curious if Talos is interesting when you throw your patch at it?

Comment 36

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #35)
> Trevor I'm still curious if Talos is interesting when you throw your patch
> at it?

fwiw the results are https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=3111b41f4229
which suggests no statistically significant difference.

Comment 37

[alexander :surkov (:asurkov)]

has been fixed by bug 1260860, as it removed todo part from test from bug 1249400