Closed
Bug 398928
Opened 16 years ago
Closed 15 years ago
Allow moving a unified window by dragging its toolbar
Categories
(Toolkit :: Toolbars and Toolbar Customization, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b2
People
(Reporter: asaf, Assigned: mstange)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(6 files, 5 obsolete files)
4.73 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
8.83 KB,
patch
|
neil
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
2.11 KB,
patch
|
Details | Diff | Splinter Review | |
21.81 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
872 bytes,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
3.79 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
Flags: blocking-firefox3?
Reporter | ||
Updated•16 years ago
|
Priority: -- → P2
Reporter | ||
Comment 1•16 years ago
|
||
Attachment #283925 -
Flags: review?(mconnor)
Reporter | ||
Updated•16 years ago
|
Summary: Allow moving unified windows by dragging their windows → Allow moving a unified window by dragging its toolbox
Updated•16 years ago
|
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.
Comment 3•16 years ago
|
||
If we want Bug 303110 for the beta, this needs to be in for it too.
Target Milestone: Firefox 3 M10 → Firefox 3 M9
Updated•16 years ago
|
Attachment #283925 -
Flags: review?(mconnor) → review+
Comment 4•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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]
Comment 7•16 years ago
|
||
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.
Comment 9•16 years ago
|
||
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•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
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
Comment 13•16 years ago
|
||
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•16 years ago
|
||
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?
Reporter | ||
Updated•16 years ago
|
Whiteboard: [has patch][has reviews] → [needs new patch][pending proto landing]
Updated•16 years ago
|
Target Milestone: Firefox 3 beta2 → Firefox 3
Comment 18•16 years ago
|
||
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]
Comment 19•16 years ago
|
||
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
Comment 20•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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]
Updated•16 years ago
|
Attachment #312312 -
Flags: superreview?(roc)
Attachment #312312 -
Flags: review?(neil)
Comment 23•16 years ago
|
||
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
Comment 24•16 years ago
|
||
(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•16 years ago
|
||
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+
Updated•16 years ago
|
Whiteboard: [decision needed] → [has patch][needs review neil r]
Comment 27•16 years ago
|
||
(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•16 years ago
|
||
(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•16 years ago
|
||
(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•16 years ago
|
||
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+
Updated•16 years ago
|
Whiteboard: [has patch][needs review neil r] → [has patch][has reviews]
Comment 31•16 years ago
|
||
(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.
Updated•16 years ago
|
Attachment #312312 -
Flags: approval1.9?
Comment 32•16 years ago
|
||
Comment on attachment 312312 [details] [diff] [review] updated patch a1.9=beltzner
Attachment #312312 -
Flags: approval1.9? → approval1.9+
Comment 33•16 years ago
|
||
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•16 years ago
|
||
Accursed customization! Yes, please back it out.
Comment 35•16 years ago
|
||
Roc, see comment 33. Is it possible to restrict the HitTesting to only left-clicks?
Comment 36•16 years ago
|
||
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.
Comment 38•16 years ago
|
||
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?
Updated•16 years ago
|
Version: unspecified → Trunk
Comment 39•16 years ago
|
||
I also noticed that dragging from chrome around the search box in the primary toolbar does not work.
Comment 40•16 years ago
|
||
sorry, nevermind. disregard my previous comment. there's nothing special around the search bar.
Comment 41•16 years ago
|
||
(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.
Updated•16 years ago
|
Summary: Allow moving a unified window by dragging its toolbox → Allow moving a unified window by dragging its toolbar
Comment 42•16 years ago
|
||
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.
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [has patch][has reviews] → [has patch][has reviews] (needs backout, per comment 42)
Comment 43•16 years ago
|
||
OK, backed it out
Keywords: checkin-needed
Whiteboard: [has patch][has reviews] (needs backout, per comment 42) → [has patch][has reviews]
Comment 44•16 years ago
|
||
Clearing whiteboard due to backout of the patch.
Hardware: PC → All
Whiteboard: [has patch][has reviews]
Comment 45•16 years ago
|
||
Prior to backout, this probably caused bug 430167 as well
Comment 46•16 years ago
|
||
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+
Updated•16 years ago
|
Flags: blocking-firefox3.1?
Comment 50•16 years ago
|
||
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.
Updated•16 years ago
|
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Assignee | ||
Updated•15 years ago
|
Assignee: enndeakin → mstange
Assignee | ||
Comment 53•15 years ago
|
||
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.)
Assignee | ||
Comment 54•15 years ago
|
||
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)
Assignee | ||
Comment 55•15 years ago
|
||
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•15 years ago
|
||
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?
Assignee | ||
Comment 57•15 years ago
|
||
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•15 years ago
|
||
It should work without it.
Assignee | ||
Comment 59•15 years ago
|
||
You're right, it does work without it.
Attachment #339412 -
Attachment is obsolete: true
Attachment #339412 -
Flags: review?(mano)
Assignee | ||
Updated•15 years ago
|
Attachment #339423 -
Flags: review?(mano)
Comment 60•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Attachment #339423 -
Flags: review?(mano) → review?(enndeakin)
Assignee | ||
Comment 62•15 years ago
|
||
(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•15 years ago
|
||
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.
Assignee | ||
Comment 64•15 years ago
|
||
(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•15 years ago
|
||
(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.
Assignee | ||
Comment 66•15 years ago
|
||
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)
Comment 67•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #340532 -
Flags: review?(enndeakin) → review-
Assignee | ||
Comment 68•15 years ago
|
||
(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)
Comment 69•15 years ago
|
||
> 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•15 years ago
|
||
> > 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.
Assignee | ||
Comment 71•15 years ago
|
||
(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 72•15 years ago
|
||
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+
Assignee | ||
Comment 73•15 years ago
|
||
(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?
Assignee | ||
Comment 74•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #341163 -
Flags: review?(neil) → review+
Comment 75•15 years ago
|
||
Just glanced over the latest patches here, but doesn't it still suffer the same problem I already described in comment 4?
Assignee | ||
Comment 76•15 years ago
|
||
It doesn't suffer from that problem - see comment 53 for an explanation.
Assignee | ||
Comment 77•15 years ago
|
||
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: 15 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3 → Firefox 3.1b2
Comment 78•15 years ago
|
||
(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.
Assignee | ||
Comment 79•15 years ago
|
||
I backed out because test_statusbar.xul was failing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 80•15 years ago
|
||
(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•15 years ago
|
||
(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.
Assignee | ||
Comment 82•15 years ago
|
||
(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•15 years ago
|
||
(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.
Assignee | ||
Comment 84•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #343441 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 85•15 years ago
|
||
pushed: http://hg.mozilla.org/mozilla-central/rev/b66468507a59
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Comment 86•15 years ago
|
||
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?
Updated•15 years ago
|
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 → ---
Updated•15 years ago
|
Target Milestone: --- → mozilla1.9.1b2
Updated•15 years ago
|
Assignee: nobody → mstange
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Updated•15 years ago
|
Comment 89•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•