Closed Bug 886444 Opened 7 years ago Closed 6 years ago

[Australis] Can't drag window while in customize mode

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: liuche, Assigned: jaws)

References

Details

(Whiteboard: [Australis:M9][Australis:P2])

Attachments

(3 files)

Window should be draggable when in customize mode.

STR:
Enter customize mode
Click on window bar and drag

Expected:
Window moves like all other Firefox windows

Actual:
Unresponsive to dragging, sometimes exits customize mode.
Depends on: 881909
Whiteboard: [Australis:M?][Australis:P1]
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M?][Australis:P2]
Blocks: 881909
Component: General → Toolbars and Customization
No longer depends on: 881909
I noticed this on Windows as well, so marking as x-platform.
OS: Mac OS X → All
Hardware: x86 → All
To my uninitiated mind, it looks like this was done on purpose:

http://hg.mozilla.org/projects/ux/file/da2be617863d/browser/components/customizableui/content/toolbar.xml#l323

Mike, this seems to have been something you did in bug 864425, but I didn't see it being discussed in a quick skim of the comments. Can we just remove that check? Was there a particular reason you disabled this - will it break customization dnd?
Flags: needinfo?(mconley)
Assignee: nobody → gijskruitbosch+bugs
This WFM on mac, at least. Will test on Windows in a second. However, we probably also want to do something about the fact that the margin on the window isn't draggable, assuming that doesn't lose us the customization exiting behaviour...
Status: NEW → ASSIGNED
Flags: needinfo?(mconley)
This patch seems to work on Windows as well. I couldn't figure out how to make WindowDraggingUtils behave when passing it a window as an element - it was just breaking exiting customization mode, not enabling dragging. I'm not sure if/how we can fix that, but this patch should at least be a good first step in regaining some measure of draggability.
Comment on attachment 776998 [details] [diff] [review]
Should not stop people dragging the window while customizing

Review of attachment 776998 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to :Gijs Kruitbosch from comment #2)
> To my uninitiated mind, it looks like this was done on purpose:
> 
> http://hg.mozilla.org/projects/ux/file/da2be617863d/browser/components/
> customizableui/content/toolbar.xml#l323
> 
> Mike, this seems to have been something you did in bug 864425, but I didn't
> see it being discussed in a quick skim of the comments. Can we just remove
> that check? Was there a particular reason you disabled this - will it break
> customization dnd?

I didn't have reasoning to disable it - those are pretty much verbatim copies of the toolkit bindings. I just copied them over so that they could inherit from our special toolbar binding.

Dao might know if there's a good reason to make toolbars in customize mode non-draggable. I'll needinfo? him.

Also, we *used* to have dragging capabilities on the window frame (at least the top of it), but I think that got lost in some of my customization mode performance work (probably related to how we don't disallow TabsInTitlebar when entering customization mode anymore).

::: browser/components/customizableui/content/toolbar.xml
@@ +331,5 @@
>              let tmp = {};
>              Components.utils.import("resource://gre/modules/WindowDraggingUtils.jsm", tmp);
>              let draggableThis = new tmp.WindowDraggingElement(this);
>              draggableThis.mouseDownCheck = function(e) {
>                // Don't move while customizing.

If we're going to change this, we should remove this comment.
Hey Dao - do you know what the reason is to not have the toolbars be draggable when customizing in the old bindings?
Flags: needinfo?(dao)
I don't remember. Maybe WindowDraggingUtils.jsm couldn't handle it at that time and now that's fixed? Or maybe it's a problem for the toolkit toolbar customization code but not for yours?

Did you check out the bug that added this code?
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #8)
> I don't remember. Maybe WindowDraggingUtils.jsm couldn't handle it at that
> time and now that's fixed? Or maybe it's a problem for the toolkit toolbar
> customization code but not for yours?
> 
> Did you check out the bug that added this code?

I have now - eventually traced it to bug 398928. This comment[1] just seems to assume that we shouldn't have draggable toolbars in customization mode, though a quick skim of the bug doesn't reveal why this is a requirement.

Markus, do you remember why?

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=398928#c54
Flags: needinfo?(mstange)
I don't really remember. Maybe because enabling window dragging would drag the window when dragging for example a flexible spacer from the toolbar into the customization panel? I don't know whether that would actually happen. Or maybe I thought that it would be too frustrating to have the window move under you when you try to drag a toolbarbutton and miss it by a few pixels.
In any case, I think it was personal judgement at the time and not really a well-researched decision.
Flags: needinfo?(mstange)
Comment on attachment 776998 [details] [diff] [review]
Should not stop people dragging the window while customizing

Review of attachment 776998 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, based on this, I'm find removing that condition for dragging.

So this solves part of the problem. The next thing is to make the window frame draggable in customization mode. I think we should keep this bug open and work on that here.
Attachment #776998 - Flags: review?(mconley) → review+
Comment on attachment 776998 [details] [diff] [review]
Should not stop people dragging the window while customizing

Pushed with nits fixed, https://hg.mozilla.org/projects/ux/rev/cf38a7ab3035

Leaving this open for figuring out the rest of it...
Attachment #776998 - Flags: checkin+
... and delegating that to mconley per IRC discussion... :-)
Assignee: gijskruitbosch+bugs → mconley
Attached patch PatchSplinter Review
This patch removes the ability to exit customization mode by clicking on the expanded window borders. I've also removed the padding-top from Windows and OSX. 

On OS X the TabsToolbar gets 9px of margin-top while in customization mode to match the padding-top that is applied to the #titlebar (this dimension needs to move to keep the visual design matching the spec while in customization mode).

I didn't adjust the padding-top for Linux because we still show the application title bar which has a distinct look and is already draggable.
Assignee: mconley → jaws
Attachment #817885 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 817885 [details] [diff] [review]
Patch

Review of attachment 817885 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if you can get UX/UI-review buy-in on these changes. Screenshots might help - I'm guessing this still makes the navbar narrower?

I mean, I also think this patch does several things where depending on UX feedback we might not have to do all of them. In particular:
- if we remove the-padding top, AIUI we could keep the close listener on the rest of the padded sides
- if we remove the close listener, maybe we can keep the padding-top?
- maybe we could make the close listener explicitly ignore the padding-top area, and/or make it listen for mouseup rather than mousedown, and check that no window drag had occurred?

I'm not sure if my understanding of the above possibilities is correct; please correct me where/if necessary before asking for UX feedback. :-)

::: browser/themes/osx/browser.css
@@ +3952,5 @@
>    border-bottom-width: 0;
>  }
>  
>  #main-window[customizing] #TabsToolbar {
> +  margin-top: 9px;

I'm... worried that this is highly magical. But there's a separate bug on our OS X tab background stuff being a little iffy, so, OK, I guess?
Attachment #817885 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #15)
> Comment on attachment 817885 [details] [diff] [review]
> Patch
> 
> Review of attachment 817885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if you can get UX/UI-review buy-in on these changes. Screenshots might
> help - I'm guessing this still makes the navbar narrower?

Yes, this still makes the navbar narrower.

> I mean, I also think this patch does several things where depending on UX
> feedback we might not have to do all of them. In particular:
> - if we remove the-padding top, AIUI we could keep the close listener on the
> rest of the padded sides

Yes, we could do this, but I'm not sure how easy it will be for users to understand that clicking on the left, right, or bottom will exit but the top won't.

> - if we remove the close listener, maybe we can keep the padding-top?

If we removed the close listener and kept the padding-top, the window would be "hard" to drag still because users have an expectation that they can drag the window by clicking and dragging the top portion of a window. With the padding-top present, clicking or dragging on this area wouldn't exit customization mode, but it also wouldn't move the window.

> - maybe we could make the close listener explicitly ignore the padding-top
> area, and/or make it listen for mouseup rather than mousedown, and check
> that no window drag had occurred?

As long as we keep the padding-top area, we won't be able to drag it (unless we move that dimension to another element that has the dragging capability). I'm not sure how we would do the drag detection in the mouseup listener, but that could be a possibility (have to make sure that we don't exit if the user dragged right 5 pixels and then left 5 pixels to return to the origin before mouseup).

> I'm not sure if my understanding of the above possibilities is correct;
> please correct me where/if necessary before asking for UX feedback. :-)
> 
> ::: browser/themes/osx/browser.css
> @@ +3952,5 @@
> >    border-bottom-width: 0;
> >  }
> >  
> >  #main-window[customizing] #TabsToolbar {
> > +  margin-top: 9px;
> 
> I'm... worried that this is highly magical. But there's a separate bug on
> our OS X tab background stuff being a little iffy, so, OK, I guess?

This 9px number comes from the #titlebar padding-top that is set to 0 when in customization mode (when not in customization mode it has padding-top: 9px;).
Attached image Screenshot of patch
This is a screenshot of what Windows looks like with the patch. OS X looks pretty similar.
Attachment #817974 - Flags: ui-review?(shorlander)
Comment on attachment 817974 [details]
Screenshot of patch

The extra visual weight from the tab might be a little off but I think we should land it and see how it feels in practice.
Attachment #817974 - Flags: ui-review?(shorlander) → ui-review+
https://hg.mozilla.org/projects/ux/rev/176f2644c521
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M9][Australis:P2][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/cf38a7ab3035
https://hg.mozilla.org/mozilla-central/rev/176f2644c521
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P2][fixed-in-ux] → [Australis:M9][Australis:P2]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.