593687 - Once open Add-ons Manager from popup window, can not get back previous contents

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[chado]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6pre) Gecko/20100905 Firefox/4.0b6pre ID: 20100905041304

Once you open the Add-ons Manager from popup window, using Ctrl+Shift+A, 
there is no way to get back to previous contents.  This can be dataloss.


Reproducible: Always

Steps to Reproduce:
1. Open a popup window.  I mean js-opened window without toolbar.
   If you don't know/have appropriate site/page, go to this test page: 
     http://homepage3.nifty.com/chado/moz_work/window_open/open_sized_window_mini.html
     and click the "open" button: 
2. Press keyboard shortcut: Ctrl+Shift+A.

Actual Results:  
Add-ons Manager comes into the popup window.  Present trunk builds 
don't support tabs in popup window.  So, previous contents are gone.
There is no way to get back.  When the previous page contain any FORM 
data, it cause dataloss.


Expected Results:  
one of the following:
1. Open Add-ons Manager in new window when keyboard shortcut has been 
   given on popup window.
2. Make History-back available between the Add-ons Manager and previous 
   content.
3. Make keyboard shortcut, Ctrl+Shift+A, disable on popup window.


I'm seeing this on Win XP SP3 and Ubuntu 10.4 on VM Guest.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Not a core issue.  Pure front-end stuff.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The Add-ons Manager gets opened in a new tab but due to bug 586234 we fail to display the tabbar. Beside that problem I can see an extremely slow down in all panes of the manager. That should be probably filed as a new bug.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #2)
> Beside that problem I can see an extremely slow down in all
> panes of the manager. That should be probably filed as a new bug.

Filed as bug 594438.

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

requires bug 586234

Comment 5

[Dão Gottwald [:dao]]

Darn, switchToTabHavingURI's callback argument is used by tests. :(

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Removing it would make the patch for bug 565667 simpler, so that would be nice.

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch v2

This is slightly more complicated than the first patch because that couldn't really deal with about:addons being open in multiple tabs. The alternative would be to keep aCallback in switchToTabHavingURI and add something similar to

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> Created attachment 484987 [details] [diff] [review]
> patch v2
> 
> This is slightly more complicated than the first patch because that couldn't
> really deal with about:addons being open in multiple tabs. The alternative
> would be to keep aCallback in switchToTabHavingURI and add something similar to

... and add something similar to openUILinkIn, or let switchToTabHavingURI implement parts of openUILinkIn, but this doesn't seem preferable.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 484987 [details] [diff] [review]
patch v2

There is a lot of browser code here and I'm not technically a browser peer, plus I'd really like to get Gavin's take on the whole EM-ping-pong-loaded stuff, it feels kind of clumsy to me but for the life of me I can't think of a better alternative that solves the problem. I do worry how this is going to turn out when we add more and more of our UI to in-content like this. If gavin is ok with it then you have my r+ for the extension manager bits.

Comment 10

[Dave Townsend [:mossop]]

Attachment: experiment

I was working on a patch for bug 565686 but I quickly realised that that is strongly tied to this bug and to bug 565667. This was the rough approach I came up with and though it probably isn't complete yet it seems like it would solve all three without the added ping/pongs. What do you think of this as an alternative dao?

Comment 11

[Dão Gottwald [:dao]]

I'd be reluctant to expanding openUILinkIn like this, as it adds to a general-purpose API which we need to maintain forever. The EM-loaded notification could more or less be changed at will. For instance, I think it would be nice if we could avoid it altogether and just navigate to about:addons#themes rather than calling contentwin.loadView.

Similarly I'm not really attached to EM-ping/pong, but they also can be changed or thrown away any time. I'm not sure getExistingBrowser is the API we'd want eventually (I think you're potentially racing with the content load event when calling browser.contentWindow.wrappedJSObject.loadView).

Comment 12

[Dave Townsend [:mossop]]

It seems like if we're planning on moving more and more UI to be in content then we're going to need a general purpose API to help handle these cases.

Comment 13

[Dão Gottwald [:dao]]

If we add more in-content UI I think we should try not to follow the loadView pattern...

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> If we add more in-content UI I think we should try not to follow the loadView
> pattern...

If that's the real problem then we should just change it

Comment 15

[Dão Gottwald [:dao]]

I'm very much interested in doing so, but for the FF4 time frame dealing with loadView by temporary means seems more practical to me.

Comment 17

[Dietrich Ayala (:dietrich)]

Sounds like the narrower scoped patch is the way to go here, given the time at hand. We can address the broader landscape of general infrastructure to support in-content UI in Firefox 6.

Comment 18

[Dietrich Ayala (:dietrich)]

Review requested last October, no response yet. Dave, is there anyone else that is qualified to review this?

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Finding another reviewer wouldn't help, the people who need to make the decision are already involved. Dave, Dão and I just need to work it out.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

I think I prefer Dão's patch, just because it avoids adding a new general-purpose API (or modifying openLinkIn) and is more isolated at this point. The only problem is that it breaks at least two add-ons that already depend on specifics of switchToTabHavingURI:

https://addons.mozilla.org/en-US/firefox/addon/data-manager/ (relies on the callback)
https://addons.mozilla.org/en-US/firefox/addon/tab-mix-plus/ (monkeypatches switchToTabHavingURI in a way that would break with the switch to openUILinkIn)

Kairo maintains the first extension and tabmixplus is only marked as compatible with 3.7a6pre, so maybe that isn't a problem (we can also try contacting the tabmixplus authors).

Dave, what do you think?

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #20)
> I think I prefer Dão's patch, just because it avoids adding a new
> general-purpose API (or modifying openLinkIn) and is more isolated at this
> point. The only problem is that it breaks at least two add-ons that already
> depend on specifics of switchToTabHavingURI:
> 
> https://addons.mozilla.org/en-US/firefox/addon/data-manager/ (relies on the
> callback)
> https://addons.mozilla.org/en-US/firefox/addon/tab-mix-plus/ (monkeypatches
> switchToTabHavingURI in a way that would break with the switch to openUILinkIn)
> 
> Kairo maintains the first extension and tabmixplus is only marked as compatible
> with 3.7a6pre, so maybe that isn't a problem (we can also try contacting the
> tabmixplus authors).
> 
> Dave, what do you think?

I still dislike the approach but have no better alternative than that I suggested already so it's fine by me.

As an aside I started playing with trying to pass the view in the url as the fragment ID but ran into all sorts of problems because about: uris don't support fragment IDs and so things like XUL persistence and the session history management started breaking :(

Comment 22

[Robert Kaiser]

I probably can just fork switchToTabHavingURI for Data Manager, or fork the new way being implemented here, so something like that. Still, it's convenient to have that simple callback-based solution that switchToTabHavingURI offers.

CCing Philip as SeaMonkey implements switchToTabHavingURI as well and he is the add-on expert there.

Comment 23

[Philip Chee]

So what we need to do is to prevent the addon manager opening in a popup window. Here is a suggested approach:

> function switchToTabHavingURI(aURI, aOpenNew, aCallback) {
>   function switchIfURIInWindow(aWindow) {
>     if (!("gBrowser" in aWindow))

First check if we are in a popup window
      if (!("gBrowser" in aWindow || aWindow.document.documentElement.getAttribute("chromehidden")))

>       return false;

You can leverage on openUILinkIn() because it will skip popup windows (in Firefox)

This is what we did in SeaMonkey:

>   // No opened tab has that url.
>   if (aOpenNew) {
>     let browserWin = openUILinkIn(aURI.spec, "tabfocused");
>     if (aCallback) {
>       browserWin.addEventListener("pageshow", function(event) {
>         if (event.target.location.href != aURI.spec)
>           return;
>         browserWin.removeEventListener("pageshow", arguments.callee, true);
>         aCallback(browserWin.getBrowser().selectedBrowser);
>       }, true);
>     }
>     return true;
>   }

We use tabfocused here to prevent the addonManager opening in a background tab. SeaMonkey's openUILinkIn() returns the new window or tab. I copied this from Firefox not that long ago. But I just noticed that your current implementation removed the returns. You could back out that change in your implementation, and Bob's your Uncle.

Comment 24

[Philip Chee]

I don't think KaiRo wants the data manager opening in a popup window either - or any in content UI so a general purpose approach that can be reused would be preferable.

Comment 25

[Dão Gottwald [:dao]]

openUILinkIn doesn't have a return value on 1.9.2 and I don't think it had one on trunk. openNewTabWith had one, but it was removed because it wouldn't consistently return a tab anymore. It seems like the same would be true for openUILinkIn(..., "tab"), so I don't think we want to add that API.

Comment 26

[Philip Chee]

Hmm. You're right it was openNewTabWith. But thinking laterally for a bit. How about just blocking Ctrl+Shift+A while in a popuop window for now and then revisit this bug post 4.0? What's the use case for opening the addons tab while in a popup window?

Comment 27

[Robert Kaiser]

I also think we probably want to have something like switchToTabHavingURI be there and stay for the future, as there are a number of plans to move other things into in-tab UI, and esp. things like preferences probably want something in the style of the callback solution we currently have there.

But I somewhat agree with comment #26 in that it would probably good to fix this with a workaround for FF4 so that this release can go out fast and then go for a better solution for the next release.

Comment 28

[Dão Gottwald [:dao]]

> What's the use case for opening the addons tab while in a popup window?

A disabled plug-in being used in a popup or having installed something from a popup, for instance.

Post 4.0 it would be nice to get hashes to work for about: URIs, for which we wouldn't need return values or callback support in every involved function either.

Comment 29

[Dão Gottwald [:dao]]

> I also think we probably want to have something like switchToTabHavingURI be
> there and stay for the future, as there are a number of plans to move other
> things into in-tab UI, and esp. things like preferences probably want something
> in the style of the callback solution we currently have there.

switchToTabHavingURI is there to switch to a tab rather than communicate with the content. As for what we really want for in-content UI, see above.

Comment 30

[Robert Kaiser]

Well, I'd rather fork a callback-based thing than use a URL with a hash for Data Manager, esp. as handing over URL-like domain name identifiers and panes, and actions in panes in one has tag can get ugly fast and will stay in the urlbar as long as you have the tab open, and I even don't want to reload or such things if something else calls the already open tab with a different view. But then, that's all a different bug and doesn't belong here, I guess.

Comment 31

[Dão Gottwald [:dao]]

Hash changes don't reload.

Comment 32

[(no longer active)]

Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 484987 [details] [diff] [review]
patch v2

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js

>+  Services.obs.addObserver(function (aSubject, aTopic, aData) {
>+    Services.obs.notifyObservers(window, "EM-pong", "");
>+  }, "EM-ping", false);

Don't you need to remove this observer eventually, to avoid leaking the entire window via a closure?

Comment 34

[Dão Gottwald [:dao]]

Attachment: patch v3

addressed comment 33

Comment 35

[Dão Gottwald [:dao]]

Attachment: patch v3

for real this time

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 511743 [details] [diff] [review]
patch v3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> function BrowserOpenAddonsMgr(aView) {

>+  var newLoad = !switchToTabHavingURI("about:addons", true);
>+
>+  if (newLoad && aView) {

This check seems to be redundant - if aView is non-null, then newLoad must be true (else the ping/pong would have found the window above). It's a bit odd to fall back to using switchToTabHavingURI (and have it looks for open addon manager tabs) when we already know there are none - wouldn't it be simpler to just always use the ping/pong codepath, with only the loadView call being conditional on aView, and then fall back to just calling openUILinkIn directly if that fails? I guess that would involve having to re-implement the isTabEmpty check. I don't feel strongly - if you leave it as-is you get additional protection against ping/pong failing for whatever reason, I suppose... but then it seems like you should probably handle the !newLoad && aView case too (just call loadView on the selected tab's window?). Or you could just remove the newLoad check and add a comment.

Comment 37

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b18cd90bc0a7

I removed the newLoad check but kept the variable around to make this function easily hackable in the future.

Comment 38

[chado]

Now ctrl+shift+A in the popup window opens Add-ons Manager within:
  the new tab for existing usual window. Otherwise, 
  the new window when there is no usual window.

Verified on Win XP SP3 native and Ubuntu 10.10 on VM Guest with:
  Mozilla/5.0 (Windows NT 5.1; rv:2.0b12pre) Gecko/20110212 Firefox/4.0b12pre 
    ID:20110212030346 9698ac3f1c61 and 
  Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110213 Firefox/4.0b12pre 
    ID:20110213030344 51702867d932 .

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dao, time-wise any chance to create an automated test? Otherwise I will create a manual and a Mozmill test.