Closed Bug 576669 Opened 12 years ago Closed 11 years ago

Trying to enable plug-in loses currently selected tab

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- -

People

(Reporter: bzbarsky, Unassigned)

References

Details

(Keywords: regression, Whiteboard: fixed by bug 593687 [AddonsRewrite])

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.
Boriss can you take a look and see what you think we should do here.
Keywords: uiwanted
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.
blocking2.0: --- → ?
Component: Add-ons Manager → Tabbed Browser
Keywords: regression
OS: Mac OS X → All
Product: Toolkit → Firefox
QA Contact: add-ons.manager → tabbed.browser
Hardware: x86 → All
Whiteboard: [AddonsRewrite]
(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.
Summary: Trying to enable plug-in loses currently selected tab → Closing in-content (about:) tab focuses the last instead of the last selected tab
> 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.
Yeah, the failure is in step 8. The use of switchToTabHavingURI in BrowserOpenAddonsMgr breaks this use case.
Component: Tabbed Browser → Add-ons Manager
Product: Firefox → Toolkit
QA Contact: tabbed.browser → add-ons.manager
Summary: Closing in-content (about:) tab focuses the last instead of the last selected tab → Trying to enable plug-in loses currently selected tab
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?
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.
(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.
Beltzner agrees, this is annoying but not blocking. Would approve a safe patch if it turned up in my queue though
blocking2.0: ? → -
Keywords: regression
Status: NEW → RESOLVED
Closed: 11 years ago
Depends on: 593687, 616386
Keywords: uiwanted
Resolution: --- → FIXED
Whiteboard: [AddonsRewrite] → fixed by bug 593687 [AddonsRewrite]
Target Milestone: --- → mozilla2.0b10
You need to log in before you can comment on or make changes to this bug.