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)
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.
Comment 2•15 years ago
|
||
Double-clicking to maximize/restore the window doesn't work below the title bar either.
Comment 4•15 years ago
|
||
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•15 years ago
|
||
It wasn't,but now that glass is built in the ui guidelines say it should be draggable.
Comment 6•15 years ago
|
||
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•15 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.
Comment 8•15 years ago
|
||
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?
Assignee | ||
Comment 9•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 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 14•14 years ago
|
||
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•14 years ago
|
||
Felipe, will this work on non-glass desktops as well?
Assignee | ||
Comment 16•14 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•14 years ago
|
||
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)
Comment 18•14 years ago
|
||
(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•14 years ago
|
||
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•14 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•14 years ago
|
Attachment #453171 -
Attachment is obsolete: true
Attachment #453171 -
Flags: review?(enndeakin)
Comment 21•14 years ago
|
||
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•14 years ago
|
||
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 23•14 years ago
|
||
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•14 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.
Comment 25•14 years ago
|
||
(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•14 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•14 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
Comment 28•14 years ago
|
||
(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•14 years ago
|
||
Sorry, I don't follow.. Doesn't the patch makes any blank toolbar area that is over glass or the lwtheme draggable?
Comment 30•14 years ago
|
||
No, it doesn't... All toolbars adopt the lwtheme background.
Assignee | ||
Comment 31•14 years ago
|
||
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•14 years ago
|
||
Dispatch HitTest event from widget to define what areas are blank toolbars
Attachment #453577 -
Flags: review?(jmathies)
Assignee | ||
Comment 33•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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 37•14 years ago
|
||
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•14 years ago
|
||
> (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)
Comment 39•14 years ago
|
||
(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•14 years ago
|
||
But where could I mark the _alive to false? The bindings are removed by means of a CSS rule no longer applying.
Comment 41•14 years ago
|
||
_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•14 years ago
|
||
Aaah ok, I wouldn't imagine that. Just tested, it works. Posting new patch
Assignee | ||
Comment 43•14 years ago
|
||
changes done
Attachment #453702 -
Attachment is obsolete: true
Attachment #453707 -
Flags: review?(mstange)
Attachment #453702 -
Flags: review?(mstange)
Comment 44•14 years ago
|
||
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•14 years ago
|
||
Thanks! I can do that in a follow-up later.
Assignee | ||
Updated•14 years ago
|
Attachment #453707 -
Flags: review?(enndeakin)
Assignee | ||
Comment 46•14 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
Comment 47•14 years ago
|
||
Reviewers: if you could expedite review on this, it'd be appreciated, it's a b1 blocker.
blocking2.0: --- → beta1+
Whiteboard: [needs reviews]
Comment 48•14 years ago
|
||
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 | ||
Updated•14 years ago
|
Attachment #453605 -
Attachment is obsolete: true
Comment 50•14 years ago
|
||
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)
Comment 51•14 years ago
|
||
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•14 years ago
|
||
(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•14 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 54•14 years ago
|
||
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•14 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•14 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
Comment 57•14 years ago
|
||
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•14 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?
Comment 59•14 years ago
|
||
Can we take these discussions to a new bug, please?
Comment 60•14 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 | ||
Comment 62•14 years ago
|
||
Posting unbitrotted patches for the native appraoch and re-asking b1? blocker.. This would get us Aero Snap
Assignee | ||
Comment 63•14 years ago
|
||
Attachment #453575 -
Attachment is obsolete: true
Attachment #454231 -
Flags: review?(Olli.Pettay)
Attachment #453575 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 64•14 years ago
|
||
Attachment #453707 -
Attachment is obsolete: true
Attachment #454232 -
Flags: review?(enndeakin)
Attachment #453707 -
Flags: review?(enndeakin)
Assignee | ||
Comment 65•14 years ago
|
||
Attachment #453577 -
Attachment is obsolete: true
Attachment #454233 -
Flags: review?
Attachment #453577 -
Flags: review?(jmathies)
Reporter | ||
Updated•14 years ago
|
Reporter | ||
Comment 66•14 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 67•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/48980abe7a46
Thanks felipe!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•14 years ago
|
Whiteboard: [needs reviews]
Reporter | ||
Comment 73•14 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.
Assignee | ||
Comment 75•14 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.
Updated•14 years ago
|
Updated•14 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
You need to log in
before you can comment on or make changes to this bug.
Description
•