make sure an accessible tree is updated on DOM tree removals

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

spin off bug 1276857
(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).
Flags: needinfo?(wchen)
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?
If you're convinced that this enables you to maintain whatever invariants the accessible tree cares about, then yes.
Posted patch patchSplinter Review
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)
Assignee: nobody → surkov.alexander
Attachment #8770636 - Attachment is obsolete: true
Flags: needinfo?(wchen)
Attachment #8772496 - Flags: review?(yzenevich)
(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.
(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 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) ?
Attachment #8772496 - Flags: review?(yzenevich) → review+
(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
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa663127d37bd77c3eb92f75879b1ff603995187
Bug 1286598 - make sure an accessible tree is updated on DOM tree removals, r=yzen
https://hg.mozilla.org/mozilla-central/rev/fa663127d37b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Attachment #8772496 - Flags: approval-mozilla-aurora?
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.
Attachment #8772496 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8772496 - Flags: approval-mozilla-beta?
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.
Attachment #8772496 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.