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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Michael Li]

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 1

[Michael Li]

Attachment: Bug 1296420: Don't add node to DocAccessible's invalidation list if it's a target of aria-owns.

Review commit: https://reviewboard.mozilla.org/r/73396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73396/

Comment 2

[alexander :surkov (:asurkov)]

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

Comment 3

[Michael Li]

(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

[alexander :surkov (:asurkov)]

(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.

Comment 5

[Michael Li]

Comment on attachment 8783704 [details]
Bug 1296420: Don't add node to DocAccessible's invalidation list if it's a target of aria-owns.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73396/diff/1-2/

Comment 6

[alexander :surkov (:asurkov)]

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?

Comment 7

[Michael Li]

Comment on attachment 8783704 [details]
Bug 1296420: Don't add node to DocAccessible's invalidation list if it's a target of aria-owns.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73396/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Phil Ringnalda (:philor)]

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

Comment 10

[Michael Li]

Alex, should we push this now that bug 1295603 is resolved?

Comment 11

[alexander :surkov (:asurkov)]

sure, is try server ok now?

Comment 12

[Michael Li]

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

[alexander :surkov (:asurkov)]

it would be nice to run a try server build against mc, I usually do this before landing just in case.

Comment 14

[Michael Li]

Looks good to me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7562416713e

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ec434174ad58