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

VERIFIED FIXED in Firefox 66

Status

()

defect
P2
normal
VERIFIED FIXED
6 months ago
5 months ago

People

(Reporter: itiel_yn8, Assigned: smaug)

Tracking

({regression})

unspecified
Firefox 66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 unaffected, firefox65 unaffected, firefox66 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

6 months ago
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.
Reporter

Comment 1

6 months ago
(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.
(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.
Flags: needinfo?(itiel_yn8)
Reporter

Comment 3

6 months ago
(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.
Flags: needinfo?(itiel_yn8)

Updated

6 months ago
Blocks: 1089326
(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 :)
Reporter

Comment 5

6 months ago
(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.
Olli should have a look at this then.
Flags: needinfo?(bugs)
Priority: -- → P2

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

Assignee

Comment 9

5 months ago

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

Assignee

Comment 10

5 months ago
Posted patch middle_click.diff (obsolete) — Splinter Review

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.

Assignee: nobody → bugs
Flags: needinfo?(bugs)
Attachment #9038939 - Flags: review?(mconley)
Assignee

Comment 12

5 months ago
Comment on attachment 9038939 [details] [diff] [review]
middle_click.diff

or Gijs
Attachment #9038939 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #9038939 - Flags: review?(mconley) → review-
Assignee

Comment 14

5 months ago

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

Attachment #9039353 - Flags: review?(mconley)
Assignee

Updated

5 months ago
Attachment #9038939 - Attachment is obsolete: true
Attachment #9038939 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 9039353 [details] [diff] [review]
middle_click_2.diff

Thanks! I appreciate the second revision. :)
Attachment #9039353 - Flags: review?(mconley) → review+

Comment 16

5 months ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3d84fc1391
properly detect middle clicks on open space of tabstrip, r=mconley

Comment 17

5 months ago
bugherder
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Reporter

Comment 18

5 months ago

Fixed on latest Nightly.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.