tabs.create without a windowId should target only normal windows

VERIFIED FIXED in Firefox 59

Status

defect
P5
normal
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: michel.gutierrez, Assigned: bsilverberg)

Tracking

(Blocks 1 bug)

58 Branch
mozilla59
x86_64
Linux
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 wontfix, firefox59 verified)

Details

(Whiteboard: [windows])

Attachments

(1 attachment)

Reporter

Description

2 years ago
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.
Reporter

Updated

2 years ago
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Version: 56 Branch → 58 Branch
Component: Untriaged → WebExtensions: Frontend
Product: Firefox → Toolkit

Comment 1

2 years ago
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?
Assignee: nobody → bob.silverberg
Priority: -- → P5
Reporter

Comment 2

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

Comment 3

2 years ago
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?
Flags: needinfo?(aswan)
Reporter

Comment 4

2 years ago
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.
: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)
Flags: needinfo?(aswan) → needinfo?(michel.gutierrez)
Reporter

Comment 6

2 years ago
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 ?
Flags: needinfo?(michel.gutierrez)
Reporter

Comment 7

2 years ago
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.
(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.
Assignee

Comment 9

2 years ago
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?
Reporter

Comment 10

2 years ago
(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 !
Assignee

Comment 11

2 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [windows][design-decision-needed]
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().
Assignee

Comment 13

2 years ago
(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().
Summary: WebExtensions windows.getLastFocused() not filtering on window types → tabs.create without a windowId should target only normal windows
Assignee

Updated

2 years ago
Whiteboard: [windows][design-decision-needed] → [windows]
Reporter

Comment 14

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
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...
Attachment #8931006 - Flags: review?(aswan) → review-
Comment hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
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.
Attachment #8931006 - Flags: review?(aswan) → review+
Assignee

Updated

2 years ago
Blocks: 1421709
Comment hidden (mozreview-request)

Comment 22

2 years ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/100e07f0bd49
tabs.create without a windowId should target only normal windows, r=aswan

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/100e07f0bd49
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 24

2 years ago
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.
Flags: needinfo?(bob.silverberg)
Assignee

Updated

2 years ago
Flags: needinfo?(bob.silverberg) → qe-verify-

Updated

a year ago
Duplicate of this bug: 1400264

Comment 26

a year ago
Marking verified per the comment on the dupe saying this works now.
Status: RESOLVED → VERIFIED

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.