429287 - Tabbed browser should respond to DOMModalDialogClosed by restoring previously selected tab

Status: NEW

(Firefox :: Tabbed Browser, enhancement)

Description

[timeless]

It currently respond to DOMWillOpenModalDialog, it should store the open tab and then restore it.

Comment 1

[timeless]

Attachment: handle paired event

Comment 2

[timeless]

i believe that Camino already does something effectively like this

Comment 3

[Sean P.]

I found this from Bug 405239 (https://bugzilla.mozilla.org/show_bug.cgi?id=405239) which is marked as "WONTFIX" even though a lot of people seem to dislike this new "feature" of FF3.  I think this sounds like the sanest way of dealing with this.  The current changing of tab focus with each dialog popup is very annoying (FF2 did not have this problem).

Comment 4

[Jeremy Nickurak]

In lieu of 405239 getting fixed (hopefully by a 'tab bar' like the remember-password ui), this would certainly help me.

Comment 5

[Craig Longman]

I was just preparing to revert the FF2 to get away from the frustrating behaviour of switching the tabs that FF3 does, when I found this thread.

It looks like a very good solution was submitted by timeless in April, and I was wondering if or when this would be available in FF3?  I don't think there is anyway to just apply this to an existing installation, right?

I have no desire to allow all sites to store permanent cookies, but some sites I really do want to allow it.  The 'ask me' is the only viable option I can find right now.

Thanks for a great product.

Comment 6

[fswl1234]

if i understand you right, there's other option:

"I have no desire to allow all sites to store permanent cookies"
privacy - cookies - keep until i close firefox

"but some sites I really do want to allow it"
privacy - cookies - exceptions

do NOT let "clear private data" deal cookies though, it doesn't respect this "exceptions"

Comment 7

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

Comment on attachment 315946 [details] [diff] [review]
handle paired event

>Index: mozilla/browser/base/content/tabbrowser.xml

>+      <handler event="DOMModalDialogClosed" phase="capturing">

>+          if (!this.mDialogStack.length)

this.mModalDialogStack

Test coverage for this shouldn't be very hard. You can open tabs and should be able to triggers alert()s from content with the browser chrome test harness.
http://developer.mozilla.org/en/Browser_chrome_tests
http://mxr.mozilla.org/seamonkey/source/browser/base/content/test/

Comment 8

[Des Courtney]

I'm not a code contributor to either project, but my understanding is that Camino (the Mac specific offshoot of Mozilla) already does this.  It shouldn't be any harder to put this in as a XUL feature compared with a Cocoa one, given some of the code snippets already posted to this bug...

Comment 9

[Des Courtney]

My bad. Missed timeless's comment above

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, but Neil, can we get a review?

Comment 11

[[e-mail]]

Comment on attachment 315946 [details] [diff] [review]
handle paired event

>+          this.mModalDialogStack = [];
Make this a <field> please.

>+        <![CDATA[
>+          if (!event.isTrusted)
>+            return;
>+          if (!this.mDialogStack.length)
>+            return;
>+          if (this.selectedTab != this.mModalDialogStack[0].newTab)
>+            return;
>+          var pair = this.mModalDialogStack.shift();
>+          this.selectedTab = pair.oldTab;
Assuming that you do need this protection (I think thread manager allows you to easily post multiple modal dialogs against the same window) how do you propose to deal with the leaks you'll cause when modal dialogs are closed in the wrong order?

Comment 12

[[e-mail]]

(In reply to comment #7)
> Test coverage for this shouldn't be very hard. You can open tabs and should be
> able to triggers alert()s from content with the browser chrome test harness.
How do you close them again ;-)

Comment 13

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

You can use a polling timer that uses the window watcher to find commonDialogs (see e.g. toolkit/components/passwordmgr/test/prompt_common.js ).

Comment 14

[James Risner]

Please do not implement this suggestion.  Please mark this as WONTFIX.

Bug 405239 was finally fixed, which reverts to FF2 feature of "not switching focus to the other window wanting a cookie."  If you fix this bug, you will reintroduce the concept of switching to another background tab whenever it wants to set a cookie.  It is not sufficient to retain the current tab context, switch to the tab wanting a cookie for the dialog box, then revert to the previous tab.  The root of the annoyance is the focus switching in the first place.

At no point should a program switch focus for any reason other than explicit request of the user.  A background window wishing to set a cookie is a prime example of a case where the user did not explicitly request the focus to be stolen away.

Comment 15

[Jesse Ruderman]

I think this bug now applies to everything *except* the cookie dialog.

Comment 16

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

The fix for bug 405239 made the cookie dialog avoid setting a parent at all, which means this patch won't affect it.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 315946 [details] [diff] [review]
handle paired event

Clearing old review request. It seems comment 7 and comment 11 still need to be addressed anyways before this bug can proceed.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 315946 [details] [diff] [review]
handle paired event

I will clear gavin's r+ as well since this shouldn't appear as it is ready to land and will need to be rebased.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.