Don't add node to DocAccessible's invalidation list if it's a target of aria-owns

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

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

Tracking

(Depends on: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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

2 years ago
Assignee: nobody → mili
(Assignee)

Updated

2 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

2 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
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

2 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?
(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.
Attachment #8783704 - Flags: review?(surkov.alexander)
Comment hidden (mozreview-request)

Comment 6

2 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

2 years ago
Keywords: checkin-needed

Comment 8

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

Updated

2 years ago
Depends on: 1295603
(Assignee)

Updated

2 years ago
Blocks: 1299957
(Assignee)

Comment 10

2 years ago
Alex, should we push this now that bug 1295603 is resolved?
Flags: needinfo?(surkov.alexander)
sure, is try server ok now?
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 12

2 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.
it would be nice to run a try server build against mc, I usually do this before landing just in case.
(Assignee)

Comment 14

2 years ago
Looks good to me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7562416713e
Keywords: checkin-needed

Comment 15

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ec434174ad58
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

2 years ago
Depends on: 1360160
You need to log in before you can comment on or make changes to this bug.