555081 - Can't move the window using the titlebar with custom titlebar drawing or glass areas below the titlebar

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement)

Description

[Jim Mathies [:jimm]]

I keep clicking on the glass area in the menubar just below the real titlebar to move the window, but the window doesn't respond to this. (Especially around the button on the far right. If we're going to extend glass down, I think window drags should not be restricted to a "special" area of glass, all glass should work the same. 

Not sure if this should be addressed in win32 or if we can do it up in script.

Comment 2

[Steffen Wilberg]

Double-clicking to maximize/restore the window doesn't work below the title bar either.

Comment 4

[Peter Henkel [:Terepin]]

I don't remember that it was working before. I tried clicking and draging anywahere in FX3.6's window and nothing happened. Unless I'm missing something here.

Comment 5

[Kai Patrick]

It wasn't,but now that glass is built in the ui guidelines say it should be draggable.

Comment 6

[Peter Henkel [:Terepin]]

Glass has nothing to do with browser's UI elements. It's only effect, it doesn't change funcionalityin in any way. That's like saying it should be possible when you using Personas.

Comment 7

[Thomas Stache]

(In reply to comment #6)
> Glass has nothing to do with browser's UI elements. It's only effect, it
> doesn't change funcionalityin in any way. That's like saying it should be
> possible when you using Personas.

Sure Aero Glass has an influence here: the title bar and the toolbar (or tab bar with tabs on top) meld together, so right now we have an invisible border above which you can drag, and below which you cannot. For the user it's better that the whole Glass area can be used to grab the window - it appears to be one element, it should react as one element.

PS: Please don't start the same trolling here as on deviantart.

Comment 8

[Peter Henkel [:Terepin]]

Then the name "Can't move the window using glass areas below the title bar", which implies it is a bug, should be renamed to "Add ability to drag and min/max window from entire Glass area", which implies it is a feature request. But this may be a bit problematic, because when tabs will be in titlebar, there won't be much glass space from where you will able to drag the window. I think more suitable name would be "Add ability to drag and min/max window from entire area above page content". That means tab bar, bookmarks bar, nav bar and title bar.

P.S.: What are you talking about?

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: Just set toolbar binding

This feature was done for OSX on bug 398928. It created a new binding for toolbars and added handling code to drag the window around over blank parts of the toolbars (hboxes, spacers..).  Just adding the binding on winstripe too is enough to make it work here (this is watch this patch does)

However, with this method Windows doesn't trigger the win7 effects of maximizing the window when you move it to the screen edges. If we want this we'll have to handle extra native messages, and I've a WIP patch for that

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: Native handling - WIP 1

We need to handle the non-client hittest messages and inform Windows that we're on an area that should be handled like an extended titlebar.

This patch checks if the mouse is over a glass region and uses the same component as bug 398928 to determine if we're in a blank toolbar area. There's some child/parent window handling there to get the correct messages and offsets.

There are some things to solve yet: I'm not sure what would be the best way to send the message to the WindowDraggingUtils component and receive an answer. At the moment I'm sending a fake mouseover message and calling preventDefault() when the window should be moved. This has 2 problems, one which it seems that preventDefault() is not working, and the other that generating the extra mouseover makes menu blink a little, so it's clearly not the best solution. Suggestions welcome.

Also, after we're drawing an entirely custom window, what areas will bring the system menu when right-clicked, and what will bring our own menus? this will need to be defined at some point.

Comment 11

[Markus Stange [:mstange]]

Instead of using NS_MOUSE_MOVE and mouseover, could you maybe add a new event message NS_MOUSE_DRAG_WINDOW and convert nsMouseEvents with that message into MozDragWindow DOM events?
Smaug, is that a good idea?

I think we also need something like this on Mac in order to be able to fix bug 465542.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: Set draggable bindings for the toolbars

Even before the native handling part is done, we can get these bindings patch reviewed which makes moving the windows possible, so this bug won't get in the way of beta1. These bindings will still be necessary for the native handling patch, so there's no need to backout then later.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comments on the patch: Dao, I'm not sure if it's ok to change the hbox to windowdragbox or if it break anything. From a quick look I didn't see any downsides.
I also moved the draggable bindings from pinstripe to toolkit. I believe there's no reason to keep that and duplicate on winstripe unless we don't want this behavior on Linux.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 452950 [details] [diff] [review]
Set draggable bindings for the toolbars

>--- a/toolkit/content/xul.css
>+++ b/toolkit/content/xul.css

Enn needs to review this, but I'll comment on your changes anyway.

>+toolbar:not([nowindowdrag="true"]) {
>+  -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");

This seems wrong for toolbars that aren't glassed on Windows.

>+statusbar:not([nowindowdrag="true"]) {
>+  -moz-binding: url("chrome://global/content/bindings/general.xml#statusbar-drag");

We probably want this only on OS X.

>+windowdragbox {
>+  -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");

I'm not sure we want all instances of <windowdragbox> to behave like this on all platforms. Probably not.

>--- a/toolkit/themes/pinstripe/global/global.css
>+++ b/toolkit/themes/pinstripe/global/global.css

>-windowdragbox {
>-  -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");
>-}

This is duplicated in downloads-aero.css as well.

Comment 15

[Jim Mathies [:jimm]]

Felipe, will this work on non-glass desktops as well?

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Since the non-glass themes also feature an integrated look of titlebar+toolbars, I think this should work for non-glass too.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Attachment: Set draggable bindings for the toolbars - v2

Less intrusive approach here. I think that in the end we'll want this behavior on all platforms if the look&feel will be of a merged titlebar/toolbar area. However, I'm keeping this patch now only on browser-aero.css inside the -moz-window-compositor query.

Setting review to Enn as suggested by Dao.

> >+windowdragbox {
> >+  -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");
> 
> I'm not sure we want all instances of <windowdragbox> to behave like this on
> all platforms. Probably not.

Well I think this was the original intent of windowdragbox. But you mean they shouldn't have necessarily an effect on all platforms, right? In any case, I'm just setting the draggable binding now to #appmenu-button-container instead of windowdragbox

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #17)
> Created an attachment (id=453171) [details]
> Set draggable bindings for the toolbars - v2
> 
> Less intrusive approach here. I think that in the end we'll want this behavior
> on all platforms if the look&feel will be of a merged titlebar/toolbar area.
> However, I'm keeping this patch now only on browser-aero.css inside the
> -moz-window-compositor query.

Your patch still sets this on all toolbars rather than those being glassed, i.e. the menu, tab and/or nav bars.

> Setting review to Enn as suggested by Dao.

I was referring to xul.css specifically.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 453171 [details] [diff] [review]
Set draggable bindings for the toolbars - v2

>--- a/toolkit/content/xul.css
>+++ b/toolkit/content/xul.css
>@@ -269,17 +269,17 @@ toolbox[customizing="true"] > toolbar[cu
> toolbar[type="menubar"] {
>   min-height: 0 !important;
>   border: 0 !important;
> }
> %endif
> 
> %ifdef XP_WIN
> toolbar[type="menubar"][autohide="true"] {
>-  -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-menubar-autohide");
>+  -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-menubar-autohide") !important;
>   overflow: hidden;
> }

This is bogus. browser-aero.css shouldn't assign a different binding to the autohiding menu toolbar in the first place.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #18)
> (In reply to comment #17)
> > Created an attachment (id=453171) [details] [details]
> > Set draggable bindings for the toolbars - v2
> > 
> > Less intrusive approach here. I think that in the end we'll want this behavior
> > on all platforms if the look&feel will be of a merged titlebar/toolbar area.
> > However, I'm keeping this patch now only on browser-aero.css inside the
> > -moz-window-compositor query.
> 
> Your patch still sets this on all toolbars rather than those being glassed,
> i.e. the menu, tab and/or nav bars.

Oh yeah, that was originally on purpose, to extend to any toolbar. There's no way to check if a toolbar is being glassed, is there? So you think I should make a whitelist approach with #toolbar-menubar, #TabsToolbar and #navbar (if not on bottom of tabs) ?

Comment 21

[Dão Gottwald [:dao]]

Yes, browser-aero.css should target these toolbars specifically. It has all the data is needs, since it determines which toolbars are transparent and which aren't.

Also, we need #appmenu-button-container to be draggable regardless of glass and on XP.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Attachment: Set draggable bindings for the toolbars - v3

I _think_ I got it right this time... To stop dragging when the binding is removed I had to add code to check the current binding, as I tried to use the XBL destructor and it doesn't work.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 453232 [details] [diff] [review]
Set draggable bindings for the toolbars - v3

>+  /* Make the window draggable by glassed toolbars and
>+     the appmenu container (bug 555081) */
>+  #toolbar-menubar:not([autohide="true"]),
>+  #navigator-toolbox[tabsontop="true"] > #TabsToolbar,
>+  #navigator-toolbox:not([tabsontop="true"]) > #nav-bar,
>+  #navigator-toolbox:not([tabsontop="true"]) > #nav-bar + #customToolbars + #PersonalToolbar[collapsed="true"] + #TabsToolbar:last-child {
>+    -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
>+  }

Now this just needs to do something sensible for the :-moz-lwtheme case.

>+  #appmenu-button-container {
>+    -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");
>+  }

It still seems that we want this for all Windows versions and regardless of glass.

>+        let binding = this._window.getComputedStyle(this._elem, null).
>+                                   getPropertyValue("-moz-binding");

Nit: trailing dot should be on the second line instead.

Did you find the corresponding XBL bug? Bug 83635 for instance is supposed to be fixed soon.

Comment 24

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #23)
> (From update of attachment 453232 [details] [diff] [review])
> >+  /* Make the window draggable by glassed toolbars and
> >+     the appmenu container (bug 555081) */
> >+  #toolbar-menubar:not([autohide="true"]),
> >+  #navigator-toolbox[tabsontop="true"] > #TabsToolbar,
> >+  #navigator-toolbox:not([tabsontop="true"]) > #nav-bar,
> >+  #navigator-toolbox:not([tabsontop="true"]) > #nav-bar + #customToolbars + #PersonalToolbar[collapsed="true"] + #TabsToolbar:last-child {
> >+    -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
> >+  }
> 
> Now this just needs to do something sensible for the :-moz-lwtheme case.

When there is a lwtheme applied, the area where it's on still can't be dragged natively, so I think the same binding should apply to it.

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 453232 [details] [diff] [review] [details])
> > >+  /* Make the window draggable by glassed toolbars and
> > >+     the appmenu container (bug 555081) */
> > >+  #toolbar-menubar:not([autohide="true"]),
> > >+  #navigator-toolbox[tabsontop="true"] > #TabsToolbar,
> > >+  #navigator-toolbox:not([tabsontop="true"]) > #nav-bar,
> > >+  #navigator-toolbox:not([tabsontop="true"]) > #nav-bar + #customToolbars + #PersonalToolbar[collapsed="true"] + #TabsToolbar:last-child {
> > >+    -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
> > >+  }
> > 
> > Now this just needs to do something sensible for the :-moz-lwtheme case.
> 
> When there is a lwtheme applied, the area where it's on still can't be dragged
> natively, so I think the same binding should apply to it.

That area won't be a glass area, though, which is this bug's topic. Those toolbars aren't going to look different from other toolbars in that case either.

Comment 26

[:Felipe Gomes (needinfo for replies!)]

But the top of the window and the toolbars will have the same appearance (the background image), so there would be an invisible line where you could or could not move the window, wouldn't?

Comment 27

[:Felipe Gomes (needinfo for replies!)]

This topic is outdated, making it clear with: "Can't move the window when using custom titlebar drawing"

Comment 28

[Dão Gottwald [:dao]]

(In reply to comment #26)
> But the top of the window and the toolbars will have the same appearance (the
> background image), so there would be an invisible line where you could or could
> not move the window, wouldn't?

There would, but your patch doesn't eliminate the line but pushes it to some random spot.

Comment 29

[:Felipe Gomes (needinfo for replies!)]

Sorry, I don't follow.. Doesn't the patch makes any blank toolbar area that is over glass or the lwtheme draggable?

Comment 30

[Dão Gottwald [:dao]]

No, it doesn't... All toolbars adopt the lwtheme background.

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 1 - new mouse event

Add MozMouseHitTest event

Comment 32

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 2 - widget

Dispatch HitTest event from widget to define what areas are blank toolbars

Comment 33

[:Felipe Gomes (needinfo for replies!)]

Attachment: Par 3 - WindowDraggingUtils

Make WindowDraggingUtils listen for MozHitTest event on windows and respond with aEvent.preventDefault() when the window should be dragged

Comment 34

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 4 - apply bindings

Ok dao, I think I got what you're saying.. so now when on lwtheme I apply the binding to all toolbars since all of them will get the background..
(I don't apply the bind to the menubar because it's applied anyway by the other rule anyway, and this one would override it if in autohide)

Let me know if this is the correct thing now, or what I should do to get it correctly

Comment 35

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 4 - apply bindings

Minor change, the rule for the appmenu container ended up in an ifdef

Comment 36

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 3 - WindowDraggingUtils

Just changed the #ifdef MACOSX to #ifndef XP_WIN.. Makes more sense this way

Comment 37

[Markus Stange [:mstange]]

Comment on attachment 453630 [details] [diff] [review]
Part 3 - WindowDraggingUtils

>diff --git a/toolkit/content/Makefile.in b/toolkit/content/Makefile.in
>--- a/toolkit/content/Makefile.in
>+++ b/toolkit/content/Makefile.in
>@@ -79,19 +79,19 @@ DEFINES += -DMOZ_TOOLKIT_SEARCH
> endif
> 
> ifdef ENABLE_TESTS
> DIRS += tests
> endif
> 
> EXTRA_JS_MODULES = \
>   InlineSpellChecker.jsm \
>-  WindowDraggingUtils.jsm \
>   PopupNotifications.jsm \
>   $(NULL)
> 
> EXTRA_PP_JS_MODULES = \
>   debug.js \
>   LightweightThemeConsumer.jsm \
>   Services.jsm \
>+  WindowDraggingUtils.jsm \
>   $(NULL)

What does this do?

>diff --git a/toolkit/content/WindowDraggingUtils.jsm b/toolkit/content/WindowDraggingUtils.jsm
>--- a/toolkit/content/WindowDraggingUtils.jsm
>+++ b/toolkit/content/WindowDraggingUtils.jsm
>@@ -34,31 +34,45 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> let EXPORTED_SYMBOLS = [ "WindowDraggingElement" ];
> 
> function WindowDraggingElement(elem, window) {
>   this._elem = elem;
>   this._window = window;
>+#ifndef XP_WIN
>   this._elem.addEventListener("mousedown", this, false);
>+#else
>+  this._elem.addEventListener("MozMouseHittest", this, false);
>+#endif

Flip these two around so you can write #ifdef XP_WIN. Also below.

> }
> 
> WindowDraggingElement.prototype = {
>   mouseDownCheck: function(e) { return true; },
>   dragTags: ["box", "hbox", "vbox", "spacer", "label", "statusbarpanel", "stack",
>              "toolbaritem", "toolbarseparator", "toolbarspring", "toolbarspacer",
>-             "radiogroup", "deck", "scrollbox"],
>+             "radiogroup", "deck", "scrollbox", "tabs", "arrowscrollbox"],

I'm also making this change in bug 573791. I don't need it on Mac until we have a proper tabs-on-top appearance there, but feel free to land that patch together with this one.

>+  dragBindings: ['url("chrome://global/content/bindings/toolbar.xml#toolbar-drag")',
>+                 'url("chrome://global/content/bindings/general.xml#statusbar-drag")',
>+                 'url("chrome://global/content/bindings/general.xml#windowdragbox")'],
>   handleEvent: function(aEvent) {
>+#ifndef XP_WIN
>     switch (aEvent.type) {
>       case "mousedown":
>         if (aEvent.button != 0 || !this.mouseDownCheck.call(this._elem, aEvent) ||
>             aEvent.getPreventDefault())
>           return;
> 
>+        // The draggable binding might have been removed from this element
>+        let binding = this._window.getComputedStyle(this._elem, null)
>+                                  .getPropertyValue("-moz-binding");
>+        if (this.dragBindings.indexOf(binding) == -1)
>+          return;

In which cases does this happen?

>+    if (aEvent.button != 0 || !this.mouseDownCheck.call(this._elem, aEvent) ||
>+        aEvent.getPreventDefault())
>+      return;
>+
>+    // The draggable binding might have been removed from this element
>+    let binding = this._window.getComputedStyle(this._elem, null)
>+                              .getPropertyValue("-moz-binding");
>+    if (this.dragBindings.indexOf(binding) == -1)
>+      return;
>+
>+    let target = aEvent.originalTarget, parent = aEvent.originalTarget;
>+    while (parent != this._elem) {
>+      let mousethrough = parent.getAttribute("mousethrough");
>+      if (mousethrough == "always")
>+        target = parent.parentNode;
>+      else if (mousethrough == "never")
>+        break;
>+      parent = parent.parentNode;
>+    }
>+    while (target != this._elem) {
>+      if (this.dragTags.indexOf(target.localName) == -1)
>+        return;
>+      target = target.parentNode;
>+    }

Please put this code into its own function so that you don't have to duplicate it.

Comment 38

[:Felipe Gomes (needinfo for replies!)]

Attachment: Par 3 - WindowDraggingUtils

> (From update of attachment 453630 [details] [diff] [review])
> >diff --git a/toolkit/content/Makefile.in b/toolkit/content/Makefile.in
> >--- a/toolkit/content/Makefile.in
> >+++ b/toolkit/content/Makefile.in
> >@@ -79,19 +79,19 @@ DEFINES += -DMOZ_TOOLKIT_SEARCH
> > endif
> > 
> > ifdef ENABLE_TESTS
> > DIRS += tests
> > endif
> > 
> > EXTRA_JS_MODULES = \
> >   InlineSpellChecker.jsm \
> >-  WindowDraggingUtils.jsm \
> >   PopupNotifications.jsm \
> >   $(NULL)
> > 
> > EXTRA_PP_JS_MODULES = \
> >   debug.js \
> >   LightweightThemeConsumer.jsm \
> >   Services.jsm \
> >+  WindowDraggingUtils.jsm \
> >   $(NULL)
> 
> What does this do?

This is needed to have this file go through the preprocessor for the #ifdefs

> 
> >diff --git a/toolkit/content/WindowDraggingUtils.jsm b/toolkit/content/WindowDraggingUtils.jsm
> >--- a/toolkit/content/WindowDraggingUtils.jsm
> >+++ b/toolkit/content/WindowDraggingUtils.jsm
> >@@ -34,31 +34,45 @@
> >  *
> >  * ***** END LICENSE BLOCK ***** */
> > 
> > let EXPORTED_SYMBOLS = [ "WindowDraggingElement" ];
> > 
> > function WindowDraggingElement(elem, window) {
> >   this._elem = elem;
> >   this._window = window;
> >+#ifndef XP_WIN
> >   this._elem.addEventListener("mousedown", this, false);
> >+#else
> >+  this._elem.addEventListener("MozMouseHittest", this, false);
> >+#endif
> 
> Flip these two around so you can write #ifdef XP_WIN. Also below.

Done

> 
> > }
> > 
> > WindowDraggingElement.prototype = {
> >   mouseDownCheck: function(e) { return true; },
> >   dragTags: ["box", "hbox", "vbox", "spacer", "label", "statusbarpanel", "stack",
> >              "toolbaritem", "toolbarseparator", "toolbarspring", "toolbarspacer",
> >-             "radiogroup", "deck", "scrollbox"],
> >+             "radiogroup", "deck", "scrollbox", "tabs", "arrowscrollbox"],
> 
> I'm also making this change in bug 573791. I don't need it on Mac until we have
> a proper tabs-on-top appearance there, but feel free to land that patch
> together with this one.

I removed this change from this patch and will land your patch

> 
> >+  dragBindings: ['url("chrome://global/content/bindings/toolbar.xml#toolbar-drag")',
> >+                 'url("chrome://global/content/bindings/general.xml#statusbar-drag")',
> >+                 'url("chrome://global/content/bindings/general.xml#windowdragbox")'],
> >   handleEvent: function(aEvent) {
> >+#ifndef XP_WIN
> >     switch (aEvent.type) {
> >       case "mousedown":
> >         if (aEvent.button != 0 || !this.mouseDownCheck.call(this._elem, aEvent) ||
> >             aEvent.getPreventDefault())
> >           return;
> > 
> >+        // The draggable binding might have been removed from this element
> >+        let binding = this._window.getComputedStyle(this._elem, null)
> >+                                  .getPropertyValue("-moz-binding");
> >+        if (this.dragBindings.indexOf(binding) == -1)
> >+          return;
> 
> In which cases does this happen?

If the binding is removed (which we do depending on tabs-on-top/personas), the DraggingUtils is already started on the element and it won't stop moving. I tried using a XBL destructor but it didnt work (bug 230068, bug 83635), so I had to use this

> 
> Please put this code into its own function so that you don't have to duplicate
> it.

done

Comment 39

[Markus Stange [:mstange]]

(In reply to comment #38)
> This is needed to have this file go through the preprocessor for the #ifdefs

Oh, duh. Sure.

> > >+        // The draggable binding might have been removed from this element
> > >+        let binding = this._window.getComputedStyle(this._elem, null)
> > >+                                  .getPropertyValue("-moz-binding");
> > >+        if (this.dragBindings.indexOf(binding) == -1)
> > >+          return;
> > 
> > In which cases does this happen?
> 
> If the binding is removed (which we do depending on tabs-on-top/personas), the
> DraggingUtils is already started on the element and it won't stop moving. I
> tried using a XBL destructor but it didnt work (bug 230068, bug 83635), so I
> had to use this

Ah, I see. I think another way to solve this problem would be to have an "_alive" field in our three drag bindings and check for that field in the mouseDownCheck callback.
We're doing something like that for our progressbars:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/progressmeter.xml#72

Then we wouldn't have to hardcode the binding URLs here.

Can you try that?

Comment 40

[:Felipe Gomes (needinfo for replies!)]

But where could I mark the _alive to false?  The bindings are removed by means of a CSS rule no longer applying.

Comment 41

[Markus Stange [:mstange]]

_alive will stop being true automatically when the binding is removed (I think it becomes undefined, but I'm not sure). Don't ask me why it works that way :)

Comment 42

[:Felipe Gomes (needinfo for replies!)]

Aaah ok, I wouldn't imagine that. Just tested, it works. Posting new patch

Comment 43

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 3 - WindowDraggingUtils v4

changes done

Comment 44

[Markus Stange [:mstange]]

Comment on attachment 453707 [details] [diff] [review]
Part 3 - WindowDraggingUtils v4

>+    // Maybe we have been removed from the document
>+    if(!this._elem._alive)
>+      return false;

Actually I was thinking of pulling that check into the mouseDownCheck callback function:
  let draggableThis = new WindowDraggingElement(this, window);
  draggableThis.mouseDownCheck = function(e) { return this._alive; }

But I guess your way is fine, too.

Please also ask enndeakin for review because I'm not a toolkit peer.

Comment 45

[:Felipe Gomes (needinfo for replies!)]

Thanks! I can do that in a follow-up later.

Comment 46

[:Felipe Gomes (needinfo for replies!)]

Neil, this patch adds handling on the WindowDraggingUtils for Windows. Instead of performing the dragging as on OSX, it just decides if the window should be dragged or not and pass that information on preventDefault(), which is used on the widget side to let the OS handle the dragging

Comment 47

[Mike Beltzner [:beltzner, not reading bugmail]]

Reviewers: if you could expedite review on this, it'd be appreciated, it's a b1 blocker.

Comment 48

[Dão Gottwald [:dao]]

Comment on attachment 453605 [details] [diff] [review]
Part 4 - apply bindings

>+  toolbar:not(#toolbar-menubar):-moz-lwtheme {

use this instead:

#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#fullscr-toggler):-moz-lwtheme

Comment 49

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 4 - apply bindings (checked in)

carrying r+

Comment 50

[Shawn Wilsher :sdwilsh]

Comment on attachment 453765 [details] [diff] [review]
Part 4 - apply bindings (checked in)

http://hg.mozilla.org/mozilla-central/rev/dfe0f2a0f3d0

Comment 51

[Mike Beltzner [:beltzner, not reading bugmail]]

Jimm: felipe tells me that the bit that sdwilsh pushed is enough to solve the key problem for beta1 (not being able to drag a window that uses the drawing into the title bar code) and the rest of this is about adding native handling for it. As such, I'm moving this off the b1 blocking list - let me know if that sounds right!

Comment 52

[:Felipe Gomes (needinfo for replies!)]

Attachment: fix SetCapture

(Not sure if I should post this patch here in this bug or on bug 513162)

Now that our main content area is no longer a child window, it'd miss the SetCapture call, and when dragging the window if the mouse goes outside of it we'd lose the mouseup event (when we're handling it by our own).
No reason to not let the top level window call SetCapture too.

Comment 53

[Jim Mathies [:jimm]]

Comment on attachment 453782 [details] [diff] [review]
fix SetCapture

I'll land this with bug 513162.

Comment 54

[Neil Deakin]

Comment on attachment 453707 [details] [diff] [review]
Part 3 - WindowDraggingUtils v4

>   this._elem = elem;
>   this._window = window;
>+#ifdef XP_WIN
>+  this._elem.addEventListener("MozMouseHittest", this, false);

What does this event do and why does it different than mousedown? Can you make the name clearer as to its purpose?


>+    // Maybe we have been removed from the document
>+    if(!this._elem._alive)
>+      return false;

So does this mean that the new event get called after mousedown?

Comment 55

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #54)
> (From update of attachment 453707 [details] [diff] [review])
> >   this._elem = elem;
> >   this._window = window;
> >+#ifdef XP_WIN
> >+  this._elem.addEventListener("MozMouseHittest", this, false);
> 
> What does this event do and why does it different than mousedown? Can you make
> the name clearer as to its purpose?

This comes from a Windows message that is used to determine if the pointer is over a non-client area (titlebar, close button, etc).  If we answer that we're over a regular client area, typical mousedown/mousemove/mouseup will follow. If we call preventDefault(), Windows consume these messages and use it for dragging the window or clicking the close button, for example.

Does this makes sense for this name? Don't know what would be a better name.

> >+    // Maybe we have been removed from the document
> >+    if(!this._elem._alive)
> >+      return false;
> 
> So does this mean that the new event get called after mousedown?

No, this is just a workaround for the bindings that start this component (in short, a #toolbar is switched to #toolbar-drag). If the binding gets removed, that's a way to get that info. I tried using destructors but that didn't work.

Comment 56

[Jim Mathies [:jimm]]

(In reply to comment #52)
> Created an attachment (id=453782) [details]
> fix SetCapture
> 
> (Not sure if I should post this patch here in this bug or on bug 513162)
> 
> Now that our main content area is no longer a child window, it'd miss the
> SetCapture call, and when dragging the window if the mouse goes outside of it
> we'd lose the mouseup event (when we're handling it by our own).
> No reason to not let the top level window call SetCapture too.


http://hg.mozilla.org/mozilla-central/rev/21ee5212a537

Comment 57

[[not reading bugmail]]

Testing the latest hourly with the most recent patch. not sure what your expected behavior should be based on the comments above for this, but here is what I get by testing the latest change:

1) When dragging from the menu bar now to drag from top to bottom of the screen the whole browser window is jumpy.. 
2) The mouse coordinates vs the window frame doesn't stay where in the same place.

Comment 58

[:Felipe Gomes (needinfo for replies!)]

> 1) When dragging from the menu bar now to drag from top to bottom of the screen
> the whole browser window is jumpy.. 

Does this only happen during startup time (or when your processor is overloaded)? During startup it gets somewhat jumpy for me but after the browser settles it goes smoothly.

> 2) The mouse coordinates vs the window frame doesn't stay where in the same
> place.

Is this when the window is maximized and you drag it down, thus unmaximizing? Or is there other scenario?

Comment 59

[Markus Stange [:mstange]]

Can we take these discussions to a new bug, please?

Comment 60

[Tiago Sá]

I believe this has landed, since I can move the window with all the glass on the latest nightly. But what I can't do is aero shake. I can aero shake with the normal title bar, but not with the rest of the glass.

I can't search bugzilla at the moment, can someone please check if there's a bug for it and file it if there isn't?

Comment 62

[:Felipe Gomes (needinfo for replies!)]

Posting unbitrotted patches for the native appraoch and re-asking b1? blocker.. This would get us Aero Snap

Comment 63

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 1 - MozHittest event

Comment 64

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 2 - Window dragging utils

Comment 65

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 3 - Widget

Comment 66

[Jim Mathies [:jimm]]

Comment on attachment 454233 [details] [diff] [review]
Part 3 - Widget

+ sdfasfd

don't forget to remove that!

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 454231 [details] [diff] [review]
Part 1 - MozHittest event

>diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp
>--- a/content/base/src/nsContentUtils.cpp
>+++ b/content/base/src/nsContentUtils.cpp
>@@ -469,16 +469,17 @@ nsContentUtils::InitializeEventTable() {
> 
>   static const EventNameMapping eventArray[] = {
>     { nsGkAtoms::onmousedown,                   NS_MOUSE_BUTTON_DOWN, EventNameType_All, NS_MOUSE_EVENT },
>     { nsGkAtoms::onmouseup,                     NS_MOUSE_BUTTON_UP, EventNameType_All, NS_MOUSE_EVENT },
>     { nsGkAtoms::onclick,                       NS_MOUSE_CLICK, EventNameType_All, NS_MOUSE_EVENT },
>     { nsGkAtoms::ondblclick,                    NS_MOUSE_DOUBLECLICK, EventNameType_HTMLXUL, NS_MOUSE_EVENT },
>     { nsGkAtoms::onmouseover,                   NS_MOUSE_ENTER_SYNTH, EventNameType_All, NS_MOUSE_EVENT },
>     { nsGkAtoms::onmouseout,                    NS_MOUSE_EXIT_SYNTH, EventNameType_All, NS_MOUSE_EVENT },
>+    { nsGkAtoms::onMozMouseHittest,             NS_MOUSE_MOZHITTEST, EventNameType_All, NS_MOUSE_EVENT },

Not EventNameType_All, but EventNameType_None

Comment 68

[:Felipe Gomes (needinfo for replies!)]

Attachment: Widget

Updated version to follow HTTOP checks. Should avoid resizer cursor over App button. Also adds support for toolbar's context menus

Comment 69

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 454232 [details] [diff] [review]
Part 2 - Window dragging utils

Looks fine here.

Comment 70

[:Felipe Gomes (needinfo for replies!)]

Attachment: Full patch

hg diff -r qparent  with all the latest patches, unbitrotted

r=smaug,mstange,vlad,jmathies

(just need review on the latest widget changes)

Comment 71

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/48980abe7a46

Thanks felipe!

Comment 73

[Jim Mathies [:jimm]]

(In reply to comment #68)
> Created an attachment (id=454324) [details]
> Widget
> 
> Updated version to follow HTTOP checks. Should avoid resizer cursor over App
> button. Also adds support for toolbar's context menus

Felipe, do we need these additional mouse event handlers? (I think we might want to file a new bug if we do.) Also, I was reading on the menu, I guess there's an HTSYSMENU for right click we could return.

Comment 75

[:Felipe Gomes (needinfo for replies!)]

I've tried the HTSYSMENU, but it's meant for the app icon on the upper left. It just means that the system menu is also triggered by left click, but it still doesn't come up for some other reason. I'll follow up the handlers for the system menu on bug 574859.