Closed Bug 1617788 Opened 4 years ago Closed 4 years ago

OOP iframe focus other than via keyboard (click/JS/a11y) gets lost when switching away from Firefox

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla78
Fission Milestone M5b
Tracking Status
firefox78 --- fixed

People

(Reporter: Jamie, Assigned: hsivonen)

References

Details

Attachments

(1 file)

I tested this on Windows. I assume it occurs on other platforms as well, but I'm not sure.

STR (with Fission enabled):

  1. Open https://hsivonen.fi/fission-host.html
  2. Click the second input (which is inside an iframe).
  3. Alt+tab to another app.
  4. Alt+tab back to Firefox.
  5. Type some text.
    • Expected: The input clicked in step 2 should be focused and the typed characters should be entered there.
    • Actual: The input isn't focused.

If you focus the input via the tab key instead, this works as expected.

I tried this with the patch from bug 1614297. It did not fix the issue.

Priority: -- → P2

M5 for focus breakage

Fission Milestone: --- → M5

Henri, are you the right person to fix this focus bug?

Flags: needinfo?(hsivonen)

I'll take a look.

Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Flags: needinfo?(hsivonen)

Moving P2 M5 bugs to M5b milestone

Fission Milestone: M5 → M5b

As suspected, in the keyboard case, mFocusedElement of the inner window of top-level Web content contains the iframe. In the mouse case, it's nullptr.

The iframe was set as mFocusedElement from

BrowserBridgeChild`::RecvRequestFocus () at /BrowserBridgeChild.cpp:126
nsContentUtils::RequestFrameFocus () at /nsContentUtils.cpp:4351
nsFocusManager::SetFocus () at /nsFocusManager.cpp:449
nsFocusManager::SetFocusInner () at /nsFocusManager.cpp:1534
nsFocusManager::Focus () at /nsFocusManager.cpp:2310
{virtual override thunk({offset(-32)}, nsGlobalWindowOuter::SetFocusedElement)} ()
nsGlobalWindowOuter::SetFocusedElement () at /nsGlobalWindowOuter.cpp:6749
nsGlobalWindowInner::SetFocusedElement () at /nsGlobalWindowInner.cpp:4098

In the keyboard case, we do:
https://searchfox.org/mozilla-central/rev/3fd53c47864fedb916f0ed8f002f15456324f729/dom/ipc/BrowserChild.cpp#2633

This needs to be moved such that the SendRequestFocus call happens regardless of focus method.

(In reply to Henri Sivonen (:hsivonen) from comment #9)

This needs to be moved such that the SendRequestFocus call happens regardless of focus method.

It needs to be hoisted to SetFocusInner.

The previous attempt broke tests. It's unclear to me why it broke the tests: Clearly, an occasional request to focus an iframe happened such that the iframe wasn't already the focused element, but I haven't yet looked into why the iframe wasn't already the focused element in those case.

Let's try limiting the change to focus change by mouse, which knowingly leaves focus-by-JS cases unfixed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=baae80af6e8cc4c8fe0223b2dfde85dc3e022ed6

(It's not clear to me what flags focus by a11y sets.)

(In reply to Henri Sivonen (:hsivonen) from comment #13)

(It's not clear to me what flags focus by a11y sets.)

A11y currently passes 0 for aFlags:
https://searchfox.org/mozilla-central/rev/202a285024f174c2d2bf2152d9cba90a03723eab/accessible/generic/Accessible.cpp#742

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

(In reply to Henri Sivonen (:hsivonen) from comment #13)

(It's not clear to me what flags focus by a11y sets.)

A11y currently passes 0 for aFlags:
https://searchfox.org/mozilla-central/rev/202a285024f174c2d2bf2152d9cba90a03723eab/accessible/generic/Accessible.cpp#742

OK. The current patch appears OKish for FLAG_BYKEY and FLAG_BYMOUSE. I'll need to do more investigation to come up with a criterion for filtering unwanted 0-flag cases from the wanted 0-flag cases. (If that fails, let's consider adding a FLAG_BYAT.)

(In reply to Henri Sivonen (:hsivonen) from comment #13)

but I haven't yet looked into why the iframe wasn't already the focused element in those case.

At least in the case of browser/components/urlbar/tests/browser/browser_autoFill_caretNotAtEnd.js, when there was a focus mismatch, it was in the parent process and focus was in a HTML input element (presumably the URL bar) when Web content requested focus in a way that the test didn't like.

The unwanted case in the above test is triggered by an XPCOM nsIFocusManager::SetFocus call inside the test harness or browser chrome, AFAICT.

Maybe the most tractable approach is to set explicit flags in all cases where we want the propagation back to process containing the frame. That would argue for adding a FLAG_BYAT in the a11y case and perhaps even a FLAG_BYJS for the WebIDL .focus().

However, I could easily be missing a better approach. Neil, is there a better approach than the attached patch plus a new FLAG_BYAT flag for the a11y case that counts neither as mouse nor as key?

Flags: needinfo?(enndeakin)

(In reply to Henri Sivonen (:hsivonen) from comment #17)

Try unifying the mouse and key cases:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ada8dec3073278da1a4489e0a520f7f091c1b6f3

FAIL.

(In reply to Henri Sivonen (:hsivonen) from comment #18)

Try requesting focus for the BYMOVEFOCUS case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a5b05bf8dce5ec3f2312182389737ec1c93a34e

PASS.

(In reply to Henri Sivonen (:hsivonen) from comment #19)

Try requesting focus for the BYTOUCH case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84e8f3a9f27e346aaf3ae7d7f8ed4b89c7e9006e

PASS.

(In reply to Henri Sivonen (:hsivonen) from comment #20)

Try requesting focus for the BYLONGPRESS case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d336f05b6119c1a0b4b83d5f721d5635b1b90297

PASS.

(In reply to Henri Sivonen (:hsivonen) from comment #21)

Try requesting focus for the BYELEMENTFOCUS case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c207bf39486b832901891e58c20041913afb8f66

Probably PASS.

Attachment #9135127 - Attachment description: Bug 1617788 - Restore non-keyboard-caused focus after switching to another app and back. → Bug 1617788 - Restore focus after switching to another app and back even for non-keyboard causes.
Attachment #9135127 - Attachment description: Bug 1617788 - Restore focus after switching to another app and back even for non-keyboard causes. → Bug 1617788 - Restore focus in out-of-focus iframe after switching to another app and back even for non-keyboard causes.

(In reply to Henri Sivonen (:hsivonen) from comment #16)

perhaps even a FLAG_BYJS for the WebIDL .focus().

This is what FLAG_BYELEMENTFOCUS already is.

accessible/tests/browser/e10s/browser_caching_attributes.js, which with this patch leaks a window until shutdown, runs the code added in this patch once in response to a JSWindowActor Spawn task calling .focus() on an HTMLInputElement. (Not a very useful finding, but writing it down anyway.)

(In reply to Henri Sivonen (:hsivonen) from comment #27)

accessible/tests/browser/e10s/browser_caching_attributes.js, which with this patch leaks a window until shutdown, runs the code added in this patch once in response to a JSWindowActor Spawn task calling .focus() on an HTMLInputElement. (Not a very useful finding, but writing it down anyway.)

The frame focus request ends up requesting that the XULFrameElement in parent be focused. However, it already is focused. Since a new thing doesn't get focused, it's unclear to me how there is a new window leak.

The window leaks until shutdown in the content process. So far, the only difference apart from sending the IPC message in the content process with the patch and without that I can see is an extra Addref/Release pair on BrowserChild, which is cycle collected, but it seems implausible that it would have this kind of ripple effects.

It appears that mFocusedWindow in the content process gets cleared appropriately.

(In reply to Henri Sivonen (:hsivonen) from comment #30)

It appears that mFocusedWindow in the content process gets cleared appropriately.

It also clears its other fields.

I'm very tempted to disable the test.

Requesting review instead of needinfo.

Flags: needinfo?(enndeakin)
Blocks: 1613899
Depends on: 1629827

Per Emilio's review comments from bug 1629827, trying to add a dedicated flag for this case instead of inferring it from other flags:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76abf15a189cf8d143157809550deaf49e5027c0

That was very orange. Let's try again with the new flag set in nsFocusManager's internal callers of nsFocusManager::Focus that previously set flags to zero.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95821b7f2b77dadd47cd1fa712cb760b05a29e5e

Blocks: 1634363
Blocks: 1635530
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1122294e6be7
Restore focus in out-of-focus iframe after switching to another app and back even for non-keyboard causes. r=NeilDeakin
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: