Closed Bug 398928 Opened 13 years ago Closed 11 years ago

Allow moving a unified window by dragging its toolbar

Categories

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

All
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: mano, Assigned: mstange)

References

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

Details

Attachments

(6 files, 5 obsolete files)

Summary: Allow moving unified windows by dragging their windows → Allow moving a unified window by dragging its toolbox
Flags: blocking-firefox3? → blocking-firefox3+
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.
If we want Bug 303110 for the beta, this needs to be in for it too.
Target Milestone: Firefox 3 M10 → Firefox 3 M9
Attachment #283925 - Flags: review?(mconnor) → review+
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.
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...
Its not perfect, but a first step, and good enough for M9.

We'll want to fix this better for final.
Whiteboard: [has patch][has reviews]
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.
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.
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.
(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.
Attached patch something move like this (obsolete) — Splinter Review
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.
We'll hold off on this for a better fix, since the new theme is not going to be in b1.
Target Milestone: Firefox 3 M9 → Firefox 3 M10
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?
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?
Whiteboard: [has patch][has reviews] → [needs new patch][pending proto landing]
Duplicate of this bug: 414722
Blocks: 417581
Target Milestone: Firefox 3 beta2 → Firefox 3
Duplicate of this bug: 422393
Duplicate of this bug: 424327
Neil, Mano's pretty slammed, can you take a stab at this and see if its still in scope?
Assignee: mano → enndeakin
Status: ASSIGNED → NEW
Whiteboard: [needs new patch][pending proto landing] → [needs status update]
Attached patch updated patchSplinter Review
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.
Attachment #286841 - Attachment is obsolete: true
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?
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.
Neil, I think we need to take this patch and see what happens, can you try for reviews ASAP?
Whiteboard: [needs status update] → [decision needed]
Attachment #312312 - Flags: superreview?(roc)
Attachment #312312 - Flags: review?(neil)
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?
Status: NEW → ASSIGNED
(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?
Neil: that may be true, but the visible button is often smaller than the actual element, especially in the current Mac theme.
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 :-).
Attachment #312312 - Flags: superreview?(roc) → superreview+
Whiteboard: [decision needed] → [has patch][needs review neil r]
(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.
(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.
 
(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 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"/>.
Attachment #312312 - Flags: review?(neil) → review+
Whiteboard: [has patch][needs review neil r] → [has patch][has reviews]
(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.

Attachment #312312 - Flags: approval1.9?
Comment on attachment 312312 [details] [diff] [review]
updated patch

a1.9=beltzner
Attachment #312312 - Flags: approval1.9? → approval1.9+
I checked this in, but now I realize that it will prevent context-clicking on the toolbar as well. Should probably back this out.
Accursed customization! Yes, please back it out.
Roc, see comment 33. Is it possible to restrict the HitTesting to only left-clicks?
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!
You could add event information to nsDisplayListBuilder so that when we're doing hit-testing for an event, the button is available. 
Depends on: 429742
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?
Version: unspecified → Trunk
I also noticed that dragging from chrome around the search box in the primary toolbar does not work.
sorry, nevermind. disregard my previous comment. there's nothing special around the search bar.
(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.
Summary: Allow moving a unified window by dragging its toolbox → Allow moving a unified window by dragging its toolbar
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.
Keywords: checkin-needed
Whiteboard: [has patch][has reviews] → [has patch][has reviews] (needs backout, per comment 42)
OK, backed it out
Keywords: checkin-needed
Whiteboard: [has patch][has reviews] (needs backout, per comment 42) → [has patch][has reviews]
Clearing whiteboard due to backout of the patch.
Hardware: PC → All
Whiteboard: [has patch][has reviews]
Prior to backout, this probably caused bug 430167 as well
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.
Flags: blocking-firefox3+ → blocking-firefox3-
Comment on attachment 312312 [details] [diff] [review]
updated patch

Removing approval1.9+ per schrep's request.
Attachment #312312 - Flags: approval1.9+
Duplicate of this bug: 433056
Duplicate of this bug: 436124
Flags: blocking-firefox3.1?
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.
Duplicate of this bug: 444194
Duplicate of this bug: 443697
Blocks: 449644
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Assignee: enndeakin → mstange
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.)
Attached patch new approach, rev1 (obsolete) — Splinter Review
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.
Attachment #339412 - Flags: review?(mano)
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 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?
I thought it's needed because of
 this._window.addEventListener("mousemove", this, false);
                                            ^^^^
but I haven't tested if it works without it.
It should work without it.
Attached patch rev2: remove QI stuff (obsolete) — Splinter Review
You're right, it does work without it.
Attachment #339412 - Attachment is obsolete: true
Attachment #339412 - Flags: review?(mano)
Attachment #339423 - Flags: review?(mano)
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.
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.
Attachment #339423 - Flags: review?(mano) → review?(enndeakin)
(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?
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.
(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?
(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.
Attached patch rev3: move it out of radio.xml (obsolete) — Splinter Review
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.
Attachment #339423 - Attachment is obsolete: true
Attachment #340532 - Flags: review?(enndeakin)
Attachment #339423 - Flags: review?(enndeakin)
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.
Attachment #340532 - Flags: review?(enndeakin) → review-
Attached patch rev4: address review comments (obsolete) — Splinter Review
(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!
Attachment #340532 - Attachment is obsolete: true
Attachment #340791 - Flags: review?(enndeakin)
> 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.
> > 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.
(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.
Attachment #340791 - Attachment is obsolete: true
Attachment #340925 - Flags: review?(enndeakin)
Attachment #340791 - Flags: review?(enndeakin)
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.
Attachment #340925 - Flags: review?(enndeakin) → review+
(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?
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.)
Attachment #341163 - Flags: review?(neil)
Attachment #341163 - Flags: review?(neil) → review+
Just glanced over the latest patches here, but doesn't it still suffer the same problem I already described in comment 4?
It doesn't suffer from that problem - see comment 53 for an explanation.
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3 → Firefox 3.1b2
(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.
I backed out because test_statusbar.xul was failing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
(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.
(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) {} ?
(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.
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).
Attachment #343441 - Flags: review?(enndeakin)
Attachment #343441 - Flags: review?(enndeakin) → review+
pushed: http://hg.mozilla.org/mozilla-central/rev/b66468507a59
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Assignee: mstange → nobody
Component: General → Toolbars and Toolbar Customization
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1-
Flags: blocking-firefox3-
Keywords: pp
Product: Firefox → Toolkit
QA Contact: general → toolbars
Target Milestone: Firefox 3.1b2 → ---
Target Milestone: --- → mozilla1.9.1b2
Assignee: nobody → mstange
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Remaining issue filed as bug 460682.
Blocks: 460682
No longer blocks: 460682
Depends on: 460682
Keywords: pp
Duplicate of this bug: 461777
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
(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.
(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.
Blocks: 465542
Blocks: 465590
Blocks: 467184
Depends on: 477751
You need to log in before you can comment on or make changes to this bug.