403942 - Cancelling "add new toolbar" *after* closing customize panel will freeze tab switching functionality

Status: VERIFIED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect, P1)

Description

[Tony Chung [:tchung]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.8) Gecko/20071004 Firefox/2.0.0.8
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.8) Gecko/20071004 Firefox/2.0.0.8

If you cancel the "Add new Toolbar" action on customizing toolbar, it will freeze the browser from navigating to other tabs.  Need to force quit the browser and restart to resume your work.


Reproducible: Always

Steps to Reproduce:
1. Launch Fx3 beta 1 rc3.  Open up a few new tabs.
2. right click toolbar > customize...
3. Click "Add new Toolbar" button
4. Click done on the customize toolbar
5. Then click "Cancel" for New Toolbar window
6. Verify you are unable to switch to any of your open tabs on the browser.

Browser is now in a hosed state.  Need to Force > Quit to restore your work.

Note: this seems to occur after Bug 403557 is executed.   perhaps its related?
Actual Results:  
Frozen tab switching.

Expected Results:  
Able to move around tabs

Comment 1

[Tony Chung [:tchung]]

Forces user to hard restart.  

Comment 2

[Phil Ringnalda (:philor)]

Not terribly surprising that you get into a bad state: your steps involve closing the parent of a modal .prompt() before closing the child. But it won't be possible to tell whether it's a Widget: Cocoa bug that it allows you to interact with the parent while the prompt is up until bug 395334 keeps the panel from getting on top of everything on earth.

Comment 3

[Neil Deakin]

Yes, this is fixed by the patch in bug 395334.

Comment 4

[Phil Ringnalda (:philor)]

Neil: I'm still not sure whose bug it really is, but it's *worse* after the landing from bug 395334 - now, you open the panel, open the sheet from the panel, click the Done button in the panel, and the whole window hides... somewhere, I'm not sure where. Cmd+tab to get it back, and you're back to step 5 in the original STR.

Comment 5

[Josh Aas]

Marking this as a dupe per comment #3, the window hiding in comment #4 is bug 406342.

Comment 6

[Phil Ringnalda (:philor)]

Um, no. Comment 3 was completely incorrect: bug 395334 did not fix this, it just introduced another step where your window is not only broken, but also hidden.

Bug 406342 may or may not also fix this, at the same time that it fixes that window hiding, which would make this "fixed by bug 406342" not "duplicate of", but it certainly isn't a duplicate of bug 395334.

Comment 7

[Phil Ringnalda (:philor)]

And just to be as unobscure as possible, it was *not* fixed by bug 406342, it's still possible to interact with the customization panel (in every way, though only clicking Done makes the browser unusable) while the add toolbar sheet is up and in theory being modal.

Comment 8

[Stan Shebs]

Bug is still happening in today's trunk. It doesn't seem to interfere with the creation and use of new windows, or use of menu or toolbar in the multiple-tab window - everything just goes to the last tab. Quitting doesn't work while the multiple-tab window is up, but you can close that window and then quitting works normally, so at least force quits aren't necessary. It looks like simple loss of mouse focus for the window, with no way to get it back.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

This sounds like a problem with sheet modality (since both the
Customize dialog and the New Toolbar dialog are sheets).

The New Toolbar dialog should be modal in such a way as to prevent
anything from happening when the user clicks on the Customize dialog's
Done button.  This is how it works on the 1.8 branch (tested in
Firefox 2.0.0.11).

So this bug seems related to bug 410170, and this behavior may (like
that described at bug 410170) may end up getting changed by the fix
for bug 395465 (which may end up changing all the sheets into ordinary
modal dialogs/windows).

Comment 10

[Stan Shebs]

In the absence of a general solution for sheets that want to be modal, I'm looking at a smaller fix, which is simply to tear down the New Toolbar dialog (which is an nsPromptService) when the Customize dialog is exited.

Comment 11

[Neil Deakin]

(In reply to comment #9)
> This sounds like a problem with sheet modality (since both the
> Customize dialog and the New Toolbar dialog are sheets).
> 

The customize dialog isn't a sheet, it's a popup with the window as a parent.

Comment 12

[Stan Shebs]

Attachment: patch to disable done button

Here is a simple patch that simply disables the "Done" button while the add new toolbar dialog is up. Not as powerful a solution as changing around window types at a low level, but since that work is still in progress, this solves the immediate problem.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Sounds like a good idea to me.

I hope to get to the larger problem in time for beta 4.

Comment 14

[Asaf Romano (gone)]

Comment on attachment 296778 [details] [diff] [review]
patch to disable done button

1. As for the workaround, this will break both Fx (on windows and linux) and Thunderbird (everywhere) as you didn't add the same id to customizeToolbar.xul
2. So the issue is that the modal feature is simply broken when used within a noautohide-panel, are you sure you're willing to ship this as-is?

Comment 15

[Stan Shebs]

Attachment: improved patch to disable done button

OK, this version addresses the portability issue.

And yes, this is not the ideal solution, but our timing is such that the necessary low-level work may not even get done for Firefox 3, so we need to do something else. This at least prevents the user from getting wedged in a nasty way.

Comment 16

[Asaf Romano (gone)]

Comment on attachment 298567 [details] [diff] [review]
improved patch to disable done button

>Index: toolkit/content/customizeToolbar.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/customizeToolbar.js,v
>retrieving revision 1.40
>diff -u -8 -p -r1.40 customizeToolbar.js
>--- toolkit/content/customizeToolbar.js	15 Nov 2007 03:38:16 -0000	1.40
>+++ toolkit/content/customizeToolbar.js	22 Jan 2008 23:45:48 -0000
>@@ -512,20 +512,25 @@ function addNewToolbar()
>                                 .getService(Components.interfaces.nsIPromptService);
> 
>   var stringBundle = document.getElementById("stringBundle");
>   var message = stringBundle.getString("enterToolbarName");
>   var title = stringBundle.getString("enterToolbarTitle");
> 
>   var name = {};
> 
>+  var doneButton = document.getElementById("donebutton");
>+  doneButton.disabled = true;
>+

a comment referring to this bug would be nice...

r=mano.  Would you file a bug for your backend work?

Comment 17

[Steven Michaud [:smichaud] (Retired)]

I think the patch I'm working on for bug 395465 has got this problem
fixed (at a lower level than Stan's patch).

It still needs some testing, but I hope to post it tomorrow.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

I've just posted my patch for bug 395465.  Here's a tryserver build
made using it:

https://build.mozilla.org/tryserver-builds/2008-01-25_15:[e-mail]-bugzilla395465/[e-mail]

Comment 19

[Steven Michaud [:smichaud] (Retired)]

I've now posted a revision of my bug 395465 patch.  Here's a tryserver
build made with it:

https://build.mozilla.org/tryserver-builds/2008-01-26_10:[e-mail]-bugzilla395465-rev1/[e-mail]

Comment 20

[Stan Shebs]

Attachment: Patch with added comment

OK, a version reflecting feedback and ready for checkin.

Comment 21

[Reed Loden [:reed]]

Checking in browser/base/content/customizeToolbarSheet.xul;
/cvsroot/mozilla/browser/base/content/customizeToolbarSheet.xul,v  <--  customizeToolbarSheet.xul
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/content/customizeToolbar.js;
/cvsroot/mozilla/toolkit/content/customizeToolbar.js,v  <--  customizeToolbar.js
new revision: 1.42; previous revision: 1.41
done
Checking in toolkit/content/customizeToolbar.xul;
/cvsroot/mozilla/toolkit/content/customizeToolbar.xul,v  <--  customizeToolbar.xul
new revision: 1.21; previous revision: 1.20
done

Comment 22

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

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko Minefield/3.0b4pre ID:2008020623