All users were logged out of Bugzilla on October 13th, 2018

[Devtools][Responsive mode] The "touchend" event should not have the removed touch

RESOLVED FIXED in Firefox 47

Status

RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

unspecified
Firefox 47

Firefox Tracking Flags

(firefox46 affected, firefox47 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8722707 [details]
testcase-touchstart.html

STR:
1. Load the attachment.
2. Press "ctrl shift M" to switch to responsive mode.
3. Click the "simulate touch events" button.
4. Reload the page.
5. Click in the green square.

=> It shows:
touchstart 1
touchend 1

"1" is "event.touches.length". The "touchend" event should show "0" because it should have an empty event.touches TouchList.


The same STR on Firefox OS shows:

touchstart 1
touchend 0

which is expected according to the spec [1].

I haven't tried Firefox/Android but I suspect this is like FxOS.

[1] http://w3c.github.io/touch-events/#dfn-touchend
(Assignee)

Comment 1

3 years ago
I tried Aurora 46 and Nightly 47.
(Assignee)

Updated

3 years ago
status-firefox46: --- → affected
status-firefox47: --- → affected
(Assignee)

Comment 2

3 years ago
Created attachment 8722892 [details] [diff] [review]
0001-Bug-1250691-Devtools-Responsive-mode-The-touchend-ev.patch

Here is a possible patch.

Hey Ryan, do you know if this code is tested ? Where should I put some tests about this ?
Attachment #8722892 - Flags: feedback?(jryans)
(Assignee)

Comment 3

3 years ago
(note: the patch is untested yet)
(Assignee)

Comment 4

3 years ago
Comment on attachment 8722892 [details] [diff] [review]
0001-Bug-1250691-Devtools-Responsive-mode-The-touchend-ev.patch

Review of attachment 8722892 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/touch/simulator-content.js
@@ +288,4 @@
>      let touches = document.createTouchList(point);
>      let targetTouches = touches;
>      let changedTouches = touches;
> +    if (name === "touchend") {

even if we don't handle "touchcancel" anywhere, do you think I should test for it here as a future-proof rule of least surprise ?
Comment on attachment 8722892 [details] [diff] [review]
0001-Bug-1250691-Devtools-Responsive-mode-The-touchend-ev.patch

Review of attachment 8722892 [details] [diff] [review]:
-----------------------------------------------------------------

Here's an existing test you could extend:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsivedesign/test/browser_responsiveui_touch.js

::: devtools/shared/touch/simulator-content.js
@@ +288,4 @@
>      let touches = document.createTouchList(point);
>      let targetTouches = touches;
>      let changedTouches = touches;
> +    if (name === "touchend") {

Sure, let's add `touchcancel` too.
Attachment #8722892 - Flags: feedback?(jryans) → feedback+
(Assignee)

Comment 6

3 years ago
Created attachment 8723796 [details] [diff] [review]
patch v2

Here is a patch with a passing test (the same test is failing on master).

I'll start a try tomorrow and also check this is working with my more complex workload.
Assignee: nobody → felash
Attachment #8722892 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8723796 - Flags: review?(jryans)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8723796 [details] [diff] [review]
patch v2

actually it's still the old patch
Attachment #8723796 - Flags: review?(jryans)
(Assignee)

Comment 9

3 years ago
Created attachment 8724138 [details] [diff] [review]
patch v2
Attachment #8723796 - Attachment is obsolete: true
Attachment #8724138 - Flags: review?(jryans)
Attachment #8724138 - Flags: review?(jryans) → review+
Thanks for working on this!
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4972f77869de
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Duplicate of this bug: 985120

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.