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)
Tracking
()
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):
- Open https://hsivonen.fi/fission-host.html
- Click the second input (which is inside an iframe).
- Alt+tab to another app.
- Alt+tab back to Firefox.
- 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.
Reporter | ||
Comment 1•4 years ago
|
||
I tried this with the patch from bug 1614297. It did not fix the issue.
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 4•4 years ago
|
||
Henri, are you the right person to fix this focus bug?
Assignee | ||
Comment 5•4 years ago
|
||
I'll take a look.
Assignee | ||
Comment 7•4 years ago
|
||
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
.
Assignee | ||
Comment 8•4 years ago
|
||
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
Assignee | ||
Comment 9•4 years ago
|
||
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.
Assignee | ||
Comment 10•4 years ago
|
||
(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
.
Assignee | ||
Comment 11•4 years ago
|
||
Assignee | ||
Comment 12•4 years ago
|
||
Assignee | ||
Comment 13•4 years ago
|
||
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.)
Reporter | ||
Comment 14•4 years ago
|
||
(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
Assignee | ||
Comment 15•4 years ago
|
||
(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.
Assignee | ||
Comment 16•4 years ago
|
||
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?
Assignee | ||
Comment 17•4 years ago
|
||
Try unifying the mouse and key cases:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ada8dec3073278da1a4489e0a520f7f091c1b6f3
Assignee | ||
Comment 18•4 years ago
|
||
Try requesting focus for the BYMOVEFOCUS
case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a5b05bf8dce5ec3f2312182389737ec1c93a34e
Assignee | ||
Comment 19•4 years ago
|
||
Try requesting focus for the BYTOUCH
case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84e8f3a9f27e346aaf3ae7d7f8ed4b89c7e9006e
Assignee | ||
Comment 20•4 years ago
|
||
Try requesting focus for the BYLONGPRESS
case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d336f05b6119c1a0b4b83d5f721d5635b1b90297
Assignee | ||
Comment 21•4 years ago
|
||
Try requesting focus for the BYELEMENTFOCUS
case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c207bf39486b832901891e58c20041913afb8f66
Assignee | ||
Comment 22•4 years ago
|
||
(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.
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 23•4 years ago
|
||
Assignee | ||
Comment 24•4 years ago
|
||
(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.
Assignee | ||
Comment 25•4 years ago
|
||
Assignee | ||
Comment 26•4 years ago
|
||
Assignee | ||
Comment 27•4 years ago
|
||
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.)
Assignee | ||
Comment 28•4 years ago
|
||
(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 aJSWindowActor
Spawn
task calling.focus()
on anHTMLInputElement
. (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.
Assignee | ||
Comment 29•4 years ago
|
||
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.
Assignee | ||
Comment 30•4 years ago
|
||
It appears that mFocusedWindow
in the content process gets cleared appropriately.
Assignee | ||
Comment 31•4 years ago
|
||
(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.
Assignee | ||
Comment 32•4 years ago
|
||
Assignee | ||
Comment 33•4 years ago
|
||
Rebase to central to see if some of the oranges change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48d7fa97d6c9545270d9ccbcc69604abbbb54096
Assignee | ||
Comment 34•4 years ago
|
||
Requesting review instead of needinfo.
Assignee | ||
Comment 35•4 years ago
|
||
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
Assignee | ||
Comment 36•4 years ago
|
||
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
Assignee | ||
Comment 37•4 years ago
|
||
Assignee | ||
Comment 38•4 years ago
|
||
Assignee | ||
Comment 39•4 years ago
|
||
Let's see if the wpt assert goes away with a rebase:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=284748f227eb3e6e89a3fd8f335a79911649b746
Comment 40•4 years ago
|
||
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
Comment 41•4 years ago
|
||
bugherder |
Description
•