Closed Bug 1457338 Opened 2 years ago Closed 2 years ago

Tap in tab strip stops mouse drags

Categories

(Core :: Widget: Win32, defect)

60 Branch
All
Windows
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox60 --- wontfix
firefox61 --- verified
firefox62 --- verified

People

(Reporter: agashlin, Assigned: johannh)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
: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)?
Flags: needinfo?(bugmail)
Flags: needinfo?(agashlin)
Flags: in-testsuite?
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
Flags: needinfo?(bugmail) → needinfo?(jhofmann)
(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:kats@mozilla.com) 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.
Flags: needinfo?(agashlin)
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!
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)
Comment on attachment 8973196 [details]
Bug 1457338 - Stop touch drag on mouseUp.

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

Sure... why not :/
Attachment #8973196 - Flags: review?(bugmail) → review+
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...
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!
Flags: needinfo?(agashlin)
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.
Flags: needinfo?(agashlin)
(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!
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!
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.
I filed bug 1460280 for the tests. Adam, can you please file the bugs you mentioned in comment 9?

Thanks!
Flags: needinfo?(jhofmann)
Flags: needinfo?(jhofmann) → needinfo?(agashlin)
https://hg.mozilla.org/mozilla-central/rev/1aeaff1e13f8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(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.
Flags: needinfo?(agashlin)
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
Attachment #8973196 - Flags: approval-mozilla-beta?
Comment on attachment 8973196 [details]
Bug 1457338 - Stop touch drag on mouseUp.

One-liner fix for tab dragging via touch. Approved for 61.0b5.
Attachment #8973196 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment on attachment 8973196 [details]
Bug 1457338 - Stop touch drag on mouseUp.

let's do this for esr60 as well
Attachment #8973196 - Flags: approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.