Closed Bug 555081 Opened 15 years ago Closed 14 years ago

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

Categories

(Firefox :: Theme, enhancement)

x86
Windows 7
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta2+

People

(Reporter: jimm, Assigned: Felipe)

References

Details

Attachments

(7 files, 13 obsolete files)

(deleted), patch
Felipe
: review+
Details | Diff | Splinter Review
(deleted), patch
jimm
: review+
Details | Diff | Splinter Review
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
(deleted), patch
vlad
: review+
Details | Diff | Splinter Review
(deleted), patch
jimm
: review+
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
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.
Double-clicking to maximize/restore the window doesn't work below the title bar either.
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.
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.
(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?
Blocks: 513162
Attached patch Just set toolbar binding (obsolete) (deleted) — Splinter Review
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
Attached patch Native handling - WIP 1 (obsolete) (deleted) — Splinter Review
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.
Attached patch Set draggable bindings for the toolbars (obsolete) (deleted) — Splinter Review
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)
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-
Felipe, will this work on non-glass desktops as well?
Since the non-glass themes also feature an integrated look of titlebar+toolbars, I think this should work for non-glass too.
Attached patch Set draggable bindings for the toolbars - v2 (obsolete) (deleted) — Splinter Review
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.
(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) ?
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.
Attached patch Set draggable bindings for the toolbars - v3 (obsolete) (deleted) — Splinter Review
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.
(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.
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?
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.
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.
Attached patch Part 1 - new mouse event (obsolete) (deleted) — Splinter Review
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)
Attached patch Part 2 - widget (obsolete) (deleted) — Splinter Review
Dispatch HitTest event from widget to define what areas are blank toolbars
Attachment #453577 - Flags: review?(jmathies)
Attached patch Par 3 - WindowDraggingUtils (obsolete) (deleted) — Splinter Review
Make WindowDraggingUtils listen for MozHitTest event on windows and respond with aEvent.preventDefault() when the window should be dragged
Attachment #453580 - Flags: review?(mstange)
Attached patch Part 4 - apply bindings (obsolete) (deleted) — Splinter Review
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)
Attached patch Part 4 - apply bindings (obsolete) (deleted) — Splinter Review
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)
Attached patch Part 3 - WindowDraggingUtils (obsolete) (deleted) — Splinter Review
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.
Attached patch Par 3 - WindowDraggingUtils (obsolete) (deleted) — Splinter Review
> (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?
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 :)
Aaah ok, I wouldn't imagine that. Just tested, it works. Posting new patch
Attached patch Part 3 - WindowDraggingUtils v4 (obsolete) (deleted) — Splinter Review
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+
Thanks! I can do that in a follow-up later.
Attachment #453707 - Flags: review?(enndeakin)
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+
carrying r+
Attachment #453765 - Flags: review+
Attachment #453605 - Attachment is obsolete: true
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+
Attached patch fix SetCapture (deleted) — Splinter Review
(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)
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?
(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.
(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.
> 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?
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?
Depends on: 574702
Posting unbitrotted patches for the native appraoch and re-asking b1? blocker.. This would get us Aero Snap
Attached patch Part 1 - MozHittest event (deleted) — Splinter Review
Attachment #453575 - Attachment is obsolete: true
Attachment #454231 - Flags: review?(Olli.Pettay)
Attachment #453575 - Flags: review?(Olli.Pettay)
Attached patch Part 2 - Window dragging utils (deleted) — Splinter Review
Attachment #453707 - Attachment is obsolete: true
Attachment #454232 - Flags: review?(enndeakin)
Attachment #453707 - Flags: review?(enndeakin)
Attached patch Part 3 - Widget (deleted) — Splinter Review
Attachment #453577 - Attachment is obsolete: true
Attachment #454233 - Flags: review?
Attachment #453577 - Flags: review?(jmathies)
Blocks: 574724
Depends on: 574859
Blocks: 574951
No longer blocks: 513162
Depends on: 513162
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+
Attached patch Widget (deleted) — Splinter Review
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+
Attached patch Full patch (deleted) — Splinter Review
hg diff -r qparent with all the latest patches, unbitrotted r=smaug,mstange,vlad,jmathies (just need review on the latest widget changes)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs reviews]
(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.
Depends on: 575243
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
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
Depends on: 575515
Depends on: 575899
Depends on: 576182
Depends on: 585492
No longer depends on: 585492
Depends on: 585493
Depends on: 585946
No longer depends on: 585493
Depends on: 590035
Depends on: 612073
Depends on: 656089
No longer depends on: 656089
Blocks: 675443
Depends on: 987215
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: