1572860 - Audit usage of nsIDocShellTreeItem in IsARIALive

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement, P2)

Description

[Kannan Vijayan [:djvj]]

in accessible/base/nsAccUtils.h, within the IsAriaLive function (https://searchfox.org/mozilla-central/source/accessible/base/nsAccUtils.cpp#464):

This function traverses the document heirarchy up the chain, looking for the "aria live" attribute along the way.

With fission, this lookup will terminate early if an out-of-process document is encountered.

The logic within the loop also obtains the document from each item in the tree, and then traverses elements within the document to find the attribute. This is a heavyweight operation and turning all of these into IPC calls seems too heavyweight (objections welcome).

The right way to fix this is probably to factor this function out to separate the tree-item traversal and the document-element traversal, and have all of the document-element traversal logic execute in the remote process (which may forward to yet another process, or back to the same process, depending on how the browsingcontext-tree is organized).

Comment 1

[Chris Peterson [:cpeterson]]

Kannan says replacing nsIDocShellTreeItem calls should block enabling Fission in Nightly (M6).

Comment 2

[James Teh [:Jamie]]

Note that we do the same thing when setting live container attributes:
https://searchfox.org/mozilla-central/rev/d69ec052bed8700af7a283e37b60b4af22734930/accessible/generic/Accessible.cpp#1044
We do this in both places so that outer document authors can override live region markup on a widget they used in an iframe, as implemented in bug 419416. While I can see some use cases for this, I don't know of any usage in the wild. Furthermore, it doesn't work in Chrome:
data:text/html,<div aria-live="polite"><iframe src="data:text/html,<div>foo</div>">
The foo div does not have the container-live attribute.
Given the above, I think we should just restrict this to a single document.

Comment 3

[Chris Peterson [:cpeterson]]

Jamie, can someone on your team please audit this use of the nsIDocShellTreeItem interface in IsARIALive?

If this code is broken with Fission, fixing it blocks enabling Fission is Nightly and your team should prioritize (or delegate) the fix accordingly.

If this code works as-is with Fission, we don't need to remove this usage of nsIDocShellTreeItem until when we remove nsIDocShellTreeItem entirely (bug 1607591) after we ship Fission MVP.

Fission documentation about replacing nsIDocShellTree Item:
https://wiki.mozilla.org/Project_Fission/DocShell_Tree_Replace

:farre's presentation with examples of replacing nsIDocShellTreeItem with BrowsingContext, WindowContext, SyncedContexts, and BrowsingContextGroup:
https://docs.google.com/presentation/d/1K4j6ngty64TZjJNS5qH-MBoOm3TI2dJedVsbH8jUhKE/edit#slide=id.g6e35225e5d_1_264

Comment 4

[James Teh [:Jamie]]

See comment 2. TL;DR: This code doesn't behave as expected with Fission, but it doesn't break anything anyone cares about and thus isn't worth fixing. So, the thing to do here is to remove the functionality so it's consistent for Fission and non-Fission. I'd say that makes it low priority, but probably something we should do before MVP to avoid confusion.

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to James Teh [:Jamie] from comment #4)

See comment 2. TL;DR: This code doesn't behave as expected with Fission, but it doesn't break anything anyone cares about and thus isn't worth fixing. So, the thing to do here is to remove the functionality so it's consistent for Fission and non-Fission.

Remove ARIA support? Does Chromium support ARIA? Will need a long deprecation period, including public announcements, to warn web developers?

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA

Comment 6

[James Teh [:Jamie]]

Goodness no. :) I meant remove support for allowing ARIA live region markup in outer documents to affect inner documents. Firefox supports this, but there's no mention of this in the spec, it's not used in the wild and Chromium doesn't support it.

Comment 7

[James Teh [:Jamie]]

Attachment: Bug 1572860: Remove the ability for an outer document to override live region markup in an inner iframe document.

This is something that was implemented a long time ago, but it isn't covered in any spec, other browsers don't implement it and I don't know of any usage in the wild.
This doesn't work with OOP iframes, since what we're doing here requires the documents to be in the same process.
Given it isn't used or specified, the simplest solution is to just remove the behaviour altogether.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/987e73f47fda
Remove the ability for an outer document to override live region markup in an inner iframe document. r=MarcoZ

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/987e73f47fda