576669 - Trying to enable plug-in loses currently selected tab

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Open add-ons manager.
2   Click "plug-ins" tab.
3)  Disable flash.
4)  Close add-ons manager.
5)  Load a page that has a flash plugin on it.
6)  Open a new tab.
7)  Go back to the tab the page in step 5 was loaded in.
8)  Click on the "This plug-in is disabled message".  The add-ons manager opens.
9)  Enable flash.
10) Close the add-ons manager.

EXPECTED RESULTS: You're back to the page you opened in step 5 and can reload to get your flash stuffs.

ACTUAL RESULTS: You're at the tab you opened in step 6, and have to try to find the step 5 tab (which might be a long ways off to the left on the tab strip).

ADDITIONAL DETAILS: We should probably open the add-ons manager to the right of the current tab, not all the way at the end of the tabstrip.  But we should definitely not select some random other tab when the add-ons manager is closed after being opened from a given tab.

Marking as regression, since the separate-window add-ons manager clearly didn't have this problem.

Comment 1

[Dave Townsend [:mossop]]

Boriss can you take a look and see what you think we should do here.

Comment 2

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

Well, not sure if this is a regression but it's something new we have now with all the in content pages. It's even simpler to reproduce and really would apply to all other about: pages and not only the add-ons manager. So it makes more sense to stick it into the tabbrowser component.

Steps:
1. Open a couple of tabs
2. Select the first tab
3. Open an in content page, i.e. the add-ons manager
4. Close the tab with the in content page

After step 4 the first tab should be selected and not the last one.

Comment 3

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

(In reply to comment #0)
> ADDITIONAL DETAILS: We should probably open the add-ons manager to the right of
> the current tab, not all the way at the end of the tabstrip.  But we should

IMO it should still behave like the new tab button. When you open an about: page it is not related to the currently selected tab and shouldn't be opened right next to it.

Updating the summary to handle the discussion and implementation of the correct closing behavior.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> When you open an about: page it is not related to the currently selected tab

Uh...  PLEASE read the steps in comment 0.  In this case, the about: page was opened by the browser itself, in direct response to interaction with the currently selected tab.  It has everything in the world to do with the currently selected tab.  I was very specific about the problem I filed the bug on; I'm not exactly convinced your hijacking of it in comment 2 to address a much broader (and perhaps harder) problem...

I don't see why this is a tabbrowser issue; isn't the problem that the way the plug-in UI opens the add-ons manager doesn't set up the right tab parenting associations?  You can't expect the tabbrowser to magically guess those; the caller needs to tell it what to do.

Comment 5

[Dão Gottwald [:dao]]

Yeah, the failure is in step 8. The use of switchToTabHavingURI in BrowserOpenAddonsMgr breaks this use case.

Comment 6

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

Sorry for that. You are right. And the same thing needs to be fixed now for the "Open Add-ons Manager" button in the doorhanger notification. Are using both paths the same underlying code?

Comment 7

[Dave Townsend [:mossop]]

I suspect what what this would look like is this. If the users clicks on the plugin thing and it opens the add-ons manager then when closing that tab we should go back to the tab they came from.

If the users switches between tabs between opening the add-ons manager and closing it then I think we should just forget that and act normally.

I'm not sure if we switch back to the plugin's tab if the add-ons manager was already open and we just switch to it rather than opening a new one, but I suspect we should.

This is probably not terribly difficult to implement without touching tabbrowser at all, just add some appropriate TabClose/TabSelect event handlers from switchToTabHavingURI.

All that said though I suspect that this won't block release but I'd like to speak to some of the drivers first.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> I suspect what what this would look like is this. If the users clicks on the
> plugin thing and it opens the add-ons manager then when closing that tab we
> should go back to the tab they came from.
> 
> If the users switches between tabs between opening the add-ons manager and
> closing it then I think we should just forget that and act normally.

Using gBrowser.loadOneTab will do that for you. You should pass {relatedToCurrent: true} as well.

> This is probably not terribly difficult to implement without touching
> tabbrowser at all, just add some appropriate TabClose/TabSelect event handlers
> from switchToTabHavingURI.

We probably don't want this for switchToTabHavingURI calls in general.

Comment 9

[Dave Townsend [:mossop]]

Beltzner agrees, this is annoying but not blocking. Would approve a safe patch if it turned up in my queue though