1457338 - Tap in tab strip stops mouse drags

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Steps to reproduce:
1. Create a new tab (Ctrl+T)
2. Tap (with the touchscreen) on the tab handle
3. Try to drag the tabs around with the mouse

Result:
Dragging tabs, or any other drag 'n drop with the mouse (besides text selection), does not work for the window

Expected:
Dragging with the mouse should continue to work

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e6ecac1e488465a34471999306e1776e7fe11a1d&tochange=83a2d9c67bb62fa3355ea7cc0541c8c296190a40

Comment 1

[:Gijs (he/him)]

:kats, might you know what's going on here? I'm pretty worried about this turning up as a regression for 60...

Also setting in-testsuite flag because it'd be nice if we got some automated test coverage for this type of issue. Not a lot of us use Windows as our primary machines, and if/when we do, apparently not a lot of us use touch screens or it seems we would have noticed this sooner...


Adam: is there a workaround to get dragging to work around after breaking it as you suggest (tapping it again; something else)?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Hm, weird. I can't reproduce this on my Surface device with either Nightly or Beta.

I agree it would be nice to get automated test coverage for this but in general testing these sorts of user interactions is pretty hard and brittle. We have some machinery [1] for simulating touch input and we use it for testing touch-based APZ interactions, but I don't think we have any chrome/UI tests that use it. It should be possible but I haven't done much in the way of chrome mochitests before so I don't really know where to start.

Johann, are you able to reproduce this issue?

[1] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/dom/interfaces/base/nsIDOMWindowUtils.idl#608,639

Comment 3

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to :Gijs (he/him) from comment #1)
> 
> Adam: is there a workaround to get dragging to work around after breaking it
> as you suggest (tapping it again; something else)?

I haven't found anything yet. Touch dragging tabs still works. If I open a new window that new window behaves normally.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Hm, weird. I can't reproduce this on my Surface device with either Nightly
> or Beta.

fwiw, I'm on a Dell XPS 15-inch (2017) (aka XPS 15 9560 Signature Edition) running Windows 10 Pro build 16299.371. Maybe Dell has some funky touch driver, but it doesn't look like it from Device Manager.

Comment 4

[Johann Hofmann [:johannh]]

Man, these touch bugs always come in before the long weekends. I'll have a chance to take a look at this at the earliest on Wednesday next week. I have a Dell XPS 13 from 2017, so I guess my chances of reproducing it aren't so bad. I'll post an update once I've investigated. Generally the description totally sounds like it could have been produced by bug 1446904 (though I still think we did the right thing there).

During fixing bug 1446904 I noticed a couple of places where we're not setting mInTouchDrag = false, but where it would intuitively make sense to do so. Back in the day I thought it might do more harm than good to change it, but now that would be my first approach in investigating...

Thanks for reporting this!

(In reply to :Gijs (he/him) from comment #1)
> :kats, might you know what's going on here? I'm pretty worried about this
> turning up as a regression for 60...

I'm not really worried about this for 60, considering that we've lived with bug 1446904 for two releases. That also affected touch users and in a much worse way. It also seems to affect only certain systems.

> Also setting in-testsuite flag because it'd be nice if we got some automated
> test coverage for this type of issue. Not a lot of us use Windows as our
> primary machines, and if/when we do, apparently not a lot of us use touch
> screens or it seems we would have noticed this sooner...

I agree with kats that it sounds pretty brittle to test. I think I've tried testing this for the original touch drag feature but wasn't really successful, though I can try again. In general I don't see a way how this bug could have been caught by automated tests, it sounds pretty edge case-y (first touch-tapping and then dragging tabs with the mouse) unless you have a specific regression test for it.

> Adam: is there a workaround to get dragging to work around after breaking it
> as you suggest (tapping it again; something else)?

Presumably the tap enters a touch drag session but doesn't end it, so touch-dragging again should fix it.

Thanks everyone!

Comment 5

[Johann Hofmann [:johannh]]

Attachment: Bug 1457338 - Stop touch drag on mouseUp.

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

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8973196 [details]
Bug 1457338 - Stop touch drag on mouseUp.

https://reviewboard.mozilla.org/r/241690/#review247514

Sure... why not :/

Comment 7

[Johann Hofmann [:johannh]]

Yeah, so this patch fixes the issue for me. I'll put up a try build in a second to make it easier for Adam to try it out...

Still looking into testing...

Comment 8

[Johann Hofmann [:johannh]]

Adam, can you check whether the build here fixes the issue for you:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59ced74cd362d1de442ae38082990c8cd730fe64&selectedJob=176963983

Thanks!

Comment 9

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Thanks Johann, just touch tapping doesn't lock up mouse dragging anymore! However I'm noticing that actually touch dragging a tab will cause the next mouse drag (of tabs or otherwise) to be ignored (not always, though I'm not sure what the difference is, and not if you click the mouse first).

After the first drag things seem to go back to normal, but there is also this odd issue:
1. Select text
2. Touch drag the current tab
3. Attempt to mouse drag the selected text, nothing seems to happen though the text becomes unselected at the end of the drag
4. Select a different section of text
5. Attempt to drag this new section

The drag is animated as if you were dragging the text from step 1 instead of the current selection from step 4. If both selections are in a textarea, the text from step 4 is removed when the drag and drop completes but the text from step 1 is inserted at the target.

Comment 10

[Johann Hofmann [:johannh]]

(In reply to Adam Gashlin [:agashlin] from comment #9)
> Thanks Johann, just touch tapping doesn't lock up mouse dragging anymore!
> However I'm noticing that actually touch dragging a tab will cause the next
> mouse drag (of tabs or otherwise) to be ignored (not always, though I'm not
> sure what the difference is, and not if you click the mouse first).
> 
> After the first drag things seem to go back to normal, but there is also
> this odd issue:
> 1. Select text
> 2. Touch drag the current tab
> 3. Attempt to mouse drag the selected text, nothing seems to happen though
> the text becomes unselected at the end of the drag
> 4. Select a different section of text
> 5. Attempt to drag this new section
> 
> The drag is animated as if you were dragging the text from step 1 instead of
> the current selection from step 4. If both selections are in a textarea, the
> text from step 4 is removed when the drag and drop completes but the text
> from step 1 is inserted at the target.

Are these regressions from my patch? If not I'd consider all these edge-casey enough that we can just file backlog bugs for them.

Thanks for testing!

Comment 11

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Hofmann [:johannh] from comment #10)
> Are these regressions from my patch? If not I'd consider all these
> edge-casey enough that we can just file backlog bugs for them.

I'm not sure, they don't seem to have been introduced by bug 1362065 which added the touch gesture, but it is hard to tell after bug 1446904 since currently mouse drags don't work at all after touch interactions with tabs.

I think things are sufficiently less broken with your patch that it'd probably be fine to file as another bug or two. Thanks!

Comment 12

[Johann Hofmann [:johannh]]

Right, I meant whether the patch in this bug introduced any new regressions, not the one in bug 1362065. :)

I presume there are more cases where we don't correctly exit touchdrag and it just didn't really occur before because few people were using the double-tap-to-drag feature.

As for testing, there's https://searchfox.org/mozilla-central/rev/f30847c12e5fb13791401ed4330ced3e1b9c8d81/gfx/layers/apz/test/mochitest/apz_test_native_event_utils.js#292 but I can't really seem to get it to work on the tab strip in my local testing. It feels like this requires a lot more digging into the synthesizing code which I just don't have the bandwidth for right now, so if we want to land this patch we probably need to defer having test coverage for touch dragging to a follow-up bug.

In general touch drag is already covered by all the mouse drag tests because the whole thing was written so that the platform layer just applies the mouse drag when a touch drag is performed, so we would just need to have regression tests for the edge cases in the platform code, e.g. this bug.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1aeaff1e13f8
Stop touch drag on mouseUp. r=kats

Comment 14

[Johann Hofmann [:johannh]]

I filed bug 1460280 for the tests. Adam, can you please file the bugs you mentioned in comment 9?

Thanks!

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1aeaff1e13f8

Comment 16

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Johann Hofmann [:johannh] from comment #14)
> I filed bug 1460280 for the tests. Adam, can you please file the bugs you
> mentioned in comment 9?

Filed bug 1460637 and bug 1460640.

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 8973196 [details]
Bug 1457338 - Stop touch drag on mouseUp.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1446904
[User impact if declined]: Touch-dragging tabs in the tab strip or customize mode makes mouse-dragging afterwards impossible (on certain systems)
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0, note that we have only been able to reproduce this on Dell Touch Laptops so far
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: One line fix that exits touch drag on certain mouse events. If there is a problem with this patch it affects touch dragging at best.
[String changes made/needed]: None

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8973196 [details]
Bug 1457338 - Stop touch drag on mouseUp.

One-liner fix for tab dragging via touch. Approved for 61.0b5.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ede4d13c43c6

Comment 20

[Adrian Florinescu [:aflorinescu]]

I've managed to reproduce the issue as described in comment 0 on a Dell Optiplex 9020 AIO desktop with Windows 8.1 x64 Pro with touch support on 61.0b3 20180507191226.

Verified as fixed on 62.0a1 20180515100038 and on 61.0b5 20180514150347.

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8973196 [details]
Bug 1457338 - Stop touch drag on mouseUp.

let's do this for esr60 as well

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/12bcae394943