1646486 - Fix GetInProcessParentDocument usage in Document::IsVisibleConsideringAncestors

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task)

Description

[Kris Maglione [:kmag]]

It isn't Fission-compatible

Comment 1

[Chris Peterson [:cpeterson]]

Emilio, what happens if this code is wrong for Fission OOP iframes? Do we need to fix this before we enable Fission?

See also GetVisibility bug 1646561.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This is only called by a11y code. It seems we'd create accessibles for hidden fission iframes, which seems a bit bad, but probably not insanely bad?

Though I think this may work without the "consideringancestors" bit? Why does this need to look at ancestor documents? I'd expect all subdocuments of an invisible document to be also marked as invisible, otherwise we should fix that, as that wastes resources.

Ah, it seems this can only differ during visibility changes: https://bugzilla.mozilla.org/show_bug.cgi?id=850797#c1

Jaime, do you have any opinion here?

Comment 3

[James Teh [:Jamie]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

This is only called by a11y code. It seems we'd create accessibles for hidden fission iframes, which seems a bit bad, but probably not insanely bad?

We already handle the hidden Fission iframe case here:
https://searchfox.org/mozilla-central/rev/227f22acef5c4865503bde9f835452bf38332c8e/dom/ipc/BrowserParent.cpp#1237

Ah, it seems this can only differ during visibility changes: https://bugzilla.mozilla.org/show_bug.cgi?id=850797#c1

Jaime, do you have any opinion here?

Looking at that bug, we still need this, but it's only relevant in-process. As noted above, hidden OOP iframes are handled elsewhere (in the parent process).

I'd be inclined to just rename this to IsVisibleConsideringInProcessAncestors. If that's too ugly for Document, I guess we could move it into a11y::nsAccUtils.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yup, that sounds good. Either way about keeping it in Document or not.

Comment 5

[Neha Kochar [:neha]]

Moving to Future based on Jamie's comment 3 indicating this is fine for now.

Comment 6

[Neha Kochar [:neha]]

Jamie, this sounds like a trivial fix and can prevent future mistakes. Can you find an assignee to fix and close this?

Comment 7

[James Teh [:Jamie]]

Attachment: Bug 1646486: Move dom::Document::IsVisibleConsideringAncestors to a11y::nsCoreUtils::IsDocumentVisibleConsideringInProcessAncestors.

Renaming the function makes it clear that it only deals with in-process ancestors, which is an important distinction for Fission.
A11y is the only consumer of this and it only cares about in-process ancestors in this case (OOP stuff is handled elsewhere), so the functionality doesn't need to change.
Since a11y is the only consumer, move this into the a11y code.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13ad365a1338
Move dom::Document::IsVisibleConsideringAncestors to a11y::nsCoreUtils::IsDocumentVisibleConsideringInProcessAncestors. r=emilio

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/13ad365a1338