Closed Bug 1663931 Opened 4 years ago Closed 4 years ago

Keep focus in iframe and make the new doc take focus when an iframe changes remoteness

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
88 Branch
Project Flags:
Fission Milestone M7
Tracking Flags:
Tracking Status
firefox88 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1663931 - Avoid moving focus when changing iframe remoteness.
47 bytes, text/x-phabricator-request
Details | Review

Bug 1649099 reveals that if we focus an iframe that hasn't loaded, yet, we move the focus to the initial about:blank and then the real document loads without firing focus events. However, in this case, Chrome does fire events in the real document that loads.

Figure out if Chrome's behavior is legitimate per spec and what, if anything, we want to do about this.

Blocks: fission-focus
Severity: -- → N/A
Priority: -- → P3

We should figure out the desired behavior before Fission M7 Beta.

Fission Milestone: --- → M7
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

The attached tests show the opposite of what I expected:

I expected Chrome to have some magic to defer the focus request delivery to the iframe until the iframe has started loading its non-about:blank doc. Yet, I don't see that happening.

OTOH, I do see Firefox fire onfocus inside the frame in the in-process case. Both in the case where .focus() is called before the event loop has had a chance to spin after iframe creation and in the case where .focus() runs from the next available event loop spin (via self-postMessage).

I'll try to understand what our in-process case is doing. I'm rather surprised by the observed behavior.

It looks like the lazy about:blank strikes again.

When we try to focus a newly-created in-process iframe, we end up not finding a document in there, so we proceed running the focusing steps with nullptr as the element to focus. The in some manner that I didn't debug fully, we have things in such a state that the real document believes it needs to take focus upon loading.

I'll try to trigger the about:blank creation early in the focusing steps to see what happens if I do that.

It's not about:blank at least not in an obvious way.

When the real document is loaded in the frame, it runs

::PresShell::UnsuppressPainting () at PresShell.cpp:3923
::PresShell::UnsuppressAndInvalidate () at PresShell.cpp:3900
{virtual override thunk({offset(-32)}, nsGlobalWindowOuter::SetReadyForFocus)} ()
nsGlobalWindowOuter::SetReadyForFocus () at nsGlobalWindowOuter.cpp:6872
nsGlobalWindowInner::SetReadyForFocus () at nsGlobalWindowInner.cpp:4376
nsFocusManager::WindowShown

Then the OOP case returns early at https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#956-958 but the in-process case does not.

Anyway, it looks like, by design, if focus was in an iframe and we load a new doc in that iframe, we want to focus something in the newly loading doc and to fire the focus event while doing so.

While that makes sense, it's not clear to me that, given what Chrome does, we should be firing the event.

Clearly, it looks like the intent (which Fission breaks) of the existing code is to act as if focusing a document that is loaded in an iframe if focus was previously in that iframe.

Need to check if 1) the spec says that a document in a browsing context should get focused if focus used to be in the doc previously occupying the browsing context and 2) if the focus event should fire in that case.

Chrome's behavior suggests the answer might be "No" at least for the 2 (or there are even more distinctions to consider).

The focused browsing context in the OOP iframe to differs from the focused window in the in-process case at least due to the remoteness change destroying the docshell for the about:blank, which runs PageHidden on its window, which ends up (via nsFocusManager::WindowHidden) calling nsFocusManager::ClearFocus, which moves the focus to the ancestor of the iframe being navigated in a remoteness-changing way.

If we wanted to make Fission match non-Fission, we should not be clearing focus in a manner that changes the focused browsing context on a remoteness change. I think the right way would be adding code to nsFocusManager::WindowHidden to detect the case where the remoteness of the browsing context of aWindow is changing and in that case only null out mFocusedWindow and return early.

No news about explaining the observations about Chrome this time.

The Gecko patch I attached makes Fission align with non-Fission on this point. That is, it takes Gecko further away from passing the tests in the other patch.

It seems better to make potential further adjustments from the position of Fission and non-Fission aligning with each other and Fission matching the non-Fission intent rather than accidentally doing something else.

Here's a demo: https://hsivonen.com/test/moz/navigation-focus/ . I think this is more approachable for understanding what's going on that the attached WPT.

Findings:

  1. Once the iframe has been focused (when it has the initial about:blank) non-Fission Firefox, Chrome, and Safari all keep the iframe as the activeElement of the parent.
  2. Fission Firefox moves focus to body of the parent when the iframe changes remoteness. In the light of the previous point, this is clearly a bug. (And fixed by the attached Gecko patch.)
  3. Non-Fission Firefox fires the focus event on each framed document. Chrome and Safari either don't fire the event or fire it unobservably early.
  4. Non-Fission Firefox and Safari consider the body element of the last framed document focused in the end state: Clicking it doesn't refocus it. However, in Chrome, clicking it fires the focus event, which suggests that in Chrome the frame is left in a weird intermediate state where the framer considers the frame focused but the framee does not consider itself focused. However, if you alt-tab away from Chrome and back, the frame is eligible for focus restoration.

I think the non-Fission Firefox and the Safari behaviors are defensible. I think the Fission behavior (without the patch) or the Chrome behavior aren't defensible.

Whether the focus event should Fire observably as in non-Fission Firefox or whether the focus should be considered taken unobservably early as in Safari is debatable. I think our behavior is more useful for Web devs, as it allows reacting to focus.

I think the actions to be taken here at this time are:

  1. Taking the Gecko patch.
  2. Opening a spec issue.

Whether the WPTs landing along the Gecko patch should test for the event depends on how quickly the spec issue reaches agreement on what should happen.

See Also: → https://github.com/whatwg/html/issues/6209
Summary: Figure out what to do about differences with Chrome when focusing an iframe that hasn't loaded yet → Keep focus in iframe and make the new doc take focus when an iframe changes remoteness
Attachment #9191602 - Attachment is obsolete: true

Combining the patches and moving the tests that test the focus event to the unupstreamable directory.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96ea5581c5aab244cc07f01c51291ac0ff33be9a

Severity: N/A → S3
Type: task → defect

Fission x-orig failure with dom/events/test/test_bug409604.html seems like it's actually caused by the patch. :-(

TEST-UNEXPECTED-ERROR | http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/mochitest/test_input_events_for_identical_values.html | called finish() multiple times
and
TEST-UNEXPECTED-ERROR | http://mochi.test:8888/tests/layout/base/tests/test_bug842853-2.html | called finish() multiple times
also look like potentially actually caused by this patch.

TEST-UNEXPECTED-FAIL | browser/components/uitour/test/browser_UITour_showNewTab.js | leaked 1 window(s) until shutdown [url = about:newtab]
looks inconveniently consistent, but it's hard for me to see how the non-Fission case could be caused by this patch.

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

TEST-UNEXPECTED-ERROR | http://mochi.test:8888/tests/layout/base/tests/test_bug842853-2.html | called finish() multiple times
also look like potentially actually caused by this patch.

Caught this in rr.

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

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

TEST-UNEXPECTED-ERROR | http://mochi.test:8888/tests/layout/base/tests/test_bug842853-2.html | called finish() multiple times
also look like potentially actually caused by this patch.

Caught this in rr.

This run onload with anchor #anchor twice. I think this is due to https://searchfox.org/mozilla-central/rev/23c25cd32a1e87095301273937b4ee162f41e860/docshell/base/nsDocShell.cpp#4000 failing to abort the document synchronously. Am I reading that code right and, indeed, it fails to abort the document synchronously?

Flags: needinfo?(rjesup)

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

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

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

TEST-UNEXPECTED-ERROR | http://mochi.test:8888/tests/layout/base/tests/test_bug842853-2.html | called finish() multiple times
also look like potentially actually caused by this patch.

Caught this in rr.

This run onload with anchor #anchor twice. I think this is due to https://searchfox.org/mozilla-central/rev/23c25cd32a1e87095301273937b4ee162f41e860/docshell/base/nsDocShell.cpp#4000 failing to abort the document synchronously. Am I reading that code right and, indeed, it fails to abort the document synchronously?

Splitting this needinfo into bug 1682493.

Flags: needinfo?(rjesup)

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

TEST-UNEXPECTED-ERROR | http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/mochitest/test_input_events_for_identical_values.html | called finish() multiple times

Filed bug 1682517.

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

Fission x-orig failure with dom/events/test/test_bug409604.html seems like it's actually caused by the patch. :-(

Bug 1682519.

Curiously, this seems to make browser/components/uitour/test/browser_UITour_showNewTab.js leak a window consistently even in the non-Fission case even after rebasing.

Depends on: 1682493

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

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

...and everything went orange. :-(

Re-establishing the baseline to make sure that the cause was my latest tweak and not the rebase:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3908fb130f5584029c071968527dd267c692dc2

Let's see if the new code that takes nuking into account also fixes the previously-filed follow-ups:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50db6587414dcf2f2adc90de3e31756ffaa38074

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

Let's see if the new code that takes nuking into account also fixes the previously-filed follow-ups:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50db6587414dcf2f2adc90de3e31756ffaa38074

Nope. :-(

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

Sigh. Failure with the review comments addressed:
https://treeherder.mozilla.org/jobs?repo=try&revision=7b44771efd82b77abd183ab9e65830a7d5057779&selectedTaskRun=VOwr5LqpSYWl-HC-8sDpeQ.0

Wrong state of the patch.

Try addressing only the SetFocusedWindowInternal part of the review comments:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2165d225d82722c7c03760048908f5b681c90126

Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c63ecdad6a3c Avoid moving focus when changing iframe remoteness. r=nika,mccr8
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27725 for changes under testing/web-platform/tests

https://hg.mozilla.org/mozilla-central/rev/c63ecdad6a3c

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox88: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Regressions: 1694238
Upstream PR merged by moz-wptsync-bot
Blocks: 1682517
Regressions: 1756233
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: