398928 - Allow moving a unified window by dragging its toolbar

Status: VERIFIED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect, P2)

Description

[Asaf Romano (gone)]

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Comment 2

[Josh Aas]

Does this work if you drag on a spatial separator in addition to the chrome behind buttons?

Shouldn't this bug be filed under toolkit? Really it is toolkit functionality with Firefox as its first consumer.

Comment 3

[Shawn Wilsher :sdwilsh]

If we want Bug 303110 for the beta, this needs to be in for it too.

Comment 4

[Dave Townsend [:mossop]]

This patch really doesn't do what is necessary. It suffers from the problem that if you move your mouse too fast then the cursor goes out of window and stops dragging it. This turns out to happen for me pretty much anytime I drag the window upwards unless I'm careful.

Comment 5

[Colin Barrett [:cbarrett]]

Is there a reason that it can't just grab all mouse moved events until a mouse up?

That seems like it should be easy to do...

Comment 6

[Mike Connor [:mconnor]]

Its not perfect, but a first step, and good enough for M9.

We'll want to fix this better for final.

Comment 7

[Colin Barrett [:cbarrett]]

Mossop noted on IRC that the Songbird guys do this, without the problems displayed in this patch. He said he didn't know how, but contacting them or looking at their code might be useful.

Only songbird guy I know is bent, and AIUI he's a toolkit hacker, not a frontend guy.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We use a <titlebar> element inside a xul <stack> to get this effect, see:

http://publicsvn.songbirdnest.com/browser/trunk/app/content/bindings/draggable.xml

Note that we had to hack a few things to get this to work properly, see bug 383670 and this:

http://publicsvn.songbirdnest.com/browser/trunk/patches/mozilla/mozilla_trunk/bug-toolkit-mousethrough.patch

All the JS methods we tried ended up causing subtle problems like comment 4.

Comment 9

[Neil Deakin]

Yes, you should be using <titlebar allowevents="true"> around the content that will drag (the toolbox?). <titlebar> does mouse capturing so mouse events fire even when the cursor is outside the window.

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #9)
> Yes, you should be using <titlebar allowevents="true"> around the content that
> will drag (the toolbox?). <titlebar> does mouse capturing so mouse events fire
> even when the cursor is outside the window.

Just did a quick experiment with this, made the toolbox a binding with an anonymous titlebar then all the children in that. Without allow events it did as expected, dragged the window around nicely. As soon as I set allowevents="true" the toolbar buttons become usable but I could not longer drag with it.

Comment 11

[Neil Deakin]

Attachment: something move like this

Something I just quickly put together, but I haven't tested it extensively. Not complete of course, and would need to be combined with Mano's patch.

Comment 12

[Mike Connor [:mconnor]]

We'll hold off on this for a better fix, since the new theme is not going to be in b1.

Comment 13

[Colin Barrett [:cbarrett]]

IMO dragging by the status bar is more important that dragging by the toolbox. I keep trying to do it with the new theme.

Should I open a new bug or can this one absorb that concern too?

Comment 14

[Dave Townsend [:mossop]]

Given how the new theme is looking out it'd be kinda nice to extend this so we can drag the preferences window by it's toolbar too (and presumably add-ons manager and page info if they get the same treatment).

Looks like even DOMi has a unified look now. I wonder whether on mac we should just make all toolboxes drag the main window?

Comment 18

[Mike Connor [:mconnor]]

Neil, Mano's pretty slammed, can you take a stab at this and see if its still in scope?

Comment 19

[Neil Deakin]

Attachment: updated patch

It's isn't really possible currently to detect if the mouse is over something that we as humans would think is a background. Instead, this patch allows the toolbox on Mac to be dragged if the mouse is over certain elements (hbox, toolbarspacer, etc).

However, I think this may be risky as it may break extensions that put things on the toolbar.

Comment 20

[Mike Connor [:mconnor]]

Looking at this, I don't think the risk is substantial.  As I understand it, the risk is that elements on the list in the patch might be expecting to do something else with the drag events.  If that's the case, I don't think I'm that worried, especially if its something extensions can handle in a reasonable fashion.  Can we at least look at landing this for now?

Comment 21

[Craig Francis]

I think the main issue is that on the OSX theme, you cannot see where the window title area (dragable area) ends... so its quite possible for the user to click and attempt to drag a little bit too low, where it does not drag the window, and as such does not do what the user expects.

Now taking that the main toolbar is controlled by Firefox (not an extension), would it be possible for it to handle the dragging?

It would be nice to drag the toolbars, but the main issue is just the 10px space at the top, which looks like its part of the dragable area.

Comment 22

[Mike Connor [:mconnor]]

Neil, I think we need to take this patch and see what happens, can you try for reviews ASAP?

Comment 23

[Neil Deakin]

mconnor, this patch makes anything in the <toolbox> draggable, maybe we want just the navigation toolbar and not the bookmarks toolbar or some other specific set of toolbars?

Comment 24

[[e-mail]]

(In reply to comment #19)
> It's isn't really possible currently to detect if the mouse is over something
> that we as humans would think is a background.
Don't all controls get an interesting frame, usually a button or menu?

Comment 25

[Mike Connor [:mconnor]]

Neil: that may be true, but the visible button is often smaller than the actual element, especially in the current Mac theme.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 312312 [details] [diff] [review]
updated patch

A bit hacky, but OK. Just check for the XUL namespace as well, though.

I'm pleased that the display list framework makes this hack actually reasonably clean to implement :-).

Comment 27

[[e-mail]]

(In reply to comment #25)
> Neil: that may be true, but the visible button is often smaller than the actual
> element, especially in the current Mac theme.
Hoping that you're taking to me, what my question actually meant was "wouldn't it be more reliable to check the frame against a list of known frame types instead of checking the content tag against a list of known tag names."

Also I think Enn can use <content allowevents="true"/> in his binding.

Comment 28

[Neil Deakin]

(In reply to comment #27)
> Hoping that you're taking to me, what my question actually meant was "wouldn't
> it be more reliable to check the frame against a list of known frame types
> instead of checking the content tag against a list of known tag names."

Most frames don't return anything interesting for GetType. That said, I think that if even if they did, we would still need a whitelist so I don't see that this would be less hacky.
 

Comment 29

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

(In reply to comment #23)
> mconnor, this patch makes anything in the <toolbox> draggable, maybe we want
> just the navigation toolbar and not the bookmarks toolbar or some other
> specific set of toolbars?

Anywhere within the area of toolbars on the browser window, add-ons manager, library (nee: bookmarks organizer), page info dialog, and options dialog should be either a button or an area that can drag the window. So sayeth OSX, so say we all.

I wouldn't expect the tabstrip or anything that takes -moz-appearance: toolbox to be draggable, though, of course.

Comment 30

[[e-mail]]

Comment on attachment 312312 [details] [diff] [review]
updated patch

>+  <binding id="toolbox-drag" display="xul:titlebar"
>+           extends="chrome://global/content/bindings/toolbar.xml#toolbox">
>+    <implementation>
>+      <constructor>
>+        this.setAttribute("allowevents", "true");
>+      </constructor>
>+    </implementation>
>+  </binding>
It's not clear why this needs allowevents="true" but assuming it does I'd write it as <content allowevents="true"/>.

Comment 31

[Neil Deakin]

(In reply to comment #30)
> It's not clear why this needs allowevents="true" but assuming it does

Otherwise, the mouse events would get redirected to the titlebar is all cases. We only want that for backgrounds.

> I'd write it as <content allowevents="true"/>.

OK, I'll make that change.

Comment 32

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

Comment on attachment 312312 [details] [diff] [review]
updated patch

a1.9=beltzner

Comment 33

[Neil Deakin]

I checked this in, but now I realize that it will prevent context-clicking on the toolbar as well. Should probably back this out.

Comment 34

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

Accursed customization! Yes, please back it out.

Comment 35

[Neil Deakin]

Roc, see comment 33. Is it possible to restrict the HitTesting to only left-clicks?

Comment 36

[Shawn Wilsher :sdwilsh]

After playing a bit with the nightly, I'd like to bring up a few points about this:
1) It's weird to be able to drag my window with my bookmarks toolbar
2) I cannot drag my window with the statusbar (although I suppose that's not the toolbox, it's still expected on the mac I think)
3) This is really cool!

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You could add event information to nsDisplayListBuilder so that when we're doing hit-testing for an event, the button is available. 

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Further to the issue I filed with bug 429742 you will also not be able to d&d any bookmark location from the location bar onto the Bookmarks Toolbar. Shall I file a new bug or will we cover it with the back-out of the patch?

Comment 39

[Asa Dotzler [:asa]]

I also noticed that dragging from chrome around the search box in the primary toolbar does not work.

Comment 40

[Asa Dotzler [:asa]]

sorry, nevermind. disregard my previous comment. there's nothing special around the search bar.

Comment 41

[J. Gatellier]

(In reply to comment #40)
> sorry, nevermind. disregard my previous comment. there's nothing special around
> the search bar.

No, what you said in Comment #39 is true there are some inconsistencies, you can drag from chrome just under the URL bar but not from under the search bar.

Comment 42

[Neil Deakin]

The bug should be assumed to have a backout-needed keyword set if there was one. I won't be able to in the short term.

Comment 43

[Neil Deakin]

OK, backed it out

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Clearing whiteboard due to backout of the patch.

Comment 45

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

Prior to backout, this probably caused bug 430167 as well

Comment 46

[Mike Connor [:mconnor]]

We're going to need to punt on this until 3.next, since there's clearly some regression risk here, and I don't t hink we can continue to block on this.

Comment 47

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 312312 [details] [diff] [review]
updated patch

Removing approval1.9+ per schrep's request.

Comment 50

[Kris Hunt]

I'm not a programmer, but after using Firefox 3 for a couple days, I am absolutely appalled that you cannot drag a window by any part of the chrome, including the status bar. You claim how much better Firefox is than Safari, but for Pete's sake, at least Safari lets you drag the danged window without having to guess where the invisible cutoff is between draggable and undraggable areas.

Comment 53

[Markus Stange [:mstange]]

Attachment: change Mano's patch to use mouse capturing

Just for the record, this is all that's needed to make Mano's patch work correctly: The mousemove handler just has to be set on the window, not on the toolbox. When the mouse button is held down, the window automatically does mouse capturing.

(Mano, sorry for the confusion on IRC; what I said about changing nsFrame.cpp was rubbish.)

Comment 54

[Markus Stange [:mstange]]

Attachment: new approach, rev1

This patch makes everything draggable that has the dark chrome color (except the tabstrip):
 - All <toolbar>s:
     We default to the dark chrome color for generic toolbars, so we should
     default to draggability, too.
 - Status bars
 - Unified toolbars (<radiogroup> or <stack>) of the Preferences window, the
   Addons manager and the Page Info dialog
 - The Downloads Manager bottom bar

Unlike in Neil's patch, the bindings aren't set in an #ifdef'd part of xul.css, but in the theme. That way, every theme can decide which elements to make draggable, regardless of OS.
I chose not to make the whole toolbox draggable, but the toolbars in it one by one, because themes might want to make individual toolbars in a toolbox not draggable (e.g. because they're styled differently).

I didn't continue Neil's approach for the following reasons:
 - I don't like messing with display lists, it feels hacky.
 - Extending display lists to do different things based on the pressed mouse
   button feels even more hacky.
 - I couldn't think of a solution to bug 430167, which was caused by Neil's
   patch: The drag drop event is directly sent to the titlebar frame, which
   doesn't know how to drop bookmarks.
 - Dragging shouldn't be possible while customizing the toolbar.

This patch also makes the throbber in the navbar draggable.

Comment 55

[Markus Stange [:mstange]]

There's another difference to Mano's patch: The drag is initiated on the mousedown event instead of the draggesture event. Drag gestures have a certain threshold (I think the first 5 pixels or so are ignored) that I don't want.

We can't just remove the drag threshold completely because it's there for a reason; e.g. when dragging a tab, the threshold prevents accidental drags.

Comment 56

[Dão Gottwald [:dao]]

Comment on attachment 339412 [details] [diff] [review]
new approach, rev1

>+++ b/toolkit/content/WindowDraggingUtils.jsm

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");

>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMEventListener])

QueryInterface (and hence XPCOMUtils.jsm) isn't needed, is it?

Comment 57

[Markus Stange [:mstange]]

I thought it's needed because of
 this._window.addEventListener("mousemove", this, false);
                                            ^^^^
but I haven't tested if it works without it.

Comment 58

[Dão Gottwald [:dao]]

It should work without it.

Comment 59

[Markus Stange [:mstange]]

Attachment: rev2: remove QI stuff

You're right, it does work without it.

Comment 60

[Neil Deakin]

This approach is actually more hacky, since you're recreating what the <titlebar> frame is supposed to already be doing.

Also, you're using hardcoded moz-binding references to toolkit widgets in various places outside of there.

Comment 61

[Neil Deakin]

That said, having the theme be able to override the behaviour, and allowing other non-toolbars to be window-draggable is an advantage of using the Mano/Markus approach.

Comment 62

[Markus Stange [:mstange]]

(In reply to comment #60)
> This approach is actually more hacky, since you're recreating what the
> <titlebar> frame is supposed to already be doing.

True.

> Also, you're using hardcoded moz-binding references to toolkit widgets in
> various places outside of there.

Is that a bad thing? Doesn't browser depend on toolkit?

Comment 63

[Neil Deakin]

This patch seems to just make a bunch of arbitrary elements have a window-drag capability, but those are quite specific to where they are used.

The radiogroup for example only makes sense to have this in the specific case where its used. (I assume the extension manager is using a radiogroup because the preference window does this absuridty as well, but that's a different issue).

It seems better to just have an element which wraps all of these, but then that's just the titlebar.

You shouldn't be requiring code outside of toolkit/content/widgets to be referring to bindings that way.

Comment 64

[Markus Stange [:mstange]]

(In reply to comment #63)
> This patch seems to just make a bunch of arbitrary elements have a window-drag
> capability, but those are quite specific to where they are used.

Right. I tried to set the drag bindings in the same places where the appearance for these elements is specified, since draggability is directly related to their dark grey look. The fact that these elements are made of a number of different element types (toolbar, statusbar, stack, radiogroup, hbox) is unfortunate but has been that way for a long time.

> The radiogroup for example only makes sense to have this in the specific case
> where its used.

Which is why I set the binding there, and not in a generic radiogroup rule.

> (I assume the extension manager is using a radiogroup because
> the preference window does this absuridty as well, but that's a different
> issue).

I don't know why the extension manager uses a radiogroup.

> It seems better to just have an element which wraps all of these, but then
> that's just the titlebar.

I don't understand this comment. Do you want me to make changes to the cross-platform XUL code and wrap all these draggable elements in <titlebar> elements?

> You shouldn't be requiring code outside of toolkit/content/widgets to be
> referring to bindings that way.

Wouldn't that break the theme's control over draggability?

Why exactly should code outside of toolkit/content/widgets not refer to bindings?

Comment 65

[Neil Deakin]

(In reply to comment #64)
> > The radiogroup for example only makes sense to have this in the specific case
> > where its used.
> 
> Which is why I set the binding there, and not in a generic radiogroup rule.
> 

But you didn't do that. You put the binding in radio.xml.

Comment 66

[Markus Stange [:mstange]]

Attachment: rev3: move it out of radio.xml

I moved it into toolkit/mozapps/extensions/content/extensions.xml (for the Add-ons Manager) and to toolkit/content/widgets/preferences.xml (for the Preferences window).
I also removed an unused binding from browser.xml.

Comment 67

[Neil Deakin]

Patch comments:

+Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Why did you add this back in?
 
+        let target = aEvent.target, parent = aEvent.target;
+        while (parent != this._elem) {
+          if (parent.getAttribute("mousethrough") == "always")

The mousethrough can also be overridden on a child with mousethrough="never".

+
+      <!-- Set by customizeToolbar.js -->
+      <field name="_customizing">false</field>
+      <property name="customizing"
+                onget="return this._customizing"
+                onset="return this._customizing = val;"/>

A field called 'customizing' would accomplish the same thing.
 
 #viewGroup {
-  -moz-binding: url("chrome://mozapps/skin/extensions/extensions.xml#radiogroupWrapper");
+  -moz-binding: url("chrome://mozapps/skin/extensions/extensions.xml#radiogroupWrapper-drag");

Why this change? You could just leave the binding id as is.

You could also make box-drag a new element, and make just a single binding instead of some of the bindings you've added:

- extensions can use this on the element that is radiogroupWrapper.
- for downloads, #search is an hbox which can use this.
- the preferences pane selector can just be wrapped in such an element.

If you don't want to do that, the box-drag binding should at least be moved into the downloads code somewhere.

I think toolbar/statusbar would work better as toolbar[windowdrag="true"] or somesuch with a binding applied to that (as in my patch).

Then, a theme can just disable dragging using:

toolbar[windowdrag="true"] { -moz-binding: none; }

This isn't perfect, but it avoids the theme needing to know details about the binding implementation in order to enable it, and as a side effect the theme can't enable dragging on toolbars that shouldn't have it.

Comment 68

[Markus Stange [:mstange]]

Attachment: rev4: address review comments

(In reply to comment #67)
> Patch comments:
> 
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> 
> Why did you add this back in?

Oops. That's what I get from not syncing my patches across my trees.

> 
> +        let target = aEvent.target, parent = aEvent.target;
> +        while (parent != this._elem) {
> +          if (parent.getAttribute("mousethrough") == "always")
> 
> The mousethrough can also be overridden on a child with mousethrough="never".

I changed the logic to take that into consideration.

> +
> +      <!-- Set by customizeToolbar.js -->
> +      <field name="_customizing">false</field>
> +      <property name="customizing"
> +                onget="return this._customizing"
> +                onset="return this._customizing = val;"/>
> 
> A field called 'customizing' would accomplish the same thing.

Good idea.

> You could also make box-drag a new element, and make just a single binding
> instead of some of the bindings you've added:

That's a great idea. Done. (I hope I understood this correctly.)

> - extensions can use this on the element that is radiogroupWrapper.

I'm going to remove the radiogroupWrapper in bug 456214, so I set it on the radiogroup's parent element (a stack, #topStackBar). I did the same for the pageInfo dialog.

> - for downloads, #search is an hbox which can use this.
> - the preferences pane selector can just be wrapped in such an element.

Done.

> I think toolbar/statusbar would work better as toolbar[windowdrag="true"] or
> somesuch with a binding applied to that (as in my patch).

I did that in this patch, but I don't like it. In my opinion, all toolbars and statusbars should default to draggability. I don't think XUL apps / extensions should be required to add additional attributes in order to get the platform's default behavior.
Moreover, it creates a discrepancy between statusbars' and toolbars' default styling (dark gray) and their default behavior (no draggability).

I can see two alternatives here:
 (1) Make windowdrag "default to true" - i.e. set the drag binding for
     toolbar/statusbar and the non-drag binding for
     toolbar/statusbar[windowdrag="false"]
 (2) Change pinstripe's defaults toolbar/statusbar styling to a light gray
     and only use the dark gray for toolbar/statusbar[windowdrag="true].
I'm in favor of (1).

> This isn't perfect, but it avoids the theme needing to know details about the
> binding implementation in order to enable it,

It still needs to know them for all these other draggable things that are not toolbars or statusbars. I don't think it's that much of an improvement to justify taking it out of the theme's control.

> and as a side effect the theme
> can't enable dragging on toolbars that shouldn't have it.

It can still enable it by setting the binding, can't it?
And I don't think that limiting themes in such a way is desirable.

Thanks for the thorough review!

Comment 69

[Neil Deakin]

> I did that in this patch, but I don't like it. In my opinion, all toolbars and
> statusbars should default to draggability.

OK, that seems to make more sense then. So what you had before for toolbar/statusbar is ok, although I think xul.css would be a better place for the binding references. Not as big a deal though.

>  (1) Make windowdrag "default to true" - i.e. set the drag binding for
>      toolbar/statusbar and the non-drag binding for
>      toolbar/statusbar[windowdrag="false"]

This is also a good idea, except that attribute name should be reversed in some way such that the default can be false, and setting it to true makes it non-draggable.

Comment 70

[Neil Deakin]

> > You could also make box-drag a new element, and make just a single binding
> > instead of some of the bindings you've added:
> 
> That's a great idea. Done. (I hope I understood this correctly.)

By this I mean create a new element tag. (<windowdragbox> or somesuch, maybe in
the future we could just remove the nsTitleBarFrame and share this binding if
it works better) Authors outside of the toolkit directory should be treating
the bindings as opaquely as possible. If people are using specific binding
urls, it makes it harder to change later if we decide to redo this feature
differently.

Comment 71

[Markus Stange [:mstange]]

Attachment: rev5: use <windowdragbox>

(In reply to comment #69)
> > I did that in this patch, but I don't like it. In my opinion, all toolbars and
> > statusbars should default to draggability.
> 
> OK, that seems to make more sense then. So what you had before for
> toolbar/statusbar is ok, although I think xul.css would be a better place for
> the binding references. Not as big a deal though.

I moved it to global.css to emphasize the correlation to the appearance.

> 
> >  (1) Make windowdrag "default to true" - i.e. set the drag binding for
> >      toolbar/statusbar and the non-drag binding for
> >      toolbar/statusbar[windowdrag="false"]
> 
> This is also a good idea, except that attribute name should be reversed in some
> way such that the default can be false, and setting it to true makes it
> non-draggable.

I called it "nowindowdrag"... not very creative, I know.

Comment 72

[Neil Deakin]

Comment on attachment 340925 [details] [diff] [review]
rev5: use <windowdragbox>

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

This last one should be in xul.css though. No reason to limit this element to Mac only.

Comment 73

[Markus Stange [:mstange]]

(In reply to comment #72)
> (From update of attachment 340925 [details] [diff] [review])
> >+windowdragbox {
> >+  -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");
> > }
> 
> This last one should be in xul.css though. No reason to limit this element to
> Mac only.

But since it's used the cross-platform XUL code, wouldn't that mean that all the things that I wrapped in it become draggable on other platforms, too? I don't think we want that...

Or should I ifdef the <windowdragbox> tags in the XUL code?

Comment 74

[Markus Stange [:mstange]]

Attachment: fix preferences pane switching

Wrapping the preference window's radio group in a <windowdrag> element broke prefpane switching. This patch restores the functionality.
(The original line is from bug 274712, which was largely unreviewed.)

Comment 75

[Dave Townsend [:mossop]]

Just glanced over the latest patches here, but doesn't it still suffer the same problem I already described in comment 4?

Comment 76

[Markus Stange [:mstange]]

It doesn't suffer from that problem - see comment 53 for an explanation.

Comment 77

[Markus Stange [:mstange]]

pushed:
http://hg.mozilla.org/mozilla-central/rev/151e51ec625e
http://hg.mozilla.org/mozilla-central/rev/aff392727ff4

I didn't address comment 72; see comment 73.

Comment 78

[Dave Townsend [:mossop]]

(In reply to comment #73)
> (In reply to comment #72)
> > (From update of attachment 340925 [details] [diff] [review] [details])
> > >+windowdragbox {
> > >+  -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");
> > > }
> > 
> > This last one should be in xul.css though. No reason to limit this element to
> > Mac only.
> 
> But since it's used the cross-platform XUL code, wouldn't that mean that all
> the things that I wrapped in it become draggable on other platforms, too? I
> don't think we want that...
> 
> Or should I ifdef the <windowdragbox> tags in the XUL code?

Did Neil agree over IRC or email or something to leave the binding style where it was? I think it would be useful to just have this defined for all platforms.

Probably not worth a bug but I did notice that the dragging behaviour of windowdragbox doesn't match that of the title bar. When I have my second monitor on to the right of the main screen I cannot drag the window to the left into the mac menu bar with the titlebar, however it is possible with the windowdragbox.

Comment 79

[Markus Stange [:mstange]]

I backed out because test_statusbar.xul was failing.

Comment 80

[Markus Stange [:mstange]]

(In reply to comment #78)
> Did Neil agree over IRC or email or something to leave the binding style where
> it was?

I didn't ask him because he's apparently away. Instead I asked the other Neil on IRC; he didn't see the point of comment 72 either.

> I think it would be useful to just have this defined for all platforms.

I agree that it would be useful, I just don't know what to do about comment 73.
Let's just wait for Neil's return before relanding.

> Probably not worth a bug but I did notice that the dragging behaviour of
> windowdragbox doesn't match that of the title bar. When I have my second
> monitor on to the right of the main screen I cannot drag the window to the left
> into the mac menu bar with the titlebar, however it is possible with the
> windowdragbox.

Uh. I have no idea if this is something we can fix.

---

The unit test orange was caused by this line in the bindings' constructor:
Components.utils.import("resource://gre/modules/WindowDraggingUtils.jsm");

Accessing Components.utils from a XUL document without sufficient privileges fails, so whenever a XUL content document uses a windowdragbox, toolbar or statusbar element, a "Permission denied" error fires.

Comment 81

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #79)
> I backed out because test_statusbar.xul was failing.

If you need to back out again, please ask someone from RelEng to run
 find /builds/moz2_slave/mozilla-central-macosx{,-debug}/build/obj-firefox -name 'WindowDraggingUtils.jsm' -exec rm -v {} \;
on bm-xserve16 thru 19. This prevents this bustage
 rsync warning: some files vanished before they could be transferred (code 24) at /SourceCache/rsync/rsync-30/rsync/main.c(717)
during packaging.

Comment 82

[Markus Stange [:mstange]]

(In reply to comment #80)
> Accessing Components.utils from a XUL document without sufficient privileges
> fails, so whenever a XUL content document uses a windowdragbox, toolbar or
> statusbar element, a "Permission denied" error fires.

Neil, what should I do about this? Should I simply wrap the constructor in
try { ... } catch (e) {} ?

Comment 83

[Neil Deakin]

(In reply to comment #73)
> (In reply to comment #72)
> > (From update of attachment 340925 [details] [diff] [review] [details])
> > >+windowdragbox {
> > >+  -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");
> > > }
> > 
> > This last one should be in xul.css though. No reason to limit this element to
> > Mac only.
> 
> But since it's used the cross-platform XUL code, wouldn't that mean that all
> the things that I wrapped in it become draggable on other platforms, too? I
> don't think we want that...

I suppose it doesn't matter too much. It depends on whether we expect the drag behaviour by default and themes have to disable it, or whether themes have to explicitly enable it by setting the binding.

Comment 84

[Markus Stange [:mstange]]

Attachment: suppress "Permission denied" errors

This patch simply wraps all binding constructors in try { ... } catch (e) {}. Is this the right way to go?

I also added "stack" to the dragTags because otherwise the Add-ons manager's toolbar isn't draggable (bug 459655).

Comment 85

[Markus Stange [:mstange]]

pushed: http://hg.mozilla.org/mozilla-central/rev/b66468507a59

Comment 86

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081019 Minefield/3.1b2pre ID:20081019020302

Tested on toolbars, statusbar, and throbber in main window, pop-ups windows, preferences, view source, library, error console, add-ons manager and download manager. Further everything works fine in the customize toolbar dialog, means all elements can be added/removed by d&d which includes the throbber.

There is only one small issue with the bookmarks toolbar which I'll file as a new bug.

Beside that shouldn't we make the faked statusbar in the add-ons manager a real statusbar like we have in the download manager?

Comment 87

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Remaining issue filed as bug 460682.

Comment 89

[Hordur Freyr Yngvason]

This is a great improvement, but still needs some tweaking. For instance, moving an inactive window by dragging anywhere except title bar only works with cmd-drag.

Also, moving between spaces only seems to work by dragging the title bar.

I'm using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081118 Minefield/3.1b2pre

Comment 90

[Shawn Wilsher :sdwilsh]

(In reply to comment #89)
> This is a great improvement, but still needs some tweaking. For instance,
> moving an inactive window by dragging anywhere except title bar only works with
> cmd-drag.
> 
> Also, moving between spaces only seems to work by dragging the title bar.
You should file new bugs on those issues.  They won't get looked at if you just post a comment to a resolved bug.

Comment 91

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #90)
> You should file new bugs on those issues.  They won't get looked at if you just
> post a comment to a resolved bug.

Further you should add this bug to the depends field of your new bugs so we will have a connection between them. Thanks.