1219215 - Closing tab disappears immediately, and that (for 200ms) causes mouse actions to apply to parent element (<tabs>). See STR

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[arni2033]

Attachment: video - scenario (A) unexpected movement of tabs.webm

STR:   (Win7_64, Nightly 44, 32bit, ID 20151027030239, new profile)
1.   Open new firefox window.
2.   Open enough tabs to cause overflow in tabs toolbar. Open ~5 more tabs. Scroll tabs list to the end
3.   Move mouse pointer over the 3rd tab from the _end_ (actually you can try any tab except last one)
4.   Move mouse pointer between tab start and tab title start (there's about 5-10px)
5.A) Middle-click where the pointer is placed, then (you've got 200ms of tab width transition)
     move pointer towards the next tab (within tabs toolbar).
5.B) Middle-click where the pointer is placed, then (you've got 200ms of tab width transition)
     middle-click again.

Result:       A) 3rd tab from the end closed, and tabs moved to the end of scrollable area (<tabs>)
              B) 3rd tab from the end closed, and new active tab opened at the end of tabs list

Expectations: A) Tab from Step 3 should close, but tabs shouldn't move to the end of scrollable area
              B) Tab from Step 3 should close, and no other tabs should open or close

Note: It worked fine in Fx28-. I wanted to file this _ANNOYING_ bug long time ago, but didn't know
      how to describe what I saw. NOW I know exactly what this bug is, and how to fix it, so filing it. 

Possible fix: I tested this CSS using extension "Stylish" and it was OK.
>   .tab-background:not([pinned]):not([fadein])
>   {
>     transition: visibility 230ms ease-out 0s;
>   }
230ms is required because of these rules:
>   .tabbrowser-tab:not([pinned])
>   {
>     transition: min-width 200ms ease-out 0s, max-width 230ms ease-out 0s;
>   }
>   .tabbrowser-tab:not([pinned]):not([fadein])
>   {
>     max-width: 0.1px;
>     min-width: 0.1px;
>     visibility: hidden;
>   }
Tab's width smoothly changes from 100px (or more) to 0.1px. The bug described above makes it clear that closing tabs should stay visible during that transition. So I used the same transition-duration.

Fx20 worked nice, and its behavior looked exactly like this solution. I think that somewhere between 28 and 31 that visibility transition was removed during Australis development. That was wrong decision.
If current mock-up strictly tells that closing tab must not look hovered - it can be fixed with by CSS:
>   .tabbrowser-tab:not([pinned]):not([fadein])
>   {
>     opacity: 0;
>   }

Comment 1

[arni2033]

Hm, the CSS solution in comment 0 doesn't look correct. I remember it didn't work with Devtools Theme or only worked with that theme. Anyway, I'm using the following userstyle for about a month, and it was OK all along. It can be used as a starting point (probably it's not an appropriate solution)
>   .tabbrowser-tab:not([pinned]):not([fadein]) {
>     opacity:0!important;
>     transition: min-width 200ms ease-out, max-width 230ms ease-out, visibility 230ms ease-out!important;
>   }
>   
>   .tab-background:not([pinned]):not([fadein])
>   {
>     transition: visibility 230ms ease-out 0s;
>   }

It was regressed between 2013-12-02 and 2013-12-03 by bug 944136. Nobody seems to care about tab closing user experience much, huh... Makes me sad actually. At least that title was easy to detect
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2581b84e0ca1&tochange=8648aa476eef

Comment 2

[Chris H-C :chutten]

The problem appears to be the WM_MOUSELEAVE events we generate on WM_NCMOUSEMOVE in nsWindow: http://mzl.la/1IOtISp

Since the tabbar overlays the nonclient area in Windows, a mousemove during the tab closing animation is a WM_NCMOUSEMOVE.

The WM_MOUSELEAVE generates a `mouseout` which unlocks the tab sizing in tabbrowser.xml which causes the shift from the front.

Ideally we wouldn't need to WM_MOUSELEAVE on WM_NCMOUSEMOVE. Near as I can tell, the original bug that necessitated the code (bug 613781) doesn't regress when I comment out the case.

Unfortunately, when I comment out the case, this bug still occurs, since the process still receives a WM_MOUSELEAVE event (possibly due to TrackMouseEvent thinking that the mouse has left the window?) when you do that sideways shimmy.

Deeper into the rabbit hole I go...

Comment 3

[:Gijs (he/him)]

So after going down the rabbithole with Chris, it seems the main issue here is that WindowDraggingUtils::shouldDrag returns true when hovering/clicking on an empty bit of the <tabs> element, because all the ancestors of the target of the MozMouseHitTest events are in the list of draggable elements.

This makes sense because if you do this when the <tabs> element isn't overflown, you're interacting with an empty bit of tabstrip and you likely expect "normal" titlebar handing (aero snap, maximize/restore when you doubleclick, the lot).

However, we shouldn't do this if the tabbar is overflown, because normally there shouldn't be empty space anyway, and when there is (like in this case) it's really only temporary and there is no way that as a user, you're purposefully trying to interact with the non-client area (titlebar).

I tried to fix this by adding a specific check for this case in the mouseDownCheck handler we use inside TabsInTitlebar.

Unfortunately, that doesn't fix the issue.

The reason it doesn't work is that we create tons of WindowDraggingUtils instances. TabsInTitlebar creates two:

https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/browser/base/content/browser-tabsintitlebar.js#224-228

but every single draggable toolbar binding creates another:

https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/browser/components/customizableui/content/toolbar.xml#383

Note also the discrepancy with the toolkit version:

https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/toolkit/content/widgets/toolbar.xml#510

Felipe, this seems really messy. It seems like we'll be processing each of these events at least 3 times for the tabs toolbar, and 2 times for other draggable toolbars inside the browser toolbox. What do you think is the best way to fix this? It almost seems to me like there should be a singleton (per window?) that checks a list of mouseDownCheck functions per element, or something.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

So yeah, it's getting messy. It was way more centralized when we first created it, but over the years and theme changes it has gotten worse.

In theory there's nothing wrong with multiple listeners, it's the nature of the event system. The real problem is that preventDefault() is awful because there's no way to undo it.

I think the main issue now is that the TabsToolbar is getting two WindowDraggingUtils, one in CSS and one through tabsintitlebar. If it works, I believe the easiest way would be to remove the toolbar-drag binding from CSS.

My suggestion, however, would be to not go into this rabbit hole, remove most of WindowDraggingUtils and just use the -moz-window-dragging attribute as mstange suggested in bug 1107779 comment 7. We would still need to keep a simpler version of WindowDraggingUtils for Linux, but that would be much simpler.

Comment 5

[:Gijs (he/him)]

(In reply to :Felipe Gomes (needinfo me!) from comment #4)
> So yeah, it's getting messy. It was way more centralized when we first
> created it, but over the years and theme changes it has gotten worse.
> 
> In theory there's nothing wrong with multiple listeners, it's the nature of
> the event system. The real problem is that preventDefault() is awful because
> there's no way to undo it.
> 
> I think the main issue now is that the TabsToolbar is getting two
> WindowDraggingUtils, one in CSS and one through tabsintitlebar. If it works,
> I believe the easiest way would be to remove the toolbar-drag binding from
> CSS.
> 
> My suggestion, however, would be to not go into this rabbit hole, remove
> most of WindowDraggingUtils and just use the -moz-window-dragging attribute
> as mstange suggested in bug 1107779 comment 7. We would still need to keep a
> simpler version of WindowDraggingUtils for Linux, but that would be much
> simpler.

Thanks for the explanations and hints. I don't know offhand how hard all of the options here are (how would the -moz-window-dragging thing work with a partially filled tab bar?), but I'll try to take a look next week.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

The way that the the partially filled tab bar works on OSX is that the tabbar itself is draggable, but the tabs define themselves as a no-drag region: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#272

It looks like the windows widget is already receiving the region information: http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#2710

So it should be a matter of setting the appropriate CSS on the windows theme (as some of that is %ifdef'ed for OSX), and making nsWindow::ClientMarginHitTestPoint consult the region instead of dispatching the eMouseHitTest event.

Comment 7

[Chris H-C :chutten]

This is starting to sound familiar... like I've heard it before somewhere generally in the area of bug 1107779 comment 7

+:mstange

Comment 8

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1219215 - part 0: fix context menu code to take window dragging region into account, r=jimm

Review commit: https://reviewboard.mozilla.org/r/30581/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30581/

Comment 9

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

Review commit: https://reviewboard.mozilla.org/r/30583/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30583/

Comment 10

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1219215 - part 2: make the devedition theme behave like the normal theme, r?dao,felipe

Review commit: https://reviewboard.mozilla.org/r/30585/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30585/

Comment 11

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1219215 - part 3: ignore middle clicks to open tabs in the middle of closing tabs unless past the end of the last visible tab, r?dao

Review commit: https://reviewboard.mozilla.org/r/30591/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30591/

Comment 12

[:Gijs (he/him)]

I believe this patchset also fixes most of bug 1220486 and bug 1236191, and some of bug 1195067 - I think dragging using whitespace in the bookmarks toolbar is essentially by design.

Comment 13

[:Gijs (he/him)]

Stealing this - hope you don't mind, Chris! :-)

Comment 14

[arni2033]

Is this considered as general bug? Then I'd like to mention another scenario:
0. Hide Title Bar. Move New Tab button to Menu or remove it completely.
1. Open more than 2 tabs in tabs toolbar w/o triggering tab resize (they should have maximal width)
2. Hover mouse over the close button of the last tab (don't move mouse after this step)
3. Double-Click the close button

AR: The window changes its state (maximized/normal)
ER: (the same as with Title Bar shown) The window should stay still
> screencast:   https://dl.dropboxusercontent.com/s/c5lo5dk3tp0wd98/screencast%202%20-%20bug%201219215%20comment%2014.webm?dl=0

I often encounter this when New Tab button is placed in Tabs Toolbar, but I thought that it would be easier to reproduce without that button. I remembered that when you mentioned middle-click in comments (I encounter that one too). So please test if these patches fix this scenario as well.

Ideally, in this case (Steps 1-3) I'd like to see tabs sliding to the right, towards the mouse pointer. But it looks like a big UI change (and enhancement), so you I haven't filed this request.

Comment 15

[:Gijs (he/him)]

(In reply to arni2033 from comment #14)
> Is this considered as general bug? Then I'd like to mention another scenario:
> 0. Hide Title Bar. Move New Tab button to Menu or remove it completely.
> 1. Open more than 2 tabs in tabs toolbar w/o triggering tab resize (they
> should have maximal width)
> 2. Hover mouse over the close button of the last tab (don't move mouse after
> this step)
> 3. Double-Click the close button
> 
> AR: The window changes its state (maximized/normal)
> ER: (the same as with Title Bar shown) The window should stay still
> > screencast:   https://dl.dropboxusercontent.com/s/c5lo5dk3tp0wd98/screencast%202%20-%20bug%201219215%20comment%2014.webm?dl=0
> 
> I often encounter this when New Tab button is placed in Tabs Toolbar, but I
> thought that it would be easier to reproduce without that button. I
> remembered that when you mentioned middle-click in comments (I encounter
> that one too). So please test if these patches fix this scenario as well.

It doesn't, and I don't actually know offhand how we would fix it at all. Please file a separate bug for this issue.

The reason middle-click was mentioned was your scenario B in the description, which required a separate fix to scenario A.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8707139 [details]
MozReview Request: Bug 1219215 - part 0: fix context menu code to take window dragging region into account, r=jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30581/diff/1-2/

Comment 17

[:Gijs (he/him)]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30583/diff/1-2/

Comment 18

[:Gijs (he/him)]

Comment on attachment 8707141 [details]
MozReview Request: Bug 1219215 - part 2: make the devedition theme behave like the normal theme, r?dao,felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30585/diff/1-2/

Comment 19

[:Gijs (he/him)]

Comment on attachment 8707142 [details]
MozReview Request: Bug 1219215 - part 3: ignore middle clicks to open tabs in the middle of closing tabs unless past the end of the last visible tab, r?dao

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30591/diff/1-2/

Comment 20

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1219215 - part 4: remove dispatching of MozMouseHittest on Windows, r=jimm

Review commit: https://reviewboard.mozilla.org/r/30937/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30937/

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 8707964 [details]
MozReview Request: Bug 1219215 - part 4: remove dispatching of MozMouseHittest on Windows, r=jimm

https://reviewboard.mozilla.org/r/30937/#review27769

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8707139 [details]
MozReview Request: Bug 1219215 - part 0: fix context menu code to take window dragging region into account, r=jimm

https://reviewboard.mozilla.org/r/30581/#review27771

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

https://reviewboard.mozilla.org/r/30583/#review27865

*-* a dream come true!

::: browser/base/content/browser.css
(Diff revision 2)
> -
> -toolbar[customizable="true"]:not([nowindowdrag="true"]) {
> -  -moz-binding: url("chrome://browser/content/customizableui/toolbar.xml#toolbar-drag");
> -}

Great that this is being removed! I just wonder if this selector was necessary for anything on linux? I see that there are other selectors under themes/linux but this is the most generic one. Did you do a quick test on linux to see if nothing became undraggable?

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8707141 [details]
MozReview Request: Bug 1219215 - part 2: make the devedition theme behave like the normal theme, r?dao,felipe

https://reviewboard.mozilla.org/r/30585/#review27867

Comment 25

[:Gijs (he/him)]

(In reply to :Felipe Gomes (needinfo me!) from comment #23)
> Comment on attachment 8707140 [details]
> MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and
> other callers of WindowDraggingUtils, replace with using window-dragging
> region everywhere, r?dao,felipe
> 
> https://reviewboard.mozilla.org/r/30583/#review27865
> 
> *-* a dream come true!
> 
> ::: browser/base/content/browser.css
> (Diff revision 2)
> > -
> > -toolbar[customizable="true"]:not([nowindowdrag="true"]) {
> > -  -moz-binding: url("chrome://browser/content/customizableui/toolbar.xml#toolbar-drag");
> > -}
> 
> Great that this is being removed! I just wonder if this selector was
> necessary for anything on linux? I see that there are other selectors under
> themes/linux but this is the most generic one. Did you do a quick test on
> linux to see if nothing became undraggable?

This one is %ifdef XP_MACOSX, so it already didn't apply on Windows and Linux. TBH, not sure why this one wasn't removed when the OS X version of this code was implemented. OS X stopped sending mozmousehittest events, so the code was effectively dead already.

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

Please rename ONLY_FOR_PANEL_OSES to something along the lines of CSS_WINDOW_DRAGGING_SUPPORT. I'm assuming -moz-window-dragging: drag hasn't been implemented by GTK widget code. If something else is going on here in terms of platform support, please clarify.

/^(win|macosx)/i.test(AppConstants.platform) is unnecessarily lax, the strings should be "win" or "macosx" exactly. Please use something like:

AppConstants.platform == "win" || AppConstants.platform == "macosx"

or:

["win", "macosx"].includes(AppConstants.platform)

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 8707141 [details]
MozReview Request: Bug 1219215 - part 2: make the devedition theme behave like the normal theme, r?dao,felipe

#PlacesToolbarItems instead of #personal-bookmarks scrollbox

What's the end result here on Windows? Am I reading this patch correct in that the bookmarks toolbar would be a drag handle and the nav toolbar would not?

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 8707142 [details]
MozReview Request: Bug 1219215 - part 3: ignore middle clicks to open tabs in the middle of closing tabs unless past the end of the last visible tab, r?dao

this code needs some documentation

Comment 29

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #27)
> What's the end result here on Windows? Am I reading this patch correct in
> that the bookmarks toolbar would be a drag handle and the nav toolbar would
> not?

Yes. Note that the buttons (bookmarks/folders and any other items you put on the toolbar) still won't be a drag handle, either - just "empty space" on the toolbar, including empty space inside the bookmarks toolbar items.

Comment 30

[arni2033]

Attachment: screenshot 1 bug 1219215 (comment 30) - the whole window can be dragged. Again.png

(In reply to :Gijs Kruitbosch from comment #29)
> just "empty space" on the toolbar, including empty space inside the bookmarks toolbar items.
Do you mean this small red area on the screenshot?   v_v

Comment 31

[:Gijs (he/him)]

(In reply to arni2033 from comment #30)
> Created attachment 8708994 [details]
> screenshot 1 bug 1219215 (comment 30) - the whole window can be dragged.
> Again.png
> 
> (In reply to :Gijs Kruitbosch from comment #29)
> > just "empty space" on the toolbar, including empty space inside the bookmarks toolbar items.
> Do you mean this small red area on the screenshot?   v_v

It's much bigger for me, and there's no dropmarker, in which case it makes sense that it is draggable. This bug doesn't change the case you're talking about.

Once this all lands, please file a separate bug for the overflown bookmarks toolbar case - there's currently no attribute on the bookmarks toolbar indicating it's overflowing (or not) and so there is no way to fix what you're seeing with just an updated CSS selector, and this bug is already pretty large in scope.

Comment 32

[Dão Gottwald [:dao]]

There's empty space in the nav toolbar, both visually and in terms of interaction. There's no visible border between that and the empty space in the bookmarks toolbar, so it seems like the latter will be a hit-and-miss target.

I believe current behavior is that these toolbars aren't draggable at all on Windows. We should maintain that.

Comment 33

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #32)
> There's empty space in the nav toolbar, both visually and in terms of
> interaction. There's no visible border between that and the empty space in
> the bookmarks toolbar, so it seems like the latter will be a hit-and-miss
> target.
> 
> I believe current behavior is that these toolbars aren't draggable at all on
> Windows. We should maintain that.

That is not the case right now when using a lightweight theme. I can make the LWT case match the non-LWT case, if you prefer...

Comment 34

[Dão Gottwald [:dao]]

Why do these cases need to have the same behavior? The reasoning for the lightweight theme behavior was that the border between the title bar and toolbars is less clear there.

Comment 35

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #34)
> Why do these cases need to have the same behavior? The reasoning for the
> lightweight theme behavior was that the border between the title bar and
> toolbars is less clear there.

That really depends on the lwtheme. I think the draggable navbar is surprising to people when using a lwtheme, especially considering it is almost impossible to hit on purpose anyway, and misclicks when aiming for the location/search bar or a toolbarbutton are likely more common. Thoughts?

Comment 36

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #35)
> (In reply to Dão Gottwald [:dao] from comment #34)
> > Why do these cases need to have the same behavior? The reasoning for the
> > lightweight theme behavior was that the border between the title bar and
> > toolbars is less clear there.
> 
> That really depends on the lwtheme.

Generally it doesn't. In case you're thinking of themes that want to style the title bar differently: they can't do that accurately, so this would also lead to to unpredictable behavior even if we restricted dragging to the title bar.

> I think the draggable navbar is
> surprising to people when using a lwtheme, especially considering it is
> almost impossible to hit on purpose anyway, and misclicks when aiming for
> the location/search bar or a toolbarbutton are likely more common. Thoughts?

I don't have a strong opinion on this but would prefer to keep the current behavior.

Comment 37

[:Gijs (he/him)]

Comment on attachment 8707139 [details]
MozReview Request: Bug 1219215 - part 0: fix context menu code to take window dragging region into account, r=jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30581/diff/2-3/

Comment 38

[:Gijs (he/him)]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30583/diff/2-3/

Comment 39

[:Gijs (he/him)]

Comment on attachment 8707141 [details]
MozReview Request: Bug 1219215 - part 2: make the devedition theme behave like the normal theme, r?dao,felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30585/diff/2-3/

Comment 40

[:Gijs (he/him)]

Comment on attachment 8707142 [details]
MozReview Request: Bug 1219215 - part 3: ignore middle clicks to open tabs in the middle of closing tabs unless past the end of the last visible tab, r?dao

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30591/diff/2-3/

Comment 41

[:Gijs (he/him)]

Comment on attachment 8707964 [details]
MozReview Request: Bug 1219215 - part 4: remove dispatching of MozMouseHittest on Windows, r=jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30937/diff/1-2/

Comment 42

[:Gijs (he/him)]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

Carrying over r+

Comment 43

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #40)
> Comment on attachment 8707142 [details]
> MozReview Request: Bug 1219215 - part 3: ignore middle clicks to open tabs
> in the middle of closing tabs unless past the end of the last visible tab,
> r?dao
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/30591/diff/2-3/

I hope the comment here is sufficient. If not, please let me know what it's lacking.

Comment 44

[Dão Gottwald [:dao]]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

I don't think we can set -moz-window-dragging: drag; for toolbox by default. It's non-native behavior on Windows.

Same for toolbar:not([nowindowdrag="true"]) and statusbar:not([nowindowdrag="true"]), which were clearly tailored to OS X where this is standard behavior.

Comment 45

[:Gijs (he/him)]

Comment on attachment 8707139 [details]
MozReview Request: Bug 1219215 - part 0: fix context menu code to take window dragging region into account, r=jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30581/diff/3-4/

Comment 46

[:Gijs (he/him)]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30583/diff/3-4/

Comment 47

[:Gijs (he/him)]

Comment on attachment 8707141 [details]
MozReview Request: Bug 1219215 - part 2: make the devedition theme behave like the normal theme, r?dao,felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30585/diff/3-4/

Comment 48

[:Gijs (he/him)]

Comment on attachment 8707142 [details]
MozReview Request: Bug 1219215 - part 3: ignore middle clicks to open tabs in the middle of closing tabs unless past the end of the last visible tab, r?dao

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30591/diff/3-4/

Comment 49

[:Gijs (he/him)]

Comment on attachment 8707964 [details]
MozReview Request: Bug 1219215 - part 4: remove dispatching of MozMouseHittest on Windows, r=jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30937/diff/2-3/

Comment 50

[Dão Gottwald [:dao]]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

It looks like adding 'toolbox' to the rule in xul.css might cause nowindowdrag="true" on toolbars to be ignored. Am I missing something? Why did you make this change when apparently this rule didn't have to target 'toolbox' on OS X to this day?

Comment 51

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #50)
> Comment on attachment 8707140 [details]
> MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and
> other callers of WindowDraggingUtils, replace with using window-dragging
> region everywhere, r?dao,felipe
> 
> It looks like adding 'toolbox' to the rule in xul.css might cause
> nowindowdrag="true" on toolbars to be ignored. Am I missing something? Why
> did you make this change when apparently this rule didn't have to target
> 'toolbox' on OS X to this day?

This was an oversight. We don't need it.

However, then I looked at the <titlebar> thing again, too. I don't think that's necessary, either. But when testing, I realized something else:

The overlapping of the <vbox id="titlebar"/> in Firefox, and the <toolbox id="navigator-toolbox"/> breaks custom titlebar buttons. IIRC we only use these on Windows 10 (where I checked and I can see this issue). Presumably this is because the region gets calculated in DOM order by the elements that have -moz-window-dragging: drag, and only descendants can override the region as delimited by their parents, but descendants cannot "cut out" a bit of a region delimited by siblings of their parents, even if their parents' bounds overlap with those of the siblings of those parents. Markus, is this right? Is this fixable on the layout side?

Comment 52

[Markus Stange [:mstange]]

(In reply to :Gijs Kruitbosch from comment #51)
> but descendants cannot "cut out" a bit
> of a region delimited by siblings of their parents, even if their parents'
> bounds overlap with those of the siblings of those parents.

I think they can, if the parent's sibling comes before the parent in DOM order.

The algorithm that layout uses is rather simple: We do a depth-first traversal on the frame tree. We have a current window dragging region. For each frame, if the frame's -moz-window-dragging value is drag, this frame's area gets added to the window dragging region, otherwise it gets subtracted from the window dragging region. [1]

Does this help? I'd rather not make the layout code any more complicated, unless it requires elaborate workarounds on the frontend side.

[1] https://dxr.mozilla.org/mozilla-central/rev/b67316254602a63bf4e568198a5c7d3288a9db27/layout/base/nsDisplayList.cpp#1235

Comment 53

[:Gijs (he/him)]

Comment on attachment 8707139 [details]
MozReview Request: Bug 1219215 - part 0: fix context menu code to take window dragging region into account, r=jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30581/diff/4-5/

Comment 54

[:Gijs (he/him)]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30583/diff/4-5/

Comment 55

[:Gijs (he/him)]

Comment on attachment 8707141 [details]
MozReview Request: Bug 1219215 - part 2: make the devedition theme behave like the normal theme, r?dao,felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30585/diff/4-5/

Comment 56

[:Gijs (he/him)]

Comment on attachment 8707142 [details]
MozReview Request: Bug 1219215 - part 3: ignore middle clicks to open tabs in the middle of closing tabs unless past the end of the last visible tab, r?dao

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30591/diff/4-5/

Comment 57

[:Gijs (he/him)]

Comment on attachment 8707964 [details]
MozReview Request: Bug 1219215 - part 4: remove dispatching of MozMouseHittest on Windows, r=jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30937/diff/3-4/

Comment 58

[:Gijs (he/him)]

This will need a fix in bug 1241275 before we can land it. Otherwise, this is all still the same, but rebased, and with the toolbox and titlebar updated.

Note that AFAICT nothing actually uses a XUL <titlebar> element anywhere, and its custom C++ dragging code is pretty busted both before and after these changes. Doesn't seem awfully relevant, but I kept it where it was for now. If people want we can file a separate bug to remove support for it entirely.

Comment 59

[Dão Gottwald [:dao]]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

> /* We need the toolbox in order to get the space above the tabs toolbar draggable -
>  * the toolbox overlaps the titlebar due to its negative margin-bottom, and so
>  * we need to specify that it's draggable for that to work: */
> #main-window[tabsintitlebar] #navigator-toolbox,

Don't you need to set -moz-window-dragging: no-drag; on toolbars to cancel this out?

Could making #TabsToolbar use padding-top instead of margin-top address this in a better way? Or setting pointer-events: none; on the toolbox?

Comment 60

[Markus Stange [:mstange]]

Hmm, with bug 1241275, I'd think that part wouldn't even be necessary. If you don't set any -moz-window-dragging value on the toolbox, then it doesn't have a different -moz-window-dragging value than its parent, so it will no longer subtract itself from the draggable region.

Comment 61

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #59)
> Comment on attachment 8707140 [details]
> MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and
> other callers of WindowDraggingUtils, replace with using window-dragging
> region everywhere, r?dao,felipe
> 
> > /* We need the toolbox in order to get the space above the tabs toolbar draggable -
> >  * the toolbox overlaps the titlebar due to its negative margin-bottom, and so
> >  * we need to specify that it's draggable for that to work: */
> > #main-window[tabsintitlebar] #navigator-toolbox,
> 
> Don't you need to set -moz-window-dragging: no-drag; on toolbars to cancel
> this out?

Not in my testing. Perhaps because <toolbaritem> and <toolbarbutton> all have no-drag. On Windows, that takes up the entire default set of navbar/bookmarks toolbar anyway. We might still need it for other toolbars and/or if you customize the bookmarks toolbar to have other stuff in and be half-empty. I'll check when I'm back on my Windows machine this afternoon.

> Could making #TabsToolbar use padding-top instead of margin-top address this
> in a better way?

This seems liable to causing perf regressions, being a more complex change, and requiring changes to TabsInTitlebar code (potentially also causing behavioral regressions if we're not very careful). I would prefer not to change that as part of this bug.

> Or setting pointer-events: none; on the toolbox?

That would require setting pointer-events: auto; on all the kids of the toolbar, right, because it would inherit (according to MDN) ? Which doesn't seem like it'd be that different from doing the same with -moz-window-dragging. Which then makes me think I'm misunderstanding your suggestion... (ni for this)

This might be a way to get this to land without a patch for bug 1241275 though, assuming it works and doesn't regress anything. Not sure what this would do for the mouseleave/mouseenter events. AIUI they should be OK but it'd be worth checking. Again, will poke about once I'm back at my Windows machine.

(In reply to Markus Stange [:mstange] from comment #60)
> Hmm, with bug 1241275, I'd think that part wouldn't even be necessary. If
> you don't set any -moz-window-dragging value on the toolbox, then it doesn't
> have a different -moz-window-dragging value than its parent, so it will no
> longer subtract itself from the draggable region.

Oh, right. So many solutions! :-)

Comment 62

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #61)
> > Could making #TabsToolbar use padding-top instead of margin-top address this
> > in a better way?
> 
> This seems liable to causing perf regressions,

Really? Of course this can never be ruled out with absolute certainty, but I wouldn't expect it to have perf impact.

> being a more complex change,

For the sake of saner code as the end result, yes.

> and requiring changes to TabsInTitlebar code

From a brief look this would be code removal...?

> (potentially also causing
> behavioral regressions if we're not very careful).

Well, sure, it could regress something, but diffuse fear shouldn't prevent us from touching our own code. To the contrary, this kind of fear should motivate us to simplify stuff :(

> I would prefer not to change that as part of this bug.

In a separate bug then?

> > Or setting pointer-events: none; on the toolbox?
> 
> That would require setting pointer-events: auto; on all the kids of the
> toolbar, right, because it would inherit (according to MDN) ? Which doesn't
> seem like it'd be that different from doing the same with
> -moz-window-dragging. Which then makes me think I'm misunderstanding your
> suggestion... (ni for this)

No, I was just brainstorming.

Comment 63

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #62)
> (In reply to :Gijs Kruitbosch from comment #61)
> > > Could making #TabsToolbar use padding-top instead of margin-top address this
> > > in a better way?
> > 
> > This seems liable to causing perf regressions,
> 
> Really? Of course this can never be ruled out with absolute certainty, but I
> wouldn't expect it to have perf impact.

At this point I basically expect everything that touches the frames that make up the tabstrip to change TART numbers, at a minimum. If I understand layout correctly, your suggestion would make the frame for the tabs toolbar bigger (as e.g. backgrounds, borders and box-shadows would need to be painted in/around a larger box). That impact seems like it would be net negative if it is detectable.

> > being a more complex change,
> 
> For the sake of saner code as the end result, yes.
> 
> > and requiring changes to TabsInTitlebar code
> 
> From a brief look this would be code removal...?

Having also taken a brief look, I'm not sure what code you think we could remove. We swap out margin-top for padding-top in the CSS, update any explicit height references, and then? We still need to take margins into account in the tabsintitlebar JS code, because of the negative bottom margin in the default theme, and because of other themes...  Plus I'm not sure how the aero fog would play with adding 15px to the top of the tabs toolbar height.

> > (potentially also causing
> > behavioral regressions if we're not very careful).
> 
> Well, sure, it could regress something, but diffuse fear shouldn't prevent
> us from touching our own code. To the contrary, this kind of fear should
> motivate us to simplify stuff :(

I mean, the fear is 'diffuse' because I had no opportunity to try it, but purely theoretically I can see a number of issues as noted in this comment. That said, I would love to simplify some of our tabsintitlebar stuff. I'm not sure it *can* be simplified easily. Even trying to fix seemingly simple bugs (eg. bug 1005098) seems to trigger all kinds of magical regressions. Definitely follow-up fodder, IMO.

On the upside as far as code simplicity is concerned, this patch-set does actually simplify our window dragging story, as far as I can tell, which is a start! :-)

> > I would prefer not to change that as part of this bug.
> 
> In a separate bug then?

Sure. Out of curiosity, are there other benefits that you see to making the spacing here padding- rather than margin-based? It seems we can just omit the selector for this bug, once bug 1241275 gets addressed, and that should solve the issue at hand. If there *are* other reasons, happy to file a bug to investigate doing that, but besides this bug I'm just not sure I know of reasons.


I just tested the pointer-events solution you proposed, and unfortunately that still breaks, presumably because the toolbox is now implicitly "no-drag", and because it gets hit later in the depth-first traversal, that gets subtracted from the draggable region. As Markus noted, fixing bug 1241275 would address this.

Leaving the toolbox selector in, as noted earlier, leaves the titlebar buttons on win10 non-functional until bug 1241275 gets addressed. So either with or without that selector (and the toolbars fixed to be non-drag), we wouldn't be able to land this without 1241275. So I'll resubmit the patches with the toolbox selector removed. If you want a version of a patch for that bug to test with, https://pastebin.mozilla.org/8857238 should work (make sure to do a full ./mach build because of how many times nsDisplayList.h gets included).

Comment 64

[:Gijs (he/him)]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30583/diff/5-6/

Comment 65

[:Gijs (he/him)]

Comment on attachment 8707141 [details]
MozReview Request: Bug 1219215 - part 2: make the devedition theme behave like the normal theme, r?dao,felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30585/diff/5-6/

Comment 66

[:Gijs (he/him)]

Comment on attachment 8707142 [details]
MozReview Request: Bug 1219215 - part 3: ignore middle clicks to open tabs in the middle of closing tabs unless past the end of the last visible tab, r?dao

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30591/diff/5-6/

Comment 67

[:Gijs (he/him)]

Comment on attachment 8707964 [details]
MozReview Request: Bug 1219215 - part 4: remove dispatching of MozMouseHittest on Windows, r=jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30937/diff/4-5/

Comment 68

[Dão Gottwald [:dao]]

Comment on attachment 8707140 [details]
MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and other callers of WindowDraggingUtils, replace with using window-dragging region everywhere, r?dao,felipe

Is there a GTK widget bug on file for -moz-window-dragging support?

Comment 69

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #63)
> At this point I basically expect everything that touches the frames that
> make up the tabstrip to change TART numbers, at a minimum. If I understand
> layout correctly, your suggestion would make the frame for the tabs toolbar
> bigger (as e.g. backgrounds, borders and box-shadows would need to be
> painted in/around a larger box). That impact seems like it would be net
> negative if it is detectable.

Yeah, could be, but who knows... I still wouldn't dismiss this a priori because it could potentially have a measurable impact. I.e. if we decide it's worth a shot, we should just measure if there's a perf issue and then we can still dismiss the idea. It's easy enough to make a straw man patch for this and send it to the try server.

> > > and requiring changes to TabsInTitlebar code
> > 
> > From a brief look this would be code removal...?
> 
> Having also taken a brief look, I'm not sure what code you think we could
> remove. We swap out margin-top for padding-top in the CSS,

The padding would automatically be part of the computed height. It would basically do exactly what this JS code emulates for margin as far as I can see.

> > > I would prefer not to change that as part of this bug.
> > 
> > In a separate bug then?
> 
> Sure. Out of curiosity, are there other benefits that you see to making the
> spacing here padding- rather than margin-based? It seems we can just omit
> the selector for this bug, once bug 1241275 gets addressed, and that should
> solve the issue at hand. If there *are* other reasons, happy to file a bug
> to investigate doing that, but besides this bug I'm just not sure I know of
> reasons.

Not sure, but even just simplifying the TabsInTitlebar calculations a bit would be nice.

Comment 70

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #69)
> (In reply to :Gijs Kruitbosch from comment #63)
> > Sure. Out of curiosity, are there other benefits that you see to making the
> > spacing here padding- rather than margin-based? It seems we can just omit
> > the selector for this bug, once bug 1241275 gets addressed, and that should
> > solve the issue at hand. If there *are* other reasons, happy to file a bug
> > to investigate doing that, but besides this bug I'm just not sure I know of
> > reasons.
> 
> Not sure, but even just simplifying the TabsInTitlebar calculations a bit
> would be nice.

Filed bug 1241890.

(In reply to Dão Gottwald [:dao] from comment #68)
> Comment on attachment 8707140 [details]
> MozReview Request: Bug 1219215 - part 1: remove window-drag bindings and
> other callers of WindowDraggingUtils, replace with using window-dragging
> region everywhere, r?dao,felipe
> 
> Is there a GTK widget bug on file for -moz-window-dragging support?

Filed bug 1241885.

Comment 71

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5c70e5f2fb9f
https://hg.mozilla.org/integration/fx-team/rev/29dd8f5e46f8
https://hg.mozilla.org/integration/fx-team/rev/cd12368d02f8
https://hg.mozilla.org/integration/fx-team/rev/a67a25a5af74

Comment 72

[:Gijs (he/him)]

Landed everything except the middle-click fix together with the fix from bug 1241275 (which Markus asked me to land in his absence). I renumbered the 'parts' as the middle-click fix is completely orthogonal to removing the mozmousehittest dispatching anyway. I'll update the review request tomorrow assuming this sticks. Setting leave-open so this doesn't get closed.

Comment 73

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5c70e5f2fb9f
https://hg.mozilla.org/mozilla-central/rev/29dd8f5e46f8
https://hg.mozilla.org/mozilla-central/rev/cd12368d02f8
https://hg.mozilla.org/mozilla-central/rev/a67a25a5af74

Comment 74

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1219215 - final part: ignore middle clicks to open tabs in the middle of closing tabs unless past the end of the last visible tab, r?dao

Review commit: https://reviewboard.mozilla.org/r/32993/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32993/

Comment 75

[tabmix.onemen]

After this bug landed left click with modifiers on TabsToolbar are blocked

Comment 76

[:Gijs (he/him)]

(In reply to tabmix.onemen from comment #75)
> After this bug landed left click with modifiers on TabsToolbar are blocked

It's not clear to me what this means. On buttons? On tabs? On other UI elements? On what OS(es)? Please file a followup bug with a more precise description of the before/after behaviour.

Comment 77

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

Comment on attachment 8714279 [details]
MozReview Request: Bug 1219215 - final part: ignore middle clicks to open tabs in the middle of closing tabs unless past the end of the last visible tab, r?dao

https://reviewboard.mozilla.org/r/32993/#review30031

Big thanks for the comment block too.

::: browser/base/content/tabbrowser.xml:5515
(Diff revision 1)
> +            let ltr = (window.getComputedStyle(this, null).direction == "ltr");

I'm surprised we don't cache this somewhere.

Comment 78

[tabmix.onemen]

(In reply to :Gijs Kruitbosch from comment #76)
> (In reply to tabmix.onemen from comment #75)
> > After this bug landed left click with modifiers on TabsToolbar are blocked
> 
> It's not clear to me what this means. On buttons? On tabs? On other UI
> elements? On what OS(es)? Please file a followup bug with a more precise
> description of the before/after behaviour.

Filed bug 1245401 -  Mouse left click with modifiers shouldn't start window dragging

Comment 79

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/f001cd4a711d

Comment 80

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f001cd4a711d