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

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

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.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d69317ef2dea

Comment 4

[Carsten Book [:Tomcat]]

sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=43929921&tree=Mozilla-Inbound

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Try push at https://tbpl.mozilla.org/?tree=Try&rev=3d7cb20f3660 includes this change

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8456909 [details] [diff] [review]
Patch v2

I guess we could try this.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c752c9ef9596

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c752c9ef9596