Last Comment Bug 425844 - [10.5] Print/Page Setup Menu items grayed out after printing a page using File | Print
: [10.5] Print/Page Setup Menu items grayed out after printing a page using Fil...
Status: VERIFIED FIXED
[See comments #26 and #37]
: regression, verified1.9.0.7, verified1.9.1
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: P2 normal with 6 votes (vote)
: mozilla1.9.2a1
Assigned To: John Daggett (:jtd)
:
: Markus Stange [:mstange]
Mentors:
: 431952 435864 438790 440507 441513 442556 443598 444643 448731 449559 450109 451319 453731 456610 458054 460671 463713 465745 467327 469088 474764 478620 (view as bug list)
Depends on:
Blocks: 372571
  Show dependency treegraph
 
Reported: 2008-03-28 16:51 PDT by Marcia Knous [:marcia - use ni]
Modified: 2009-03-02 13:43 PST (History)
52 users (show)
jaas: wanted‑next+
jaas: blocking1.9.1+
jaas: blocking1.9-
jaas: wanted1.9.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
utterly lame patch, v.0.1, bracket calls to print dialog code with menu fixup (3.77 KB, patch)
2009-01-25 19:26 PST, John Daggett (:jtd)
jaas: review-
Details | Diff | Splinter Review
patch, v.0.2, cleanup and remove debugging code (4.42 KB, patch)
2009-01-26 21:05 PST, John Daggett (:jtd)
no flags Details | Diff | Splinter Review
patch, v.0.3, simpler version, just paint main window menu bar (3.74 KB, patch)
2009-01-27 17:04 PST, John Daggett (:jtd)
jaas: review-
Details | Diff | Splinter Review
patch, v.0.4, changes based on review comments (4.01 KB, patch)
2009-01-28 17:14 PST, John Daggett (:jtd)
jaas: review+
Details | Diff | Splinter Review
patch, v.0.4a, patch for 3.0 cvs trunk (4.60 KB, patch)
2009-01-29 00:55 PST, John Daggett (:jtd)
no flags Details | Diff | Splinter Review
patch, v.0.4b, fixup (4.58 KB, patch)
2009-01-29 02:37 PST, John Daggett (:jtd)
no flags Details | Diff | Splinter Review
patch, v.0.4c, create 1.9.0 branch version (7.42 KB, patch)
2009-02-01 18:23 PST, John Daggett (:jtd)
jaas: review+
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review

Description Marcia Knous [:marcia - use ni] 2008-03-28 16:51:14 PDT
Seen using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008032804 Minefield/3.0pre.

STR:
1. Go to a web page and select File | Print | Save as PDF
2. Go back to the File menu and inspect the file choices.

Expected: All would be active
Actual: New Tab, Close, Save Page As, Send Link, Page Setup and Print are greyed out. I can get the menu items back easily by switching focus off the page.

This does not happen using the equivalent nightly on Tiger.
Comment 1 Marcia Knous [:marcia - use ni] 2008-03-28 19:15:29 PDT
I actually just noticed when testing on PPC mac that many other menus are greyed out besides the File Menu after following the STR in Comment 0. Switching focus out of the window and back restores all the menu items to active.
Comment 2 Henrik Skupin (:whimboo) 2008-03-29 05:14:08 PDT
That's a regression which has been caused by a fix with the last days. Running a test with beta5 rc2 doesn't show this problem. I'll try to get the regression window.

Asking for blocking-firefox3 because of limited functionality.
Comment 4 Steven Michaud [:smichaud] (Retired) 2008-03-31 09:22:33 PDT
I'm not sure I've understood your description, Marcia.  But as far as
I can tell this isn't a bug.

The Save As dialog you get when you do File | Print | Save as PDF is a
native modal dialog (one provided by the OS, and not by Gecko).  While
it's running, it correctly disables a number of menu items, including
the ones you mention.  The idea is that this is a modal dialog, and
you need to finish whatever tasks it requires before you go off to do
anything else.  It works the same way in Safari.

When the dialog closes (whether you press Save or Cancel) the
greyed-out menu items always get re-enabled (at least in my tests).
Comment 5 Henrik Skupin (:whimboo) 2008-03-31 09:27:00 PDT
(In reply to comment #4)
> When the dialog closes (whether you press Save or Cancel) the
> greyed-out menu items always get re-enabled (at least in my tests).

Exactly this doesn't happen under OS X 10.5. After you close the print dialog these menu items are not re-enabled. You have to switch the focus (like Marcia said) to get the menu items re-enabled. 

Comment 6 Steven Michaud [:smichaud] (Retired) 2008-03-31 09:50:25 PDT
> After you close the print dialog these menu items are not re-enabled.

I can't reproduce this.  I tested with today's nightly on OS X 10.5.2 (Intel).
Comment 7 Steven Michaud [:smichaud] (Retired) 2008-03-31 09:58:19 PDT
> I can't reproduce this.

Sorry, I take that back.  I _can_ reproduce it..
Comment 8 Steven Michaud [:smichaud] (Retired) 2008-03-31 10:31:07 PDT
My patch for bug 422827 landed in Henrik's regression range from comment #3, and I was a _little_ afraid ... but I've now exonerated it.

Disabling my patch for bug 422827 doesn't fix this bug -- which shows that it's unrelated.
Comment 9 Steven Michaud [:smichaud] (Retired) 2008-03-31 10:43:58 PDT
> Possibly related: bug 425593

Nope.  Backing out that bug's patch doesn't fix this problem.
Comment 10 Henrik Skupin (:whimboo) 2008-03-31 16:07:07 PDT
Steven, are you able to verify the regression range? If not, I was wrong. Otherwise another patch listed by the above query should be responsible for.
Comment 11 Steven Michaud [:smichaud] (Retired) 2008-03-31 16:17:14 PDT
> Steven, are you able to verify the regression range?

I haven't had a chance to (I haven't tried yet).  I hope to do so tonight or tomorrow.
Comment 12 philippe (part-time) 2008-04-01 01:03:33 PDT
Within the regression range, bug 425593 is probably covering up (hiding) the issue.

Starting with this hourly build: 20080326_1830, the print dialogue throws up a sheet (bug 425593). Clicking in the sheet to dismiss it shifts the focus around (somehow the steps in comment 1), and restores the functionality of the menus

I would be more suspicious of bug 372571 (that also causes 'vanishing menu issues' in Camino, see bug 426011). Bug 372571 landed slightly earlier than the regression range.
And with the 20080326_1732 hourly build, I can't reproduce the issue.
Comment 13 Steven Michaud [:smichaud] (Retired) 2008-04-01 08:22:49 PDT
Philippe, where do you get these hourly builds?
Comment 14 philippe (part-time) 2008-04-01 16:21:56 PDT
(In reply to comment #13)
> Philippe, where do you get these hourly builds?
> 

http://hourly-archive.localgho.st/mac.html
Comment 15 Thomas K. (:tom) 2008-04-04 08:41:57 PDT
*** Bug 426990 has been marked as a duplicate of this bug. ***
Comment 16 Thomas K. (:tom) 2008-04-04 08:48:04 PDT
(In reply to comment #12)
> Within the regression range, bug 425593 is probably covering up (hiding) the
> issue.

Not really, see bug 426990.  I experienced the exact same thing, and it's because an OS window is taking focus.  Oddly, the upload dialog doesn't do it, but whenever I upgrade my nightly build and have to reauthorize access to the 1passwd keychain for the changed Minefield.app, coming back to Minefield breaks New Tab (etc).  I've duped my bug here because I'm certain it's the same, it's accessing an OS X feature and doesn't really recognize that focus has returned.  That's why (I think) switching apps back and forth fixes the issue.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040404 Minefield/3.0pre
Comment 17 Josh Aas 2008-04-08 13:07:21 PDT
This bug, whatever might have triggered it, is the result of our Carbon printing dialog doing whatever it wants to our Cocoa menus. We can't override that in the obvious way like we can for Cocoa app-modal dialogs. We need to get rid of our Carbon printing dialog and replace it with a Cocoa one. That said, there may be a way to work around this, which we can do for Gecko 1.9.0.x.

This bug is much less severe now that the patch for bug 398514 has landed. Now the keyboard commands will work just fine even in the menu items are disabled.
Comment 18 Thomas K. (:tom) 2008-04-08 14:15:02 PDT
@Josh Aas

That makes sense for printing, but does that mean that bug 426990 should be unmarked as a dupe since it's a separate bug with the same effects?
Comment 19 Josh Aas 2008-04-08 15:23:31 PDT
I'd leave them both open for now, until we dig into more gritty details.
Comment 20 Marcia Knous [:marcia - use ni] 2008-05-08 11:51:57 PDT
Just as a point of info it happens when not only with Print to PDF, but print too so I am updating the summary of this bug.
Comment 21 Steven Michaud [:smichaud] (Retired) 2008-05-14 15:16:17 PDT
Bug 433778 is probably related ... though it isn't a dup.
Comment 22 Henrik Skupin (:whimboo) 2008-05-15 15:07:01 PDT
(In reply to comment #21)
> Bug 433778 is probably related ... though it isn't a dup.

Perhaps Masayuki can help here...
Comment 23 Mats Palmgren (:mats) 2008-05-27 07:56:16 PDT
*** Bug 435864 has been marked as a duplicate of this bug. ***
Comment 24 Samir Unni 2008-06-06 06:55:27 PDT
OS X 10.5.3 here, with FF3RC2, and when I click Print, then Cancel, I have to either create a new Firefox window or switch to another application, or open a Firefox sub-window (preferences, open a file, etc). Switching tabs doesn't do anything.
Comment 25 Thomas K. (:tom) 2008-06-06 15:10:33 PDT
(In reply to comment #24)
> OS X 10.5.3 here, with FF3RC2, and when I click Print, then Cancel, I have to
> either create a new Firefox window or switch to another application, or open a
> Firefox sub-window (preferences, open a file, etc). Switching tabs doesn't do
> anything.
> 

Switching tabs doesn't because all existing tabs have the bug.  Try it again, but this time after hitting cancel, do Command-T to open a new tab.  I think that, for some reason, re-enabled the menu items (at least for me).
Comment 26 Marcia Knous [:marcia - use ni] 2008-06-06 15:32:24 PDT
Another way to get the menu item re-enabled is to switch focus outside the Firefox window. When you focus back on the app you will again have the "print" menu available. Just a workaround until we get this bug fixed...
Comment 27 Marco De Vitis 2008-06-14 08:24:35 PDT
Just for the record, this bug is still here in FF3RC3 on OSX 10.5.3 (Intel).

It "simply" looks like menus are not reset after the Print dialog is closed. If you open the Print dialog and look at the menus while the focus is on it, you will see that many menu items are grayed out, which is probably correct, as you cannot do any browsing-related operation on the Print dialog. But the problem is that these grayed out menus stay the same even after closing the Print dialog somehow (hitting Cancel, printing, etc.).
Comment 28 Steven Michaud [:smichaud] (Retired) 2008-06-16 09:20:40 PDT
*** Bug 438790 has been marked as a duplicate of this bug. ***
Comment 29 Marcia Knous [:marcia - use ni] 2008-06-19 09:58:34 PDT
*** Bug 431952 has been marked as a duplicate of this bug. ***
Comment 30 Phil Ringnalda (:philor) 2008-06-19 13:40:34 PDT
*** Bug 440507 has been marked as a duplicate of this bug. ***
Comment 31 Steven Michaud [:smichaud] (Retired) 2008-06-26 16:21:31 PDT
*** Bug 441513 has been marked as a duplicate of this bug. ***
Comment 32 Phil Ringnalda (:philor) 2008-06-29 09:50:46 PDT
*** Bug 442556 has been marked as a duplicate of this bug. ***
Comment 33 José Jeria 2008-07-04 05:55:25 PDT
*** Bug 443598 has been marked as a duplicate of this bug. ***
Comment 34 Ryan Lovett 2008-07-08 13:13:53 PDT
We also see this bug. Mac OS X 10.5.3 on PPC using 3.0.
Comment 35 philippe (part-time) 2008-07-10 16:33:27 PDT
*** Bug 444643 has been marked as a duplicate of this bug. ***
Comment 36 Ron Lockhart 2008-07-19 10:11:59 PDT
I am seeing this same problem on 10.5.4. Going to Page Setup will then gray out the Page Setup and Print options in the File menu.
Comment 37 Thomas K. (:tom) 2008-07-19 11:10:46 PDT
Please do not comment that you still see this bug.  It is known to exist and with the large amount of duplicates, it is confirmed that many people are seeing it.  It is confirmed.  Commenting that you still see it e-mails all 'subscribed' to this bug with 'bugspam'.  It will not cause the bug to become fixed any sooner.  Patches, however, are welcome.
Comment 38 philippe (part-time) 2008-07-31 19:56:52 PDT
*** Bug 448731 has been marked as a duplicate of this bug. ***
Comment 39 Phil Ringnalda (:philor) 2008-08-07 09:10:02 PDT
*** Bug 449559 has been marked as a duplicate of this bug. ***
Comment 40 Henrik Skupin (:whimboo) 2008-08-11 10:24:44 PDT
*** Bug 450109 has been marked as a duplicate of this bug. ***
Comment 41 philippe (part-time) 2008-08-20 00:12:26 PDT
*** Bug 451319 has been marked as a duplicate of this bug. ***
Comment 42 Phil Ringnalda (:philor) 2008-09-04 17:39:26 PDT
*** Bug 453731 has been marked as a duplicate of this bug. ***
Comment 43 Marcia Knous [:marcia - use ni] 2008-09-23 14:54:08 PDT
*** Bug 456610 has been marked as a duplicate of this bug. ***
Comment 44 RJS 2008-09-23 15:14:00 PDT
As one of the people who reported this, I am more interested in understanding when we will see a version that has the fix for it. When is a new release due? RJ
Comment 45 Thomas K. (:tom) 2008-09-23 15:18:04 PDT
You won't see a version that has a fix for it until at least one day after someone has submitted a patch for it.
Comment 46 RJS 2008-09-23 15:58:30 PDT
Is anyone working on a patch for it?

How do I get access to a patch when it is available?
Comment 47 Thomas K. (:tom) 2008-09-23 16:01:00 PDT
(In reply to comment #46)
> Is anyone working on a patch for it?
> 
> How do I get access to a patch when it is available?

Yes, and just follow bug 456646 and wait until it has been marked fixed.  That means that the patch has been pushed into the repository and nightlies are being built from it.  Please refrain from needlessly commenting on the bug, as this creates extra bugmail for everyone on the CC list.  Thanks.
Comment 48 Ryan Lovett 2008-09-23 16:46:57 PDT
> Please refrain from needlessly commenting on the bug, as this creates extra bugmail for everyone on the CC list.

This wasn't "needless commenting", it was informative followup. Thanks RJS for your questions and Thomas for your answers. As someone on the CC list I'm glad there has been some progress.

Ryan
Comment 49 Thomas K. (:tom) 2008-09-23 16:51:12 PDT
(In reply to comment #48)
> This wasn't "needless commenting", it was informative followup.

Ryan, I was referring to bug 456646, because he said:
>I split this work into a new bug to get a better tracking.

He obviously created the new bug to make it easier to resolve, as the only comments should be ones related to the patch and not related to a new dupe or questions about when the bug will be resolved.

Sorry for the confusion.
Comment 50 Dave Townsend [:mossop] 2008-10-19 09:47:09 PDT
*** Bug 460671 has been marked as a duplicate of this bug. ***
Comment 51 RJS 2008-10-20 00:02:38 PDT
Have tracked through the 50 comments. (My comments were no.s 44 and 46.)

I am not familiar with the protocols re bug fixes, as I am just a user of Firefox, not a developer.

Is anyone working on the fix? Have looked at 456646 and there seems no progress there.

When this item is fixed where do I go to get an updated version?

Thanks

RJ
Comment 52 Wayne Mery (:wsmwk, NI for questions) 2008-10-20 03:40:26 PDT
(In reply to comment #51)
> Have tracked through the 50 comments. (My comments were no.s 44 and 46.)
> 
> I am not familiar with the protocols re bug fixes, as I am just a user of
> Firefox, not a developer.
> 
> Is anyone working on the fix? Have looked at 456646 and there seems no progress
> there.

your answer is partly in comment 45 and 47 - they get fixed when someone has the time and the insight to code the solution. It could be hours, it could be weeks - asking when generally doesn't improve the odds of it happening sooner.


> When this item is fixed where do I go to get an updated version?

when the patch lands, the next day it will be in ftp://ftp.mozilla.org/pub/firefox/nightly/latest-trunk/
Comment 53 Dave Townsend [:mossop] 2008-11-07 14:02:16 PST
*** Bug 463713 has been marked as a duplicate of this bug. ***
Comment 54 Marcia Knous [:marcia - use ni] 2008-11-19 13:48:49 PST
*** Bug 465745 has been marked as a duplicate of this bug. ***
Comment 55 Marcia Knous [:marcia - use ni] 2008-11-19 13:53:11 PST
*** Bug 458054 has been marked as a duplicate of this bug. ***
Comment 56 philippe (part-time) 2008-12-01 03:08:00 PST
*** Bug 467327 has been marked as a duplicate of this bug. ***
Comment 57 RJS 2008-12-01 13:01:14 PST
Lots of items marked as duplicates - the duplicates showing many people are seeing the problem. But no one has done anything to fix it. (I would, but have no technical skills.)

Is there a work around?
Comment 58 David Humphrey (:humph) 2008-12-01 13:05:02 PST
> Is there a work around?

Sure, don't use the menu to print: cmd+p
Comment 59 Thomas K. (:tom) 2008-12-01 13:16:49 PST
(In reply to comment #58)
> > Is there a work around?
> 
> Sure, don't use the menu to print: cmd+p

Or see the workaround in comment 26, mentioned in the whiteboard, and the request not to say "I still see this and nobody's posted a workaround" in comment 37, also mentioned in the whiteboard.
Comment 60 Phil Ringnalda (:philor) 2008-12-11 01:05:16 PST
*** Bug 469088 has been marked as a duplicate of this bug. ***
Comment 61 Steven Michaud [:smichaud] (Retired) 2009-01-22 16:07:11 PST
*** Bug 474764 has been marked as a duplicate of this bug. ***
Comment 62 John Daggett (:jtd) 2009-01-23 00:58:24 PST
This bug is actually a lot more serious than would appear at first glance.  There are actually two parts to this bug: (1) while the Page Setup or Print dialogs are up, many menu items are enabled which shouldn't be and (2) once the dialog is dismissed, the state of menus remains in the the same state it was in while those dialogs were up.  The first part is cosmetic, nothing bad happens, but the second is why so many folks have logged dupes on this, because it hinders functionality.

So it's not just some items in the File menu, but *most* menu items are out of sync:

    File >
      Close               enable ==> disable
      Save Page As        enable ==> disable
      Send Link           enable ==> disable
      Page Setup          enable ==> disable
      Print               enable ==> disable
    
    Edit >
      Undo                disable ==> enable (WTF?!?)
      Redo                disable ==> enable
      Cut                 disable ==> enable
      Paste               disable ==> enable
      Delete              disable ==> enable
      Find                disable ==> enable
      Find Again          disable ==> enable
      
    View >
      *all*               enable ==> disable
      
    Bookmarks >
      Bookmark This Page  enable ==> disable
      Bookmark All Tabs   enable ==> disable
      
    Tools >
      Page Info           enable ==> disable
      
    Window >
      Minimize            enable ==> disable
      Zoom                enable ==> disable
  
We currently call the Carbon printing API's PMSessionPageSetupDialog and PMSessionPrintDialog, which is a bit odd in a Cocoa app.  So I'm sure the right approach is to move to Cocoa printing API's as Josh suggests.  However that's a big, out-of-scope task for either 3.1 or a 3.0 hotfix.  So I would suggest this, that we call a simple utility method that iterates over menu bar items and assures that they are in sync with the content, similar to what's going on here:

http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsMenuItemX.mm#357

Call this method directly or from an event here:

http://mxr.mozilla.org/mozilla-central/source/embedding/components/printingui/src/mac/nsPrintingPromptServiceX.mm#349

and here:

http://mxr.mozilla.org/mozilla-central/source/embedding/components/printingui/src/mac/nsPrintingPromptServiceX.mm#487

I really, really think this needs to block for 1.9.1.

Screenshot of menus before and after bringing up the Page Setup dialog (before = top, after = bottom):

http://people.mozilla.org/~jdaggett/menusbeforeafter.png

Steps to reproduce:

1. Go to any page with text
2. Select a word
3. File > Page Setup
4. Click Cancel

Result: Menus change state from before to after as in the screenshot

Expected result: Menus should return to their original state
Comment 63 Henrik Skupin (:whimboo) 2009-01-23 01:39:48 PST
John, please also see bug 456646, which handles the switch from Carbon to Cocoa.
Comment 64 Samuel Sidler (old account; do not CC) 2009-01-23 08:40:09 PST
Re-nominating based on the workaround proposal in comment 62.
Comment 65 Marcia Knous [:marcia - use ni] 2009-01-23 11:27:14 PST
I agree with Comment 62. In my daily work, this is probably the most annoying Mac bug I see consistently and the one most often mentioned by Hendrix mac users.
Comment 66 John Daggett (:jtd) 2009-01-25 17:44:39 PST
I tracked down the change that caused this problem, it was the patch checked in to fix bug 372571, 2008-03-26 20:42 (josh).

To reproduce this, pull CVS trunk with MOZ_CO_DATE="26 Mar 2008 20:35 PDT".  Build and the problem does not occur.  Apply the patch for 372571 and it occurs.

This jives with the appearance of this bug in nightly builds, I came up with a slightly different regression window than the one suggested in comment 3 (and Philippe suggested it might be this bug in comment 12):

Bug does not occur: 2008-03-26-04 (4:47 AM)
Bug occurs:         2008-03-27-04 (4:49 AM)

Code changes during this time period:

http://tinyurl.com/macmenubugchanges
Comment 67 John Daggett (:jtd) 2009-01-25 19:26:23 PST
Created attachment 358791 [details] [diff] [review]
utterly lame patch, v.0.1, bracket calls to print dialog code with menu fixup

I don't know why this works but taking the two new routines from bug 372571, PrepareForNativeAppModalDialog and CleanUpAfterNativeAppModalDialog in nsCocoaUtils, and bracketing the calls to Carbon print dialog routines seems to eliminate the problem.  I stuck access methods for these in nsPrintSettingsX because I'm not quite sure how to call widget code from within the printgui component.  Lameness supreme...

Josh and/or Steven, can you take a look and let me know what you think the correct way of fixing this is?
Comment 68 Olli Pettay [:smaug] 2009-01-26 01:00:23 PST
Comment on attachment 358791 [details] [diff] [review]
utterly lame patch, v.0.1, bracket calls to print dialog code with menu fixup

>+++ b/widget/public/nsIPrintSettingsX.idl
You should update the uuid
..and remove printfs ;)
Comment 69 Josh Aas 2009-01-26 10:07:00 PST
Comment on attachment 358791 [details] [diff] [review]
utterly lame patch, v.0.1, bracket calls to print dialog code with menu fixup

I tried something like this a while ago and it had no effect - the menu preparation did nothing for Carbon dialogs. I may have missed a spot or made some other mistake though.

Please remove the debugging printfs and rev the uuid like smaug said.

+  /* terribly rude hack */ 

Please remove this comment. This is how things are supposed to work given the way XUL and Mac OS X interact.

+  [noscript] void PrepareForNativeAppModalDialog();
+  [noscript] void CleanUpAfterNativeAppModalDialog();

The first character of method names should not be capitalized in idl files. See methods above the ones you added.

Once that is taken care of, if the patch works in your testing I'm fine with the code. This needs to be tested on Mac OS X 10.4 and 10.5 before landing - if you don't have one of those versions to test on let us know, smichaud can probably help you out.
Comment 70 Josh Aas 2009-01-26 10:11:34 PST
blocking1.9.1+ so long as the approach in John Daggett's patch works. With a workable strategy we should not ship with this bug. However, if John's patch doesn't work out and we get back to rewriting our print dialogs in Cocoa we're going to have to minus this bug since that changes is unacceptable for 1.9.1 at this point.
Comment 71 John Daggett (:jtd) 2009-01-26 20:21:52 PST
This is not strictly dependent on Cocoa printing dialogs, bug 456646, removing the dependency.
Comment 72 John Daggett (:jtd) 2009-01-26 21:05:32 PST
Created attachment 358993 [details] [diff] [review]
patch, v.0.2, cleanup and remove debugging code

Cleaned up version of the original patch.  It would be nice to have a better place to put this but leaving it as is for now.

Tested on 10.5, the graying out behavior is gone.  Although the code uses code simiilar to the code used to display an Edit menu when opening a file, this menu doesn't appear.  So Edit menu behavior remains broken, same as before.
Comment 73 John Daggett (:jtd) 2009-01-26 22:30:03 PST
Tested on 10.4.  Before the patch when selecting Page Setup/Print, the entire menu bar would appear but edit controls (cut/copy/paste) would not work.  With this patch a single Edit menu appears and edit controls are usable to some degree but not all.  So the behavior is better, but there are still problems with edit controls that need to get fixed.  But that's a much smaller problem than menu items being out of whack on 10.5.

Try server build available here for those who want to test:

http://tinyurl.com/fixmenudisable
Comment 74 Henrik Skupin (:whimboo) 2009-01-27 03:46:09 PST
John, I've tested the tryserver build and it looks great on 10.5. Everything is accessible after the the print dialog was open. All menus look fine.
Comment 75 Delete 2009-01-27 09:16:20 PST
I have tested the tryserver build with 10.5. (10.5.6) as well and had some strange hangs with Print and Save Page, which were not reproducible. But this one is reproducible now. Before was running FF 3.0.5. Start build, open a webpage and try Save Page as ... 
Build hangs after some sec. and uses 99% Cpu.
Comment 76 Samuel Sidler (old account; do not CC) 2009-01-27 10:26:01 PST
John, after this patch lands on m-c and 1.9.1, can you please work up a 1.9.0 patch?
Comment 77 John Daggett (:jtd) 2009-01-27 12:11:42 PST
(In reply to comment #75)
> I have tested the tryserver build with 10.5. (10.5.6) as well and had some
> strange hangs with Print and Save Page, which were not reproducible. But this
> one is reproducible now. Before was running FF 3.0.5. Start build, open a
> webpage and try Save Page as ... 
> Build hangs after some sec. and uses 99% Cpu.

This sounds like a separate bug.  Do you open the Save Page As dialog without choosing either Page Setup or Print first?  If so, this patch isn't the problem, this affects 3.1 beta builds also most likely.

Are these the steps you're using?

1. Start tryserver build
2. Open webpage
3. File > Save Page As

Result: build hangs and CPU goes to 99%

Is this what you're seeing?  (I'm on 10.4 right now, will test on 10.5 a little later).
Comment 78 John Daggett (:jtd) 2009-01-27 12:37:46 PST
(In reply to comment #76)
> John, after this patch lands on m-c and 1.9.1, can you please work up a 1.9.0
> patch?

will do.
Comment 79 John Daggett (:jtd) 2009-01-27 15:15:33 PST
Testing on 10.5, can't reproduce the behavior noted in comment 72.
Comment 80 John Daggett (:jtd) 2009-01-27 17:04:59 PST
Created attachment 359175 [details] [diff] [review]
patch, v.0.3, simpler version, just paint main window menu bar

I think this is a much simpler version of the fix, namely to simply repaint the menu bar of the main window after calling either the page setup or print dialogs:

  NSWindow* mainWindow = [NSApp mainWindow];
  if (mainWindow)
    [WindowDelegate paintMenubarForWindow:mainWindow];

This fixes the menu state problem on 10.5 and leaves 10.4 behavior completely unaffected.  It's effectively the equivalent of the workarounds mentioned, to switch to another window and then back again to restore the menu state.
Comment 81 Delete 2009-01-27 17:24:31 PST
(In reply to comment #77)
It's true, John, it is a seperate bug. I checked this bug and it appears only if there are more than two files to store in sub folder. It doesn't matter if these are include files (js, css...) or images.
There is no bug reported at this time, I guess. Could be this a bug only in this build? And/or under which version it should be reported?
Comment 82 John Daggett (:jtd) 2009-01-27 17:35:52 PST
Yeah, my guess is you've found a bug in 3.1/3.2 code that's not in 3.0.  The try server build is equivalent to using the latest nightly build, so try your test with a latest trunk nightly:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

If you run into a problem with one of these builds, log a bug with version "Trunk" and include the build number (<app> menu > About Minefield).  Be sure to include steps to reproduce, sounds like you're seeing a problem when saving a specific type of page, right?
Comment 83 John Daggett (:jtd) 2009-01-27 18:53:57 PST
Try server build with latest patch for those who want to test:

http://tinyurl.com/fixmenudisablev03
Comment 84 philippe (part-time) 2009-01-27 19:50:45 PST
(In reply to comment #82)
> Yeah, my guess is you've found a bug in 3.1/3.2 code that's not in 3.0.  The
> try server build is equivalent to using the latest nightly build, so try your
> test with a latest trunk nightly:
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
> 
> If you run into a problem with one of these builds, log a bug with version
> "Trunk" and include the build number (<app> menu > About Minefield).  Be sure
> to include steps to reproduce, sounds like you're seeing a problem when saving
> a specific type of page, right?

it is a known issue - and already fixed: bug 475078.
Comment 85 Josh Aas 2009-01-28 07:19:22 PST
Comment on attachment 359175 [details] [diff] [review]
patch, v.0.3, simpler version, just paint main window menu bar

>+   * After bringing up Carbon print dialogs, clean up menus.
>+   */
>+  [noscript] void cleanUpAfterDialog();
>+  

Lets call this "cleanUpAfterCarbonDialog" to make it clear when this is to be used. Please note a bug number in the comment for this new method. Get rid of the extra newline added after the method.

>+NS_IMETHODIMP nsPrintSettingsX::CleanUpAfterDialog()
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
>+
>+  NSWindow* mainWindow = [NSApp mainWindow];
>+  if (mainWindow)
>+    [WindowDelegate paintMenubarForWindow:mainWindow];
>+  
>+  NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(noErr);
>+}

The exception code here is returning "noErr", a Carbon return value for no error, but the method signature says it returns an nsresult value. Change "NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(noErr)" to "NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT".

Also, we probably want to paint the menu bar for the hidden window if there is no main window after the dialog closes. See the logic for that in "nsCocoaUtils::CleanUpAfterNativeAppModalDialog()". The hidden window is the window that we use when there appears to be no windows open in a Gecko application.
Comment 86 John Daggett (:jtd) 2009-01-28 17:14:58 PST
Created attachment 359423 [details] [diff] [review]
patch, v.0.4, changes based on review comments
Comment 88 John Daggett (:jtd) 2009-01-29 00:55:57 PST
Created attachment 359487 [details] [diff] [review]
patch, v.0.4a, patch for 3.0 cvs trunk

1.9.0 patch, as per comment 76.  die bug die
Comment 89 John Daggett (:jtd) 2009-01-29 02:37:55 PST
Created attachment 359495 [details] [diff] [review]
patch, v.0.4b, fixup

adjusted to changed location for hidden menu routine
Comment 90 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-01-29 14:43:27 PST
John-san, you must not change any interfaces on stable branch. You need to create a new interface for the new APIs.
See following patch:
https://bugzilla.mozilla.org/attachment.cgi?id=217349&action=diff
Comment 91 Vladimir Vukicevic [:vlad] [:vladv] 2009-01-29 15:45:56 PST
(In reply to comment #90)
> John-san, you must not change any interfaces on stable branch. You need to
> create a new interface for the new APIs.
> See following patch:
> https://bugzilla.mozilla.org/attachment.cgi?id=217349&action=diff

It should be fine to change the stable interface provided the UUID is revved and/or that the new function is added to the end.  I think both things are being done here, so it should be fine.
Comment 92 Henrik Skupin (:whimboo) 2009-01-29 16:02:07 PST
Thanks John! I think we can remove relnote now.

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090129 Minefield/3.2a1pre ID:20090129020336

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090129 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090129021345
Comment 93 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-01-29 16:28:58 PST
(In reply to comment #91)
> (In reply to comment #90)
> > John-san, you must not change any interfaces on stable branch. You need to
> > create a new interface for the new APIs.
> > See following patch:
> > https://bugzilla.mozilla.org/attachment.cgi?id=217349&action=diff
> 
> It should be fine to change the stable interface provided the UUID is revved
> and/or that the new function is added to the end.  I think both things are
> being done here, so it should be fine.

Really? I was not allowed in the bug (see bug 187772 comment 31).
Comment 94 Vladimir Vukicevic [:vlad] [:vladv] 2009-01-29 17:06:18 PST
In that particular case, it's likely because nsIRollupListener was added to the concrete implementation class' inheritance tree -- which may or may not have changed the concrete class layout.  I think it still should have been fine though, as people should have been QI'ing anyway to get the other implementations; mconnor was just being overzealous, I guess.

But, I don't know for sure -- would need a call from the 1.9.0 drivers, though.  To me, it would be fine to just add it at the end.
Comment 95 Samuel Sidler (old account; do not CC) 2009-01-29 17:38:32 PST
We have never allowed interface changes on stable branches under any circumstances.
Comment 96 John Daggett (:jtd) 2009-02-01 18:23:03 PST
Created attachment 360034 [details] [diff] [review]
patch, v.0.4c, create 1.9.0 branch version

Same patch but moved into a separate interface for 1.9.0.x branch code.
Comment 97 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-02-02 08:35:04 PST
nsIPrintSettingsX_MOZILLA_1_9_BRANCH should be inherited nsIPrintSettingsX. Then:

-NS_IMPL_ISUPPORTS_INHERITED1(nsPrintSettingsX, 
+NS_IMPL_ISUPPORTS_INHERITED2(nsPrintSettingsX, 
                              nsPrintSettings, 
-                             nsIPrintSettingsX)
+                             nsIPrintSettingsX,
+                             nsIPrintSettingsX_MOZILLA_1_9_BRANCH)

can be:

 NS_IMPL_ISUPPORTS_INHERITED1(nsPrintSettingsX, 
                              nsPrintSettings, 
-                             nsIPrintSettingsX)
+                             nsIPrintSettingsX_MOZILLA_1_9_BRANCH)

And when a client gets nsIPrintSettingsX_MOZILLA_1_9_BRANCH interface, it can also use nsIPrintSettingsX interface. So, you can reduce to query interface in nsPrintingPromptServiceX.mm.
Comment 98 Josh Aas 2009-02-02 08:50:28 PST
Comment on attachment 360034 [details] [diff] [review]
patch, v.0.4c, create 1.9.0 branch version

Masayuki is probably right, waiting for a response from John.
Comment 99 Josh Aas 2009-02-02 08:52:34 PST
Comment on attachment 360034 [details] [diff] [review]
patch, v.0.4c, create 1.9.0 branch version

I'm actually fine either way, though please try what Masayuki suggested. The rest of this looks fine to me.
Comment 100 Olli Pettay [:smaug] 2009-02-02 09:15:35 PST
(In reply to comment #97)
> nsIPrintSettingsX_MOZILLA_1_9_BRANCH should be inherited nsIPrintSettingsX.
Could or should, right.

> Then:
> 
> -NS_IMPL_ISUPPORTS_INHERITED1(nsPrintSettingsX, 
> +NS_IMPL_ISUPPORTS_INHERITED2(nsPrintSettingsX, 
>                               nsPrintSettings, 
> -                             nsIPrintSettingsX)
> +                             nsIPrintSettingsX,
> +                             nsIPrintSettingsX_MOZILLA_1_9_BRANCH)
> 
> can be:
> 
>  NS_IMPL_ISUPPORTS_INHERITED1(nsPrintSettingsX, 
>                               nsPrintSettings, 
> -                             nsIPrintSettingsX)
> +                             nsIPrintSettingsX_MOZILLA_1_9_BRANCH)
This is not true. This would mean the the object doesn't support nsIPrintSettingsX, but it supports only nsIPrintSettingsX_MOZILLA_1_9_BRANCH.
QueryInterfacing to nsIPrintSettingsX would fail.
Comment 101 Daniel Veditz [:dveditz] 2009-02-02 15:23:59 PST
Comment on attachment 360034 [details] [diff] [review]
patch, v.0.4c, create 1.9.0 branch version

Approved for 1.9.0.7, a=dveditz for release-drivers.

>+interface nsIPrintSettingsX_MOZILLA_1_9_BRANCH : nsISupports

subclassing nsIPrintSettingsX instead of nsISupports would have accomplished what comment 97 was getting at. Olli's right about the macros, though.
Comment 102 John Daggett (:jtd) 2009-02-02 20:16:42 PST
I'm going to leave this as-is.  I can definitely see the reason for subclassing vs. using a mix-in class but this is a very simple piece of code on a branch, I don't see any compelling reason to add more complexity in the form of subclasses to the problem.
Comment 103 John Daggett (:jtd) 2009-02-02 20:18:16 PST
Checked in to CVS trunk
Comment 104 Henrik Skupin (:whimboo) 2009-02-04 13:47:09 PST
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009020404 GranParadiso/3.0.7pre ID:2009020404
Comment 105 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-02-04 22:09:28 PST
(In reply to comment #101)
> subclassing nsIPrintSettingsX instead of nsISupports would have accomplished
> what comment 97 was getting at. Olli's right about the macros, though.

Oh, right. I didn't do so in bug 187772, sorry.
Comment 106 philippe (part-time) 2009-02-15 04:34:17 PST
*** Bug 478620 has been marked as a duplicate of this bug. ***
Comment 107 Henrik Skupin (:whimboo) 2009-02-28 22:56:07 PST
Marcia, as you have said this is not fixed for 10.6. Is this still true?
Comment 108 Philippe Villoz 2009-03-01 00:27:44 PST
Firefox is currently the only application on MacOSX Intel 10.5.6 that have this bug.
Not be able to print in 2009 is very poor and we will switch back to Safari and forget Firefox.
Free application must not be buggy
Comment 109 Henrik Skupin (:whimboo) 2009-03-01 11:15:29 PST
(In reply to comment #108)
> Firefox is currently the only application on MacOSX Intel 10.5.6 that have this
> bug.

This will be fixed in the next beta version of Firefox 3.1 and for Firefox 3.0.7. Please stay tuned.
Comment 110 Marcia Knous [:marcia - use ni] 2009-03-02 13:43:29 PST
I will check 10.6 the next time I am in the lab and report back.

Note You need to log in before you can comment on or make changes to this bug.