Closed
Bug 1296420
Opened 8 years ago
Closed 8 years ago
Don't add node to DocAccessible's invalidation list if it's a target of aria-owns
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: michael.li11702, Assigned: michael.li11702)
References
Details
Attachments
(1 file)
For example, in our aria owns test file https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/doc_treeupdate_ariaowns.html, t1_checkbox, t1_button, t2_owned, t4_child1, and t4_child2 are on the invalidation list but they shouldn't be since the list should only contain inaccessible nodes referred to by IDRefs.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mili
Assignee | ||
Updated•8 years ago
|
Summary: Don't put grandchild node on DocAccessible's invalidation list if it's a target of aria-owns → Don't add grandchild to DocAccessible's invalidation list if it's a target of aria-owns
Assignee | ||
Updated•8 years ago
|
Summary: Don't add grandchild to DocAccessible's invalidation list if it's a target of aria-owns → Don't add node to DocAccessible's invalidation list if it's a target of aria-owns
Comment 2•8 years ago
|
||
thinking of testing, this should make a trick: <script> document.body.innerHTML = ` <span id='s'></span> <div aria-owns='s'></div> `; </script> span shouldn't fire show event, only div should be a target of show event also it would be interesting to test something like this, <script> document.body.innerHTML = ` <span id='s'></span> <div aria-owns='s'></div> <div aria-labelledby='s'></div> `; </script> it looks like your patch doesn't handle this case
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to alexander :surkov from comment #2) > thinking of testing, this should make a trick: > > <script> > document.body.innerHTML = ` > <span id='s'></span> > <div aria-owns='s'></div> > `; > </script> > > span shouldn't fire show event, only div should be a target of show event > DoARIAOwnsRelocation right now adds a show event for the aria-owns target, should we change it so that it won't, similar to how CacheChildrenInSubtree doesn't add show events for the added children?
Comment 4•8 years ago
|
||
(In reply to Michael Li from comment #3) > (In reply to alexander :surkov from comment #2) > > thinking of testing, this should make a trick: > > > > <script> > > document.body.innerHTML = ` > > <span id='s'></span> > > <div aria-owns='s'></div> > > `; > > </script> > > > > span shouldn't fire show event, only div should be a target of show event > > > DoARIAOwnsRelocation right now adds a show event for the aria-owns target, that's ok for now, you should make sure that you don't have a 2nd show event fired by the invalidation list processing. So if you write a mochitest (without your patch), then you should see a 'dupe show event' error. > should we change it so that it won't, similar to how CacheChildrenInSubtree > doesn't add show events for the added children? I'd say coalescence from your other bug should do the work, because DoARIAOwnsRelocation should fire events in general.
Updated•8 years ago
|
Attachment #8783704 -
Flags: review?(surkov.alexander)
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8783704 [details] Bug 1296420: Don't add node to DocAccessible's invalidation list if it's a target of aria-owns. https://reviewboard.mozilla.org/r/73396/#review71800 ::: accessible/generic/DocAccessible.cpp:1384 (Diff revision 2) > + // Check if the node is a target of aria-owns, and if so, don't process > + // it here and let DoARIAOwnsRelocation process it. > + bool shouldProcess = true; > + AttrRelProviderArray* list = > + mDependentIDsHash.Get(nsDependentAtomString(content->GetID())); > + if (list) { if there's no list, then nothing to process, right? maybe bool shouldProcess = !!list; ::: accessible/tests/mochitest/events/test_aria_owns.html:34 (Diff revision 2) > + > + /** > + * Aria-owns target shouldn't have a show event. > + * Markup: > + * <div id="t1_fc" aria-owns="t1_owns"></div> > + * <div id="t1_owns"></div> span? ::: accessible/tests/mochitest/events/test_aria_owns.html:67 (Diff revision 2) > + * Target of both aria-owns and other aria attribute like aria-labelledby > + * shouldn't have a show event. > + * Markup: > + * <div id="t2_fc" aria-owns="t1_owns"></div> > + * <div id="t2_sc" aria-labelledby="t2_owns"></div> > + * <div id="t2_owns"></div> span too?
Attachment #8783704 -
Flags: review?(surkov.alexander) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/ec1ebef4eb86 Don't add node to DocAccessible's invalidation list if it's a target of aria-owns. r=surkov
Keywords: checkin-needed
Comment 9•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/fx-team/rev/b7e8b15d90da for https://treeherder.mozilla.org/logviewer.html#?job_id=11296959&repo=fx-team
Assignee | ||
Comment 10•8 years ago
|
||
Alex, should we push this now that bug 1295603 is resolved?
Flags: needinfo?(surkov.alexander)
Assignee | ||
Comment 12•8 years ago
|
||
Here's one from over a week ago with this patch, the coalescence one, and the small patch for moving nodes (though that one's probably not needed): https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bb7f1bca82d Looking over all the failed tests, I don't think any are related to the changes I made.
Comment 13•8 years ago
|
||
it would be nice to run a try server build against mc, I usually do this before landing just in case.
Assignee | ||
Comment 14•8 years ago
|
||
Looks good to me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7562416713e
Keywords: checkin-needed
Comment 15•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec434174ad58 Don't add node to DocAccessible's invalidation list if it's a target of aria-owns. r=surkov
Keywords: checkin-needed
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ec434174ad58
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.
Description
•