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)
DevTools
Responsive Design Mode
Tracking
(firefox46 affected, firefox47 fixed)
RESOLVED
FIXED
Firefox 47
People
(Reporter: julienw, Assigned: julienw)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
I tried Aurora 46 and Nightly 47.
Assignee | ||
Updated•9 years ago
|
status-firefox46:
--- → affected
status-firefox47:
--- → affected
Assignee | ||
Comment 2•9 years ago
|
||
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•9 years ago
|
||
(note: the patch is untested yet)
Assignee | ||
Comment 4•9 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•9 years ago
|
||
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 | ||
Comment 7•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8723796 -
Flags: review?(jryans)
Assignee | ||
Comment 8•9 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•9 years ago
|
||
Attachment #8723796 -
Attachment is obsolete: true
Attachment #8724138 -
Flags: review?(jryans)
Attachment #8724138 -
Flags: review?(jryans) → review+
Thanks for working on this!
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 11•9 years ago
|
||
Keywords: checkin-needed
Comment 12•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•