1290057 - WebExtensions: onFocusChanged does not fire when Firefox loses/gains focus

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[sixtyten]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057

Steps to reproduce:

1. run chrome.windows.onFocusChanged.addListener(console.log.bind(console, "focus:")); in a WebExtensions debugger
2. switch focus between various windows

Platform: Windows 10
Firefox version: Nightly 50.0a1 (2016-07-26)


Actual results:

The window id was only logged when a Firefox window gained focus and it was different from the previously focused Firefox window.


Expected results:

The window id should have been logged also when Firefox lost focus to a non-Firefox window (chrome.windows.WINDOW_ID_NONE according to the docs), or gained focus from one.

Comment 1

[sixtyten]

FWIW: While Chrome does not fire this either on (at least) W10, this is due to a bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=387377
The documentation is clear about sending chrome.windows.WINDOW_ID_NONE when all browser windows have lost focus:
https://developer.chrome.com/extensions/windows#event-onFocusChanged
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/windows/onFocusChanged

As a workaround, I currently run a content script in every page to catch DOM focus events and send those to the extension, but this has numerous limitations, among them that it doesn't work on pages that don't allow content scripts.

The implementation uses WindowManager.topWindow and topWindow uses Services.wm.getMostRecentWindow("navigator:browser"), which is (afaik) unaware of lost focus:
https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/browser/components/extensions/ext-windows.js#36
https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/browser/components/extensions/ext-utils.js#569

Comment 2

[Tomislav Jovanovic :zombie]

Attachment: bug 1290057 - fire onFocusChanged when Firefox loses/gains focus

Review commit: https://reviewboard.mozilla.org/r/85772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85772/

Comment 3

[Tomislav Jovanovic :zombie]

This is a simple case of replacing `WindowManager.topWindow` with `Services.focus.activeWindow`.

However, when switching between two Firefox windows, in the blur event callback, neither of them has focus, so we end up sending a superfluous WINDOW_ID_NONE event (at least on Windows 7).

While Chrome docs allow this [1], we can avoid it by waiting a tick before processing the event. To my understanding, this is not racy, all events are still processed in the right order, using current state.

Existing tests pass. Manually confirmed this fixes the bug, as we don't really have the ability to write a test for this.

[1] https://developer.chrome.com/extensions/windows#event-onFocusChanged

Comment 4

[Tomislav Jovanovic :zombie]

Comment on attachment 8800975 [details]
bug 1290057 - fire onFocusChanged when Firefox loses/gains focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85772/diff/1-2/

Comment 5

[Tomislav Jovanovic :zombie]

And it looks like that's very similar to what Chrome does, as that original patch breaks existing tests on linux only (just as noted in Chrome docs).

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

Updated the test to ignore the blur event.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8800975 [details]
bug 1290057 - fire onFocusChanged when Firefox loses/gains focus

https://reviewboard.mozilla.org/r/85772/#review84630

::: browser/components/extensions/test/browser/browser_ext_windows_events.js:26
(Diff revision 2)
> +        browser.test.assertTrue([1, 3, 5, 7].includes(focusCount),
> +                                "Ignoring a superfluous WINDOW_ID_NONE (blur) event on some platforms.");

Hmm... this doesn't seem entirely safe. What if we get `NONE > 1 > 2 > NONE > 1`, for instance? Does this reliably happen every time on the platforms where it happens? What platforms does it happen on?

Comment 7

[Tomislav Jovanovic :zombie]

Comment on attachment 8800975 [details]
bug 1290057 - fire onFocusChanged when Firefox loses/gains focus

https://reviewboard.mozilla.org/r/85772/#review84640

::: browser/components/extensions/test/browser/browser_ext_windows_events.js:26
(Diff revision 2)
> +        browser.test.assertTrue([1, 3, 5, 7].includes(focusCount),
> +                                "Ignoring a superfluous WINDOW_ID_NONE (blur) event on some platforms.");

It happens on Linux, just as described in Chrome docs. I removed the "wait a tick" Promise locally, and could replicate the test failures exactly on Windows 7 as well.

Fixed the test and submitted to Try, to confirm my understanding of what is happening: there is a reliable blur event, every time we switch focus between Firefox windows. Was waiting for test results before flagging you for review.

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8800975 [details]
bug 1290057 - fire onFocusChanged when Firefox loses/gains focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85772/diff/2-3/

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8800975 [details]
bug 1290057 - fire onFocusChanged when Firefox loses/gains focus

https://reviewboard.mozilla.org/r/85772/#review86370

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f28003313db0
fire onFocusChanged when Firefox loses/gains focus r=kmag

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 51.0a2 (2016-11-07) under Windows 10.
Verified on Firefox 52.0a1 (2016-11-07) under Windows 10 64-bit and Ubuntu 16.04 32-bit and I’ve noticed the following:
  - The window id is successfully logged while switching focus between a Firefox window and a non-Firefox window 
  - The window id is successfully logged while switching focus between Firefox windows
  - The window id is successfully logged while switching focus between a Firefox window and Developer Tools/Browser Console/Library windows
 
But,

  - There is no window id logged while switching focus between Developer tools, Browser Console and Library windows
  - There is no window id logged while switching focus between Developer tools/Browser Console/Library windows and a non-Firefox window


Any thoughts about this?

Comment 13

[Tomislav Jovanovic :zombie]

I think this is ok, as the browser.windows API is meant meant to deal mainly with "browser" ("web content") windows.  Those other window types don't have a window id, since they are not meant to be manipulated by this API.

And since the event is fired when switching away from a browser window to one of these non-browser windows, I believe this is correct behavior.