1235821 - Dragging tab isn't placed under the mouse pointer if I start drag-n-drop after closing some tabs

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[arni2033]

Attachment: screencast 1 - Dragging tab isn't placed under the mouse pointer.webm

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20151223030323
STR:
1. Open as many tabs in tabs toolbar as possible without causing overflow
2. Let say you opened N tabs. Hover mouse over (N-2)th tab, middle-click that tab
    [(N-2)th tab will close, and 2 tabs in the end of list will slide to the left a bit.
     Now penultimate tab in the list is placed under the mouse pointer]
3. Middle-click the penultimate tab [it will close, and last tab will slide to the left
     Now the last tab in the list is placed under the mouse pointer]
4. Press left mouse button (don't release it yet to start drag-n-drop)
5. Move mouse to the left

Result:       
 All tabs will stretch to take all tabs toolbar (because 2 tabs were closed)
 But the dragging tab is placed ~100px to the left from mouse pointer

Expectations: 
 The dragging tab must be placed directly under the mouse pointer

Comment 1

[arni2033]

// Looks like somebody was going to (unintentionally?) implement yet another annoying tab bug for 5-10 release cycles, just like bug 1219215.

> pushlog_url:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0288a0a7003ffba272dc8040567e225d7115d9bc&tochange=5efb9a11196ba113b830cf4717d85513e3468783

Comment 2

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
regression that was uplifted to beta 44

Chris, can you have a look when you're back?

Comment 3

[Chris H-C :chutten]

Definitely reproducible (though mine is offset to the right, not left).

Reverting my fix to bug 506815 does indeed fix this regression. Bother.

Let's see where it went wrong, hm?

Comment 4

[Chris H-C :chutten]

tabbrowser.xml is listening for mousmoves during drag. And is getting them. So it's unlocking the tab sizing code (_unlockTabSizing) which resizes the tabs, which moves the tab you were dragging to the side.

So the bug is that it's unlocking the tab sizing code. But why is it suddenly doing that now?

There's a dumb three-line fix for this which translates to: if the user's dragging a tab, don't resize them:
(at the top of _unlockTabSizing)
if (this.getAttribute("movingtab") == "true") {
  return;
}

But that doesn't explain why, in Windows only, all of a sudden, this becomes relevant.

So I'm going to keep digging. My guess is that the removal of MouseTrailer is somehow having some sort of knock-on effect where it is either sending too many or not enough mousemoves. (maybe to do with nsWindow's sLastMouseMovePoint not being updated as promptly as it used to, or maybe there's something that's going wrong in tabbrowser.xml's if (isEndTab) code)

Comment 5

[arni2033]

Well, I know a bug where tabs resize all of a sudden. Remove it from CC list if it's irrelevant.

Comment 6

[Chris H-C :chutten]

Not sure if it's relevant, but I can't reproduce that bug in Nightly with or without my three-line hack. Maybe bug 506815 took care of it for you? (you reported against Nightly 44, which bug 506815's fix didn't hit until Beta).

I can reproduce it in release 43 (which reminds me, I should restart that browser...)

Comment 7

[arni2033]

That one is reproducible from ~29 to current Nightly. It's only reproducible on Windows with titlebar hidden, AFAICT. If you still can't reproduce it, remove it from the list.

Comment 8

[Chris H-C :chutten]

Ah, it was harder for me to reproduce in today's Nightly, but I think I have the trick of it now. Still reproducible as you say.

I thought it might've been fixed because bug 506815 operated on a 200ms timer, and your STR were very specific about that timing. Coincidence, I guess.

I think you might be right, though, that this is related. It seems as though the code that's supposed to enforce the 'don't move the tabs around when the mouse is hovering' depends on there not being any mousemove events being sent. Which makes no sense, because mousemove events should continue to be sent... the spacer shouldn't be removed until mouseout in any case.

Still digging. Thank you for your feedback so far.

Comment 9

[arni2033]

 What I saw even before bug 506815 is that when I move mouse above #tabbrowser-tabs not hovering any tab, somehow browser always resizes tabs. Part of this is bug 1219215. That happens only on Windows, only with titlebar hidden, so I came to conclusion that it's caused by some binding (when I hide titlebar, some bindings are applied, right?). I don't read the code much, usually only test.

 In comment 4 you said that this bug is specific for Windows, so I now believe that bug 506815 triggered the described Windows-only bug to happen in more cases. That's all, hope this will help.

Comment 10

[Chris H-C :chutten]

Attachment: 0001-bug-1235821-Don-t-track-mouseleave-when-capturing-mo.patch

So the problem is excess `mouseout` events, not `mousemove`. These can be caused in two ways:

* WM_NCMOUSEMOVE events (mouse movements over the 'nonclient' (titlebar, etc) area of the window) are treated as the mouse leaving, so Firefox emits a synthetic WM_MOUSELEAVE when it receives one. This mechanism is suppressed if we are currently capturing the mouse.

* TrackMouseEvent interacting with ::SetCapture (TrackMouseEvent generally only prompts WM_MOUSELEAVE when the mouse leaves the entire window area. When you ::SetCapture, it adheres to its older meaning and will emit WM_MOUSELEAVE on the first mouse movement after the call).

Either will cause the generation of `mouseout` events which trigger the unlocking of the tab sizing which causes both this bug and bug 1219215.

The first one isn't a problem for this bug (as it is suppressed during capture), and the second one is fixed by my patch.

Unfortunately this leaves bug 1219215 high and dry until someone with a deeper understanding of how the Windows UI works can assist me in coming up with a better solution for WM_NCMOUSEMOVE events. (I will update that bug with these findings shortly).

Comment 11

[:Gijs (he/him)]

Comment on attachment 8704192 [details] [diff] [review]
0001-bug-1235821-Don-t-track-mouseleave-when-capturing-mo.patch

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

This should be reviewed by someone with better knowledge of our win32 widget layer than me, sorry. Passing on to Jim.

Comment 12

[Chris H-C :chutten]

Attachment: 0001-bug-1235821-Don-t-track-mouseleave-when-capturing-mo.patch

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94cd77a18981

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94cd77a18981

Comment 15

[:Gijs (he/him)]

Is this patch upliftable to 44 and 45?

Comment 16

[Chris H-C :chutten]

Attachment: Same patch, tweaked by one byte to apply on aurora/beta.

Uplifting this sounds like a solid plan.

Approval Request Comment
[Feature/regressing bug #]: bug 506815 caused this.
[User impact if declined]: Users may experience odd tab dragging appearance and behaviour.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8ca15c6680c
Manual Testing: STR, tearaway tabs, reentry of tornaway tabs, dragging of selected text and other content elements
[Risks and why]: Little. This change ensures that TrackMouseEvent won't emit a WM_MOUSELEAVE when the mouse is "captured" (a button is being held, like when you're dragging things). This is mostly corner-case stuff.
[String/UUID change made/needed]: None.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8705170 [details] [diff] [review]
Same patch, tweaked by one byte to apply on aurora/beta.

Aurora45+, seems like a contained fix.

Beta44-, I am denying this based on the more stringed Beta uplift criteria that allows only for fixing critical regressions, sec and stability issues. To me this fix, does not meet that bar. Thanks!

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

[e-mail], could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 19

[arni2033]

(In reply to Ritu Kothari (:ritu) from comment #18)
> [e-mail], could you...
Stop disclosing my email to unregistered users (as said in bug 1234936 comment 27)   [OR]
announce your demands. Ok? 
I already changed the displayed name to simply "arni2033" because of this, but you keep doing it
Btw you could just tell me once to verify every "Fixed" issue I filed (if that's so important)

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/687d3ecf6c87

Comment 21

[:Gijs (he/him)]

(In reply to Ritu Kothari (:ritu) from comment #17)
> Comment on attachment 8705170 [details] [diff] [review]
> Same patch, tweaked by one byte to apply on aurora/beta.
> 
> Aurora45+, seems like a contained fix.
> 
> Beta44-, I am denying this based on the more stringed Beta uplift criteria
> that allows only for fixing critical regressions, sec and stability issues.
> To me this fix, does not meet that bar. Thanks!

TBH, it's a new regression in 44 caused by bug 506815. I'd prefer not to ship it. The patch is pretty contained. I mean, either that or back 506815 out of beta, but I don't know how Chris feels about that (+needinfo).

The other option is deciding that the regression is too much of an edgecase to worry about.

Comment 22

[Chris H-C :chutten]

A patch backing bug 506815 out of beta _also_ does not meet the beta44 criteria (criteria here: https://groups.google.com/forum/#!topic/mozilla.dev.planning/jDiniugtwgU )

So the decision :ritu and I made was that this was enough of an edge case that we could ship it for a release. It is weird, unwelcome, and odd behaviour... and had the timing been a little less tight on 44 we could've gotten it in.

But it wasn't, so we couldn't and now we'll ship it for six weeks until 45.

On the plus side, _not_ backing out bug 506815 should mean improved battery life for Windows users on 44.

Comment 23

[arni2033]

Verified fixed on:   Win7_64, Nightly 46, 32bit, ID 20160110030214

Comment 24

[mahmoudi dris]

status-firefox45: fixed → verified (Mac os X El Capitan 10.11.2 )