1387861 - Dragging tabs is completely broken

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Oriol Brufau [:Oriol]]

1. Open lots of tabs (e.g. use Ctrl+T)
2. Mousedown a tab, drag it to some side

Expected: The tab is reordered to where the mouse is.
Result: It moves to another place.

I have 500 opened tabs, with this I lose track of them and Nightly is completely unusable. This needs a quick fix or backout but 1387084.

Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fe13b9b9e3cea1a8d4231ae44ccdec5261c6711f&tochange=11d86d5c215907a233558afefd1eb0d20b50b7d5

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1387861 - Use _scrollbox.scrollLeft instead of scrollPosition which doesn't exist anymore, and update remaining _handleTabSelect call sites for signature change.

Review commit: https://reviewboard.mozilla.org/r/165360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165360/

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8894273 [details]
Bug 1387861 - Use _scrollbox.scrollLeft instead of scrollPosition which doesn't exist anymore, and update remaining _handleTabSelect call sites for signature change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165360/diff/1-2/

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8894273 [details]
Bug 1387861 - Use _scrollbox.scrollLeft instead of scrollPosition which doesn't exist anymore, and update remaining _handleTabSelect call sites for signature change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165360/diff/2-3/

Comment 4

[:Gijs (he/him)]

Comment on attachment 8894273 [details]
Bug 1387861 - Use _scrollbox.scrollLeft instead of scrollPosition which doesn't exist anymore, and update remaining _handleTabSelect call sites for signature change.

https://reviewboard.mozilla.org/r/165360/#review170598

r=me to unbreak stuff, but:

1) please file a bug to ensure we have better automated test coverage of this. This regression shouldn't have survived automated tests. We have automated test coverage of dnd on about:newtab (or at least, we used to before activity stream), those might be a useful template to work from as it might require synthesizing native events.
2) please file a bug to update/remove the remaining mentions of `scrollPosition` (devtools, test_mousescroll.xul). They don't look to me like they're actually still current, but we should tidy up the loose ends to be sure.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]:

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2df8633a4ff
Use _scrollbox.scrollLeft instead of scrollPosition which doesn't exist anymore, and update remaining _handleTabSelect call sites for signature change. r=Gijs

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 14

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that issue is FIXED, starting since Mozilla Firefox Nightly 57.0a1 (2017-08-07), so I'm marking this bug as VERIFIED.

Comment 16

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

(In reply to :Gijs from comment #4)
> 1) please file a bug to ensure we have better automated test coverage of
> this. This regression shouldn't have survived automated tests. We have
> automated test coverage of dnd on about:newtab (or at least, we used to
> before activity stream), those might be a useful template to work from as it
> might require synthesizing native events.
> 2) please file a bug to update/remove the remaining mentions of
> `scrollPosition` (devtools, test_mousescroll.xul). They don't look to me
> like they're actually still current, but we should tidy up the loose ends to
> be sure.

Dao, can you please take care of those two review comments from Gijs? Thanks.

Comment 17

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

Since this is fixed and verified, no need to track it for 57.