1516798 - Dragging a middle click on a tab to another tab causes a new tab to open

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[Itiel]

STR:
1. Open multiple tabs
2. Middle click on one tab, but don't release the mouse wheel
3. Drag the mouse to another tab and release the middle click on it

AR:
A new tab opens.

ER:
Nothing.

As a side bonus, if you're dragging this middle click from tab A to the same tab A but to a different area of it, the tab will close (whereas before the regression, simply nothing will happen).


I've bisected this twice and in both times I got bug 1512259 as the culprit, though this does not seem to be a right candidate as the regressor.
Regression range is 2018-12-11 to 2018-12-12.

Comment 1

[Itiel]

(In reply to Itiel from comment #0)
> As a side bonus, if you're dragging this middle click from tab A to the same
> tab A but to a different area of it, the tab will close (whereas before the
> regression, simply nothing will happen).

Forget about this part. This happens only when dragging the middle click to the upper area of the tab, and in any case- this is not what this bug is all about.

Comment 2

[Andrew Overholt [:overholt]]

(In reply to Itiel from comment #0)
> STR:
> 1. Open multiple tabs
> 2. Middle click on one tab, but don't release the mouse wheel
> 3. Drag the mouse to another tab and release the middle click on it

In Nightly on Windows 10, the middle mouse button won't drag a tab for me. Are you on Linux?

I asked smaug (CCd) on IRC and he, like you, doesn't think bug 1512259 can be the cause.

Comment 3

[Itiel]

(In reply to Andrew Overholt [:overholt] from comment #2)
> (In reply to Itiel from comment #0)
> > STR:
> > 1. Open multiple tabs
> > 2. Middle click on one tab, but don't release the mouse wheel
> > 3. Drag the mouse to another tab and release the middle click on it
> 
> In Nightly on Windows 10, the middle mouse button won't drag a tab for me.
> Are you on Linux?

I'm also on Windows 10. Using latest Nightly.
Dragging the middle mouses button doesn't actually drag the tab (and it of course shouldn't), but releasing the click on another tab causes a new tab to open.
Think of it like you're trying to close a tab by middle clicking it but then you're regretting it (while the middle mouse button is still pressed) so you're dragging the mouse elsewhere (in this case- on another tab) so Nightly would not register this middle click as a request to close the tab.

> I asked smaug (CCd) on IRC and he, like you, doesn't think bug 1512259 can
> be the cause.

I'll try bisecting a third time then.

Comment 4

[Andrew Overholt [:overholt]]

(In reply to Itiel from comment #3)
> (In reply to Andrew Overholt [:overholt] from comment #2)
> > (In reply to Itiel from comment #0)
> > > STR:
> > > 1. Open multiple tabs
> > > 2. Middle click on one tab, but don't release the mouse wheel
> > > 3. Drag the mouse to another tab and release the middle click on it
> > 
> > In Nightly on Windows 10, the middle mouse button won't drag a tab for me.
> > Are you on Linux?
> 
> I'm also on Windows 10. Using latest Nightly.
> Dragging the middle mouses button doesn't actually drag the tab (and it of
> course shouldn't), but releasing the click on another tab causes a new tab
> to open.
> Think of it like you're trying to close a tab by middle clicking it but then
> you're regretting it (while the middle mouse button is still pressed) so
> you're dragging the mouse elsewhere (in this case- on another tab) so
> Nightly would not register this middle click as a request to close the tab.

Ah! Thanks, I can reproduce now.

I'm not sure this is a super-duper common case but if we could figure out the regressing patch (thank you very much for trying again!), we could know if this were a wider problem :)

Comment 5

[Itiel]

(In reply to Andrew Overholt [:overholt] from comment #4)
> Ah! Thanks, I can reproduce now.
> 
> I'm not sure this is a super-duper common case but if we could figure out
> the regressing patch (thank you very much for trying again!), we could know
> if this were a wider problem :)

Still getting bug 1512259 as the regressor, not sure why. I'm using Mozregression-gui.

Does bug 1089326 sound plausible as the culprit, as Alice noted? It's the blocker for bug 1512259, FWIW.

Comment 6

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f6eebcc14c22762f521fb567a9588963b8720592&tochange=61570c16c2d564c24fab36713fb169c4144453e9

Regressed by: Bug 1089326

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Olli should have a look at this then.

Comment 8

[Andrew Overholt [:overholt]]

(Olli told me he's going to get to this soon)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Itiel from comment #0)

STR:

1. Open multiple tabs
2. Middle click on one tab, but don't release the mouse wheel
3. Drag the mouse to another tab and release the middle click on it

AR:
A new tab opens.

ER:
Nothing.
ahaa, I didn't know that middle clicking on the empty area next to the tabs opens a new tab.
And now that click detection finds the common ancestor as target, some event listener just thinks click was still happening on the empty area. Looking.

As a side bonus, if you're dragging this middle click from tab A to the same
tab A but to a different area of it, the tab will close (whereas before the
regression, simply nothing will happen).
That is because now we detect it properly as a click.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: middle_click.diff

this is related to bug 1219215. And originally the feature was added in bug 573438. Bug 1519160 is also relevant.
Just using the code patch, which https://hg.mozilla.org/mozilla-central/rev/f001cd4a711d added, always.

I don't see any tests for this code. None of other bugs added any.

Gijs seems to be away-ish for couple of days.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56263c0818c957e9678bb607194ca32c4ca7d5a4

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9038939 [details] [diff] [review]
middle_click.diff

or Gijs

Comment 13

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

Comment on attachment 9038939 [details] [diff] [review]
middle_click.diff

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

Sorry for the delay - I was traveling most of yesterday.

I realize there's a time crunch on this, but I can't say I'm super jazzed about making a layout flush via getBoundingClientRect more likely to hit... see below.

If using getBoundsWithoutFlushing is not possible, can promiseDocumentFlushed be use to wait until the next opportunity to cheaply query for layout information?

::: browser/base/content/tabbrowser.xml
@@ +1336,5 @@
> +          // The user middleclicked on the tabstrip. Check whether the click
> +          // was dispatched on the open space of it.
> +          let visibleTabs = this._getVisibleTabs();
> +          let lastTab = visibleTabs[visibleTabs.length - 1];
> +          let endOfTab = lastTab.getBoundingClientRect()[RTL_UI ? "left" : "right"];

Hm. So it seems that we'll trigger a layout flush now anytime we middle-click on the scrollbox, regardless of whether or not we were closing tabs (whereas before, we'd only do that check if we were in the midst of closing some tabs).

Is it possible to use windowUtils.getBoundsWithoutFlushing to get this value?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: middle_click_2.diff

This then. Seem to work well even with tab closing case at least on my machine.

Comment 15

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

Comment on attachment 9039353 [details] [diff] [review]
middle_click_2.diff

Thanks! I appreciate the second revision. :)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3d84fc1391
properly detect middle clicks on open space of tabstrip, r=mconley

Comment 17

[Cristian Brindusan [:cbrindusan]]

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

Comment 18

[Itiel]

Fixed on latest Nightly.