Bug 50386 – Event::fromUserGesture() should be based on Event creation

• Component: DOM
• Status: RESOLVED INVALID

[Kristian Monsen]

At the moment a Event is considered a UserGesture if UserGestureIndicator::processingUserGesture() returns true when the event is handled.

The correct thing to do is to base it on what UserGestureIndicator::processingUserGesture() returns when the event is created. If the JS engine handles the event on a different call stack, Event::fromUserGesture() should still return true.

This is similar to how ScheduledNavigation works.

[Kristian Monsen]

Created attachment 75374 [details]
Proposed patch

[Alexey Proskuryakov]

Shouldn't the "other call stack" just have UserGestureIndicator::processingUserGesture() respond appropriately?

It's best to implement unusual single platform features in a way that doesn't require modifications to core WebKit, as otherwise we'd keep unknowingly breaking them.

[Kristian Monsen]

(In reply to comment #2)
> Shouldn't the "other call stack" just have UserGestureIndicator::processingUserGesture() respond appropriately?
> 
> It's best to implement unusual single platform features in a way that doesn't require modifications to core WebKit, as otherwise we'd keep unknowingly breaking them.

Hi Alexey, thanks for the review!

I didn't think it was Android related, which is why I wanted reviews here without Android guards. We are using V8, so I am quite sure it can be reproduced on other V8 platforms as well.

You are right that there are other solutions, but this should at least always be correct? To do it they way you suggest will require larger changes, and will depend on the JS implementation. The problem with this solution is that there might be many other similar problems.

[Alexey Proskuryakov]

Yes, I don't see any specific problem with this patch - other that it leaves the code in a confusing state (when exactly can we use UserGestureIndicator::processingUserGesture() without breaking Android?).

There are some folks CC'ed who recently worked on user gestures, and who would be better qualified to officially review this.

[Johnny(Jianning) Ding]

Hi kristianm,
Thanks for your patch. I am not a reviewer, I recently work on the gesture APIs, so I'd like to share my different point.

Yes, currently  Event is considered as a UserGesture if UserGestureIndicator::processingUserGesture() returns true when the event is handled, that is because when event is out of event handling process, it is not user gesture any more. The gesture status should not be cached when event is created.  Please take a look the following test case.

<a onclick="alert(event.type);window.open('http://www.google.com')" id="test"> open a new window </a>
<script>
function dispatchEvent(obj, evt)  { 
  return function() {
     return obj.dispatchEvent(evt);
  }
}
function simulateClick() {
  var evt = document.createEvent("MouseEvents");
  evt.initMouseEvent("click", true, true, window,
    0, 0, 0, 0, 0, false, false, false, false, 0, null);
  var cb = document.getElementById("test"); 
  setTimeout(dispatchEvent(cb, evt), 3000);
}
</script>
<input type="button" onclick="simulateClick();" value="click me" id="btn">

On this case, when clicking button "click me", the evt is created and will be delayed to dispatch 3 seconds later and trigger a popup window to www.google.com.

In original gesture implementation, when the evt is dispatched, since the original real click event is gone, UserGestureIndicator::processingUserGesture() returns false, the popup window is considered as not user-initiated, the popup blocker can block it.

Using your way, the evt was created under a gesture and kept the status in this test case. When the evt is dispatched, it is considered as a user gesture, so the popup blocker let the popup window pass.  Some sites can use the way to create lots of events in a legal user-initiated event and dispatch them in any time they want, and popup blocker can not do anything under this case.

In NavigationScheduler, since there is no chance to create event, so it's fine to cache the gesture status.

Please correct me if I misunderstand your point. 

Adam is expert in the area, he may have more thoughts.

[Alexey Proskuryakov]

Comment on attachment 75374 [details]
Proposed patch

That's quite convincing, marking r-.

We should probably land a test case like this to avoid future regressions.

[Johnny(Jianning) Ding]

(In reply to comment #6)
> (From update of attachment 75374 [details])
> That's quite convincing, marking r-.
> 
> We should probably land a test case like this to avoid future regressions.

Will do.

[Johnny(Jianning) Ding]

Filed another bug https://bugs.webkit.org/show_bug.cgi?id=50508 and provided patch.