Closed
Bug 1294853
Opened 8 years ago
Closed 8 years ago
fire hide events before show events on move
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: michael.li11702, Assigned: surkov)
References
Details
Attachments
(5 files, 3 obsolete files)
(deleted),
text/x-review-board-request
|
surkov
:
review+
|
Details |
(deleted),
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
Assignee: nobody → mili
Assignee | ||
Comment 10•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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.
Comment 22•8 years ago
|
||
(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•8 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•8 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•8 years ago
|
Assignee: mili → administration
Comment 25•8 years ago
|
||
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•8 years ago
|
||
I'll pick this one, since I'm on EventTree stuff anyways.
Flags: needinfo?(surkov.alexander)
Assignee | ||
Comment 27•8 years ago
|
||
Attachment #8794379 -
Flags: review?(yzenevich)
Assignee | ||
Comment 28•8 years ago
|
||
Assignee | ||
Comment 29•8 years ago
|
||
Assignee | ||
Comment 30•8 years ago
|
||
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•8 years ago
|
||
Attachment #8795815 -
Flags: review?(yzenevich)
Assignee | ||
Comment 32•8 years ago
|
||
Attachment #8795815 -
Attachment is obsolete: true
Attachment #8795815 -
Flags: review?(yzenevich)
Attachment #8795816 -
Flags: review?(yzenevich)
Assignee | ||
Comment 33•8 years ago
|
||
Attachment #8795817 -
Flags: review?(yzenevich)
Assignee | ||
Comment 34•8 years ago
|
||
Assignee | ||
Comment 35•8 years ago
|
||
Attachment #8795845 -
Flags: review?(yzenevich)
Assignee | ||
Updated•8 years ago
|
Attachment #8795814 -
Attachment description: part1 → part1: fire hide event before its show event on move
Assignee | ||
Updated•8 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•8 years ago
|
Attachment #8795817 -
Attachment description: part3 → part3: propagate preceding hides up if their tree is trimmed
Assignee | ||
Updated•8 years ago
|
Summary: Move an accessible child before showing its new parent → fire hide events before show events on move
Comment 36•8 years ago
|
||
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 37•8 years ago
|
||
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 38•8 years ago
|
||
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 39•8 years ago
|
||
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 | ||
Comment 40•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/810a4f6dff0407c622e89cea3a6bb9a532c01cd0
Bug 1294853 part1 - hide should preceed its related show on a move, r=yzen
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 41•8 years ago
|
||
bugherder |
Assignee | ||
Comment 42•8 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 | ||
Comment 44•8 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•8 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
Comment 46•8 years ago
|
||
bugherder |
Assignee | ||
Comment 47•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2b0ba2164219d4fb031e5b29a28b10516c3905
Bug 1294853 part3 - pop up preceding hides from trimmed subtree, r=yzen
Comment 48•8 years ago
|
||
bugherder |
Assignee | ||
Comment 49•8 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•8 years ago
|
Keywords: leave-open
Comment 50•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 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.
Description
•