make sure an accessible tree is updated on DOM tree removals

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
2 years ago
2 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)

(Assignee)

Description

2 years ago
Created attachment 8770636 [details] [diff] [review]
mochitest based on bz testcase

spin off bug 1276857
(Assignee)

Comment 1

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

Comment 2

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

Comment 4

2 years ago
Created attachment 8772496 [details] [diff] [review]
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)
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.
(Assignee)

Comment 6

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

Comment 8

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

Comment 10

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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa663127d37b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1280387
(Assignee)

Comment 12

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

Updated

2 years ago
status-firefox49: --- → affected
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 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f3790db5cbcb
status-firefox49: affected → fixed
(Assignee)

Comment 15

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