1162771 - touchend event on media element is not sent if element is hidden on touchstart

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[David Flanagan [:djf]]

One of the nice things about touch events is that the touchend is always delivered to the event that touchstart was delivered too.  This even happens if the element is hidden after the touchstart: the touchend gets delivered to the hidden element when the user lifts their finger.

But it turns out that that is not happening for video elements and for audio elements with the control attribute, and this caused FirefoxOS bug 1160408.

The issue is demonstrated with the sample code at http://djf.net/tt.html, and affects FirefoxOS and Firefox for Android.  

STR: 

- load djf.net/tt.html into the FirefoxOS browser or into Firefox on Android

- see three colored boxes. From left to right they are a <video> element, and <audio controls> element and a <div> element.  Each one has event handlers that hide it on touchstart.

- tap on each box in turn to hide.

- the test displays a log of events received. Note that for the video and audio elements, only a touchstart event is sent.  For the div element (the rightmost box) we get a touchstart and a touchend even though the element is hidden before the user lifts their finger.

I imagine that this has something to do with the fact that audio and video elements have xul or shadow dom or something for their controls.

Comment 1

[David Flanagan [:djf]]

Kats: this is a followup to bug 1160408. I'm going to attempt a gaia-based workaround for that bug, and have filed this one for the gecko fix.

Comment 2

[David Flanagan [:djf]]

Attachment: reduced test case

I'm attaching a local copy of the test case from http://djf.net/tt.html

Comment 3

[David Flanagan [:djf]]

Note also that the bug does not occur in Chrome for Android.  In that browser the touchend is delivered properly in all three cases.

And also, the <audio> element only causes the bug if it has the controls attribute. The <video> element causes the bug even without controls. The test case doesn't actually play a video or any music, but I have verified that the bug also occurs when a video is playing.

Comment 4

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

Awesome, thanks a lot for the reduced test case!

Comment 5

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

This appears to still be an issue in Fennec nightly. I didn't check desktop but marking it blocking bug 1244402 so that we investigate this before shipping touch on desktop.

Comment 6

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

This is an issue on Windows desktop as well, and it doesn't happen in Chrome.

Comment 7

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

So I spent some time tracing through the code. It looks like the target of the event is a HTMLDivElement that's inside the anonymous subtree under the HTMLVideoElement. When the touchstart sets the display of the video element to 'none', the HTMLDivElement's parent gets set to null.

However it's a bit racy - sometimes the touchend event will sneak through before that happens, and in those cases the test page works as expected. If it doesn't sneak through, then the PreHandleEvent call at [1] goes into FragmentOrElement::PreHandleEvent on the HTMLDivElement, and at [2] parent comes out as null. The ComposedDoc() for the element is all null, so at [3] the mParentTarget gets set to null, and consequently the loop at [4] never iterates and nothing seems to fire.

It's not really clear to me why the target stored in the touch in the gTouchList is the anonymous HTMLDivElement rather than the HTMLVideoElement.

[1] http://searchfox.org/mozilla-central/rev/d9a04f71630ce4203ff0a5e26722723045d035b5/dom/events/EventDispatcher.cpp#637
[2] http://searchfox.org/mozilla-central/rev/d9a04f71630ce4203ff0a5e26722723045d035b5/dom/base/FragmentOrElement.cpp#776
[3] http://searchfox.org/mozilla-central/rev/d9a04f71630ce4203ff0a5e26722723045d035b5/dom/base/FragmentOrElement.cpp#905
[4] http://searchfox.org/mozilla-central/rev/d9a04f71630ce4203ff0a5e26722723045d035b5/dom/events/EventDispatcher.cpp#654

Comment 8

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

I wrote a naive patch that walks out of anonymous subtrees when picking the touch target. Try push to see what breaks:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=332272e4493c

Comment 9

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

Doesn't that break touch events on native anonymous content?

Comment 10

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

Probably, yes. I'm not sure what relies on that behaviour and was hoping test failures would provide some insight. Right now it looks like layout/forms/test/test_textarea_resize.html is the only consistently failing test as a result of this change.

Another option is to maybe cache both the anonymous subtree node (the HTMLDivElement) and its non-anonymous content parent (the HTMLVideoElement) in the gTouchList somewhere, and if the anonymous one gets disconnected, we pick up the non-anonymous parent. I'm not sure how well that would work, but it's something I can try. If you have other suggestions I'm happy to try those too.

Comment 11

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

(In reply to (away Aug5-14) Kartikaya Gupta (email:[e-mail]) from comment #10)
> Another option is to maybe cache both the anonymous subtree node (the
> HTMLDivElement) and its non-anonymous content parent (the HTMLVideoElement)
> in the gTouchList somewhere, and if the anonymous one gets disconnected, we
> pick up the non-anonymous parent. I'm not sure how well that would work, but
> it's something I can try. If you have other suggestions I'm happy to try
> those too.
That might work quite easily. Just need to check if the native anon isn't in composed doc anymore and switch to use the non-nac element.

Comment 12

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

I'm looking into doing the change in comment 10 / comment 11. It seems like I will need to store the non-native element for each touch point, correct? I'm planning on changing the gCaptureTouchList from
  nsRefPtrHashtable<nsUint32HashKey, dom::Touch>*
to
  nsDataHashtable<nsUint32HashKey, TouchInfo>*

where TouchInfo is something like

struct TouchInfo {
  RefPtr<mozilla::dom::Touch> mTouch;
  nsCOMPtr<nsIContent> mNonAnonymousTarget;
};

Does that seem reasonable? Or are there some simplifying assumptions I can make somewhere, like just keeping one non-anonymous target in TouchManager?

Comment 13

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

So if mTouch's target is native anonymous and isn't in composed document anymore, we would use
mNonAnonymousTarget as target?
Looks reasonable and simple.

Comment 14

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

I guess it's conceptually simple but I did a bunch of code cleanup to encapsulate the code a bit more before implementing the fix. Try push with the patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12b1a6e914a3

Comment 15

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

Attachment: Part 1 - Refactoring to better encapsulate gCaptureTouchList

Comment 16

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

Attachment: Part 2 - Rename gCaptureTouchList to sCaptureTouchList

I can take this out if you want, but I thought it was more appropriate since it's a static member now (used to be global before TouchManager existed).

Comment 17

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

Attachment: Part 3 - Store the non-anonymous ancestor in the touch list

As proposed in comment 12

Comment 18

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

Attachment: Part 4 - Actual fix and test

This is the actual fix and a test to go with it. I adapted the minimal test case that djf provided into a mochitest.

Comment 19

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

Comment on attachment 8782047 [details] [diff] [review]
Part 1 - Refactoring to better encapsulate gCaptureTouchList

>+/*static*/ already_AddRefed<nsIContent>
>+TouchManager::GetCapturedTouchTarget()
>+{
>+  nsCOMPtr<nsIContent> result = nullptr;


>+  if (gCaptureTouchList->Count() == 0) {
>+    return result.forget();
>+  }
>+  for (auto iter = gCaptureTouchList->Iter(); !iter.Done(); iter.Next()) {
>+    RefPtr<dom::Touch>& touch = iter.Data();
>+    if (touch) {
>+      dom::EventTarget* target = touch->GetTarget();
>+      if (target) {
>+        result = do_QueryInterface(target);
>+        break;
>+      }
>+    }
>+  }
>+  return result.forget();
Hmm, so this returns just any target in the list.
Could we call this method GetAnyCapturedTouchTarget

>+}
>+
>+/*static*/ bool
>+TouchManager::HasCapturedTouch(int32_t aId)
>+{
>+  return gCaptureTouchList->Get(aId, nullptr);
>+}
Don't we have Contains()?

Comment 20

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

Comment on attachment 8782050 [details] [diff] [review]
Part 3 - Store the non-anonymous ancestor in the touch list

>@@ -123,17 +133,18 @@ TouchManager::PreHandleEvent(WidgetEvent* aEvent,
>       for (uint32_t i = 0; i < touchEvent->mTouches.Length(); ++i) {
>         dom::Touch* touch = touchEvent->mTouches[i];
>         int32_t id = touch->Identifier();
>         if (!sCaptureTouchList->Get(id, nullptr)) {
>           // If it is not already in the queue, it is a new touch
>           touch->mChanged = true;
>         }
>         touch->mMessage = aEvent->mMessage;
>-        sCaptureTouchList->Put(id, touch);
>+        TouchInfo info = { touch, GetNonAnonymousAncestor(touch->mTarget) };
>+        sCaptureTouchList->Put(id, info);
So here we store the Non-Anon in sCaptureTouchList

>-        sCaptureTouchList->Put(id, touch);
>+        info.mTouch = touch;
>+        sCaptureTouchList->Put(id, info);
but here we just update info.mTouch. Could you add some comment to this latter case, that the old non-anon is still valid.

>-  static nsRefPtrHashtable<nsUint32HashKey, mozilla::dom::Touch>* sCaptureTouchList;
>+  struct TouchInfo {
Nit, { goes to its own line

Comment 21

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

Comment on attachment 8782052 [details] [diff] [review]
Part 4 - Actual fix and test

I assume you tested other browsers send touchend similarly.
Based on the comments in the bug, that is the case at least on Android Chrome.

Comment 22

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

Updated patches to address all review comments. And yeah I also tested in Chrome for desktop (Windows) and it sends the touchend properly. I don't have access to an iOS device to test safari touch.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47b7bf339876
Encapsulate the gCaptureTouchList so that it's not exposed outside of the TouchManager class. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2eed81cdcff
Rename gCaptureTouchList to sCaptureTouchList. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a92dc8fb7769
Store the non-anonymous ancestor content along with the touch info the captured touch list. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b6142bbcd5
Fall back to using the non-anonymous ancestor node of the target if the anonymous target is disconnected from the document. r=smaug

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47b7bf339876
https://hg.mozilla.org/mozilla-central/rev/e2eed81cdcff
https://hg.mozilla.org/mozilla-central/rev/a92dc8fb7769
https://hg.mozilla.org/mozilla-central/rev/d7b6142bbcd5