Closed Bug 1038930 Opened 6 years ago Closed 6 years ago

In some cases the first touchmove event doesn't get dispatched to content

Categories

(Core :: DOM: Events, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
b2g-v1.3 --- ?
b2g-v1.4 --- ?
b2g-v2.0 --- ?
b2g-v2.1 --- affected

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files)

Attached file Test case
STR:
1. On a Hamachi device running latest B2G master code, load the attached page.
2. Zoom in a little so that the page is pannable.
3. Check the third checkbox ("first point touch-move"). This has the effect that the first touch-move event following a new touchstart ("new" meaning going from zero points to non-zero points) will be prevent-defaulted.

Expected behavior: You cannot pan the page after or tap on stuff after checking this box.

Actual: On the Hamachi device you can pretty much always pan and tap on things. A Nexus 4 device behaves as expected.

I tracked this down and the problem seems to be that the first touchmove event dispatched by TabChild has identical properties to the touchstart event that came before it. This causes the Equals() check at [1] to return true, and results in the code at [2] executing, so the touchmove is never dispatched.

Eventually we receive a touchmove that has a different position or something, and that is the first touchmove that is actually dispatched to content. However in that case we don't hit the code at [3], so the touchIsNew flag remains false, and the preventDefault() on the touch is ignored.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=702cc39137a1#7654
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=702cc39137a1#7673
[3] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=702cc39137a1#7667
Attached patch PatchSplinter Review
This fixes the problem for me. An alternate fix would be to change the

if (!haveChanged) {

condition to

if (!haveChanged && !touchIsNew) {

Let me know if you prefer that solution.
Assignee: nobody → bugmail.mozilla
Attachment #8456452 - Flags: review?(bugs)
status-b2g-v1.3: --- → ?
status-b2g-v1.4: --- → ?
status-b2g-v2.0: --- → ?
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> This fixes the problem for me. An alternate fix would be to change the
> 
> if (!haveChanged) {
> 
> condition to
> 
> if (!haveChanged && !touchIsNew) {
> 
> Let me know if you prefer that solution.

This alternate solution doesn't seem to work, I'm not sure why. There might be another similar check somewhere else in the code.
Attachment #8456452 - Flags: review?(bugs) → review+
sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=43929921&tree=Mozilla-Inbound
The problem with the above patch is that it changes the changedTouches list on the first touchmove to include all the touches. This caused a test failure in the part of the test at [1] because now changedTouches included both touch points rather than just the one explicitly modified by the test. In addition, the code at [2] means that in the case where the first touchmove has 2 points (such as in the test), 2 touchmove events are dispatched to content. This also seems undesirable.

After thinking about it some more I concluded that we want to preserve the existing behaviour wherever possible, but in the case where we go from touchstart->touchmove without any of the touch points actually changing, we still want to force a single touchmove event to be dispatched. I wrote a new patch that does this by arbitarily flagging the first touch point in the list as "changed" IF no touch points actually changed. That way the test case scenario (which explicitly changes a touch point) doesn't change.

There was another part of the test that needed updating to reflect the new behavior, and so that exercises the new code changes.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/events/test/test_bug603008.html?force=1#259
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=702cc39137a1#7824
Comment on attachment 8456909 [details] [diff] [review]
Patch v2

I guess we could try this.
Attachment #8456909 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/c752c9ef9596
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.