1286598 - make sure an accessible tree is updated on DOM tree removals

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: mochitest based on bz testcase

spin off bug 1276857

Comment 1

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky [:bz] from bug 1276857 comment #43)
> > run through all accessible children of a container, walk their DOM parent chain to see if the removed node is in the chain, and if so then shutdown the subtree of an accessible child.
> 
> Yes, that was my proposal.

thinking more, checking all children all over for each removing node can be a bottle neck, say, if you remove half nodes, then it may take at least o(n^2/4). I'm not sure how to optimize it to find the removing children faster. 

On the other hand, for the nsIMutationObserver::ContentRemoved() approach I think I can detect whether an accessible for a node has been processed for removal already, and ignore those nodes.

So if nsIMutationObserver::ContentReserved registered on a DOM document triggers for shadow DOM mutations, then the approach should be workable. NI from wchen for that (bug 1276857 comment #33 for more info).

Comment 2

[alexander :surkov (:asurkov)]

I can see that nsIMutationObserver::ContentRemoved() on a document is triggered for shadow DOM changes. I tried this case.

<button onclick="remove();">Hello, world!</button>
<script>
var host = document.querySelector('button');
var root = host.createShadowRoot();
root.textContent = 'こんにちは、影の世界!';
function remove()
{
  root.textContent = '';
}
</script>

Boris, does this example enfold your concerns?

Comment 3

[Boris Zbarsky [:bzbarsky]]

If you're convinced that this enables you to maintain whatever invariants the accessible tree cares about, then yes.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

this version processes the removed trees twice, I didn't add rejecting mechanism that would ignore duping tree updates, since it complicates the code logic a bit. I suggest to get back to the issue if we see that this code pops up in profiling.

I'm sort of scary of going with the iteration children approach, because for each removal we have to iterate all children over and over, and I'm not sure how to optimize that nicely.

canceling wchen ni? as shadow DOM case seems working (mochitest is added)

Comment 5

[William Chen [:wchen]]

(In reply to alexander :surkov from comment #1)
> (In reply to Boris Zbarsky [:bz] from bug 1276857 comment #43)
> > > run through all accessible children of a container, walk their DOM parent chain to see if the removed node is in the chain, and if so then shutdown the subtree of an accessible child.
> > 
> > Yes, that was my proposal.
> 
> thinking more, checking all children all over for each removing node can be
> a bottle neck, say, if you remove half nodes, then it may take at least
> o(n^2/4). I'm not sure how to optimize it to find the removing children
> faster. 
> 
> On the other hand, for the nsIMutationObserver::ContentRemoved() approach I
> think I can detect whether an accessible for a node has been processed for
> removal already, and ignore those nodes.
> 
> So if nsIMutationObserver::ContentReserved registered on a DOM document
> triggers for shadow DOM mutations, then the approach should be workable. NI
> from wchen for that (bug 1276857 comment #33 for more info).

For now they do trigger. The behavior may change for content but we will probably always have to make it work for system level code since internally we depend on mutation observers.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to William Chen [:wchen] from comment #5)

> > So if nsIMutationObserver::ContentReserved registered on a DOM document
> > triggers for shadow DOM mutations, then the approach should be workable. NI
> > from wchen for that (bug 1276857 comment #33 for more info).
> 
> For now they do trigger. The behavior may change for content but we will
> probably always have to make it work for system level code since internally
> we depend on mutation observers.

ok, good to know, thank you. I hope that one more dependency wont' hurt for now then

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8772496 [details] [diff] [review]
patch

Review of attachment 8772496 [details] [diff] [review]:
-----------------------------------------------------------------

thanks

::: accessible/generic/DocAccessible.cpp
@@ +1144,5 @@
> +  }
> +#endif
> +  // This one and content removal notification from layout may result in
> +  // double processing of same subtrees. If it pops up in profiling, then
> +  // consider to reuse a document node cache to reject these notifications early.

nit: consider to reuse -> consider reusing

::: accessible/tests/mochitest/treeupdate/test_bug1276857.html
@@ +24,5 @@
> +        new invokerChecker(EVENT_REORDER, getNode('c1'))
> +      ];
> +
> +      this.invoke = function runTest_invoke()
> +      {

nit: { on the same line

@@ +37,5 @@
> +        };
> +        testAccessibleTree('c1', tree);
> +
> +        getNode('c1_t').querySelector('span').remove();
> +      }

nit: ;

@@ +47,5 @@
> +            { TEXT_LEAF: [] } // More text
> +          ]
> +        };
> +        testAccessibleTree('c1', tree);
> +      }

;

@@ +50,5 @@
> +        testAccessibleTree('c1', tree);
> +      }
> +
> +      this.getID = function runTest_getID()
> +      {

nit: { on the same line

@@ +52,5 @@
> +
> +      this.getID = function runTest_getID()
> +      {
> +        return 'child DOM node is removed before the layout notifies the a11y about parent removal/show';
> +      }

nit: ;

@@ +63,5 @@
> +        new invokerChecker(EVENT_REORDER, 'c2')
> +      ];
> +
> +      this.invoke = function runShadowTest_invoke()
> +      {

nit: { on the same line

@@ +76,5 @@
> +        };
> +        testAccessibleTree('c2', tree);
> +
> +        gShadowRoot.firstElementChild.querySelector('span').remove();
> +      }

;

@@ +86,5 @@
> +            { TEXT_LEAF: [] } // More text
> +          ]
> +        };
> +        testAccessibleTree('c2', tree);
> +      }

;

@@ +89,5 @@
> +        testAccessibleTree('c2', tree);
> +      }
> +
> +      this.getID = function runShadowTest_getID()
> +      {

nit: { on the same line

@@ +91,5 @@
> +
> +      this.getID = function runShadowTest_getID()
> +      {
> +        return 'child DOM node is removed before the layout notifies the a11y about parent removal/show in shadow DOM';
> +      }

;

@@ +139,5 @@
> +
> +  <script>
> +    var gShadowRoot = document.getElementById('c2_c').createShadowRoot();
> +    var template = document.getElementById('tmpl');
> +    gShadowRoot.appendChild(document.importNode(tmpl.content, true));

This is a little confusing. reference to tmp1 only works because it's an id. Do you mean template.content (template is used in this context) ?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #7)

> This is a little confusing. reference to tmp1 only works because it's an id.
> Do you mean template.content (template is used in this context) ?

yes, thanks for the catch

Comment 9

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bfe9a101440

Comment 10

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa663127d37bd77c3eb92f75879b1ff603995187
Bug 1286598 - make sure an accessible tree is updated on DOM tree removals, r=yzen

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 8772496 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: unknown (pretty old issue I think)
[User impact if declined]:missed content for screen reader users, also security issue like bug 1280387
[Describe test coverage new/current, TreeHerder]:covered by mochitest, was running on trunk for a while
[Risks and why]: moderate, changes in complicated code
[String/UUID change made/needed]:no

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8772496 [details] [diff] [review]
patch

Review of attachment 8772496 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes a screen reader issue. Take it in aurora.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3790db5cbcb

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 8772496 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: unknown (pretty old issue I think)
[User impact if declined]:missed content for screen reader users, also security issue like bug 1280387
[Describe test coverage new/current, TreeHerder]:covered by mochitest, was running on trunk and aurora for a while
[Risks and why]: moderate, changes in complicated code
[String/UUID change made/needed]:no

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8772496 [details] [diff] [review]
patch

Review of attachment 8772496 [details] [diff] [review]:
-----------------------------------------------------------------

This patch should be in 49 already. No need to be uplift to beta.