Closed Bug 1295603 Opened 8 years ago Closed 8 years ago

ignore mutation events if contained by shown or hidden container

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: michael.li11702, Assigned: michael.li11702)

References

Details

Attachments

(1 file)

Bug 1295603: Don't add subtree events to EventTree if container has show and reorder.
58 bytes, text/x-review-board-request
surkov
: review+
Details 
In EventTree, if a container is already going to be shown and reordered, we shouldn't add any show/hide events for any nodes in its subtree.
Assignee: nobody → mili
Comment on attachment 8781587 [details]
Bug 1295603: Don't add subtree events to EventTree if container has show and reorder.

https://reviewboard.mozilla.org/r/71992/#review69478

::: accessible/base/EventTree.cpp:354
(Diff revision 1)
>    prevNode->mNext.reset(new EventTree(aContainer, mDependentEvents.IsEmpty()));
>    return prevNode->mNext.get();
>  }
>  
> +bool
> +EventTree::DoesChildMatchCurrentContainer(Accessible* aChild,

These two functions could be cleaner in terms of code, including this pointer reference in the argument, but I'm not sure right now how to make it cleaner without repeating the same code in the two different places they're used in FindOrInsert.
(In reply to Michael Li from comment #0)
> In EventTree, if a container is already going to be shown and reordered, we
> shouldn't add any show/hide events for any nodes in its subtree.

I'm not sure about use case for 'reordered' part, but don't we do that already for 'shown' part? see https://dxr.mozilla.org/mozilla-central/source/accessible/base/EventTree.cpp?q=EventTree.cpp&redirect_type=direct#426

what examples do you address? could you add them into mochitests please?
Comment on attachment 8781587 [details]
Bug 1295603: Don't add subtree events to EventTree if container has show and reorder.

https://reviewboard.mozilla.org/r/71992/#review69590

::: accessible/base/EventTree.cpp:240
(Diff revision 1)
> +    // and the accessible's parent has a show event, don't insert the current
> +    // into the EventTree.
> +    Accessible* matchedParent = nullptr;
> +    if (mFireReorder &&
> +        DoesChildMatchCurrentContainer(aContainer, matchedParent) &&
> +        DoesChildHaveShowEvent(matchedParent)) {

this looks overcomplicated. 

It should be simple as
1) finding a child of mContainer that contains aContainer
2) running throuhg mDependentEvents and check if the child is matched

you do that for mFirst and mNext modifications.

mFirst case should work for
<div id='d1' hidden>
  <div id='d2' style='visibility:none'></div>
</div>
<script>
d1.hidden = false;
d2.style.visibiltiy = 'visible';
</script>

mNext case should work for:
<div id='d0'>
  <div id='d1' hidden>
    <div id='d3' style='visibility:none'></div>
  </div>
  <div>
    <div id='d2' hidden></div>
  </div>
</div>
<script>
d1.hidden=false;
d2.hidden=false;
d3.style.visibility = 'visible';
</script>

Do you want to address more cases here?
Comment on attachment 8781587 [details]
Bug 1295603: Don't add subtree events to EventTree if container has show and reorder.

https://reviewboard.mozilla.org/r/71992/#review69590

> this looks overcomplicated. 
> 
> It should be simple as
> 1) finding a child of mContainer that contains aContainer
> 2) running throuhg mDependentEvents and check if the child is matched
> 
> you do that for mFirst and mNext modifications.
> 
> mFirst case should work for
> <div id='d1' hidden>
>   <div id='d2' style='visibility:none'></div>
> </div>
> <script>
> d1.hidden = false;
> d2.style.visibiltiy = 'visible';
> </script>
> 
> mNext case should work for:
> <div id='d0'>
>   <div id='d1' hidden>
>     <div id='d3' style='visibility:none'></div>
>   </div>
>   <div>
>     <div id='d2' hidden></div>
>   </div>
> </div>
> <script>
> d1.hidden=false;
> d2.hidden=false;
> d3.style.visibility = 'visible';
> </script>
> 
> Do you want to address more cases here?

I was thinking, in our browser ariaowns test, there's one test with an accessible parent, a non-accessible child, and an accessible grandchild: https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/doc_treeupdate_ariaowns.html#20
In that case, the grandchild's show should be coalesced with the parent's show. I'm not sure how often this case might come up in a real webpage but I figure it is possible, and when it does it causes assertions for e10s.
(In reply to Michael Li from comment #5)

> I was thinking, in our browser ariaowns test, there's one test with an
> accessible parent, a non-accessible child, and an accessible grandchild:
> https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/
> doc_treeupdate_ariaowns.html#20
> In that case, the grandchild's show should be coalesced with the parent's
> show. I'm not sure how often this case might come up in a real webpage but I
> figure it is possible, and when it does it causes assertions for e10s.

do I understand right that this one is not covered by an alg from a comment above? what the event tree look like in this case?
(In reply to alexander :surkov from comment #6)
> (In reply to Michael Li from comment #5)
> 
> > I was thinking, in our browser ariaowns test, there's one test with an
> > accessible parent, a non-accessible child, and an accessible grandchild:
> > https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/
> > doc_treeupdate_ariaowns.html#20
> > In that case, the grandchild's show should be coalesced with the parent's
> > show. I'm not sure how often this case might come up in a real webpage but I
> > figure it is possible, and when it does it causes assertions for e10s.
> 
> do I understand right that this one is not covered by an alg from a comment
> above? what the event tree look like in this case?

The parent, t2_container, has a show event at the root level, and at the next level of the tree, the grandchild has a show event with its container being t2_container3. t2_container3 isn't an accessible so it doesn't get a show event, so there's no container for the parent, t2_container.
So I think with the algorithm above, when  we add the show event for the grandchild t2_owned, we insert the container t2_container3 (the child) and there's no show event for it anywhere, so it won't get coalesced.
(In reply to Michael Li from comment #7)
> > do I understand right that this one is not covered by an alg from a comment
> > above? what the event tree look like in this case?
> 
> The parent, t2_container,

t2_container1?

> has a show event at the root level, and at the
> next level of the tree, the grandchild has a show event with its container
> being t2_container3.

do I get right that t2_container1 and t2_owned gets show events? what does make them to trigger?

> t2_container3 isn't an accessible so it doesn't get a
> show event, so there's no container for the parent, t2_container.

why it doesn't have accessible, divs are accessible.
Michael, would it easier to keep this bug for use cases from comment #4? and have another one for your case from comment #7?
(In reply to alexander :surkov from comment #8)
> (In reply to Michael Li from comment #7)
> > > do I understand right that this one is not covered by an alg from a comment
> > > above? what the event tree look like in this case?
> > 
> > The parent, t2_container,
> 
> t2_container1?

Yes, my bad.

> 
> > has a show event at the root level, and at the
> > next level of the tree, the grandchild has a show event with its container
> > being t2_container3.
> 
> do I get right that t2_container1 and t2_owned gets show events? what does
> make them to trigger?
> 

Yes, we're adding the root's children and the elements in the invalidation list into the EventTree so they get show events.

> > t2_container3 isn't an accessible so it doesn't get a
> > show event, so there's no container for the parent, t2_container.
> 
> why it doesn't have accessible, divs are accessible.

I debugged this, turns out I'm only calling TreeMutation::AfterInsertion on the root's children like t2_container2 and not the their children like t2_container3 at the start, and t2_container3 also isn't part of the invalidation list so it doesn't get added there. I suppose I could fix this by adding a call to CreateSubtree on the root's children here: https://dxr.mozilla.org/mozilla-central/source/accessible/generic/DocAccessible.cpp#1469 like we do in DocAccessible::ProcessContentInserted. I wonder if doing this might get t2_container3 coalesced and removed right away though, and then later when we process the invalidation list and add the grandchild t2_owned then t2_container3 won't be in the EventTree so we wouldn't be able to use the simpler algorithm you mentioned.


(In reply to alexander :surkov from comment #9)
> Michael, would it easier to keep this bug for use cases from comment #4? and
> have another one for your case from comment #7?

Maybe if we need to use different algorithms for the two different cases, I'm not sure right now.
(In reply to Michael Li from comment #10)

> > do I get right that t2_container1 and t2_owned gets show events? what does
> > make them to trigger?
> > 
> 
> Yes, we're adding the root's children and the elements in the invalidation
> list into the EventTree so they get show events.

t2_container1 is a div, and it's not reallocated by aria-owns, so it shouldn't be in the invalidation list. I'd say that no need to put t2_owned into the list too if we do this (sounds like a separate bug).

> > > t2_container3 isn't an accessible so it doesn't get a
> > > show event, so there's no container for the parent, t2_container.
> > 
> > why it doesn't have accessible, divs are accessible.
> 
> I debugged this, turns out I'm only calling TreeMutation::AfterInsertion on
> the root's children like t2_container2 and not the their children like
> t2_container3 at the start, and t2_container3 also isn't part of the
> invalidation list so it doesn't get added there.

not sure I see a reason why you need AfterInsertion on sub children. Note, we wanted to replace those on a document show event. I think that even could fix your use case.

> I suppose I could fix this
> by adding a call to CreateSubtree on the root's children here:
> https://dxr.mozilla.org/mozilla-central/source/accessible/generic/
> DocAccessible.cpp#1469 like we do in DocAccessible::ProcessContentInserted.

the tree should be created by CacheChildrenInSubtree at this point

> I wonder if doing this might get t2_container3 coalesced and removed right
> away though, and then later when we process the invalidation list and add
> the grandchild t2_owned then t2_container3 won't be in the EventTree so we
> wouldn't be able to use the simpler algorithm you mentioned.

can you make a snapshot of EventTree to demo a problem? I'm not still following.

> (In reply to alexander :surkov from comment #9)
> > Michael, would it easier to keep this bug for use cases from comment #4? and
> > have another one for your case from comment #7?
> 
> Maybe if we need to use different algorithms for the two different cases,
> I'm not sure right now.

I'd say it's one yet more bug if the alg above doesn't help here.
(In reply to alexander :surkov from comment #11)
> (In reply to Michael Li from comment #10)
> 
> > > do I get right that t2_container1 and t2_owned gets show events? what does
> > > make them to trigger?
> > > 
> > 
> > Yes, we're adding the root's children and the elements in the invalidation
> > list into the EventTree so they get show events.
> 
> t2_container1 is a div, and it's not reallocated by aria-owns, so it
> shouldn't be in the invalidation list. I'd say that no need to put t2_owned
> into the list too if we do this (sounds like a separate bug).
> 

To clarify, t2_container1 isn't in the invalidation list, t2_owned is.

> > > > t2_container3 isn't an accessible so it doesn't get a
> > > > show event, so there's no container for the parent, t2_container.
> > > 
> > > why it doesn't have accessible, divs are accessible.
> > 
> > I debugged this, turns out I'm only calling TreeMutation::AfterInsertion on
> > the root's children like t2_container2 and not the their children like
> > t2_container3 at the start, and t2_container3 also isn't part of the
> > invalidation list so it doesn't get added there.
> 
> not sure I see a reason why you need AfterInsertion on sub children. Note,
> we wanted to replace those on a document show event. I think that even could
> fix your use case.
> 

I looked at our e10s code for handling events and it assumes the node being shown is a child of the document, I don't know how much work it would be to change that. I'm not sure how much other code would have to change too, such as taking out the EventTree insertions for the invalidation list etc. Creating a TreeMutation instance with the document's parent also didn't work. So, in general, it's a nice idea but I don't know how complicated it would be to implement it.

> > I suppose I could fix this
> > by adding a call to CreateSubtree on the root's children here:
> > https://dxr.mozilla.org/mozilla-central/source/accessible/generic/
> > DocAccessible.cpp#1469 like we do in DocAccessible::ProcessContentInserted.
> 
> the tree should be created by CacheChildrenInSubtree at this point
> 

Right, it is.

> > I wonder if doing this might get t2_container3 coalesced and removed right
> > away though, and then later when we process the invalidation list and add
> > the grandchild t2_owned then t2_container3 won't be in the EventTree so we
> > wouldn't be able to use the simpler algorithm you mentioned.
> 
> can you make a snapshot of EventTree to demo a problem? I'm not still
> following.
> 

I was thinking after the t2_container2 has its show added and we later add t2_container3 and coalesce it, the tree will look like:
{
  container: document
  shown: t2_container2
  shown: [other children of document]
}
i.e., there's no container added for t2_container3. Then later when we add t2_owned, the tree will look like:
{
  container: document
  shown: t2_container2
  shown: [other children of document]
    container: t2_container3
    shown: t2_owned
}
t2_container3 wasn't in the EventTree when t2_owned gets added so the simpler algorithm won't be able to coalesce t2_owned's show event in this case. My current patch is able to coalesce this for t2_owned since I iterate up until I find the parent under the document (t2_container2) and check if there's a show event for that parent.

> > (In reply to alexander :surkov from comment #9)
> > > Michael, would it easier to keep this bug for use cases from comment #4? and
> > > have another one for your case from comment #7?
> > 
> > Maybe if we need to use different algorithms for the two different cases,
> > I'm not sure right now.
> 
> I'd say it's one yet more bug if the alg above doesn't help here.

OK, I will consider doing a separate patch with your algorithm and the test cases from comment 4 and discussing about these other cases a bit more.
(In reply to Michael Li from comment #12)

> > t2_container1 is a div, and it's not reallocated by aria-owns, so it
> > shouldn't be in the invalidation list. I'd say that no need to put t2_owned
> > into the list too if we do this (sounds like a separate bug).
> > 
> 
> To clarify, t2_container1 isn't in the invalidation list, t2_owned is.

ok, t2_owned doesn't have to be there, it's worth to have a separate bug on this

> I looked at our e10s code for handling events and it assumes the node being
> shown is a child of the document, I don't know how much work it would be to
> change that. I'm not sure how much other code would have to change too, such
> as taking out the EventTree insertions for the invalidation list etc.
> Creating a TreeMutation instance with the document's parent also didn't
> work. So, in general, it's a nice idea but I don't know how complicated it
> would be to implement it.

it should have high priority I'd say, because as soon as we fix this bug, we create a new perf bottleneck, and thus as a next step we have to shrink EventTree when it gets big. It can be reduced up to a single node which is a whole document. Note, EventTree is yet ready for this too. We can take alternative approaches to achieve same results, but we'd need a separate bug for this.
(In reply to Michael Li from comment #12)

> > can you make a snapshot of EventTree to demo a problem? I'm not still
> > following.
> > 
> 
> I was thinking after the t2_container2 has its show added and we later add
> t2_container3 and coalesce it, the tree will look like:
> {
>   container: document
>   shown: t2_container2
>   shown: [other children of document]
> }
> i.e., there's no container added for t2_container3. Then later when we add
> t2_owned, the tree will look like:
> {
>   container: document
>   shown: t2_container2
>   shown: [other children of document]
>     container: t2_container3
>     shown: t2_owned
> }
> t2_container3 wasn't in the EventTree when t2_owned gets added so the
> simpler algorithm won't be able to coalesce t2_owned's show event in this
> case.

not sure I follow why, t2_container3 reorder is contained by shown t2_container2, my alg should handle this case by rejecting t2_container3 reorder node
Comment on attachment 8781587 [details]
Bug 1295603: Don't add subtree events to EventTree if container has show and reorder.

canceling review for now
Attachment #8781587 - Flags: review?(surkov.alexander)
(In reply to alexander :surkov from comment #13)
> (In reply to Michael Li from comment #12)
> 
> > > t2_container1 is a div, and it's not reallocated by aria-owns, so it
> > > shouldn't be in the invalidation list. I'd say that no need to put t2_owned
> > > into the list too if we do this (sounds like a separate bug).
> > > 
> > 
> > To clarify, t2_container1 isn't in the invalidation list, t2_owned is.
> 
> ok, t2_owned doesn't have to be there, it's worth to have a separate bug on
> this
> 

OK so just to clarify when reporting the new bug, a grandchild node doesn't need to be in the invalidation list if its parent is already an accessible and the parent isn't the root.

> > I looked at our e10s code for handling events and it assumes the node being
> > shown is a child of the document, I don't know how much work it would be to
> > change that. I'm not sure how much other code would have to change too, such
> > as taking out the EventTree insertions for the invalidation list etc.
> > Creating a TreeMutation instance with the document's parent also didn't
> > work. So, in general, it's a nice idea but I don't know how complicated it
> > would be to implement it.
> 
> it should have high priority I'd say, because as soon as we fix this bug, we
> create a new perf bottleneck, and thus as a next step we have to shrink
> EventTree when it gets big. It can be reduced up to a single node which is a
> whole document. Note, EventTree is yet ready for this too. We can take
> alternative approaches to achieve same results, but we'd need a separate bug
> for this.

OK let's discuss this more after we get this coalescence working and after we find a solution to the events ordering issue we mentioned here: https://bugzilla.mozilla.org/show_bug.cgi?id=1294853#c10

(In reply to alexander :surkov from comment #14)
> (In reply to Michael Li from comment #12)
> 
> > > can you make a snapshot of EventTree to demo a problem? I'm not still
> > > following.
> > > 
> > 
> > I was thinking after the t2_container2 has its show added and we later add
> > t2_container3 and coalesce it, the tree will look like:
> > {
> >   container: document
> >   shown: t2_container2
> >   shown: [other children of document]
> > }
> > i.e., there's no container added for t2_container3. Then later when we add
> > t2_owned, the tree will look like:
> > {
> >   container: document
> >   shown: t2_container2
> >   shown: [other children of document]
> >     container: t2_container3
> >     shown: t2_owned
> > }
> > t2_container3 wasn't in the EventTree when t2_owned gets added so the
> > simpler algorithm won't be able to coalesce t2_owned's show event in this
> > case.
> 
> not sure I follow why, t2_container3 reorder is contained by shown
> t2_container2, my alg should handle this case by rejecting t2_container3
> reorder node

OK, so to clarify what the algorithm would do in this case, when we add show event for t2_owned, we add the container for t2_container3, and when we do that, we look at the document (since it contains t2_container3) and loop through its dependent events to see the show event for t2_container2, and since t2_conainer2 is the direct parent of t2_container3 then we reject t2_container3?
(In reply to Michael Li from comment #16)

> > > To clarify, t2_container1 isn't in the invalidation list, t2_owned is.
> > 
> > ok, t2_owned doesn't have to be there, it's worth to have a separate bug on
> > this
> > 
> 
> OK so just to clarify when reporting the new bug, a grandchild node doesn't
> need to be in the invalidation list if its parent is already an accessible
> and the parent isn't the root.

aria owned element that we delayed accessible creation for, shouldn't be in the invalidation list

> > > I looked at our e10s code for handling events and it assumes the node being
> > > shown is a child of the document, I don't know how much work it would be to
> > > change that. I'm not sure how much other code would have to change too, such
> > > as taking out the EventTree insertions for the invalidation list etc.
> > > Creating a TreeMutation instance with the document's parent also didn't
> > > work. So, in general, it's a nice idea but I don't know how complicated it
> > > would be to implement it.
> > 
> > it should have high priority I'd say, because as soon as we fix this bug, we
> > create a new perf bottleneck, and thus as a next step we have to shrink
> > EventTree when it gets big. It can be reduced up to a single node which is a
> > whole document. Note, EventTree is yet ready for this too. We can take
> > alternative approaches to achieve same results, but we'd need a separate bug
> > for this.
> 
> OK let's discuss this more after we get this coalescence working

ok, but also we should make sure to not release with a perf regression

> and after
> we find a solution to the events ordering issue we mentioned here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1294853#c10

I'm not sure I see connection, but ok.

> (In reply to alexander :surkov from comment #14)

> > > {
> > >   container: document
> > >   shown: t2_container2
> > >   shown: [other children of document]
> > > }

> OK, so to clarify what the algorithm would do in this case, when we add show
> event for t2_owned, we add the container for t2_container3, 

not add, but try to add

> and when we do
> that, we look at the document (since it contains t2_container3) and loop
> through its dependent events to see the show event for t2_container2, and
> since t2_conainer2 is the direct parent of t2_container3 then we reject
> t2_container3?

correct
(In reply to alexander :surkov from comment #17)
> (In reply to Michael Li from comment #16)
> 
> > > > To clarify, t2_container1 isn't in the invalidation list, t2_owned is.
> > > 
> > > ok, t2_owned doesn't have to be there, it's worth to have a separate bug on
> > > this
> > > 
> > 
> > OK so just to clarify when reporting the new bug, a grandchild node doesn't
> > need to be in the invalidation list if its parent is already an accessible
> > and the parent isn't the root.
> 
> aria owned element that we delayed accessible creation for, shouldn't be in
> the invalidation list
> 

Is this also true for stuff like aria-labelledby? Since this behaviour of delayed accessible creation was also happening in browser_caching_name.js where we set aria-labelledby on an element. 

> > > > I looked at our e10s code for handling events and it assumes the node being
> > > > shown is a child of the document, I don't know how much work it would be to
> > > > change that. I'm not sure how much other code would have to change too, such
> > > > as taking out the EventTree insertions for the invalidation list etc.
> > > > Creating a TreeMutation instance with the document's parent also didn't
> > > > work. So, in general, it's a nice idea but I don't know how complicated it
> > > > would be to implement it.
> > > 
> > > it should have high priority I'd say, because as soon as we fix this bug, we
> > > create a new perf bottleneck, and thus as a next step we have to shrink
> > > EventTree when it gets big. It can be reduced up to a single node which is a
> > > whole document. Note, EventTree is yet ready for this too. We can take
> > > alternative approaches to achieve same results, but we'd need a separate bug
> > > for this.
> > 
> > OK let's discuss this more after we get this coalescence working
> 
> ok, but also we should make sure to not release with a perf regression
> 
> > and after
> > we find a solution to the events ordering issue we mentioned here:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1294853#c10
> 
> I'm not sure I see connection, but ok.
> 

Let's get these events-related issues working correctly first.

> > (In reply to alexander :surkov from comment #14)
> 
> > > > {
> > > >   container: document
> > > >   shown: t2_container2
> > > >   shown: [other children of document]
> > > > }
> 
> > OK, so to clarify what the algorithm would do in this case, when we add show
> > event for t2_owned, we add the container for t2_container3, 
> 
> not add, but try to add
> 
> > and when we do
> > that, we look at the document (since it contains t2_container3) and loop
> > through its dependent events to see the show event for t2_container2, and
> > since t2_conainer2 is the direct parent of t2_container3 then we reject
> > t2_container3?
> 
> correct

OK and when looking at dependent events and comparing them to the new container, I should only check direct parents, and I shouldn't need to check grandparents etc.? I'll start the patch for this algorithm now.
(In reply to Michael Li from comment #18)

> > aria owned element that we delayed accessible creation for, shouldn't be in
> > the invalidation list
> > 
> 
> Is this also true for stuff like aria-labelledby? Since this behaviour of
> delayed accessible creation was also happening in browser_caching_name.js
> where we set aria-labelledby on an element. 

it concerns to aria-owns only, but I don't have good ideas how invalidation list is involved here, because t2_owned goes after its owner. invalidation list should contain inaccessible nodes that are referred by IDRefs.

> > > (In reply to alexander :surkov from comment #14)

> OK and when looking at dependent events and comparing them to the new
> container, I should only check direct parents, and I shouldn't need to check
> grandparents etc.?

you should get a parent accessible which is a child of the container (reorder node), see comment #4, that may be a direct parent or grandparents. please make sure to have tests for both cases.
(In reply to alexander :surkov from comment #19)
> (In reply to Michael Li from comment #18)
> 
> > > aria owned element that we delayed accessible creation for, shouldn't be in
> > > the invalidation list
> > > 
> > 
> > Is this also true for stuff like aria-labelledby? Since this behaviour of
> > delayed accessible creation was also happening in browser_caching_name.js
> > where we set aria-labelledby on an element. 
> 
> it concerns to aria-owns only, but I don't have good ideas how invalidation
> list is involved here, because t2_owned goes after its owner. invalidation
> list should contain inaccessible nodes that are referred by IDRefs.
> 

OK I'll file the bug.

> > > > (In reply to alexander :surkov from comment #14)
> 
> > OK and when looking at dependent events and comparing them to the new
> > container, I should only check direct parents, and I shouldn't need to check
> > grandparents etc.?
> 
> you should get a parent accessible which is a child of the container
> (reorder node), see comment #4, that may be a direct parent or grandparents.
> please make sure to have tests for both cases.

Is there a faster way to get the parent accessible that's a child of the container other than to keep calling Parent() on the container we're trying to add? That's what I'm doing in the provided patch, and it seems like that code is following your algorithm (find a parent accessible that's a child of the container, see if the container has a show event for that child), I probably just need to do it in a different place, maybe just right at the start of FindOrInsert.
Right, although we won't reach that logic if mFirst is nullptr and we return early here [1], 
and we should be doing that check for all mFirst and mNext modifications, right? So in my patch I added that same check in that place at [1] but I didn't want to have the logic in 2 different places so I moved them to the 2 functions I wrote, though we could do this differently if there's a better way.

[1] https://dxr.mozilla.org/mozilla-central/source/accessible/base/EventTree.cpp#235
(In reply to Michael Li from comment #22)
> Right, although we won't reach that logic if mFirst is nullptr and we return
> early here [1], 

if you reached this part, then you should be sure that no show/hide events contain mFirst container

> and we should be doing that check for all mFirst and mNext modifications,
> right? So in my patch I added that same check in that place at [1]

same applicable to mNext
Comment on attachment 8781587 [details]
Bug 1295603: Don't add subtree events to EventTree if container has show and reorder.

https://reviewboard.mozilla.org/r/71992/#review70870

this looks good, but needs mochitests

::: accessible/base/EventTree.cpp:263
(Diff revision 2)
>  
>        // We got a match.
>        if (parent->Parent() == node->mContainer) {
> +        // If the top container has a reorder and the matched parent already has
> +        // a show/hide event, don't add the current container as it's unneeded.
> +        if (node->mFireReorder) {

maybe: reject a node if contained by show/hide event target

::: accessible/base/EventTree.cpp:264
(Diff revision 2)
>        // We got a match.
>        if (parent->Parent() == node->mContainer) {
> +        // If the top container has a reorder and the matched parent already has
> +        // a show/hide event, don't add the current container as it's unneeded.
> +        if (node->mFireReorder) {
> +          uint32_t evCount = node->mDependentEvents.Length();

this condition looks incorrect

::: accessible/base/EventTree.cpp:269
(Diff revision 2)
> +          uint32_t evCount = node->mDependentEvents.Length();
> +          for (uint32_t idx = 0; idx < evCount; idx++) {
> +            AccMutationEvent* ev = node->mDependentEvents[idx];
> +            if (ev->GetAccessible() == parent) {
> +#ifdef A11Y_LOG
> +              logging::MsgBegin("EVENTS_TREE",

it should be wrapped by logging::IsEnabled(logging::eEventTree), maybe it makes sense to keep it under logging::eVerbose tpp

::: accessible/base/EventTree.cpp:270
(Diff revision 2)
> +          for (uint32_t idx = 0; idx < evCount; idx++) {
> +            AccMutationEvent* ev = node->mDependentEvents[idx];
> +            if (ev->GetAccessible() == parent) {
> +#ifdef A11Y_LOG
> +              logging::MsgBegin("EVENTS_TREE",
> +                                "Removing child since parent has event");

maybe "rejecting a node 'cause contained by show/hide event target'

::: accessible/base/EventTree.cpp:271
(Diff revision 2)
> +            AccMutationEvent* ev = node->mDependentEvents[idx];
> +            if (ev->GetAccessible() == parent) {
> +#ifdef A11Y_LOG
> +              logging::MsgBegin("EVENTS_TREE",
> +                                "Removing child since parent has event");
> +              logging::AccessibleInfo("Removing container", aContainer);

Removing container -> container? since the message above have details
Attachment #8781587 - Flags: review?(surkov.alexander)
this should make a trick:

<script>
document.body.innerHTML = `
<div id='c1'>
  <div id='owned'></div>
</div>
<div id='c2' aria-owns='owned'>
</div>
`;
</script>

this should result in EventTree

reorder for document
  show for 'c1'
    reorder for 'c1'
      hide for 'owned'
  show for 'c2'
    reorder for 'c2'
      show for 'owned'

after the patch it should be like

reorder for document
  show for 'c1'
  show for 'c2'
Comment on attachment 8781587 [details]
Bug 1295603: Don't add subtree events to EventTree if container has show and reorder.

https://reviewboard.mozilla.org/r/71992/#review71540

::: accessible/base/EventTree.cpp:268
(Diff revision 3)
> +        for (uint32_t idx = 0; idx < evCount; idx++) {
> +          AccMutationEvent* ev = node->mDependentEvents[idx];
> +          if (ev->GetAccessible() == parent) {
> +#ifdef A11Y_LOG
> +            if (logging::IsEnabled(logging::eEventTree) &&
> +                logging::IsEnabled(logging::eVerbose)) {

logging::IsEnabledAll

::: accessible/base/EventTree.cpp:271
(Diff revision 3)
> +#ifdef A11Y_LOG
> +            if (logging::IsEnabled(logging::eEventTree) &&
> +                logging::IsEnabled(logging::eVerbose)) {
> +              logging::MsgBegin("EVENTS_TREE",
> +                  "Rejecting node since contained by show/hide target");
> +              logging::AccessibleInfo("Container", aContainer);

nit: two spaces indent or align it with "EVENTS_TREE"

::: accessible/tests/mochitest/events/test_coalescence.html:452
(Diff revision 3)
>      }
>  
> +    /**
> +     * Set a grandchild node to be the aria-owns target of child node
> +     */
> +    function test6()

btw, did you make sure that test fails without your patch?

::: accessible/tests/mochitest/events/test_coalescence.html:461
(Diff revision 3)
> +      this.fc.setAttribute("id", "t6_fc");
> +      this.owns = document.createElement("div");
> +      this.owns.setAttribute("id", "t6_owns");
> +      this.sc = document.createElement("div");
> +      this.sc.setAttribute("id", "t6_sc");
> +

I would prefer innerHTML approach as it's easier to understand the markup. Otherwise, if you want to stick with your syntax, then it'd be good to put markup into a comment.

::: accessible/tests/mochitest/events/test_coalescence.html:469
(Diff revision 3)
> +        new invokerChecker(EVENT_SHOW, this.sc),
> +        new invokerChecker(EVENT_REORDER, this.parent),
> +        new unexpectedInvokerChecker(EVENT_REORDER, this.fc),
> +        new unexpectedInvokerChecker(EVENT_REORDER, this.sc),
> +        new unexpectedInvokerChecker(EVENT_HIDE, this.owns),
> +        new unexpectedInvokerChecker(EVENT_SHOW, this.owns)

for show/hide it would be safer to provide a matcher function, that matches any accessilbe that has a DOM node that has a specified ID. Otherwise you have a chance to miss the match, for example, you cannot receive an accessilbe for a DOM node that was is target of hide event, because it's not in the tree at that point

::: accessible/tests/mochitest/events/test_coalescence.html:481
(Diff revision 3)
> +        this.parent.appendChild(this.sc);
> +        this.sc.setAttribute("aria-owns", "t6_owns");
> +      };
> +
> +      this.getID = function test6_getID() {
> +        return "set grandchild node as aria-owns target";

maybe something like (not sure how to word it better, but tried to capture the problem)? "insert accessibles, having aria-owned moving child"
Attachment #8781587 - Flags: review?(surkov.alexander)
Comment on attachment 8781587 [details]
Bug 1295603: Don't add subtree events to EventTree if container has show and reorder.

https://reviewboard.mozilla.org/r/71992/#review71544

it looks ok r=me with issues addressed, but we should make sure we have a fix for bug 1290776 (or be reasonable close to have a fix) before we land it, as it may deliver perf problems as is
Attachment #8781587 - Flags: review+
Comment on attachment 8781587 [details]
Bug 1295603: Don't add subtree events to EventTree if container has show and reorder.

https://reviewboard.mozilla.org/r/71992/#review71540

> btw, did you make sure that test fails without your patch?

Yeah, on central the test fails with all the unexpected reorder and show/hide events.

> for show/hide it would be safer to provide a matcher function, that matches any accessilbe that has a DOM node that has a specified ID. Otherwise you have a chance to miss the match, for example, you cannot receive an accessilbe for a DOM node that was is target of hide event, because it's not in the tree at that point

By matcher function do you mean GetAccessible()? I tried using that here in eventSeq but I got errors saying it couldn't get the DOM elements for the specified IDs, probably since the new elements haven't actually been inserted yet since invoke hasn't run.
Comment on attachment 8781587 [details]
Bug 1295603: Don't add subtree events to EventTree if container has show and reorder.

https://reviewboard.mozilla.org/r/71992/#review71540

> By matcher function do you mean GetAccessible()? I tried using that here in eventSeq but I got errors saying it couldn't get the DOM elements for the specified IDs, probably since the new elements haven't actually been inserted yet since invoke hasn't run.

Actually did you mean using getNode in invoke? That's what is done in test3, test4, and test5.
(In reply to Michael Li from comment #30)

> By matcher function do you mean GetAccessible()? I tried using that here in
> eventSeq but I got errors saying it couldn't get the DOM elements for the
> specified IDs, probably since the new elements haven't actually been
> inserted yet since invoke hasn't run.

like this https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/relations/test_ui_modalprompt.html#38

(In reply to Michael Li from comment #31)

> > By matcher function do you mean GetAccessible()? I tried using that here in eventSeq but I got errors saying it couldn't get the DOM elements for the specified IDs, probably since the new elements haven't actually been inserted yet since invoke hasn't run.
> 
> Actually did you mean using getNode in invoke? That's what is done in test3,
> test4, and test5.

per eventQueue.compareEvents getNode should work too, anyway if you ensured that your tests fail without the patch, and passes with it, then it has to be fine.
Blocks: 1296420
Blocks: 1299957
I'm going to push this since other bugs depend on this one. Try server results are here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bcb64701e45
Keywords: checkin-needed
(In reply to Michael Li from comment #34)
> I'm going to push this since other bugs depend on this one.

we should fix eventTree coalescence before landing this (see comment #29), because the patch as is may have serious impact on performance.
removing 'checkin-needed', Michael, let's sort out comment #29 before landing this one.
Keywords: checkin-needed
OK, do you have any ideas on what we should do to fix coalescence to solve bug 1290776?
(In reply to Michael Li from comment #37)
> OK, do you have any ideas on what we should do to fix coalescence to solve
> bug 1290776?

I bet we discussed that somewhere already. The idea is to fire hide/show events on a reorder EventTree node if it gets too many shows/hides or child reorders. It'd be good though to keep the discussion in that bug.
Summary: Don't add subtree events to EventTree if container has show and reorder → dismiss events from shown/hidden subtree
Summary: dismiss events from shown/hidden subtree → ignore mutation events if contained by shown or hidden container
(In reply to alexander :surkov from comment #40)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 569def7d611ec9c0b8b9c538f6a48b7f87c02502
> Bug 1295603 - ignore mutation events if contained by shown or hidden
> container, r=surkov

landed on Michael behalf, we have a week still to take an antidote bug 1301829 before merging day
https://hg.mozilla.org/mozilla-central/rev/569def7d611e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: