Closed Bug 1250691 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: Responsive Design Mode, defect)

defect
Not set
normal

Tracking

(firefox46 affected, firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file testcase-touchstart.html (deleted) —
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
I tried Aurora 46 and Nightly 47.
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)
(note: the patch is untested yet)
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+
Attached patch patch v2 (obsolete) (deleted) — Splinter Review
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
Attachment #8723796 - Flags: review?(jryans)
Comment on attachment 8723796 [details] [diff] [review] patch v2 actually it's still the old patch
Attachment #8723796 - Flags: review?(jryans)
Attached patch patch v2 (deleted) — Splinter Review
Attachment #8723796 - Attachment is obsolete: true
Attachment #8724138 - Flags: review?(jryans)
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: