[OS X] Mouse event handling in the title bar

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: mstange, Assigned: jaas)

Tracking

Trunk
mozilla19
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 27 obsolete attachments)

30.16 KB, image/png
Details
35.74 KB, patch
Details | Diff | Splinter Review
15.15 KB, patch
Details | Diff | Splinter Review
2.94 KB, patch
spohl
: review+
Details | Diff | Splinter Review
Posted patch wip (obsolete) — Splinter Review
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.
Posted patch wip (unbitrotted) (obsolete) — Splinter Review
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.
Posted patch wip part 1 (obsolete) — Splinter Review
Updated per comment 4.
Attachment #591556 - Attachment is obsolete: true
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?
Target Milestone: --- → mozilla12
Target Milestone: mozilla12 → ---
(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...
Attachment #592931 - Attachment is obsolete: true
Attachment #603848 - Flags: feedback?(mstange)
(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!
Attachment #603848 - Flags: feedback?(mstange) → feedback+
(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.
Assignee: nobody → paul
Status: NEW → ASSIGNED
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.
Attachment #628139 - Flags: review?(mstange)
Posted patch wip part 3 - handle sheets (obsolete) — Splinter Review
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).
Attachment #603848 - Attachment is obsolete: true
Attachment #628144 - Flags: review?(mstange)
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!
Attachment #592274 - Flags: review?(smichaud)
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.
Attachment #592274 - Flags: review?(smichaud)
Comment on attachment 628139 [details] [diff] [review]
wip part 2 - titlebar/toolbar dragging

This part looks good.
Attachment #628139 - Flags: review?(mstange) → review+
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?
Attachment #628144 - Flags: review?(mstange)
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.
Assignee: paul → joshmoz
Attachment #523566 - Attachment is obsolete: true
Attachment #592274 - Attachment description: wip (unbitrotted take 2) → wip part 1
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.
Posted patch fix v0.6 (obsolete) — Splinter Review
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.
Attachment #592274 - Attachment is obsolete: true
Attachment #628139 - Attachment is obsolete: true
Posted patch fix v1.0 (obsolete) — Splinter Review
Added sheet support with a new approach based on mstange's comments. Could use more testing.
Attachment #628144 - Attachment is obsolete: true
Attachment #671934 - Attachment is obsolete: true
Attachment #672231 - Flags: review?(mstange)
Posted patch fix v1.1 (obsolete) — Splinter Review
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.
Attachment #672238 - Flags: review?(mstange)
Attachment #672231 - Flags: review?(mstange)
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.)
Attachment #672238 - Flags: review?(mstange) → review+
Posted patch fix v1.2 (obsolete) — Splinter Review
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];
>+}
Attachment #672231 - Attachment is obsolete: true
Attachment #672238 - Attachment is obsolete: true
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.
Attachment #675137 - Flags: review?(enndeakin)
Attachment #675137 - Flags: review?(enndeakin) → review+
Try run of my latest patch here plus mstange's test fixes.

https://tbpl.mozilla.org/?tree=Try&rev=4c96029ff2e9
un-bitrot
Attachment #675137 - Attachment is obsolete: true
Posted patch fix v1.3 (obsolete) — Splinter Review
Remove some unnecessary code that mstange pointed out.
Attachment #675040 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/479dc045b543
https://hg.mozilla.org/mozilla-central/rev/6332aa884dab
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 806244
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
Attachment #694062 - Flags: feedback?(joshmoz)
Attachment #694062 - Flags: feedback?(enndeakin)
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.
Attachment #694062 - Flags: review?(enndeakin)
Attachment #694062 - Flags: feedback?(joshmoz)
Attachment #694062 - Flags: feedback?(enndeakin)
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.
Attachment #694062 - Flags: review?(enndeakin)
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
Flags: needinfo?(dao)
(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.
Flags: needinfo?(dao)
(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.
Attachment #694062 - Attachment is obsolete: true
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
Attachment #694976 - Flags: feedback?(dao)
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.
Attachment #694976 - Flags: feedback?(dao) → feedback+
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.
Attachment #694976 - Attachment is obsolete: true
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?
Attachment #697044 - Attachment is obsolete: true
Flags: needinfo?
Flags: needinfo?
Attachment #697642 - Flags: feedback?(mstange)
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.
Attachment #697642 - Attachment is obsolete: true
Attachment #697642 - Flags: feedback?(mstange)
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.
Attachment #700521 - Flags: review?(dao)
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.
Attachment #700521 - Flags: review?(dao)
(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?
Flags: needinfo?(dao)
The padding causes bug 806244, which isn't a reasonable default behavior either.
Flags: needinfo?(dao)
(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.
Attachment #700521 - Attachment is obsolete: true
Attachment #700591 - Flags: review?(enndeakin)
Attachment #700591 - Flags: review?(dao)
Updated for current trunk
Attachment #675520 - Attachment is obsolete: true
Posted patch fix v1.3 (obsolete) — Splinter Review
Updated for current trunk.
Attachment #675573 - Attachment is obsolete: true
Posted patch fix v1.3 (obsolete) — Splinter Review
Corrected oversight in previous patch.
Attachment #703608 - Attachment is obsolete: true
(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
Posted patch fix v1.3 (obsolete) — Splinter Review
Updated for current trunk.
Attachment #703699 - Attachment is obsolete: true
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.
Posted patch fix v1.3Splinter Review
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.
Attachment #704602 - Attachment is obsolete: true
(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.
Attachment #700591 - Attachment is obsolete: true
Attachment #700591 - Flags: review?(enndeakin)
Attachment #700591 - Flags: review?(dao)
Simplifying a pair of common CSS rules.
Attachment #705375 - Attachment is obsolete: true
Comment on attachment 705379 [details] [diff] [review]
Show clickable titlebar when using lightweight theme - v1.3

Hey Neil - how does this look to you?
Attachment #705379 - Flags: review?(enndeakin)
Attachment #705379 - Flags: review?(enndeakin) → review+
(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?
Flags: needinfo?(joshmoz)
(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.
Flags: needinfo?(joshmoz)
"Allow testing of MozMouseHittest window dragging, v2" is bitrotted, I can do it soon if nobody beats me to it.
Updated for current trunk.
Attachment #703607 - Attachment is obsolete: true
(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
Fixed test error. Kicking off try build to check for any other errors.
Attachment #708203 - Attachment is obsolete: true
Updated for current trunk. Carrying over r+.
Attachment #705379 - Attachment is obsolete: true
Attachment #709805 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/2f54529528a9
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Depends on: 838855
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.
Depends on: 839073
Depends on: 848632
No longer depends on: 848632
The patch for this bug caused bug 853105.
You need to log in before you can comment on or make changes to this bug.