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

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Chenxia Liu [:liuche]]

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.

Comment 1

[:Gijs (he/him)]

I noticed this on Windows as well, so marking as x-platform.

Comment 2

[:Gijs (he/him)]

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?

Comment 3

[:Gijs (he/him)]

Attachment: Should not stop people dragging the window while customizing

Comment 4

[:Gijs (he/him)]

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...

Comment 5

[:Gijs (he/him)]

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 6

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Hey Dao - do you know what the reason is to not have the toolbars be draggable when customizing in the old bindings?

Comment 8

[Dão Gottwald [: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?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(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

Comment 10

[Markus Stange [: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.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 12

[:Gijs (he/him)]

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...

Comment 13

[:Gijs (he/him)]

... and delegating that to mconley per IRC discussion... :-)

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

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.

Comment 15

[:Gijs (he/him)]

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?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(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;).

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of patch

This is a screenshot of what Windows looks like with the patch. OS X looks pretty similar.

Comment 18

[Stephen Horlander [: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.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/projects/ux/rev/176f2644c521

Comment 20

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/cf38a7ab3035
https://hg.mozilla.org/mozilla-central/rev/176f2644c521