Closed Bug 1516798 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P2)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 66
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- unaffected
firefox66 --- verified

People

(Reporter: itiel_yn8, Assigned: smaug)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

middle_click_2.diff
(deleted), patch
mconley
: review+
Details | Diff | Splinter Review
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.
(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)
(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)
Keywords: regressionwindow-wanted
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 :)
(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)

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

Attached patch middle_click.diff (obsolete) (deleted) — — 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)
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-
Attached patch middle_click_2.diff (deleted) — — Splinter Review

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

Attachment #9039353 - Flags: review?(mconley)
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+
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

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

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Fixed on latest Nightly.

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

Attachment

General

Creator:
Created:
Updated:
Size: