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

VERIFIED FIXED in Firefox 45

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: arni2033, Assigned: chutten)

Tracking

({regression})

Trunk
Firefox 46
regression
Points:
---

Firefox Tracking Flags

(firefox44- wontfix, firefox45 verified, firefox46 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8702914 [details]
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
(Reporter)

Comment 1

2 years ago
// 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
Blocks: 506815
Keywords: regression
[Tracking Requested - why for this release]:
regression that was uplifted to beta 44

Chris, can you have a look when you're back?
status-firefox44: --- → affected
status-firefox45: --- → affected
tracking-firefox44: --- → ?
Flags: needinfo?(chutten)
(Reporter)

Updated

2 years ago
Has STR: --- → yes
(Assignee)

Comment 3

2 years ago
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?
Assignee: nobody → chutten
Status: NEW → ASSIGNED
Flags: needinfo?(chutten)
(Assignee)

Comment 4

2 years ago
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)
(Reporter)

Comment 5

2 years ago
Well, I know a bug where tabs resize all of a sudden. Remove it from CC list if it's irrelevant.
See Also: → bug 1219215
(Assignee)

Comment 6

2 years ago
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...)
Flags: needinfo?(arni2033)
(Reporter)

Comment 7

2 years ago
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.
Flags: needinfo?(arni2033)
(Assignee)

Comment 8

2 years ago
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.
(Reporter)

Comment 9

2 years ago
 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.
(Assignee)

Comment 10

2 years ago
Created attachment 8704192 [details] [diff] [review]
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).
Attachment #8704192 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8704192 - Flags: review?(gijskruitbosch+bugs) → review?(jmathies)

Updated

2 years ago
Attachment #8704192 - Flags: review?(jmathies) → review+
(Assignee)

Comment 12

2 years ago
Created attachment 8704741 [details] [diff] [review]
0001-bug-1235821-Don-t-track-mouseleave-when-capturing-mo.patch
Attachment #8704192 - Attachment is obsolete: true
Attachment #8704741 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/94cd77a18981
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/94cd77a18981
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Is this patch upliftable to 44 and 45?
Flags: needinfo?(chutten)
(Assignee)

Comment 16

2 years ago
Created attachment 8705170 [details] [diff] [review]
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.
Flags: needinfo?(chutten)
Attachment #8705170 - Flags: review+
Attachment #8705170 - Flags: approval-mozilla-beta?
Attachment #8705170 - Flags: approval-mozilla-aurora?
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!
Attachment #8705170 - Flags: approval-mozilla-beta?
Attachment #8705170 - Flags: approval-mozilla-beta-
Attachment #8705170 - Flags: approval-mozilla-aurora?
Attachment #8705170 - Flags: approval-mozilla-aurora+

Updated

2 years ago
status-firefox44: affected → wontfix
tracking-firefox44: ? → -
Comment hidden (obsolete)
Comment hidden (obsolete)
https://hg.mozilla.org/releases/mozilla-aurora/rev/687d3ecf6c87
status-firefox45: affected → fixed
(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.
Flags: needinfo?(chutten)
(Assignee)

Comment 22

2 years ago
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.
Flags: needinfo?(chutten)
(Reporter)

Comment 23

2 years ago
Verified fixed on:   Win7_64, Nightly 46, 32bit, ID 20160110030214
Status: RESOLVED → VERIFIED
Flags: needinfo?(rkothari)
Flags: needinfo?(arni2033)

Updated

2 years ago
QA Whiteboard: [good first verify]
status-firefox46: fixed → verified

Comment 24

2 years ago
status-firefox45: fixed → verified (Mac os X El Capitan 10.11.2 )
status-firefox45: fixed → verified
You need to log in before you can comment on or make changes to this bug.