1933769 - Remove some focus calls in the front-end which shouldn't have an effect.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

I tried to do this in bug 1933119 but got backed out due to a test failure.

I think the test failure is legit and uncovers a bug in nsFocusManager.

We're in a situation where:

• mActiveWindow = the top level window
• mFocusedWindow = nullptr

Front-end calls promiseFocus(), and document.hasFocus() returns false. So it proceeds to call window.focus(), but that bails out in RaiseWindow here:

• https://searchfox.org/mozilla-central/rev/234f91a9d3ebef0d514868701cfb022d5f199cb5/dom/base/nsFocusManager.cpp#3022

I think bailing out is wrong and we should ensure the widget becomes focused as well. We already deal with this situation correctly here:

• https://searchfox.org/mozilla-central/rev/234f91a9d3ebef0d514868701cfb022d5f199cb5/dom/base/nsFocusManager.cpp#699-708

Furthermore, we no longer have the windows special-case of having child windows in the top level, so we can simplify a bit RaiseWindow.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1933769 - Remove no-op focus() calls. r=#tabbrowser-reviewers

Since bug 1823984, this focus() call has been a no-op on remote frames.

The previous patch also fixes the non-remote frame focus switch.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1933769 - Clean-up a bit nsFocusManager::RaiseWindow. r=smaug,edgar,hsivonen

Windows no longer has child widgets since bug 1870512, where we removed
ShouldAttachToTopLevel() special-cases.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1933769 - Move focus to window directly if we are already the active window. r=smaug

We're in a situation where:

mActiveWindow = the top level window
mFocusedWindow = nullptr

Front-end calls promiseFocus(), and document.hasFocus() returns false
(correctly).

So it proceeds to call window.focus(), but that bails out in RaiseWindow
here:

https://searchfox.org/mozilla-central/rev/234f91a9d3ebef0d514868701cfb022d5f199cb5/dom/base/nsFocusManager.cpp#3022

I think bailing out is wrong and we should ensure the widget becomes
focused as well. We already deal with this situation correctly here:

https://searchfox.org/mozilla-central/rev/234f91a9d3ebef0d514868701cfb022d5f199cb5/dom/base/nsFocusManager.cpp#699-708

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f6eb274c4d9
Clean-up a bit nsFocusManager::RaiseWindow. r=smaug

Comment 5

[amarc]

https://hg.mozilla.org/mozilla-central/rev/3f6eb274c4d9

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6c7cfdf4e11
Move focus to window directly if we are already the active window. r=smaug

Comment 7

[Cristina Horotan [:chorotan]]

Backed out changeset d6c7cfdf4e11 (Bug 1933769) for causing bv failures at outOfProcess/browser_controller.js

Backout link

Push with failures

Failure log

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4424a9d13f3f
Move focus to window directly if we are already the active window. r=smaug

Comment 9

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/4424a9d13f3f

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/842468e46637
Remove no-op focus() calls. r=tabbrowser-reviewers,dao

Comment 11

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/842468e46637