1415913 - tabs.create without a windowId should target only normal windows

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[Michel Gutierrez]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20171003101344

Steps to reproduce:

From a webext add-on, running this code:
```
browser.windows.getLastFocused({
  windowTypes: ["normal"]
})
.then((windowInfo)=>{
  console.info("windowInfo",windowInfo);
});
```
then run the code while focused window in a popup.


Actual results:

Console shows:
```
windowInfo Object { id: 104, focused: true, top: 596, left: 1029, width: 500, height: 214, incognito: false, type: "popup", state: "normal", alwaysOnTop: false, 1 more… }
```


Expected results:

I would expect the returned windowInfo to match a "normal" window (actually the last focused one), instead of the currently focused popup window.

Comment 1

[Andy McKay]

Bob, sounds very similar to bug 1406379. Could you take the time to check the other windows APIs please and see what else needs this filtering?

Comment 2

[Michel Gutierrez]

For information, doing the equivalent with `browser.windows.getAll({windowTypes:["normal"]})` only returned the "normal" windows and not the popup.

Comment 3

[Bob Silverberg [:bsilverberg]]

There are 4 API methods that accept a windowTypes array as part of the getInfo object which can be passed to them. They are:

- windows.getAll, which was updated (via bug 1406379) to use the windowTypes array to filter the returned array of window objects.

- windows.get, which accepts a windowId and returns a single window. Our implementation does not currently filter on windowTypes. In Chrome it does, and if you pass an array of types which does not include the type for the window that you are requesting it throws an exception: "No window with id: x". I don't really understand the use case for this. If you are requesting a particular window by id, why would you want to filter based on type and possibly generate an exception, when you could just check the type after having retrieved the window? 

- windows.getCurrent, which is supposed to return "the current window". Our implementation does not currently filter on windowTypes. In Chrome it does, but the behaviour I observed was unusual. If you pass an array of types which does not include the type for the "actual" current window, it returns a different window that does match one of the types. AFAIU there is only one current window, so returning a different window doesn't really make sense. I think it would make more sense to either throw an exception as windows.get does, or return undefined, if the current window does not match the windowTypes filter.

- windows.getLastFocused, which is supposed to return "the window that was most recently focused — typically the window 'on top'". Note that this does not say "the most recently focused window of a particular type". Our implementation does not currently filter on windowTypes. In Chrome it does, and it behaves similarly to what I described for windows.getCurrent. If you pass an array of types which does not include the type for the "actual" last focused window, it returns a different window that does match one of the types. My testing indicated that this not even necessarily the last focused window that does match a type. I'm not sure what logic Chrome uses to decide which window to return, but it doesn't seem accurate.

I'm not sure which, if any of these, we should actually support. 

I don't like throwing an exception from windows.get if the type doesn't match, although I suppose if you pass an array of types into windows.get you're asking for that.

I don't think that we should return anything other than the "actual" current window and last focused window from those methods. Chrome doesn't seem to get it right and I don't think it matches the meaning of the methods. If we think it's important to support a windowTypes filter for getCurrent and getLastFocused I think we should either throw an exception, or return undefined if the "actual" window doesn't match the filter.

Not really liking any of the solutions for supporting this, I would suggest we just do not support windowTypes for windows.get, windows.getCurrent and windows.getLastFocused. A developer can always check the type after the window is returned.

Andrew, what do you think?

Comment 4

[Michel Gutierrez]

Describing my use case to understand why i think supporting windowTypes filter in windows.getLastFocused() is important:

In Video DownloadHelper, a user requests an operation that requires a native app she did not install. The add-on opens a popup window with the notification message, and a button that opens a tab to the native app install page in the original normal window and closes the dialog. To open this tab, i must know in which window to do so: it should be the last focused normal window, but if the popup is not yet closed (time race here), getLastFocused() returns this popup, so i have to guess amongst the "normal" windows being opened, in which one i want to use to open the tab. My poor workaround seemed to work for me, but for the past 48hours, i read dozens of user comments saying it did not work well for them (they don't see the tab i open in "some" window). I'd mush prefer a clean solution rather that something that relies on timers hoping the popup window closing will be fast enough.

If you don't want to filter based on windowsType, at least export a timestamp to windows details telling when that window was last focused so that we can do the filtering ourselves.

Comment 5

[Andrew Swan [:aswan]]

:mig your use case makes sense, but if you just don't pass a windowId to browser.tabs.create() it is supposed to choose the top window, does that not work?

If that works in this case, I would vote for marking windowTypes as deprecated in the schema for get(), getCurrent() and getLastFocused() (and then just ignoring them in the implementation)

Comment 6

[Michel Gutierrez]

If i remember well, the tab often opened in the popup window which is definitely not what i want.

What about a "lastFocused" timestamp field to the Window object details ?

Comment 7

[Michel Gutierrez]

I gave it a try and i confirm that calling browser.tabs.create() without specifying a window id may open the tab in the popup window if this one is focused. And of course, if you close the popup at the same time, whether it opens in normal or popup is a matter of timing and luck, which is what you want to avoid as a developer.

Maybe the solution would be to open the tab in priority to a normal window if no window id is specified. I hardly imagine a use case where you'd want the tab to open in a popup as being the "normal" situation.

Comment 8

[Andrew Swan [:aswan]]

(In reply to Michel Gutierrez from comment #7)
> Maybe the solution would be to open the tab in priority to a normal window
> if no window id is specified. I hardly imagine a use case where you'd want
> the tab to open in a popup as being the "normal" situation.

I agree that would make more sense.  Its not really directly related but it makes me think of bug 1416810 and that we should generally not be treating popup windows like all other browser windows.

Comment 9

[Bob Silverberg [:bsilverberg]]

Would it be possible to get the current window before you open the popup and save its id, and then use that windowId to tell you where to open the new tab?

Comment 10

[Michel Gutierrez]

(In reply to Bob Silverberg [:bsilverberg] from comment #9)
> Would it be possible to get the current window before you open the popup and
> save its id, and then use that windowId to tell you where to open the new
> tab?

That's software, almost everything is possible. But this flow-breaking kind of hack quickly makes a product unmaintainable so i would like to avoid it. My latest workaround consists of calling windows.getLastFocused() every 200ms until it returns a window of type "normal", assuming the popup is now closed. Dirty you say ? yes it is and i'm not proud !

Comment 11

[Bob Silverberg [:bsilverberg]]

So where do we stand with this? It sounds like we're agreed on marking windowTypes as deprecated in the schema for get() and getCurrent(), but still unclear on getLastFocused(). The question remains of whether we want to implement it for  getLastFocused(), where it would return the last focused window that matches one of the types passed. I'm going to add this to the design-decision-needed triage so we can make a call on that.

Comment 12

[Andrew Swan [:aswan]]

I would vote for instead fixing browser.tabs.create() without a windowId so that it doesn't create tabs in popup windows, I think that would let us just deprecate windowTypes in getLastFocused().

Comment 13

[Bob Silverberg [:bsilverberg]]

(In reply to Andrew Swan [:aswan] from comment #12)
> I would vote for instead fixing browser.tabs.create() without a windowId so
> that it doesn't create tabs in popup windows, I think that would let us just
> deprecate windowTypes in getLastFocused().

That sounds good to me. I'll re-purpose this bug to do that, as it includes the use case, and open a separate bug to mark windowTypes as deprecated in the schema for get(), getCurrent() and getLastFocused().

Comment 14

[Michel Gutierrez]

My vote would go in favour of having a lastFocusedTimestamp field in Window objects so that an add-on would have all the options for the price of a few lines of code.

Comment 15

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1415913 - tabs.create without a windowId should target only normal windows,

This introduces a new property to WindowTracker called topNormalWindow which returns the
top window which is not a popup window. That property is then used by tabs.create
to identify the top "normal" window when a windowId is not passed to it.

A follow-up bug will be filed to look for other opportunities to use topNormalWindow
in place of topWindow in our codebase.

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

Comment 16

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8931006 [details]
Bug 1415913 - tabs.create without a windowId should target only normal windows,

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

Comment 17

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8931006 [details]
Bug 1415913 - tabs.create without a windowId should target only normal windows,

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

Comment 18

[Andrew Swan [:aswan]]

Comment on attachment 8931006 [details]
Bug 1415913 - tabs.create without a windowId should target only normal windows,

https://reviewboard.mozilla.org/r/202094/#review208578

I didn't test it but it looks like this will blow up out of the gate on Android...

Comment 19

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8931006 [details]
Bug 1415913 - tabs.create without a windowId should target only normal windows,

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

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8931006 [details]
Bug 1415913 - tabs.create without a windowId should target only normal windows,

https://reviewboard.mozilla.org/r/202094/#review209456

::: commit-message-05a91:3
(Diff revision 4)
> +Bug 1415913 - tabs.create without a windowId should target only normal windows, r?aswan
> +
> +This introduces a new property to WindowTracker called topNormalWindow which returns the

please clarify this: the new property is only the desktop implementation of WindowTracker

::: commit-message-05a91:7
(Diff revision 4)
> +
> +This introduces a new property to WindowTracker called topNormalWindow which returns the
> +top window which is not a popup window. That property is then used by tabs.create
> +to identify the top "normal" window when a windowId is not passed to it.
> +
> +A follow-up bug will be filed to look for other opportunities to use topNormalWindow

Please either file the bug and reference it here or just omit this.

Comment 21

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8931006 [details]
Bug 1415913 - tabs.create without a windowId should target only normal windows,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202094/diff/4-5/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/100e07f0bd49
tabs.create without a windowId should target only normal windows, r=aswan

Comment 23

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/100e07f0bd49

Comment 24

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 26

[:Gijs (he/him)]

Marking verified per the comment on the dupe saying this works now.