fire hide events before show events on move

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: michael.li11702, Assigned: surkov)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Right now, the new parent gets its show event before the new child is moved, when the child gets its hide under its old parent and its show under the new parent. This causes assertions in browser_caching_name.js since showing the new parent before hiding the old child attempts to re-cache the old child, and this can be fixed by moving the child before showing the new parent. The old child gets hidden, and then its show event under the new parent gets added, and then the new parent's show event gets added, and EventTree::Mutated coalesces it with the new child's show event.
(In reply to Michael Li from comment #0)
> Right now, the new parent gets its show event before the new child is moved,
> when the child gets its hide under its old parent and its show under the new
> parent. 

could you give me a code snippet that demos this problem?
Comment on attachment 8780699 [details]
Bug 1294853: Enable browser_caching_name.js since it passes tests.

https://reviewboard.mozilla.org/r/71344/#review68892

the change doesn't need a separete review, please feel free to land when it works. also it could be a part of original patch
Attachment #8780699 - Flags: review?(surkov.alexander) → review+
The first test of browser_caching_name.js shows this: https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/browser_caching_name.js#62-68
The two spans' text nodes originally are under the root, and after the invalidation list processes the spans, the text nodes are moved to the spans and this problem happens: the span's show comes first, and then the text node's hide and show events happen.
(In reply to Michael Li from comment #5)
> The first test of browser_caching_name.js shows this:
> https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/
> browser_caching_name.js#62-68
> The two spans' text nodes originally are under the root, and after the
> invalidation list processes the spans, the text nodes are moved to the spans
> and this problem happens: the span's show comes first, and then the text
> node's hide and show events happen.

I don't quite understand how CreateSubtree may affect on events ordering, because event ordering is defined by position in EventTree.
(In reply to alexander :surkov from comment #6)
> (In reply to Michael Li from comment #5)
> > The first test of browser_caching_name.js shows this:
> > https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/
> > browser_caching_name.js#62-68
> > The two spans' text nodes originally are under the root, and after the
> > invalidation list processes the spans, the text nodes are moved to the spans
> > and this problem happens: the span's show comes first, and then the text
> > node's hide and show events happen.
> 
> I don't quite understand how CreateSubtree may affect on events ordering,
> because event ordering is defined by position in EventTree.

CreateSubtree is what moves the child node from under the root to the span, moving the child is what adds the hide and show events for that child. Here's the backtrace after adding the child's hide event:
#0  mozilla::a11y::TreeMutation::Done (this=0x7fffe9cf6240) at /home/michael/Documents/gecko/accessible/base/EventTree.cpp:103
#1  0x00007f8d93538f8a in mozilla::a11y::DocAccessible::MoveChild (this=0x7f8d7fcc9a40, aChild=0x7f8d65894b00, 
    aNewParent=0x7f8d65c7e660, aIdxInParent=0) at /home/michael/Documents/gecko/accessible/generic/DocAccessible.cpp:2180
#2  0x00007f8d9353920c in mozilla::a11y::DocAccessible::CacheChildrenInSubtree (this=0x7f8d7fcc9a40, aRoot=0x7f8d65c7e660, 
    aFocusedAcc=0x7fffe9cf69d0) at /home/michael/Documents/gecko/accessible/generic/DocAccessible.cpp:2222
#3  0x00007f8d93547a7d in mozilla::a11y::DocAccessible::CreateSubtree (this=0x7f8d7fcc9a40, aChild=0x7f8d65c7e660)
    at /home/michael/Documents/gecko/accessible/generic/DocAccessible-inl.h:158
#4  0x00007f8d93537b77 in mozilla::a11y::DocAccessible::ProcessContentInserted (this=0x7f8d7fcc9a40, aContainer=0x7f8d7fcc9a40, 
    aNode=0x7f8d6d773a60) at /home/michael/Documents/gecko/accessible/generic/DocAccessible.cpp:1890
#5  0x00007f8d935363a8 in mozilla::a11y::DocAccessible::ProcessInvalidationList (this=0x7f8d7fcc9a40)
    at /home/michael/Documents/gecko/accessible/generic/DocAccessible.cpp:1381
CreateSubtree indeed may move accessibles, but it doens't affect on events ordering in general case, because event ordering is defined by EventTree. Having said that in your particular example, the patch indeed helps, because the EventTree looks this way:

reorder for document
  show for span1
  show for span2
  hide for text span1
  hide for text span2

and you do change show/hide events ordering by tweaking CreateSubtre, however the patch doesn't work in general case. For example:

<div id="list"></div>
<div id="container">
  <div id="control"></div>
</div>
<script>
document.getElementById('control').setAttribute('aria-owns', 'list');
</script>

reorder for list
  show for control
reorder for container
  hide for control
So in the second case, it's like we're moving the child from container to list? So in general, we'll have this bug when moving a child from a later parent to an earlier parent. How would you suggest fixing this order of events using EventTree?
Assignee: nobody → mili
(In reply to Michael Li from comment #9)
> So in the second case, it's like we're moving the child from container to
> list?

correct

>  So in general, we'll have this bug when moving a child from a later
> parent to an earlier parent. How would you suggest fixing this order of
> events using EventTree?

EventTree wasn't designed to preserve events ordering. Adding this feature would go at the cost of performance I think as you need to sort EventTree before events are fired. I think it should be easier to fix event consumers.
Comment on attachment 8780698 [details]
Bug 1294853: Move an accessible child before showing its new parent.

canceling review for now, as it doesn't address the problem in general
Attachment #8780698 - Flags: review?(surkov.alexander)
(In reply to alexander :surkov from comment #10)

> >  So in general, we'll have this bug when moving a child from a later
> > parent to an earlier parent. How would you suggest fixing this order of
> > events using EventTree?
> 
> EventTree wasn't designed to preserve events ordering. Adding this feature
> would go at the cost of performance I think as you need to sort EventTree
> before events are fired. I think it should be easier to fix event consumers.

alternatively, if the only issue we need to resolve is a child movement, then we could have a special ShowEvent for that like MoveEvent that will keep show and hide event pieces together. We can keep its HideEvent in EventTree for coalescence proposes and for show/hide/text chagne event ordering. which is caused by MoveEvent, then we ignore it
(In reply to alexander :surkov from comment #12)

> alternatively, if the only issue we need to resolve is a child movement,
> then we could have a special ShowEvent for that like MoveEvent that will
> keep show and hide event pieces together. We can keep its HideEvent in
> EventTree for coalescence proposes and for show/hide/text chagne event
> ordering. which is caused by MoveEvent, then we ignore it

'submit' button was hit earlier then I wanted. I think MoveEvent approach can work out, but not sure yet how to implement text change events, caused by move event, since they may depend on sibling changes.
Would we be able to implement this MoveEvent just in EventTree and not touch IPC stuff? I.e., would the MoveEvent be able to fire show/hide events in the correct order from the EventTree?
(In reply to Michael Li from comment #14)
> Would we be able to implement this MoveEvent just in EventTree and not touch
> IPC stuff? I.e., would the MoveEvent be able to fire show/hide events in the
> correct order from the EventTree?

It would probably go outside of EventTree scope, but the idea is to transform it into pair of hide/show events, so no changes on ipc side are required.
something like this should work out:
* add class MoveEvent : public AccShowEvent, that keeps reference to paired AccHideEvent
* add BeforeMove and AfterMove methods into TreeMutation to add events into EventTree
* when MoveEvent is encountered during EventTree processing, then see if paired HideEvent was fired (check eDoNotEmit flag), and if not, then fire it. When paired HideEvent is fired, then mark it eDoNotEmit.
Another idea that Trevor and I discussed was having a linked list through the EventTree's dependent events in order of when the events were inserted into the EventTree, and when we fire the events we iterate through this linked list and fire events in that order. Assuming we can get the events inserted into the EventTree in the correct order and that coalescence from the other patch is working correctly, this approach could also work.
(In reply to Michael Li from comment #17)
> Another idea that Trevor and I discussed was having a linked list through
> the EventTree's dependent events in order of when the events were inserted
> into the EventTree, and when we fire the events we iterate through this
> linked list and fire events in that order. Assuming we can get the events
> inserted into the EventTree in the correct order and that coalescence from
> the other patch is working correctly, this approach could also work.

despite the elegance of the approach, it seems an overkill if we want to resolve rare move cases only.
OK, what's a good way for me to start the patch based on the suggestions from comment 16? I understand the general concepts but am unsure of how to execute it.
(In reply to Michael Li from comment #19)
> OK, what's a good way for me to start the patch based on the suggestions
> from comment 16? I understand the general concepts but am unsure of how to
> execute it.

I'm not sure what kind of details to add on the sketch, but I'd be happy answer more concrete questions. Note, the sketch above is something from top of my head, and I'm sure, there's a nicer way to implement that. There's one thing I'm sure of: the moving would benefit from having its own event, because show/hide pair is not a best description for it.
(In reply to alexander :surkov from comment #18)
> (In reply to Michael Li from comment #17)
> > Another idea that Trevor and I discussed was having a linked list through
> > the EventTree's dependent events in order of when the events were inserted
> > into the EventTree, and when we fire the events we iterate through this
> > linked list and fire events in that order. Assuming we can get the events
> > inserted into the EventTree in the correct order and that coalescence from
> > the other patch is working correctly, this approach could also work.
> 
> despite the elegance of the approach, it seems an overkill if we want to
> resolve rare move cases only.

another issue with this approach is it breaks ordering of reorder vs show/hide events, we used to fire show/hide events and then its related reorder, I guess the suggestion is to fire all show/hide events and then all reorder events. I don't have data whether it breaks real world AT, but it would definitely break our tests.
(In reply to alexander :surkov from comment #21)
> (In reply to alexander :surkov from comment #18)
> > (In reply to Michael Li from comment #17)
> > > Another idea that Trevor and I discussed was having a linked list through
> > > the EventTree's dependent events in order of when the events were inserted
> > > into the EventTree, and when we fire the events we iterate through this
> > > linked list and fire events in that order. Assuming we can get the events
> > > inserted into the EventTree in the correct order and that coalescence from
> > > the other patch is working correctly, this approach could also work.
> > 
> > despite the elegance of the approach, it seems an overkill if we want to
> > resolve rare move cases only.

it should be fairly light weight, and I only care so much, EventTree's buggyness makes e10s really unusable.

> another issue with this approach is it breaks ordering of reorder vs
> show/hide events, we used to fire show/hide events and then its related
> reorder, I guess the suggestion is to fire all show/hide events and then all
> reorder events. I don't have data whether it breaks real world AT, but it
> would definitely break our tests.

I guess it may take some grossness, but I see no reason you can't preserve the order of reorders relative to the show and hide events.  That said the ordering has always been pretty weird because of coalescing reorder events, and the idea of reorder events seems kind of useless anyway.
(In reply to Trevor Saunders (:tbsaunde) from comment #22)

> it should be fairly light weight, and I only care so much, EventTree's
> buggyness makes e10s really unusable.

keeping a huge array of data and do not use in 99% cases at all times makes me think it has to be a better way

> > another issue with this approach is it breaks ordering of reorder vs
> > show/hide events, we used to fire show/hide events and then its related
> > reorder, I guess the suggestion is to fire all show/hide events and then all
> > reorder events. I don't have data whether it breaks real world AT, but it
> > would definitely break our tests.
> 
> I guess it may take some grossness, but I see no reason you can't preserve
> the order of reorders relative to the show and hide events.

It might be not that lightweight then, but I'm good to discuss it.

>  That said the
> ordering has always been pretty weird because of coalescing reorder events,
> and the idea of reorder events seems kind of useless anyway.

They are used by screen readers and they are coalesced the way that AT wants.
It looks like first-come-first-served approach should fail on this case:

<div id="c1"></div>
<div>
  <div id="c2"></div>
</div>
<div>
  <div id="c3"></div>
</div>
<script>
document.getElementById('c1').setAttribute('aria-owns', 'c2');
document.getElementById('c2').setAttribute('aria-owns', 'c3');
</script>

because c2 show will be fired before c3 hide.
Assignee: mili → administration
Blocks: 1272706
What's the plan here?

Alex are you going to shepherd Michael's work here into mozilla-inbound?
Flags: needinfo?(surkov.alexander)
I'll pick this one, since I'm on EventTree stuff anyways.
Flags: needinfo?(surkov.alexander)
Posted patch part1: simple case (obsolete) — Splinter Review
Attachment #8794379 - Flags: review?(yzenevich)
Assignee: administration → surkov.alexander
Attachment #8780698 - Attachment is obsolete: true
Attachment #8794379 - Attachment is obsolete: true
Attachment #8794379 - Flags: review?(yzenevich)
Attachment #8795814 - Flags: review?(yzenevich)
Posted patch part2 (obsolete) — Splinter Review
Attachment #8795815 - Flags: review?(yzenevich)
Attachment #8795814 - Attachment description: part1 → part1: fire hide event before its show event on move
Attachment #8795816 - Attachment description: part2 → part2: fire preceding hide before a parent show if the hide's show was coalesced by the parent show
Attachment #8795817 - Attachment description: part3 → part3: propagate preceding hides up if their tree is trimmed
Summary: Move an accessible child before showing its new parent → fire hide events before show events on move
Comment on attachment 8795814 [details] [diff] [review]
part1: fire hide event before its show event on move

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

looks good, thanks for some explanations in IRC
Attachment #8795814 - Flags: review?(yzenevich) → review+
Comment on attachment 8795816 [details] [diff] [review]
part2: fire preceding hide before a parent show if the hide's show was coalesced by the parent show

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

looks fine, one question

::: accessible/tests/mochitest/events/test_coalescence.html
@@ +558,5 @@
>        };
>      }
>  
> +    /**
> +     * Move 't9_c2_moved' node by aria-owns, and then move 't9_c3_moved' node

Is this actually a right description, you first move t9_c3_moved and then t9_c2_moved?
Attachment #8795816 - Flags: review?(yzenevich) → review+
Comment on attachment 8795817 [details] [diff] [review]
part3: propagate preceding hides up if their tree is trimmed

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

thanks

::: accessible/tests/mochitest/events/test_mutation.html
@@ +465,5 @@
>      {
>        return aNode.parentNode;
>      }
>  
> +    gA11yEventDumpToConsole = true; // debug stuff

logging
Attachment #8795817 - Flags: review?(yzenevich) → review+
Comment on attachment 8795845 [details] [diff] [review]
part4: do not fire preceding hides if coalesced

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

thanks , one question

::: accessible/tests/mochitest/events/test_coalescence.html
@@ +664,5 @@
> +    function test11()
> +    {
> +      this.eventSeq = [
> +        new invokerChecker(EVENT_HIDE, getNode('t11_c1_child')),
> +        new invokerChecker(EVENT_SHOW, 't11_c2_child'),

this order feels funny but ok (e.g. i would expect t11_c2_child show be after t11_c2 hide

@@ +680,5 @@
> +        // Remove a node from 't8_c1' container to give the event tree a
> +        // desired structure (the 't8_c1' container node goes first in the event
> +        // tree)
> +        getNode('t11_c1_child').remove();
> +        // then move 't8_c2_moved' from 't8_c2' to 't8_c1'.

nit comment need to be fixed (names 8->11)
Attachment #8795845 - Flags: review?(yzenevich) → review+
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9d10ac4f66badced6a4a6fee082c25fbfb1276
Bug 1294853 part2 - hide should preceed a parent show if event tree is trimmed, r=yzen
Duplicate of this bug: 1299957
(In reply to Yura Zenevich [:yzen] from comment #37)
> > +    /**
> > +     * Move 't9_c2_moved' node by aria-owns, and then move 't9_c3_moved' node
> 
> Is this actually a right description, you first move t9_c3_moved and then
> t9_c2_moved?

right, will fix it, apparently some artifacts of moving test cases between patches
(In reply to Yura Zenevich [:yzen] from comment #39)

> ::: accessible/tests/mochitest/events/test_coalescence.html
> @@ +664,5 @@
> > +    function test11()
> > +    {
> > +      this.eventSeq = [
> > +        new invokerChecker(EVENT_HIDE, getNode('t11_c1_child')),
> > +        new invokerChecker(EVENT_SHOW, 't11_c2_child'),
> 
> this order feels funny but ok (e.g. i would expect t11_c2_child show be
> after t11_c2 hide

t11_c2_child was moved out of t11_c2, and then t11_c2 was moved itself under another container. so ordering should be ok since they are not in parent-child connection anymore

> @@ +680,5 @@
> > +        // Remove a node from 't8_c1' container to give the event tree a
> > +        // desired structure (the 't8_c1' container node goes first in the event
> > +        // tree)
> > +        getNode('t11_c1_child').remove();
> > +        // then move 't8_c2_moved' from 't8_c2' to 't8_c1'.
> 
> nit comment need to be fixed (names 8->11)

sure
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/8a223f7a60a2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.