50386 2010-12-02 07:26:38 -0800 Event::fromUserGesture() should be based on Event creation 2010-12-03 22:20:09 -0800 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) All All RESOLVED INVALID P2 Normal --- 0 kristianm webkit-unassigned abarth ap jnd jorlow steveblock tony oldest_to_newest 316135 0 kristianm 2010-12-02 07:26:38 -0800 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. 316137 1 75374 kristianm 2010-12-02 07:30:49 -0800 Created attachment 75374 Proposed patch 316328 2 ap 2010-12-02 12:16:25 -0800 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. 316365 3 kristianm 2010-12-02 12:53:18 -0800 (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. 316377 4 ap 2010-12-02 13:06:50 -0800 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. 316935 5 jnd 2010-12-03 11:45:53 -0800 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. 317027 6 75374 ap 2010-12-03 13:42:26 -0800 Comment on attachment 75374 Proposed patch That's quite convincing, marking r-. We should probably land a test case like this to avoid future regressions. 317044 7 jnd 2010-12-03 14:09:21 -0800 (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. 317214 8 jnd 2010-12-03 19:06:17 -0800 Filed another bug https://bugs.webkit.org/show_bug.cgi?id=50508 and provided patch. 75374 2010-12-02 07:30:49 -0800 2010-12-03 13:42:25 -0800 Proposed patch patch.txt text/plain 2805 kristianm SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3MzEzMCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjIgQEAKKzIwMTAtMTItMDIgIEtyaXN0aWFuIE1vbnNlbiAgPGtyaXN0aWFubUBn b29nbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIEV2ZW50Ojpmcm9tVXNlckdlc3R1cmUoKSBzaG91bGQgYmUgYmFzZWQgb24gRXZlbnQgY3Jl YXRpb24KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUw Mzg2CisKKyAgICAgICAgQXQgdGhlIG1vbWVudCBhIEV2ZW50IGlzIGNvbnNpZGVyZWQgYSBVc2Vy R2VzdHVyZSBpZiBVc2VyR2VzdHVyZUluZGljYXRvcjo6cHJvY2Vzc2luZ1VzZXJHZXN0dXJlKCkg cmV0dXJucyB0cnVlIHdoZW4gdGhlIGV2ZW50IGlzIGhhbmRsZWQuCisgICAgICAgIFRoZSBjb3Jy ZWN0IHRoaW5nIHRvIGRvIGlzIHRvIGJhc2UgaXQgb24gd2hhdCBVc2VyR2VzdHVyZUluZGljYXRv cjo6cHJvY2Vzc2luZ1VzZXJHZXN0dXJlKCkgcmV0dXJucyB3aGVuIHRoZSBldmVudCBpcyBjcmVh dGVkLiBJZiB0aGUgSlMgZW5naW5lIGhhbmRsZXMgdGhlIGV2ZW50IG9uIGEgZGlmZmVyZW50IGNh bGwgc3RhY2ssIEV2ZW50Ojpmcm9tVXNlckdlc3R1cmUoKSBzaG91bGQgc3RpbGwgcmV0dXJuIHRy dWUuCisgICAgICAgIFRoaXMgaXMgc2ltaWxhciB0byBob3cgU2NoZWR1bGVkTmF2aWdhdGlvbiB3 b3Jrcy4KKworICAgICAgICBObyBuZXcgdGVzdHMuIFRoZSBiZWhhdmlvciBpcyBzaW1pbGFyIHRv IHRoZSBvbGQgYXMgbG9uZyBhcyB0aGUgZXZlbnQgaXMgbm90IGhhbmRsZWQgb24gYSBkaWZmZXJl bnQgY2FsbCBzdGFjayBieSB0aGUgSlMgZW5naW5lLiBJIGhhdmUgb25seSBzZWVuIHRoYXQgb24g QW5kcm9pZC4KKworICAgICAgICAqIFdlYkNvcmUueGNvZGVwcm9qL3Byb2plY3QucGJ4cHJvajoK KyAgICAgICAgKiBkb20vRXZlbnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RXZlbnQ6OkV2ZW50 KToKKyAgICAgICAgKFdlYkNvcmU6OkV2ZW50Ojpmcm9tVXNlckdlc3R1cmUpOgorICAgICAgICAq IGRvbS9FdmVudC5oOgorCiAyMDEwLTEyLTAyICBOaWtvbGFzIFppbW1lcm1hbm4gIDxuemltbWVy bWFubkByaW0uY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFkYW0gUm9iZW4uCkluZGV4OiBX ZWJDb3JlL2RvbS9FdmVudC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9kb20vRXZlbnQuY3Bw CShyZXZpc2lvbiA3MzExNikKKysrIFdlYkNvcmUvZG9tL0V2ZW50LmNwcAkod29ya2luZyBjb3B5 KQpAQCAtMzcsNiArMzcsNyBAQCBFdmVudDo6RXZlbnQoKQogICAgICwgbV9kZWZhdWx0UHJldmVu dGVkKGZhbHNlKQogICAgICwgbV9kZWZhdWx0SGFuZGxlZChmYWxzZSkKICAgICAsIG1fY2FuY2Vs QnViYmxlKGZhbHNlKQorICAgICwgbV93YXNVc2VyR2VzdHVyZShVc2VyR2VzdHVyZUluZGljYXRv cjo6cHJvY2Vzc2luZ1VzZXJHZXN0dXJlKCkpCiAgICAgLCBtX2V2ZW50UGhhc2UoMCkKICAgICAs IG1fY3VycmVudFRhcmdldCgwKQogICAgICwgbV9jcmVhdGVUaW1lKGNvbnZlcnRTZWNvbmRzVG9E T01UaW1lU3RhbXAoY3VycmVudFRpbWUoKSkpCkBAIC01Miw2ICs1Myw3IEBAIEV2ZW50OjpFdmVu dChjb25zdCBBdG9taWNTdHJpbmcmIGV2ZW50VHkKICAgICAsIG1fZGVmYXVsdFByZXZlbnRlZChm YWxzZSkKICAgICAsIG1fZGVmYXVsdEhhbmRsZWQoZmFsc2UpCiAgICAgLCBtX2NhbmNlbEJ1YmJs ZShmYWxzZSkKKyAgICAsIG1fd2FzVXNlckdlc3R1cmUoVXNlckdlc3R1cmVJbmRpY2F0b3I6OnBy b2Nlc3NpbmdVc2VyR2VzdHVyZSgpKQogICAgICwgbV9ldmVudFBoYXNlKDApCiAgICAgLCBtX2N1 cnJlbnRUYXJnZXQoMCkKICAgICAsIG1fY3JlYXRlVGltZShjb252ZXJ0U2Vjb25kc1RvRE9NVGlt ZVN0YW1wKGN1cnJlbnRUaW1lKCkpKQpAQCAtMjQ1LDcgKzI0Nyw3IEBAIGJvb2wgRXZlbnQ6Omlz U3BlZWNoSW5wdXRFdmVudCgpIGNvbnN0CiAKIGJvb2wgRXZlbnQ6OmZyb21Vc2VyR2VzdHVyZSgp CiB7Ci0gICAgaWYgKCFVc2VyR2VzdHVyZUluZGljYXRvcjo6cHJvY2Vzc2luZ1VzZXJHZXN0dXJl KCkpCisgICAgaWYgKCFtX3dhc1VzZXJHZXN0dXJlKQogICAgICAgICByZXR1cm4gZmFsc2U7CiAK ICAgICBjb25zdCBBdG9taWNTdHJpbmcmIHR5cGUgPSB0aGlzLT50eXBlKCk7CkluZGV4OiBXZWJD b3JlL2RvbS9FdmVudC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvZG9tL0V2ZW50LmgJKHJldmlz aW9uIDczMTE2KQorKysgV2ViQ29yZS9kb20vRXZlbnQuaAkod29ya2luZyBjb3B5KQpAQCAtMTg1 LDYgKzE4NSw3IEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKICAgICAgICAgYm9vbCBtX2RlZmF1bHRQ cmV2ZW50ZWQ7CiAgICAgICAgIGJvb2wgbV9kZWZhdWx0SGFuZGxlZDsKICAgICAgICAgYm9vbCBt X2NhbmNlbEJ1YmJsZTsKKyAgICAgICAgYm9vbCBtX3dhc1VzZXJHZXN0dXJlOwogCiAgICAgICAg IHVuc2lnbmVkIHNob3J0IG1fZXZlbnRQaGFzZTsKICAgICAgICAgRXZlbnRUYXJnZXQqIG1fY3Vy cmVudFRhcmdldDsK