1617795 - Fission a11y: Embedder document reports as focused when OOP iframe is focused

Status: VERIFIED FIXED

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

Description

[James Teh [:Jamie]]

STR (with Fission enabled):

1. Open https://hsivonen.fi/fission-host.html
2. Tab to the first input.
3. Tab again to focus the first iframe document.
4. Examine the states of the top level document accessible.
   • Expected: It should not be focused.
   • Actual: It is focused.

Similar to bug 1594623, this also causes a race condition where the top level document and the iframe document both fire focus events. Sometimes, the top level document focus event arrives last, which is bad.

It seems dom::nsFocusManager::GetFocusedWindow in the top level content process returns the window for the top level document, even though the top level document isn't focused any more.
Also, if you click the input with the mouse (or focus via JS/a11y), this doesn't happen; dom::nsFocusManager::GetFocusedWindow in the top level content process returns null.

The fix from bug 1594623 no longer has any effect and should probably be removed as part of this.

Comment 1

[James Teh [:Jamie]]

Henri, is it expected that when an OOP iframe has focus, dom::nsFocusManager::GetFocusedWindow in the tab document's content process returns the window for the tab document (instead of null)? Note that this only happens if you focus the OOP iframe via the keyboard. If you focus it via mouse click/JS/a11y, dom::nsFocusManager::GetFocusedWindow in the tab document's process returns null.

This difference in behaviour might be related to bug 1617788 in that it's inconsistent between keyboard and other methods for taking focus.

Comment 2

[Henri Sivonen (:hsivonen)]

The difference is not intentional. It's an accident arising from the partial migration from nsPIDOMWindowOuter to BrowsingContext.

Comment 3

[James Teh [:Jamie]]

I understand. To clarify, can you tell me which case we should eventually expect (once bugs are fixed)? I assume GetFocusedWindow should return null with the above STR? If it's going to return a window instead (as it does now for keyboard focus), I'll need to tweak a11y to cope with that.

Comment 4

[Henri Sivonen (:hsivonen)]

I'm not yet sure where this is going, but I expect GetFocusedWindow would return the focused window if it is in-process. Does your caller really need a DOM window instead of a BrowsingContext?

Comment 5

[James Teh [:Jamie]]

Here's what the code needs to do:

1. If there is a focused element in the current process, return that. That already works in all cases.
2. Otherwise, if the document has focus, return the DOM document, but only if it really has focus as far as the user is concerned. That is, if something in an embedded OOP iframe actually has the focus, it must not return the document.

Currently, we do 2) by calling nsFocusManager::GetFocusedWindow(). If that returns a window, we call GetExtantDoc() on that window and return the resulting document.

We can change the a11y code to do whatever, window or BrowsingContext, so long as it can do the above. If we use BrowsingContext, we'll still need some way to get the DOM document if the focused BrowsingContext is in-process.

Comment 6

[Henri Sivonen (:hsivonen)]

nsFocusManager::GetFocusedBrowsingContext() should work, and should return an out-of-process BrowsingContext in the case you need to return a nullptr instead of the document.

Comment 7

[James Teh [:Jamie]]

Attachment: Bug 1617795: Use BrowsingContext instead of DOM window to get the focused document for a11y.

nsFocusManager::GetFocusedWindow may return a window even if focus is actually inside an embedded OOP iframe.
Instead, use nsFocusManager::GetFocusedBrowsingContext, which always knows about the current focus across all processes.
If the focused context is in this process, we get its document and return it.

Comment 8

[James Teh [:Jamie]]

I've posted a patch for this, but I'm holding off on requesting review until we see how some of the other Fission focus problems are dealt with; e.g. bug 1617788, bug 1615548, bug 1614297. In particular, I'm seeing a11y focus events go missing (both with and without this patch), so I'm not sure whether this patch will be the final approach needed here.

Comment 9

[James Teh [:Jamie]]

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

The fix from bug 1594623 no longer has any effect and should probably be removed as part of this.

It turns out that this fix is still necessary. While the iframe doesn't stay focused, it does get focus long enough for a11y to catch it and fire an event.

Comment 10

[James Teh [:Jamie]]

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

I've posted a patch for this, but I'm holding off on requesting review until we see how some of the other Fission focus problems are dealt with

I decided to move ahead with this. It's been a couple of weeks now and this issue still exists. It's arguably more correct anyway. We can always iterate if things change again.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6aa2ffa5d7ca
Use BrowsingContext instead of DOM window to get the focused document for a11y. r=yzen,hsivonen

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/6aa2ffa5d7ca

Comment 13

[Cristian Fogel [:cfogel]]

Confirmed issue with 75.0a1 (2020-02-23) on macOS 10.15.3.
Fix verified with 76.0a1 - 20200317214918.