subject says it all

-> 098

not gonna happen in 098

This didn't seem to hard to fix so I attempted to write a patch. This patch includes an updated patch for bug 99987.

comments on the patch: + ::SetWindowClass(mWindowPtr, kSheetWindowClass); // we really shouldn't call this because it is deprecated if we should be using SetWindowGroup, let's just make that change now. it's available in carbonlib 1.4 right? + OSStatus err = ::InstallWindowEventHandler ( mWindowPtr, NewEventHandlerUPP(WindowEventHandler), + GetEventTypeCount(windEventList), windEventList, this, NULL ); what is GetEventTypeCount()? I couldn't find it in apple headers. + if ( myWind && self) { fix minor spacing. |if ( myWind && self ) {| - case kEventWindowDrawContent: rjc put this in for sheets. should you be removing it? + retVal = noErr; can you go ahead also and comment that returning noErr means we consume the event? just makes it crystal clear and follows the pattern already set up in that routine in the other cases. Yah, it's redundant, but with so many people tromping through this code, i'd err on the side of avoiding confusion. looking good otherwise. cc'ing rjc re: the sheet changes since i saw another patch of his this morning that touches this part of the code.

If the case of this call to ::SetWindowClass(), I think we actually want to set the class of the window, not the group, so using the window group apis would not be appropriate. Class is supposed to be an immutable property, so the only other way to set is to create the window using, the new window creation function -- CreateNewWindow(). GetEventTypeCount() is a macro defined in CarbonEvents.h as follows: #define GetEventTypeCount( t ) (sizeof( (t) ) / sizeof( EventTypeSpec )) According to my understanding of the comments CarbonEvents.h, kEventWindowDrawContent is only used if the window has the kWindowStandardHandlerAttribute. The two events need to be handled slightly differently. kEventWindowUpdate requires that we set the port and call BeginUpdate() and EndUpdate() while kEventWindowDrawContent doesn't. If we ever switch to using the standard handler, we should replace kEventWindowUpdate with kEventWindowDrawContent. I put a comment in that tries to explain this.

re: GetEventTypeCount. I must have missed it last time i looked, now i see it. thanks. ok on the SetWindowClass, but if that api has been depricated, how are we supposed to get the same functionality? i'll let rjc chime in on the drawing carbon events. I understand the difference, just he knows the exact details on why he had to include them.

As far as I know, the only other way to set the window class is to do so at creation time using the new creation function: /* * CreateNewWindow() * * Availability: * Non-Carbon CFM: in WindowsLib 8.5 and later * CarbonLib: in CarbonLib 1.0 and later * Mac OS X: in version 10.0 and later */ EXTERN_API( OSStatus ) CreateNewWindow( WindowClass windowClass, WindowAttributes attributes, const Rect * contentBounds, WindowRef * outWindow); This makes sense because you are not supposed to be able to change the class once the window is created.

Regarding SetWindowClass, it appears to me that you're already doing the right thing. mIsSheet is only true for windows that were created as sheets in the first place: if (nsToolkit::OnMacOSX() && aParent && (aInitData->mBorderStyle & eBorderStyle_sheet)) { nsWindowType parentType; aParent->GetWindowType(parentType); if (parentType != eWindowType_invisible) { // Mac OS X sheet support mIsSheet = PR_TRUE; wDefProcID = kWindowSheetProc; } } [...] mWindowPtr = ::NewCWindow(nil, &wRect, "\p", false, wDefProcID, (WindowRef)-1, goAwayFlag, (long)nsnull); Try taking out the SetWindowClass call entirely, and see if everything still works? I'm pretty sure it will... Regarding DrawContent vs WindowUpdate, yeah, you really want support DrawContent instead of WindowUpdate. On the other hand, you really *really* want to create your windows with the standard handler. That'll happen eventually, even if you're not planning on it yet, because the standard handler is a Good Thing. If sheets work now, then great. If they don't, you're going to have to untangle DrawContent/WindowUpdate, else switch over to the standard handler now and use DrawContent only. If there are problems now, I'd most expect them on windows that magically got the standard handler installed without your realizing it. Be sure to check the open/save and pagesetup/print, plus various alerts, etc.

In response to comment 8: We know that SetWindowClass() is being used correctly in the case that we are discussing. It's just that that function is deprecated, and we're thinking of another way to do the same thing. We can't just remove the call to SetWindowClass() because rjc's sheet patch uses GetFrontWindowOfClass(). As for the standard handler, I know that it is not yet being installed and shouldn't be yet until we move farther along in the CarbonEvent transition.

> cc'ing rjc re: the sheet changes since i saw another patch of his > this morning that touches this part of the code. Pink was referring to bug # 122102 (resizable sheets) As for getting rid of ::SetWindowClass() usage, just do this: #if TARGET_CARBON if (mIsSheet) { WindowAttributes attribs = kWindowNoAttributes; if (resizable) attribs |= kWindowResizableAttribute; ::CreateNewWindow(kSheetWindowClass, attribs, &wRect, &mWindowPtr); } else #endif mWindowPtr = ::NewCWindow(nil, &wRect, "\p", false, wDefProcID, (WindowRef)-1, goAwayFlag, (long)nsnull); and then you can remove the ::SetWindowClass() from farther below. > According to my understanding of the comments CarbonEvents.h, >kEventWindowDrawContent is only used if the window has the > kWindowStandardHandlerAttribute. kEventWindowDrawContent IS called for sheets without our setting the standard handler (unless its getting set by the OS behind our backs).

You're right. Apple's header docs are not quite right. We are getting kEventWindowDrawContent for sheets but not other windows even though neither have the kWindowStandardHandlerAttribute. As for CreateNewWindow(), we might as well use it for all windows, not just sheets especially since it's available in classic in WindowsLib 8.5.

Feel free, although it'll entail reworking a chunk of nsMacWindow.cpp from the looks of things. (Perhaps that would be best done in a different bug, if you want.)

A revised patch incorperating some of the feedback. (It doesn't deal with the SetWindowClass() issue) It incorperates the resizable sheet patch.

WEIRD: Handling kEventWindowDrawContent causes bug 121667.

*** Bug 121667 has been marked as a duplicate of this bug. ***

*** Bug 121671 has been marked as a duplicate of this bug. ***

Comment on attachment 66828 [details] [diff] [review] revised patch i played around with the patch this morning and found the following: - it doesn't compile as-is. I needed to add another ifdef for DoUpdate in the message pump - with the patch applied, sheets would flash (disappeaar then reappear) before rolling up. Turning off the activate/update ifdefs didn't solve it. Backing it out did. - with the patch applied, cmd keys wouldn't work when clicking a window to activate it. I had to click _again_ to get cmd keys to work. Turning off activate/update ifdefs didn't solve it. Backing it out did. So there seem to be some window activation issues with this patch, but not only that, the ifdefs don't quite work as they should.

over to dagley, i probably won't have time for these by 099 with the embedding work i'm getting pulled into.

-> mozilla1.0

ajfeldman, since you seem to have been the one working on this, any update? Is this something we can land for Moz1.0?

I have a patch that addresses the build problems and flashing sheets mentioned in comment 17. The flashing sheets seem to be caused by the fact that for an instant the parent window is being placed in front of the sheet as it is rolling up. However, I can't verify the cmd keys issue because I'm working from the mach-o build and cmd keys are pretty broken in that build to begin with. Lastly, I'm getting a lot of thread-safety warnings whenever a sheet rolls up. Since bug bug 121671 and bug 122102 are not fixed by simply switching to carbon update events as we had previously hoped, this is probably not worth doing for 1.0 unless we can quickly resolve the above issues.

fwiw, i saw the same kinds of "flashing" of sheets (comment 17) when closing them in OfficeX. It could just be carbon bugs we're running into and not something inherently wrong with the patch.

may be needed for embedding, marking topembed. Needs investigation

I agree, sheets in Mac OfficeX absolutely suck. We don't currently have any flashing sheet problems on Mac OS X, let's keep it that way.

Please note that the few lines of code to support resizable sheets on Mac OS X has been checked in (see bug # 130218)

It's been a while since there have been any comments on these Carbon Events bugs. Is there any work being done here?

Since I don't report into Internet Technologies anymore this bug needs a new owner -> saari

I wasn't really going to do this, until I noticed that we were doing too much during each activate event: we'd handle it once on a window-scope CE handler, and again on the application-scope WNE [transitional] handler. So I fixed them to be handled by the window-scope handler only, finding a system foible in the process - that's what the nsWindow part of the patch is about. As long as I was in there, I reworked activate/deactivate events to be handled by window-scope CE handlers. The IgnoreDeactivate stuff is disappearing because HiliteWindow isn't necessary anyway. The system does proper hilighting when you leave it alone. The frontmost window is activated and deactivated when the application is activated and deactivated, so the suspendResumeMessage stuff to handle application-scope activation and deactivation is unnecessary. We don't need to keep track of app-active under OS X anyway (and keeping track was buggy, since we'd assume that the app was active at launch, which isn't necessarily the case, especially with all the self-restarting...) This is the 1.8 patch. The trunk patch is the same except it gets rid of the ignoreDeactivate attribute from nsPIWidgetMac - can't do that on the branch because of interface rules.

Comment on attachment 223080 [details] [diff] [review] Patch (1.8) Rollup not happening on app deactivate?

The existing rollup code for window deactivates was dysfunctional in that it had a check that prevented it from ever succeeding. This change makes it match what we do elsewhere. Because the rollup widget should only ever be attached to an active window, this is fine. This change fixes a related bug: if you have two windows open and open a contextual menu in the foreground window, it doesn't roll up when you press command-~ to rotate to the background window.

Same as the 1.8 version, adjusted to apply to the trunk. Also removes a dead method from the private interface.

Comment on attachment 223221 [details] [diff] [review] Patch v2 (trunk) sr=pink

Checked in on trunk.

Comment on attachment 223211 [details] [diff] [review] Patch v2 (1.8) Marking a? as a reminder for when the tree reopens.

Checked in on MOZILLA_1_8_BRANCH.

The fix for this bug has caused a regression at bug 341234. Here's a very simple patch that resolves the problem (it reverts a small part of nsMacEventHandler.cpp to the way it was before the fix for this bug (106695) was landed). I don't really understand why this patch fixes bug 341234 ... but fix it it does.

Comment on attachment 225593 [details] [diff] [review] Fix for bug 341234 (regression caused by this fix) Discussion of this patch and the regression it attempts to resolve in bug 341234.