fire hide events before show events on move

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: michael.li11702, Assigned: surkov)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
(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?
(Assignee)

Comment 4

2 years ago
mozreview-review
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+
(Reporter)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
(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.
(Reporter)

Comment 7

2 years ago
(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
(Assignee)

Comment 8

2 years ago
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
(Reporter)

Comment 9

2 years ago
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?
(Reporter)

Updated

2 years ago
Assignee: nobody → mili
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Comment 11

2 years ago
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)
(Assignee)

Comment 12

2 years ago
(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
(Assignee)

Comment 13

2 years ago
(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.
(Reporter)

Comment 14

2 years ago
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?
(Assignee)

Comment 15

2 years ago
(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.
(Assignee)

Comment 16

2 years ago
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.
(Reporter)

Comment 17

2 years ago
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.
(Assignee)

Comment 18

2 years ago
(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.
(Reporter)

Comment 19

2 years ago
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.
(Assignee)

Comment 20

2 years ago
(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.
(Assignee)

Comment 21

2 years ago
(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.
(Assignee)

Comment 23

2 years ago
(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.
(Assignee)

Comment 24

2 years ago
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.
(Reporter)

Updated

2 years ago
Assignee: mili → administration
(Reporter)

Updated

2 years ago
Blocks: 1272706
What's the plan here?

Alex are you going to shepherd Michael's work here into mozilla-inbound?
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 26

2 years ago
I'll pick this one, since I'm on EventTree stuff anyways.
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 27

2 years ago
Created attachment 8794379 [details] [diff] [review]
part1: simple case
Attachment #8794379 - Flags: review?(yzenevich)
(Assignee)

Comment 30

2 years ago
Created attachment 8795814 [details] [diff] [review]
part1: fire hide event before its show event on move
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)
(Assignee)

Comment 31

2 years ago
Created attachment 8795815 [details] [diff] [review]
part2
Attachment #8795815 - Flags: review?(yzenevich)
(Assignee)

Comment 32

2 years ago
Created attachment 8795816 [details] [diff] [review]
part2: fire preceding hide before a parent show if the hide's show was coalesced by the parent show
Attachment #8795815 - Attachment is obsolete: true
Attachment #8795815 - Flags: review?(yzenevich)
Attachment #8795816 - Flags: review?(yzenevich)
(Assignee)

Comment 33

2 years ago
Created attachment 8795817 [details] [diff] [review]
part3: propagate preceding hides up if their tree is trimmed
Attachment #8795817 - Flags: review?(yzenevich)
(Assignee)

Comment 35

2 years ago
Created attachment 8795845 [details] [diff] [review]
part4: do not fire preceding hides if coalesced
Attachment #8795845 - Flags: review?(yzenevich)
(Assignee)

Updated

2 years ago
Attachment #8795814 - Attachment description: part1 → part1: fire hide event before its show event on move
(Assignee)

Updated

2 years ago
Attachment #8795816 - Attachment description: part2 → part2: fire preceding hide before a parent show if the hide's show was coalesced by the parent show
(Assignee)

Updated

2 years ago
Attachment #8795817 - Attachment description: part3 → part3: propagate preceding hides up if their tree is trimmed
(Assignee)

Updated

2 years ago
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+
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 42

2 years ago
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
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1299957
(Assignee)

Comment 44

2 years ago
(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
(Assignee)

Comment 45

2 years ago
(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
(Assignee)

Comment 49

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a223f7a60a26771863ed9b02ba6d7f72ce4ed28
Bug 1294853 part4 - do not fire preceding hides if they are coalesced, r=yzen
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 50

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8a223f7a60a2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.