874554 - require("sdk/tabs").open doesn't allow to open in the current window while being in private window

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

We are facing this limitation in the Simulator:
  https://github.com/mozilla/r2d2b2g/issues/246
  https://github.com/mozilla/r2d2b2g/pull/542/files

The existing code of high level Windows.open method doesn't allow to just open a document in the currently focused top level window.

Scenario:
* open firefox
* open a private window
* call require("sdk/windows").browserWindow.open("http://mozilla.org")

Current:
* See mozilla.org being loaded in background window

Expected:
* See mozilla.org being loaded in the current window (that appear to be private)

I'd imagine that is we do not pass any isPrivate argument, we should try to open in any particular mode but just open in the currently focused window. Especially when the addon has private-browsing permission.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1007

Pointer to Github pull-request

Comment 2

[Alexandre Poirot [:ochameau]]

Oh sorry, my report doesn't make any sense... I meant tabs.open instead of windows.open!
Third step is:
 * call require("sdk/tabs").open("http://mozilla.org")

So Erik, I'm not sure you changed anything related to Windows behavior? Is non-private argument change anything? Otherwise, we can still land your unit test.

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Alexandre Poirot (:ochameau) from comment #2)
> Oh sorry, my report doesn't make any sense... I meant tabs.open instead of
> windows.open!
> Third step is:
>  * call require("sdk/tabs").open("http://mozilla.org")
> 
> So Erik, I'm not sure you changed anything related to Windows behavior? Is
> non-private argument change anything? Otherwise, we can still land your unit
> test.

Ah this is another bug then, I noticed that it is possible for one to open a private window with the code that you provided when they do not have permission to do so...

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #3)
> (In reply to Alexandre Poirot (:ochameau) from comment #2)
> > Oh sorry, my report doesn't make any sense... I meant tabs.open instead of
> > windows.open!
> > Third step is:
> >  * call require("sdk/tabs").open("http://mozilla.org")
> > 
> > So Erik, I'm not sure you changed anything related to Windows behavior? Is
> > non-private argument change anything? Otherwise, we can still land your unit
> > test.
> 
> Ah this is another bug then, I noticed that it is possible for one to open a
> private window with the code that you provided when they do not have
> permission to do so...

I made this bug 876821

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

So I'm seeing this error:

TypeError: activeWindow is null
resource://gre/modules/commonjs/sdk/tabs/tabs-firefox.js 31

I guess we could open a new non-private window first.

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1011

Pointer to Github pull-request

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 755054 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1011

Looks good with comment improvement in tests.

But this patch will fix the assertion you have seen ,but not change much to my initial report.
The tab still open in the background window if the current one is private and I omitted to pass `isPrivate`.
Also, the user expectation for the simulator addon is to open the simulator page in the current window, no matter what is its private state.
Do you think we can achieve this via the high level tab.open API?

We had the report of a user setting "never remember history" in settings panel. What it actually does is to always be in private mode!!! With such option toggled, your menuitems library is completely disabled, as it relies on WindowTracker :/
The only way out of this is to give private-browsing permission to the addon.

I tend to think we should soften strict rules around private browsing and especially if the addon has the permission.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Alexandre Poirot (:ochameau) from comment #7)
> Comment on attachment 755054 [details]
> Pointer to Github pull request:
> https://github.com/mozilla/addon-sdk/pull/1011
> 
> Looks good with comment improvement in tests.
> 
> But this patch will fix the assertion you have seen ,but not change much to
> my initial report.
> The tab still open in the background window if the current one is private
> and I omitted to pass `isPrivate`.
> Also, the user expectation for the simulator addon is to open the simulator
> page in the current window, no matter what is its private state.
> Do you think we can achieve this via the high level tab.open API?
> 
> We had the report of a user setting "never remember history" in settings
> panel. What it actually does is to always be in private mode!!! With such
> option toggled, your menuitems library is completely disabled, as it relies
> on WindowTracker :/

Ugh that's pretty awful... I'll verify this and I think it should be a separate bug if this is so..

> The only way out of this is to give private-browsing permission to the addon.
> 
> I tend to think we should soften strict rules around private browsing and
> especially if the addon has the permission.

I guess we could allow tabs.open to open in the most recent window regardless of private state if the isPrivate flag is omitted.  This would mean that tab.window should be broken for that tab through.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)
> I guess we could allow tabs.open to open in the most recent window
> regardless of private state if the isPrivate flag is omitted.  

IMO, if no isPrivate flag is set or it is set to false (and permanent private browsing is not enabled), it should open the tab in the most recent non-private window, opening a new non-private window if none exists.
 
If isPrivate is set to true (and permanent private browsing is not enabled), it should open the tab in the most recent private window, opening a new one if none exists.

If permanent private browsing is enabled, the tab should go in the most recent private window, regardless of the isPrivate flag. (And I guess on OSX, open a new private window if all windows are closed?)


Thoughts?

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Wes Kocher (:KWierso) from comment #9)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)
> > I guess we could allow tabs.open to open in the most recent window
> > regardless of private state if the isPrivate flag is omitted.  
> 
> IMO, if no isPrivate flag is set or it is set to false (and permanent
> private browsing is not enabled), it should open the tab in the most recent
> non-private window, opening a new non-private window if none exists.
>  
> If isPrivate is set to true (and permanent private browsing is not enabled),
> it should open the tab in the most recent private window, opening a new one
> if none exists.
> 
> If permanent private browsing is enabled, the tab should go in the most
> recent private window, regardless of the isPrivate flag. (And I guess on
> OSX, open a new private window if all windows are closed?)
> 
> 
> Thoughts?

This makes the most sense to me, if a add-on dev wants a super UX then they should support private-browsing imo.

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 755054 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1011

I don't think it really needs another review, also by brief look I did not seem to see any API changes. As of global private browsing mode and stuff I think we'll have a broader discussion with other teams what we expect behavior should be as this becomes quite ridiculous.

Comment 12

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/73742801c68af15f227aaec039f0ec288c028ba2
Bug 874554:
- adding a test for opening a new non-private window when there is only a private window open
- newly opened windows use a non-private flag by default now

Comment 13

[[github robot]]

Commit pushed to integration at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/73742801c68af15f227aaec039f0ec288c028ba2
Bug 874554:

Comment 14

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/edb96df02993c4dc192eb394a4d441bad3eba265
Bug 874554: tabs.open dnw when the active window was private or when there was no active window

Conflicts:
	test/private-browsing/windows.js

https://github.com/mozilla/addon-sdk/commit/d201e4f2f075c8c15486001eb4c5a644e858da7a
Merge pull request #1011 from erikvold/874554-3

Bug 874554: tabs.open dnw when there was no active window r=@ochameau

Comment 15

[[github robot]]

Commits pushed to integration at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/edb96df02993c4dc192eb394a4d441bad3eba265
Bug 874554: tabs.open dnw when the active window was private or when there was no active window

https://github.com/mozilla/addon-sdk/commit/d201e4f2f075c8c15486001eb4c5a644e858da7a
Merge pull request #1011 from erikvold/874554-3

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm going through the list of open bugs that github robot has commented in. Is this bug fixed, Alex?

Comment 17

[Alexandre Poirot [:ochameau]]

It looks like my original bug report is still not being addressed as described in comment 7.

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

Afaict Alex suggests that if private browsing is supported, and the add-on dev omits the `private` flag when using `tabs.open` then the tab should be opened in the current active window regardless whether it is private or not.  I don't see an issue with that, are you ok with this Irakli?

However I think that if private-browsing is not supported by the add-on then tabs should never be opened in a private window.

Is that about right Alex?

Comment 19

[Alexandre Poirot [:ochameau]]

Yes, you are describing correctly what I suggested. I can easily agree that it looks bad if we want to be strict and clear about private browsing.
But we should also be carefull about the final addon behaviors we are introcuding!!
Here, when I switched to the simulator I switched from SDK developer to SDK user and I quickly realized how bad the private browsing story was when using high level API. I ended up using low level API (because I knew exactly what was going on) and toggled the private permission. But keep it mind that's just a workaround. The simulator doesn't need to do anything with private contents, it only needs to be able to display a menuitem in Firefox menu and eventually open the dashboard in the current window, no matter if its private or not. I will never require to access or set any private data.

But I think most addons won't do any of these two things. We will just end up with poor quality addons, or make addon devs frustated when they try to fix these things.

Let me repeat the typical bad scenario we faced in the simulator:
A bunch of users toggled the "never remember my history" firefox setting. It actually switchs the default firefox window to private mode! So we ended up loosing our menuitem in the Firefox menu. And when we managed to make the menu to work again, it would open a new non-private window just for the dashboard.

Comment 20

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I do understand points that Alex is making and they make sense to me. Also if I recall it correctly, the main intention for omitting private windows from `WindowTracker` was to avoid adding widgets, panels, context menus, to such windows. Now that I think about it I think it's totally reasonable to expose access to private windows and tabs just don't include them in page-mods and don't add widgets etc.. unless add-on is opted-in.

I don't remember bug number, but there is one which suggests to let users enable / disable support for private browsing per add-on. I think that's a lot better solution and if users opt-in widgets and other APIs should just handle private windows as is.

Comment 21

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

But again, I think more general document UX perspective how things should work in private windows would be super useful.

Comment 22

[Erik Vold [:erikvold] (please needinfo? me)]

+1 for some ux decision here, I don't think we should expose private windows/tabs to add-ons that haven't request private-browsing permission however.

Comment 23

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562