Bug 513158 added drawing capabilities but didn't hook up event handling. But we need event handling for bug 625989. This patch implements it in the least risky way I could think of. It doesn't attempt to solve the last three problems listed in bug 513158 comment 3.

I wanted to start playing with this so first up was unbitrotting this (fixed up a comment referencing a method that no longer exists - not sure all comments apply about non-accelerated & whatnot)

To cut down on back/forward... (Markus Stange from bug 513158 comment #3) > However, I'm not yet sure how to implement it without a compromise: > As soon as we take over mouse event handling in the titlebar, we have to use > our own faked window-dragging implementation which we're currently using for > window-dragging with the toolbar (since bug 398928). This has several > drawbacks compared to Cocoa's native window dragging that's provided in the > titlebar: > - You can't drag background windows (bug 465590). Can be fixed. > - You can't move a window between spaces by dragging it (bug 465542). > As far as I know, not fixable, but I haven't looked very hard. > - You can't drag a window while the app is hung. Not fixable. > - You can't drag a window that contains an open sheet. Might be fixable, but > not easily. So a super naive way of fixing the remaining problems is to make mouseDownCanMoveWindow return YES - it makes the window draggable between spaces and movable when there's a sheet showing. The problem though is that this will apply even when we should be handling the event ourselves. So we don't handle dragging tabs correctly (well, dragging anything that falls in the titlebar, so the customize menu ends up screwed too). It looks like we can probably call setMovableByWindowBackground:YES/NO (hat tip: http://stackoverflow.com/questions/1694582/is-there-a-way-to-make-a-custom-nswindow-work-with-spaces) as needed, though that could get interesting - we'd only want to do that if we're not going to handle the event. Also right clicks result in too much recursion (JS), but that can be figured out a bit down the line. var el = window.top.opener.document.documentElement; el.setAttribute("drawintitlebar", "true"); el.style.paddingTop = "0";

I didn't mean to paste that bit of code in comment 2, but it's there... run it in the error console to see what draw in titlebar is like (affects the window the error console was opened from)

Comment on attachment 591556 [details] [diff] [review] wip (unbitrotted) > // register for things we'll take from other applications > PR_LOG(sCocoaLog, PR_LOG_ALWAYS, ("ChildView initWithFrame: registering drag types\n")); >- [self registerForDraggedTypes:[NSArray arrayWithObjects:NSFilenamesPboardType, >- NSStringPboardType, >- NSHTMLPboardType, >- NSURLPboardType, >- NSFilesPromisePboardType, >- kWildcardPboardType, >- kCorePboardType_url, >- kCorePboardType_urld, >- kCorePboardType_urln, >- nil]]; >+ [ChildView registerViewForDraggedTypes:self]; >+ Here's a small mistake: the removal of the long call above didn't make it into your updated patch.

> It looks like we can probably call setMovableByWindowBackground:YES/NO (hat > tip: > http://stackoverflow.com/questions/1694582/is-there-a-way-to-make-a-custom- > nswindow-work-with-spaces) as needed, though that could get interesting - > we'd only want to do that if we're not going to handle the event. That's a good idea! So we'd do that in response to mouse move events. We can check whether the system window dragging should take action using the MozMouseHittest / NS_MOUSE_MOZHITTEST event which is already used on Windows for exactly that purpose. (This requires a small change to #ifdefs in WindowDraggingUtils.jsm.) The only problem I can see is the case when we're not servicing events fast enough. For example, if the mouse moves from a draggable part in the title bar to a tab, and we don't process the mouse move event on the tab quickly enough, the window will still be draggable when the tab is clicked. The system doesn't wait for us to process events before it starts the window dragging. Conversely, if the mouse is moved from content to the titlebar too fast, trying to drag the window won't work if we haven't processed the mouse event early enough. Having to click a second time to start the drag will be frustrating. Maybe we can jump in with our custom window dragging in those cases. We just need to make sure that we don't use it when native window dragging is already happening. Anyway, this will probably need a bit of experimentation.

My other idea along those lines (I haven't gotten it to work yet) is to do something like we do for gtk, where we pass the drag off to the OS (WM I guess... whatever). We would catch the drag start with WindowDragUtils & implement BeginMoveDrag in nsCocoaWindow (cutting out the fake dragging crap). We would then call [mWindow setMovableByWindowBackground:YES] and that's where I stop. I was thinking we could synthesize a mouse down event but that doesn't seem to be working. I have a bit more to play with here but I was hopeful (less so now). And then I had another idea, but I don't think that's going to work out. DispatchWindowEvent is supposed to convert the return code into a "should we do native handling" bool, but it doesn't seem to do that with much consistency. For example, when dragging a tab when it's in the titlebar, in the ChildView mouseDragged method, DispatchWindowEvent returns false (which means do handle). But that's not really accurate, so I think that path is fraught with peril. I'll look at MozMouseHittest too, thanks for the suggestion.

Updated per comment 4.

I took Markus's advice & used the mozhittest... it definitely seems to be going down a good path - the window is properly movable with tabs in titlebar (and padding collapsed). However, dragging via other chrome doesn't work. I was hoping it would just magically work if I stopped swizzling or returned YES from the swizzled method, but no such luck. I'm not 100% sure of my next step here, but I'll keep poking around. (Yes, there's a bunch of unnecessary crap in there. I'll separate the wheat from the chaff later)

Upward vertical thresholds for animated tab detaching depends on this. Thanks for working on it, Paul!

(In reply to Paul O'Shannessy [:zpao] from comment #8) > Created attachment 592931 [details] [diff] [review] > wip part 2 ("proper" titlebar dragging) > > I took Markus's advice & used the mozhittest... it definitely seems to be > going down a good path - the window is properly movable with tabs in > titlebar (and padding collapsed). However, dragging via other chrome doesn't > work. I was hoping it would just magically work if I stopped swizzling or > returned YES from the swizzled method, but no such luck. I'm not 100% sure > of my next step here, but I'll keep poking around. Have you tried always returning YES from the mouseDownCanMoveWindow methods? Cocoa should be "and"ing that value with the movableByWindowBackground value, so it should still work, I think. And I think the mouseDownCanMoveWindow value isn't synced to the window server very often; maybe put a printf in there to see whether it's called at the right times. (In reply to Frank Yan (:fryn) from comment #9) > Upward vertical thresholds for animated tab detaching depends on this. How?

> (In reply to Frank Yan (:fryn) from comment #9) > > Upward vertical thresholds for animated tab detaching depends on this. > > How? Oh, because we're using drag over events, not mouse move events, and those don't get sent to the original window if they're outside of it. I see.

(In reply to Markus Stange from comment #10) > Have you tried always returning YES from the mouseDownCanMoveWindow methods? I thought I'd tried all the combinations, but I missed the one in ChildView (I guess the one that gets swizzled out?). Returning YES from there does seem to be working! Now to step back and figure out the minimal bits here. > Cocoa should be "and"ing that value with the movableByWindowBackground > value, so it should still work, I think. And I think the > mouseDownCanMoveWindow value isn't synced to the window server very often; > maybe put a printf in there to see whether it's called at the right times. Yea, I've had printfs in the different impls - you're right that it isn't synced often. I haven't noticed exactly when it decides to do it but it's not too often. If we make them all return YES, could we get rid of the swizzling and depend on movableByWindowBackground to be called correctly?

(In reply to Paul O'Shannessy [:zpao] from comment #12) > (In reply to Markus Stange from comment #10) > > Have you tried always returning YES from the mouseDownCanMoveWindow methods? > > I thought I'd tried all the combinations, but I missed the one in ChildView > (I guess the one that gets swizzled out?). Returning YES from there does > seem to be working! Now to step back and figure out the minimal bits here. Except that this makes the window draggable in the content area, so that's no good - returning [[self window] isMovableByWindowBackground] works. I'm a bit concerned though about this path though... we're firing an additional event for _every_ mouse move. We really just want to be doing that check in ToolbarWindows right? (based on the comment for nsChildView_NSView_mouseDownCanMoveWindow, I may need to have another swizzle to catch it right...)

(In reply to Paul O'Shannessy [:zpao] from comment #12) > (In reply to Markus Stange from comment #10) > > Have you tried always returning YES from the mouseDownCanMoveWindow methods? > > I thought I'd tried all the combinations, but I missed the one in ChildView > (I guess the one that gets swizzled out?). It's not swizzled "out"; it has its name swapped at runtime with nsChildView_NSView_mouseDownCanMoveWindow and then is called at the end of the original nsChildView_NSView_mouseDownCanMoveWindow (which is now the new mouseDownCanMoveWindow) in the case where ![ourWindow isKindOfClass:[ToolbarWindow class]] || (self != contentView). I agree that swizzling is utterly confusing. > > Cocoa should be "and"ing that value with the movableByWindowBackground > > value, so it should still work, I think. And I think the > > mouseDownCanMoveWindow value isn't synced to the window server very often; > > maybe put a printf in there to see whether it's called at the right times. > > Yea, I've had printfs in the different impls - you're right that it isn't > synced often. I haven't noticed exactly when it decides to do it but it's > not too often. If we make them all return YES, could we get rid of the > swizzling and depend on movableByWindowBackground to be called correctly? I thought that should work, but your next comment says otherwise... (In reply to Paul O'Shannessy [:zpao] from comment #13) > (In reply to Paul O'Shannessy [:zpao] from comment #12) > > (In reply to Markus Stange from comment #10) > > > Have you tried always returning YES from the mouseDownCanMoveWindow methods? > > > > I thought I'd tried all the combinations, but I missed the one in ChildView > > (I guess the one that gets swizzled out?). Returning YES from there does > > seem to be working! Now to step back and figure out the minimal bits here. > > Except that this makes the window draggable in the content area, Even while movableByWindowBackground is set to NO? That's unfortunate :( > I'm a bit concerned though about this path though... we're firing an > additional event for _every_ mouse move. Let's try it first, then profile, then decide whether it's an issue. > We really just want to be doing > that check in ToolbarWindows right? Yes, I think so... not that it saves us much. Non-ToolbarWindows are sheets, popups (context menus, tooltips, panels), and (old-style) fullscreen windows. > (based on the comment for > nsChildView_NSView_mouseDownCanMoveWindow, I may need to have another > swizzle to catch it right...) Don't let this swizzle confuse you. It's for a bug that only happened when a Java applet was loaded in our process, and we never really tracked down what exactly Java did to our window.

Anyway, if the native window dragging doesn't work for us, maybe we just have to do without the things that we can't recreate. Let's look at the list again: > - You can't drag background windows (bug 465590). Can be fixed. Is already fixed. (by bug 465590) > - You can't move a window between spaces by dragging it (bug 465542). For this, there's an API since 10.6 that makes this possible: We just have to call [window setMovable:NO]. Since that will also turn off native window dragging in the title bar, we can only do it when our custom window dragging works in the title bar, too, but that's exactly what we're doing in this bug. See http://developer.apple.com/library/mac/#releasenotes/Cocoa/AppKitOlderNotes.html (look for setMovable:). > - You can't drag a window while the app is hung. Not fixable. Yeah, this would just have to go. > - You can't drag a window that contains an open sheet. Might be fixable, but > not easily. This worries me a little. For this we'd need to catch mouseDown events on the window while it's covered by a sheet, and I recall finding out that these events were blocked from us before we'd have a chance to capture them. But this was years ago, I'd have to verify this again.

(In reply to Markus Stange from comment #14) > (In reply to Paul O'Shannessy [:zpao] from comment #12) > > (In reply to Markus Stange from comment #10) > > > Have you tried always returning YES from the mouseDownCanMoveWindow methods? > > > > I thought I'd tried all the combinations, but I missed the one in ChildView > > (I guess the one that gets swizzled out?). > > It's not swizzled "out"; it has its name swapped at runtime with > nsChildView_NSView_mouseDownCanMoveWindow and then is called at the end of > the original nsChildView_NSView_mouseDownCanMoveWindow (which is now the new > mouseDownCanMoveWindow) in the case where ![ourWindow > isKindOfClass:[ToolbarWindow class]] || (self != contentView). > I agree that swizzling is utterly confusing. Ah, got it. I didn't realize the names were literally swapped. I read it as an override (and was a bit confused about how that was ever working when we didn't return NO in nsChildView_NSView_mouseDownCanMoveWindow). It makes much more sense now. (In reply to Markus Stange from comment #15) > Anyway, if the native window dragging doesn't work for us I'm naive here, but I think we can make this work. The mouseDownCanMoveWindow vs moveableByWindowBackground may not be ideal, but I'm going to keep working for a bit. I'll bark up the is/setMovable tree if it comes down to it - the downside being we haven't dropped 10.5 yet, so I think we'd have to have 2 paths? Anyway, with things essentially working, right clicks (specifically 2 finger clicks; ctrl+click works) in the titlebar when the titlebarmousehandlingview is attached end up in an infinitely recursive loop. I'm guessing there's something specific we need to forward in the TitlebarMouseHandlingView but I haven't tracked it down yet. > > - You can't drag a window that contains an open sheet. Might be fixable, but > > not easily. > > This worries me a little. For this we'd need to catch mouseDown events on > the window while it's covered by a sheet, and I recall finding out that > these events were blocked from us before we'd have a chance to capture them. > But this was years ago, I'd have to verify this again. Yea, this isn't working at all with my patch (looks like the parent window just isn't getting any events, like you said)

So I fixed the right mouse clicking loop by simply tracking if we're processing rightMouseDown. We may want to polish that up but it works. I also made sure menus work with ctrl+click by redirecting menuForEvent to the targetView as well. So as long as a window isn't covered by a sheet, I'd say we're pretty close. It's not like sheets are ever used right? I'll just pretend bug 729720 doesn't exist...

(In reply to Paul O'Shannessy [:zpao] from comment #17) > So as long as a window isn't covered by a sheet, I'd say we're pretty close. > It's not like sheets are ever used right? I'll just pretend bug 729720 > doesn't exist... Actually this seems to be working, though I swear it didn't last week.

(In reply to Paul O'Shannessy [:zpao] from comment #18) > (In reply to Paul O'Shannessy [:zpao] from comment #17) > > So as long as a window isn't covered by a sheet, I'd say we're pretty close. > > It's not like sheets are ever used right? I'll just pretend bug 729720 > > doesn't exist... > > Actually this seems to be working, though I swear it didn't last week. I rushed a bit in my excitement... dragging with a sheet works if the last setMovableByWindowBackground was YES. In other words, if the sheet comes up when the mouse event was in the draggable area or the mouse was moved directly from the draggable space out of the window to access the menu. This is actually a pretty interesting effect as it allows any part of the window that isn't the sheet to be used for moving. If it comes down to it, that may be preferable to an unmovable window (I bet we could call setMovableByWindowBackground:YES from the sheet to the parent).

So currently you handle the hit testing in -[ChildView handleMouseMoved:], which doesn't get called on ChildViews in windows behind a sheet. But I think -[BaseWindow mouseMoved:] *does* get called for those mouse move events, and the events are blocked from reaching the ChildView by our own logic in ChildViewMouseTracker::WindowAcceptsEvent. (Please verify this.) So if you were to move the hit testing a bit up in the callstack (or is it down?), the sheet stuff just might work. I'll test your patch later today.

Comment on attachment 603848 [details] [diff] [review] wip part 2 ("proper" titlebar dragging) (take 2) Nice, this works really well. Fix up the sheets (if possible) and we're good to go!

(In reply to Markus Stange from comment #20) > So currently you handle the hit testing in -[ChildView handleMouseMoved:], > which doesn't get called on ChildViews in windows behind a sheet. But I > think -[BaseWindow mouseMoved:] *does* get called for those mouse move > events, AFAICT It doesn't - it looks like we reject the ChildView in ChildViewMouseTracker::MouseEntered. -[BaseWindow mouseMoved:] is never even called (perhaps has something to do with TitlebarMouseHandlingView not forwarding mouseMoved on?) > and the events are blocked from reaching the ChildView by our own > logic in ChildViewMouseTracker::WindowAcceptsEvent. (Please verify this.) We block everything that isn't a right mouse click (which gets weird, I don't think context menus in the title bar are great...) But like I mentioned, it doesn't even look like we're getting the change to block NSMouseMoved events. Your comment in TitlebarMouseHandlingView says it won't receive NSMouseMoved events since we don't override acceptsFirstResponder, but maybe we need to? That seems like it could be another can of worms...

We do get the NSMouseMoved event coming through -[ToolbarWindow sendEvent:] though. Maybe we can add -[BaseWindow sendEvent:] and call mouseMoved directly (if we have a sheet)? It's hacky but I'm trying to throw out as many ideas as possible.

Okay, so I've found out that by returning NO from _isDocModal on the sheet, one can make the events arrive at -[BaseWindow mouseMoved:]. For example like this: @implementation BaseWindow +- (BOOL)_isDocModal +{ + return NO; +} + This is probably more of a hack than hijacking sendEvent, since it relies on undocumented methods. And it lets more mouse events through than we want to, so we'd need to add some ChildViewMouseTracker::WindowAcceptsEvent guards to the right ChildView event handler methods. Just an idea you might want to experiment with.

I took a step back from where I was before (in way too deep with ChildViewMouseTracker and sendEvent trickery) and took all of that out and take that next. So this is a cleaned up version of the patch you f+ed already - updated so it actually works and just uses setMovableByWindowBackground directly instead of through a layer in BaseWindow.

This handles sheets. This was my step back - instead of going to deep, bounce the logic back out to the window to decide if it should allow events through sheets. It's really naive & targeted for this specific case right now, but it could be expended. I still hooked into sendEvent to make the extra mouse moved calls go through in the very targeted case we need them. The downside (and I XXX commented this) right now is that my check still allows windows to be dragged by the toolbar when a sheet is open. Honestly I think that's OK and gives a bigger target (nice since we'll be taking away a large portion of the target).

Comment on attachment 592274 [details] [diff] [review] wip part 1 Steven, I know I've been throwing a lot of stuff your way recently, but here's another! This is Markus's patch, just updated by me a little while ago. I think it still applies (I have either this or a refreshed version in my queue). I don't know how much time Markus has these days to look at my followup patches, but if you got a chance to look at those too, that would be helpful. Thanks!

Comment on attachment 592274 [details] [diff] [review] wip part 1 This looks reasonable to me. But I don't feel comfortable r+ing it until I understand the context better (particularly how titlebar drawing works). I also need to see what information I can dig up using a technique that (probably) neither Markus or you has used -- method swizzling. So (sigh) I'm not going to be able to get to this right away. But I'm truly interested, and will try to fight may way back to it as soon as possible.

Comment on attachment 628139 [details] [diff] [review] wip part 2 - titlebar/toolbar dragging This part looks good.

Comment on attachment 628144 [details] [diff] [review] wip part 3 - handle sheets // XXXzpao [aWindow respondsToSelector:@selector(allowEventThroughSheet)] == NO This is because the colon is part of the selector name, i.e. it needs to be @selector(allowEventsThroughSheet:) ^ I'm not really satisfied with this part. I'd like us to be able to send hit test events and block mouse move events at the same time. That means the hit testing code would need to move out of the handleMouseMoved method. The problem with sending mouse move events to sheet-blocked windows is that this will result in mouse-over feedback (for example on tabs and bookmark buttons), cursor changes and tooltips. + //XXXzpao this should maybe be more specific to see if the event is over our + // "titlebar", however that gets defined (another hittest?). Otherwise + // the window can be dragged by the toolbar... maybe that's ok? Dragging by the toolbar is definitively something we want, even outside the titlebar, but mouseover feedback is not. So my idea is: Move the hit test event sending code somewhere else, and have ChildViewMouseTracker call that code just before it calls handleMouseMoved, but even if WindowAcceptsEvent returns false, and regardless of window type. Is that feasible?

Paul - seems like you're no longer working on this. Can I take this bug?

Please feel free. I'm finding I have much less time to work on this than I'd hoped. I don't think I have any newer patches locally, but I'll be sure to check.

Paul, Markus - how were you guys testing with tabs in the title bar? Is there a little patch that will move the tabs up? I don't really care if they are perfectly positioned, or that it makes sense for the theme in general, I just want to have a good test case for these changes.

I used Paul's code from the last two lines of comment 2, executed from the error console.

This is parts 1 and 2 updated in one patch for current trunk. I have not integrated sheet handling yet since I want to rewrite it per mstange's suggestions.

Added sheet support with a new approach based on mstange's comments. Could use more testing.

This simplifies the code for updating drag-ability w/sheets compared to fix v1.0. In some quick testing it doesn't seem to regress anything.

Comment on attachment 672238 [details] [diff] [review] fix v1.1 >diff --git a/toolkit/content/WindowDraggingUtils.jsm b/toolkit/content/WindowDraggingUtils.jsm >--- a/toolkit/content/WindowDraggingUtils.jsm >+++ b/toolkit/content/WindowDraggingUtils.jsm >@@ -1,18 +1,24 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > >+#ifdef XP_WIN >+#define USE_HITTEST >+#elifdef XP_MACOSX Should this be MOZ_WIDGET_COCOA? >+#define USE_HITTEST >+#endif You can test for two conditions in one go via #if defined(...) || defined(...)

Comment on attachment 672238 [details] [diff] [review] fix v1.1 >+#ifdef XP_WIN >+#define USE_HITTEST >+#elifdef XP_MACOSX >+#define USE_HITTEST >+#endif What Dão said. > void > ChildViewMouseTracker::MouseMoved(NSEvent* aEvent) > { > MouseEnteredWindow(aEvent); >+ //[sLastMouseEventView updateWindowDraggableStateOnMouseMove:aEvent]; remove line >@@ -2750,228 +2909,236 @@ static const NSString* kStateShowsToolba > { > // Drop all mouse events if a modal window has appeared above us. > // This helps make us behave as if the OS were running a "real" modal > // event loop. > id delegate = [self delegate]; > if (delegate && [delegate isKindOfClass:[WindowDelegate class]]) { > nsCocoaWindow *widget = [(WindowDelegate *)delegate geckoWidget]; > if (widget) { >+ if (type == NSMouseMoved) { >+ [[self mainChildView] updateWindowDraggableStateOnMouseMove:anEvent]; >+ } This is a lot simpler than what I had in mind - good! We don't even need to go through ChildViewMouseTracker for this. >+ >+ [super sendEvent:anEvent]; >+} >+ >+- (BOOL)mouseDownCanMoveWindow >+{ >+ return [self isMovableByWindowBackground]; >+} Is this needed? I don't think so. We're in an NSWindow implementation here, and mouseDownCanMoveWindow is an NSView method. This seems to work great! It looks like repainting in the titlebar is a little broken at the moment, but that's not caused by this patch, it's already broken in a current nightly. (For example tab close button hover effects don't work if tabs are completely in the title bar.)

https://tbpl.mozilla.org/?tree=Try&rev=324a69de4083 Addressed review comments, except the macro system for jsm files doesn't allow me to do anything more clever than what I've done here. Also, I haven't determined if this is unnecessary yet: >+- (BOOL)mouseDownCanMoveWindow >+{ >+ return [self isMovableByWindowBackground]; >+}

The tests in this section of "toolkit/content/tests/chrome/window_titlebar.xul" are failing: 109 // on Mac, the window can also be moved with the statusbar 110 if (navigator.platform.indexOf("Mac") >= 0) { 111 var statuslabel = document.getElementById("statuslabel"); 112 var statuslabelnodrag = document.getElementById("statuslabelnodrag"); 113 114 origoldx = window.screenX; 115 origoldy = window.screenY; 116 117 synthesizeMouse(statuslabel, 2, 2, { type: "mousedown" }); 118 synthesizeMouse(statuslabel, 22, 22, { type: "mousemove" }); 119 SimpleTest.is(window.screenX, origoldx + 20, "move window with statusbar horizontal"); 120 SimpleTest.is(window.screenY, origoldy + 20, "move window with statusbar vertical"); 121 synthesizeMouse(statuslabel, 22, 22, { type: "mouseup" }); 122 123 // event was cancelled so the drag should not have occurred 124 synthesizeMouse(statuslabelnodrag, 2, 2, { type: "mousedown" }); 125 synthesizeMouse(statuslabelnodrag, 22, 22, { type: "mousemove" }); 126 SimpleTest.is(window.screenX, origoldx + 20, "move window with statusbar cancelled mousedown horizontal"); 127 SimpleTest.is(window.screenY, origoldy + 20, "move window with statusbar cancelled mousedown vertical"); 128 synthesizeMouse(statuslabelnodrag, 22, 22, { type: "mouseup" }); 129 } Remove them and everything is fine. I need to figure out what is going on here.

Errors from tbox (same as on my local machine): 23153 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_titlebar.xul | move window with statusbar horizontal - got 200, expected 220 23154 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_titlebar.xul | move window with statusbar vertical - got 200, expected 220 23155 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_titlebar.xul | move window with statusbar cancelled mousedown horizontal - got 200, expected 220 23156 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_titlebar.xul | move window with statusbar cancelled mousedown vertical - got 200, expected 220

I've changed the test to work with the new method. Notes for Neil: This bug will change window dragging on Mac to work as on Windows: Instead of moving the window using JS in response mouse events, the widget sends a hit test event to the window and reports draggability to the OS which then does all the moving. That breaks the the statusbar draggibility test (which uses mouse events), but we can change that test to use MozMouseHittest events instead.

Try run of my latest patch here plus mstange's test fixes. https://tbpl.mozilla.org/?tree=Try&rev=4c96029ff2e9

un-bitrot

Remove some unnecessary code that mstange pointed out.

pushed to mozilla-inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/479dc045b543 http://hg.mozilla.org/integration/mozilla-inbound/rev/6332aa884dab

https://hg.mozilla.org/mozilla-central/rev/479dc045b543 https://hg.mozilla.org/mozilla-central/rev/6332aa884dab

I'm going to back this out due to bug 806244. Backout try run: https://tbpl.mozilla.org/?tree=Try&rev=7503d6818f00

backed out http://hg.mozilla.org/integration/mozilla-inbound/rev/d1b1ca54cb42

Gentle ping on this bug - the patch got backed out a little over a month ago, and there hasn't been much activity since. Josh - are you still working on this?

This is blocked by getting a solution for bug 806244 ready.

I'm not working on a solution for bug 806244 because I don't know what's going on there. Happy to move this bug forward once someone has resolved that issue.

Alright, I'll see what I can do about the lw-theme / drag problem.

So, I'm pretty new to all of this Cocoa stuff - the fix v1.3 patch in here, for example, is mostly over my head. I did, however, notice that in winstripe, with lw-themes, if we're drawing in the titlebar, then we attach the windowdragbox binding: https://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser.css#84 Adding a similar rule for pinstripe when drawintitlebar="true" makes dragging with the titlebar work for me. Not sure if this is the optimal solution, but it's what I've got. Feedback?

Comment on attachment 694062 [details] [diff] [review] Allow dragging with titlebar when drawintitlebar="true" I'm feeling more confident about this this morning. Skipping feedback, and going straight to review. To test this patch: 1) First apply the fix v1.3 patch 2) Then apply this patch 3) Open the build and apply a lightweight theme 4) Attempt to drag the window around by click-dragging on the window titlebar. Step 4 should work now.

Comment on attachment 694062 [details] [diff] [review] Allow dragging with titlebar when drawintitlebar="true" >diff -r 614b319c44e6 toolkit/content/xul.css >--- a/toolkit/content/xul.css Wed Dec 19 14:21:36 2012 -0500 >+++ b/toolkit/content/xul.css Wed Dec 19 17:05:22 2012 -0500 >@@ -29,6 +29,7 @@ > %ifdef XP_MACOSX > :root[drawintitlebar="true"] { > padding-top: 22px; >+ -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox"); You're overriding this binding here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/general.xml#67 We should use CAN_DRAW_IN_TITLEBAR on OS X, as mentioned in bug 806244.

Comment on attachment 694062 [details] [diff] [review] Allow dragging with titlebar when drawintitlebar="true" Dao is right - this likely isn't the way to go, since we don't want to override the binding on root.

Dao - I'm not sure what you mean by this: > We should use CAN_DRAW_IN_TITLEBAR on OS X, as mentioned in bug 806244. I'm currently building with CAN_DRAW_IN_TITLEBAR set, but this does not gain us the ability to drag the titlebar. Can you please elaborate a bit on your suggestion? -Mike

(In reply to Mike Conley (:mconley) from comment #60) > I'm currently building with CAN_DRAW_IN_TITLEBAR set, but this does not gain > us the ability to drag the titlebar. Right, it won't do that. We can already draw in the title bar on OS X today and Josh's patch makes it so that we can have interactive content there. CAN_DRAW_IN_TITLEBAR is used as a guard for such content, like the <titlebar> element in browser.xul. You'll also need to set the drawintitlebar attribute and remove the padding-top from xul.css.

(In reply to Dão Gottwald [:dao] from comment #61) > (In reply to Mike Conley (:mconley) from comment #60) > > I'm currently building with CAN_DRAW_IN_TITLEBAR set, but this does not gain > > us the ability to drag the titlebar. > > Right, it won't do that. We can already draw in the title bar on OS X today > and Josh's patch makes it so that we can have interactive content there. > CAN_DRAW_IN_TITLEBAR is used as a guard for such content, like the > <titlebar> element in browser.xul. You'll also need to set the > drawintitlebar attribute and remove the padding-top from xul.css. Ah, yes, I think I understand now. Thanks Dao.

Dao: Is this more like what you had in mind? Now we're *always* drawing in the titlebar. I've overridden xul.css's padding-top on the root to 0, and padded titlebar with what seems like enough space to make everything line up where it should. Assuming this is the route we'd like to go, I think we need two things: 1) Platform support to style the toolbar properly on OSX. Right now, the titlebar has a flat grey shade, whereas native titlebars have a nice gradient. We might want to support the -moz-appearance of -moz-window-titlebar on OSX. Or something similar. 2) If we're always drawing in the titlebar, then this code in toolkit's LightweightThemesConsumer will need to change: https://mxr.mozilla.org/mozilla-central/source/toolkit/content/LightweightThemeConsumer.jsm#98 It's not immediately obvious to me how to accomplish #2 in a way that doesn't disturb other Gecko apps that use lw-themes on OSX. Ideas? -Mike

Comment on attachment 694976 [details] [diff] [review] WIP patch to permanently draw in titlebar on OSX (In reply to Mike Conley (:mconley) from comment #63) > Is this more like what you had in mind? Yes. > 2) If we're always drawing in the titlebar, then this code in toolkit's > LightweightThemesConsumer will need to change: > https://mxr.mozilla.org/mozilla-central/source/toolkit/content/ > LightweightThemeConsumer.jsm#98 > > It's not immediately obvious to me how to accomplish #2 in a way that > doesn't disturb other Gecko apps that use lw-themes on OSX. Ideas? LightweightThemeConsumer.jsm can keep track of whether drawintitlebar was previously set and restore that state rather than removing the attribute unconditionally.

Thanks for the feedback dao. This patch alters LightweightThemeConsumer.jsm to check the existence of the root's tabsintitlebar attribute before modifying it the first time. If, when reverting back to the default theme, the existence was false, then we remove the drawintitlebar attribute. Otherwise, we just keep it in. Next, I need to find a way of styling the background of the titlebar natively when we draw in there.

I've figured out how to add support for -moz-window-titlebar in the Cocoa native theme. We now get a nice gradient in the drawn titlebar. Unfortunately, there's still a break between the titlebar and the nav bar. It seems that ToolbarCanBeUnified in nsNativeThemeCocoa.mm is returning false for the navigation bar, which is causing us to draw the break. Unsure why that's happening - hoping for some feedback there. Actually, I'm looking for some feedback on this whole approach. Am I on the right track?

Comment on attachment 697642 [details] [diff] [review] Make Firefox permanently draw in titlebar on OSX - WIP 3 When I was getting titlebar drawing working in bug 749394, I found that >#main-window[privatebrowsingmode] #navigator-toolbox[tabsontop="true"]::before { > -moz-appearance: none; >} was necessary to get rid of the separator in pinstripe.

(In reply to Josh Matthews [:jdm] from comment #68) > Comment on attachment 697642 [details] [diff] [review] > Make Firefox permanently draw in titlebar on OSX - WIP 3 > > When I was getting titlebar drawing working in bug 749394, I found that > > >#main-window[privatebrowsingmode] #navigator-toolbox[tabsontop="true"]::before { > > -moz-appearance: none; > >} > > was necessary to get rid of the separator in pinstripe. Thanks for the tip Josh! It's not a bad approach, and it does remove the separator, but it also changes the appearance of the window-chrome some. Here's what the titlebar and navbar look like with my patch, and your suggestion: http://i.imgur.com/qEzCw.png And here's what Nightly looks like: http://i.imgur.com/wdBDE.png It's only a subtle difference in the gradient, but I'd like to avoid changing it if possible.

So, I'm getting ahead of myself. This bug is just about getting click events working in the OSX titlebar, not drawing permanently in the OSX titlebar. I'll file a separate bug for that work. In the mean time, this small patch shows the titlebar when a lightweight theme is in use, and the titlebar can be clicked and dragged around.

Comment on attachment 700521 [details] [diff] [review] Show click-able titlebar when using lightweight theme Review of attachment 700521 [details] [diff] [review]: ----------------------------------------------------------------- This patch, when applied upon the other two patches in this bug, allows the user to drag the window around via the titlebar when a lightweight theme is in use.

Comment on attachment 700521 [details] [diff] [review] Show click-able titlebar when using lightweight theme >+#titlebar:not(:-moz-lwtheme) { >+ display: none; >+} Why is this needed? >+#main-window[drawintitlebar="true"] { >+ padding-top: 0; >+} Seems like we should remove the default padding for this from xul.css.

(In reply to Dão Gottwald [:dao] from comment #72) > Comment on attachment 700521 [details] [diff] [review] > Show click-able titlebar when using lightweight theme > > >+#titlebar:not(:-moz-lwtheme) { > >+ display: none; > >+} > > Why is this needed? > With my latest patch, we only draw in the titlebar when lightweight themes are used. If not :-moz-lwtheme, the native titlebar is shown, and we do not need to show the XUL titlebar. > >+#main-window[drawintitlebar="true"] { > >+ padding-top: 0; > >+} > > Seems like we should remove the default padding for this from xul.css. Ok - I'll update the patch.

Comment on attachment 700521 [details] [diff] [review] Show click-able titlebar when using lightweight theme Updating xul.css as per dao's suggestion.

(In reply to Dão Gottwald [:dao] from comment #72) > >+#main-window[drawintitlebar="true"] { > >+ padding-top: 0; > >+} > > Seems like we should remove the default padding for this from xul.css. Hm - so upon second thought, this would force other XUL apps that draw in the titlebar with lightweight themes (I'm thinking of Thunderbird, primarily) to insert some sort of titlebar item into their XUL in order to create the space that we're removing from xul.css. Perhaps it would be better to continue overriding this padding, instead of forcing the other XUL apps to adopt the titlebar convention?

The padding causes bug 806244, which isn't a reasonable default behavior either.

(In reply to Dão Gottwald [:dao] from comment #76) > The padding causes bug 806244, which isn't a reasonable default behavior > either. Excellent point. I'll assume there's some mechanism in place to alert XUL app developers about this. Anyhow, done. Requesting review from Enn as well, as requested at the top of xul.css.

Updated for current trunk

Updated for current trunk.

Corrected oversight in previous patch.

(In reply to Stephen Pohl [:spohl] from comment #80) > Created attachment 703699 [details] [diff] [review] > fix v1.3 > > Corrected oversight in previous patch. Hey Stephen - the patch "fix v1.3" isn't applying cleanly for me on top of trunk. Did it get bit-rotted again? -Mike

Updated for current trunk.

Comment on attachment 700591 [details] [diff] [review] Show clickable titlebar when using lightweight theme - v1.1 >diff -r 439a786c8651 browser/base/content/browser.xul > <spacer id="titlebar-spacer" flex="1"/> > <hbox id="titlebar-buttonbox-container" align="start"> > <hbox id="titlebar-buttonbox"> > <toolbarbutton class="titlebar-button" id="titlebar-min" oncommand="window.minimize();"/> > <toolbarbutton class="titlebar-button" id="titlebar-max" oncommand="onTitlebarMaxClick();"/> > <toolbarbutton class="titlebar-button" id="titlebar-close" command="cmd_closeWindow"/> > </hbox> > </hbox> The buttonbox container should probably be hidden on OS X, maybe just with a display:none in the CSS. With the current patch, you can actually click these invisible buttons in the title bar if you hit the right spot. >diff -r 439a786c8651 browser/themes/pinstripe/browser.css >+#main-window[drawintitlebar="true"] > #titlebar { >+ padding-top: 12px; >+} Let's use height: 22px here. 12px here only works because at the moment the invisible toolbar buttons contribute the rest with a height of 10px (which comes from 6px toolbarbutton padding + 4px toolbarbutton label margin).

(In reply to Markus Stange from comment #83) > Let's use height: 22px here. 12px here only works because at the moment the > invisible toolbar buttons contribute the rest with a height of 10px (which > comes from 6px toolbarbutton padding + 4px toolbarbutton label margin). By the way, you can find out things like these using the DOM Inspector's "Box Model" pane and just stepping through the DOM.

Attachment 704602 [details] [diff] had a small bug that resulted in flipped titlebar drawing. This fixes it by properly applying all changes from bug 831829.

(In reply to Markus Stange from comment #83) > Comment on attachment 700591 [details] [diff] [review] > Show clickable titlebar when using lightweight theme - v1.1 > > >diff -r 439a786c8651 browser/base/content/browser.xul > > > <spacer id="titlebar-spacer" flex="1"/> > > <hbox id="titlebar-buttonbox-container" align="start"> > > <hbox id="titlebar-buttonbox"> > > <toolbarbutton class="titlebar-button" id="titlebar-min" oncommand="window.minimize();"/> > > <toolbarbutton class="titlebar-button" id="titlebar-max" oncommand="onTitlebarMaxClick();"/> > > <toolbarbutton class="titlebar-button" id="titlebar-close" command="cmd_closeWindow"/> > > </hbox> > > </hbox> > > The buttonbox container should probably be hidden on OS X, maybe just with a > display:none in the CSS. With the current patch, you can actually click > these invisible buttons in the title bar if you hit the right spot. Oh, good call - I didn't realize that this was still showing. Nice catch. > > >diff -r 439a786c8651 browser/themes/pinstripe/browser.css > > >+#main-window[drawintitlebar="true"] > #titlebar { > >+ padding-top: 12px; > >+} > > Let's use height: 22px here. 12px here only works because at the moment the > invisible toolbar buttons contribute the rest with a height of 10px (which > comes from 6px toolbarbutton padding + 4px toolbarbutton label margin). Done.

Simplifying a pair of common CSS rules.

Comment on attachment 705379 [details] [diff] [review] Show clickable titlebar when using lightweight theme - v1.3 Hey Neil - how does this look to you?

(In reply to Mike Conley (:mconley) from comment #88) > Comment on attachment 705379 [details] [diff] [review] > Show clickable titlebar when using lightweight theme - v1.3 > > Hey Neil - how does this look to you? Excellent, thank you Neil. Josh - (In reply to Josh Aas (Mozilla Corporation) from comment #54) > I'm not working on a solution for bug 806244 because I don't know what's > going on there. Happy to move this bug forward once someone has resolved > that issue. Hey Josh - so, this last patch should make sure we don't re-open bug 806244. Are you OK to drive this one forward now?

(In reply to Mike Conley (:mconley) from comment #89) > Hey Josh - so, this last patch should make sure we don't re-open bug 806244. > Are you OK to drive this one forward now? Sure! Thanks to everyone who helped get us here, this is going to be a great step forward for Firefox on OS X.

"Allow testing of MozMouseHittest window dragging, v2" is bitrotted, I can do it soon if nobody beats me to it.

Updated for current trunk.

(In reply to Stephen Pohl [:spohl] from comment #92) > Created attachment 708203 [details] [diff] [review] > Allow testing of MozMouseHittest window dragging, v2 > > Updated for current trunk. Excellent - thanks Stephen. :) Josh - do you think these are OK to land now?

pushed to mozilla-inbound hg.mozilla.org/integration/mozilla-inbound/rev/28cd2dbfd8f9

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/3a4c239b5197 Test failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=28cd2dbfd8f9

Fixed test error. Kicking off try build to check for any other errors.

Updated for current trunk. Carrying over r+.

Try builds look green with the updated patches: https://tbpl.mozilla.org/?tree=Try&rev=93d5926d393d

pushed to mozilla-inbound, thanks Stephen! http://hg.mozilla.org/integration/mozilla-inbound/rev/2f54529528a9

PB windows are drawing tabs on the title bar on today's Nightly, is this a falloff from this patch? http://cl.ly/image/0o0g1D3D2b3G

(In reply to Reuben Morais [:reuben] from comment #101) > PB windows are drawing tabs on the title bar on today's Nightly, is this a > falloff from this patch? http://cl.ly/image/0o0g1D3D2b3G Yes - I'm taking care of this in bug 839073.

The patch for this bug caused bug 853105.