Closed Bug 1162771 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Events, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox40 --- affected
firefox51 --- fixed

People

(Reporter: djf, Assigned: kats)

References

Details

Attachments

(5 files)

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.
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.
Flags: needinfo?(bugmail.mozilla)
Attached file reduced test case
I'm attaching a local copy of the test case from http://djf.net/tt.html
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.
See Also: → 1160408
Awesome, thanks a lot for the reduced test case!
Flags: needinfo?(bugmail.mozilla)
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.
Blocks: 1244402
OS: Gonk (Firefox OS) → Android
This is an issue on Windows desktop as well, and it doesn't happen in Chrome.
Assignee: nobody → bugmail
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
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
Doesn't that break touch events on native anonymous content?
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.
(In reply to (away Aug5-14) Kartikaya Gupta (email:kats@mozilla.com) 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.
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?
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.
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
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).
Attachment #8782048 - Flags: review?(bugs)
This is the actual fix and a test to go with it. I adapted the minimal test case that djf provided into a mochitest.
Attachment #8782052 - Flags: review?(bugs)
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()?
Attachment #8782047 - Flags: review?(bugs) → review+
Attachment #8782048 - Flags: review?(bugs) → review+
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
Attachment #8782050 - Flags: review?(bugs) → review+
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.
Attachment #8782052 - Flags: review?(bugs) → review+
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.
Pushed by kgupta@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.