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

RESOLVED FIXED

Status

()

Firefox
Theme
--
enhancement
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: jimm, Assigned: Felipe)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta2+)

Details

Attachments

(7 attachments, 13 obsolete attachments)

2.08 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
3.41 KB, patch
jimm
: review+
Details | Diff | Splinter Review
7.78 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.86 KB, patch
vlad
: review+
Details | Diff | Splinter Review
3.44 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.06 KB, patch
Details | Diff | Splinter Review
18.57 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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.

Updated

7 years ago
Duplicate of this bug: 555116

Comment 2

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

Updated

7 years ago
Duplicate of this bug: 555571
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

7 years ago
It wasn't,but now that glass is built in the ui guidelines say it should be draggable.
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

7 years ago
(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.
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?
(Reporter)

Updated

7 years ago
Blocks: 513162
(Assignee)

Comment 9

7 years ago
Created attachment 452371 [details] [diff] [review]
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
Assignee: nobody → felipc
Status: NEW → ASSIGNED
(Assignee)

Comment 10

7 years ago
Created attachment 452375 [details] [diff] [review]
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.
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.
(Assignee)

Comment 12

7 years ago
Created attachment 452950 [details] [diff] [review]
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.
Attachment #452371 - Attachment is obsolete: true
Attachment #452950 - Flags: review?(dao)
(Assignee)

Comment 13

7 years ago
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 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.
Attachment #452950 - Flags: review?(dao) → review-
(Reporter)

Comment 15

7 years ago
Felipe, will this work on non-glass desktops as well?
(Assignee)

Comment 16

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

Comment 17

7 years ago
Created attachment 453171 [details] [diff] [review]
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
Attachment #452950 - Attachment is obsolete: true
Attachment #453171 - Flags: review?(enndeakin)
(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 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.
(Assignee)

Comment 20

7 years ago
(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) ?
(Assignee)

Updated

7 years ago
Attachment #453171 - Attachment is obsolete: true
Attachment #453171 - Flags: review?(enndeakin)
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.
(Assignee)

Comment 22

7 years ago
Created attachment 453232 [details] [diff] [review]
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.
Attachment #453232 - Flags: review?(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.
(Assignee)

Comment 24

7 years ago
(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.
(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.
(Assignee)

Comment 26

7 years ago
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?
(Assignee)

Comment 27

7 years ago
This topic is outdated, making it clear with: "Can't move the window when using custom titlebar drawing"
Summary: Can't move the window using glass areas below the title bar → Can't move the window when using custom titlebar drawing
(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.
(Assignee)

Comment 29

7 years ago
Sorry, I don't follow.. Doesn't the patch makes any blank toolbar area that is over glass or the lwtheme draggable?
No, it doesn't... All toolbars adopt the lwtheme background.
(Assignee)

Comment 31

7 years ago
Created attachment 453575 [details] [diff] [review]
Part 1 - new mouse event

Add MozMouseHitTest event
Attachment #452375 - Attachment is obsolete: true
Attachment #453232 - Attachment is obsolete: true
Attachment #453575 - Flags: review?(Olli.Pettay)
Attachment #453232 - Flags: review?(dao)
(Assignee)

Comment 32

7 years ago
Created attachment 453577 [details] [diff] [review]
Part 2 - widget

Dispatch HitTest event from widget to define what areas are blank toolbars
Attachment #453577 - Flags: review?(jmathies)
(Assignee)

Comment 33

7 years ago
Created attachment 453580 [details] [diff] [review]
Par 3 - WindowDraggingUtils

Make WindowDraggingUtils listen for MozHitTest event on windows and respond with aEvent.preventDefault() when the window should be dragged
Attachment #453580 - Flags: review?(mstange)
(Assignee)

Comment 34

7 years ago
Created attachment 453582 [details] [diff] [review]
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
Attachment #453582 - Flags: review?(dao)
(Assignee)

Comment 35

7 years ago
Created attachment 453605 [details] [diff] [review]
Part 4 - apply bindings

Minor change, the rule for the appmenu container ended up in an ifdef
Attachment #453582 - Attachment is obsolete: true
Attachment #453605 - Flags: review?(dao)
Attachment #453582 - Flags: review?(dao)
(Assignee)

Comment 36

7 years ago
Created attachment 453630 [details] [diff] [review]
Part 3 - WindowDraggingUtils

Just changed the #ifdef MACOSX to #ifndef XP_WIN.. Makes more sense this way
Attachment #453580 - Attachment is obsolete: true
Attachment #453630 - Flags: review?(mstange)
Attachment #453580 - Flags: review?(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.
(Assignee)

Comment 38

7 years ago
Created attachment 453702 [details] [diff] [review]
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
Attachment #453630 - Attachment is obsolete: true
Attachment #453702 - Flags: review?(mstange)
Attachment #453630 - Flags: review?(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?
(Assignee)

Comment 40

7 years ago
But where could I mark the _alive to false?  The bindings are removed by means of a CSS rule no longer applying.
_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 :)
(Assignee)

Comment 42

7 years ago
Aaah ok, I wouldn't imagine that. Just tested, it works. Posting new patch
(Assignee)

Comment 43

7 years ago
Created attachment 453707 [details] [diff] [review]
Part 3 - WindowDraggingUtils v4

changes done
Attachment #453702 - Attachment is obsolete: true
Attachment #453707 - Flags: review?(mstange)
Attachment #453702 - Flags: review?(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.
Attachment #453707 - Flags: review?(mstange) → review+
(Assignee)

Comment 45

7 years ago
Thanks! I can do that in a follow-up later.
(Assignee)

Updated

7 years ago
Attachment #453707 - Flags: review?(enndeakin)
(Assignee)

Comment 46

7 years ago
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
Reviewers: if you could expedite review on this, it'd be appreciated, it's a b1 blocker.
blocking2.0: --- → beta1+
Whiteboard: [needs reviews]
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
Attachment #453605 - Flags: review?(dao) → review+
(Assignee)

Comment 49

7 years ago
Created attachment 453765 [details] [diff] [review]
Part 4 - apply bindings (checked in)

carrying r+
Attachment #453765 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #453605 - Attachment is obsolete: true
Comment on attachment 453765 [details] [diff] [review]
Part 4 - apply bindings (checked in)

http://hg.mozilla.org/mozilla-central/rev/dfe0f2a0f3d0
Attachment #453765 - Attachment description: Part 4 - apply bindings → Part 4 - apply bindings (checked in)
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!
blocking2.0: beta1+ → beta2+
(Assignee)

Comment 52

7 years ago
Created attachment 453782 [details] [diff] [review]
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.
Attachment #453782 - Flags: review?(jmathies)
(Reporter)

Comment 53

7 years ago
Comment on attachment 453782 [details] [diff] [review]
fix SetCapture

I'll land this with bug 513162.
Attachment #453782 - Flags: review?(jmathies) → review+
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?
(Assignee)

Comment 55

7 years ago
(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.
(Reporter)

Comment 56

7 years ago
(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
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.
(Assignee)

Comment 58

7 years ago
> 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?
Can we take these discussions to a new bug, please?

Comment 60

7 years ago
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?
(Assignee)

Updated

7 years ago
Depends on: 574702
(Reporter)

Updated

7 years ago
Duplicate of this bug: 574724
(Assignee)

Comment 62

7 years ago
Posting unbitrotted patches for the native appraoch and re-asking b1? blocker.. This would get us Aero Snap
(Assignee)

Comment 63

7 years ago
Created attachment 454231 [details] [diff] [review]
Part 1 - MozHittest event
Attachment #453575 - Attachment is obsolete: true
Attachment #454231 - Flags: review?(Olli.Pettay)
Attachment #453575 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 64

7 years ago
Created attachment 454232 [details] [diff] [review]
Part 2 - Window dragging utils
Attachment #453707 - Attachment is obsolete: true
Attachment #454232 - Flags: review?(enndeakin)
Attachment #453707 - Flags: review?(enndeakin)
(Assignee)

Comment 65

7 years ago
Created attachment 454233 [details] [diff] [review]
Part 3 - Widget
Attachment #453577 - Attachment is obsolete: true
Attachment #454233 - Flags: review?
Attachment #453577 - Flags: review?(jmathies)
(Reporter)

Updated

7 years ago
Blocks: 574724
(Reporter)

Updated

7 years ago
Depends on: 574859
(Reporter)

Updated

7 years ago
Blocks: 574951
(Reporter)

Updated

7 years ago
No longer blocks: 513162
Depends on: 513162
(Reporter)

Comment 66

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

+ sdfasfd

don't forget to remove that!
Attachment #454233 - Flags: review? → review+
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
Attachment #454231 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 68

7 years ago
Created attachment 454324 [details] [diff] [review]
Widget

Updated version to follow HTTOP checks. Should avoid resizer cursor over App button. Also adds support for toolbar's context menus
Comment on attachment 454232 [details] [diff] [review]
Part 2 - Window dragging utils

Looks fine here.
Attachment #454232 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 70

7 years ago
Created attachment 454410 [details] [diff] [review]
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)
(Reporter)

Comment 71

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

Thanks felipe!
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Duplicate of this bug: 574951
(Reporter)

Updated

7 years ago
Whiteboard: [needs reviews]
(Reporter)

Comment 73

7 years ago
(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.
(Reporter)

Updated

7 years ago
Depends on: 575243
Depends on: 575248

Updated

7 years ago
Duplicate of this bug: 575240
(Assignee)

Comment 75

7 years ago
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.
Blocks: 513162
No longer depends on: 513162
Duplicate of this bug: 575349

Updated

7 years ago
Summary: Can't move the window when using custom titlebar drawing → Can't move the window using the titlebar with custom titlebar drawing or glass areas below the titlebar

Updated

7 years ago
Depends on: 575515

Updated

7 years ago
Depends on: 575899
(Reporter)

Updated

7 years ago
Depends on: 576182

Updated

7 years ago
Depends on: 585492

Updated

7 years ago
No longer depends on: 585492

Updated

7 years ago
Depends on: 585493

Updated

7 years ago
Depends on: 585946

Updated

7 years ago
No longer depends on: 585493

Updated

7 years ago
Depends on: 590035

Updated

7 years ago
Depends on: 612073

Updated

6 years ago
Depends on: 656089
(Reporter)

Updated

6 years ago
No longer depends on: 656089

Updated

6 years ago
Blocks: 675443

Updated

3 years ago
Depends on: 987215
You need to log in before you can comment on or make changes to this bug.