Bug 41061 - User's gesture initiated from select's onchange event can not be treated as user initiated in chromium
Summary: User's gesture initiated from select's onchange event can not be treated as u...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-23 06:17 PDT by Johnny(Jianning) Ding
Modified: 2010-06-29 01:13 PDT (History)
5 users (show)

See Also:


Attachments
test case (312 bytes, text/html)
2010-06-23 06:25 PDT, Johnny(Jianning) Ding
no flags Details
patch v1 (4.79 KB, patch)
2010-06-23 09:14 PDT, Johnny(Jianning) Ding
abarth: review-
Details | Formatted Diff | Diff
patch v2 (7.60 KB, patch)
2010-06-24 06:55 PDT, Johnny(Jianning) Ding
abarth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch v3 (7.96 KB, patch)
2010-06-25 04:17 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch V4 to make the test work on both Mac & Linux. (2.67 KB, patch)
2010-06-28 04:51 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Johnny(Jianning) Ding 2010-06-23 06:17:02 PDT
Please refer to http://code.google.com/p/chromium/issues/detail?id=47276 for the bug details.

After investigation, this bug was because chromium thought the popup was not user initiated.
In function:  bool ScriptController::processingUserGesture(DOMWrapperWorld*) const  (WebCore/binding/v8/ScriptController.cpp)

Chromium tests whether UserGestureIndicator::processingUserGesture() returns true before checking the current event object.  If not, it returns false to indicate  the disposition was not user initiated. 

However, in JSC (WebCore/binding/js/ScriptController.cp), it tests the current event object at first, if the event can be treated as user gesture,  JSC will treat the disposition as user initiated.

For why UserGestureIndicator::processingUserGesture() returns false (both JSC and V8) when current event is from select element (widget), it's because
now we set the UserGestureIndicator as DefinitelyUserGesture only in the event handlers written in event_handler.cpp which are called by webview. For the event handlers which are called by widget (derived from FrameScrollView), we didn't change UserGestureIndicator to DefinitelyUserGesture.  I think we probably  need to fix this behavior.

I will provide a patch to make sure the ScriptController::processingUserGesture follows the JSC's behavior.
Comment 1 Johnny(Jianning) Ding 2010-06-23 06:25:29 PDT
Created attachment 59509 [details]
test case
Comment 2 Johnny(Jianning) Ding 2010-06-23 09:14:21 PDT
Created attachment 59520 [details]
patch v1
Comment 3 Adam Barth 2010-06-23 10:26:27 PDT
Comment on attachment 59520 [details]
patch v1

The JSC implementation of this function is 10 lines long.  Why is the V8 version 50 lines?  Please update the V8 version to match the JSC version.
Comment 4 Johnny(Jianning) Ding 2010-06-24 05:50:05 PDT
The main reason for this issue is in chromium the select was implemented by a widget (derived from FramelessScrollView), all input events are delivered to widget's event handlers which don't set UserGestureIndicator to DefinitelyUserGesture.

But in safari, all input events are delivered by webview and handled by event handlers in WebCore/page/EventHandler.cpp) in which UserGestureIndicator is set to DefinitelyUserGesture.

I also changed the V8 version ScriptController::processingUserGesture to match the JSC version as much as possible.
Comment 5 Johnny(Jianning) Ding 2010-06-24 06:55:39 PDT
Created attachment 59650 [details]
patch v2
Comment 6 Adam Barth 2010-06-24 08:50:04 PDT
Comment on attachment 59650 [details]
patch v2

Much better.  Thanks.
Comment 7 Johnny(Jianning) Ding 2010-06-24 20:22:57 PDT
Since it is not actual v8 binding bug, I removed the [V8 binding] from the summary
Comment 8 Johnny(Jianning) Ding 2010-06-25 04:16:38 PDT
Hi Adam,

Sorry to bother you again,

I think I didn't exactly make the V8 version ScriptController::processingUserGesture to match the JSC version since I missed one thing in patch v2,

In JSC version ScriptController::processingUserGesture, if there is no available event, then use the returned value of UserGestureIndicator::processingUserGesture() to indicate the user gesture status.

In V8, we should match this. Please see my patch v3.

Sorry again for the inconvenience.
Comment 9 Johnny(Jianning) Ding 2010-06-25 04:17:12 PDT
Created attachment 59755 [details]
patch v3
Comment 10 WebKit Commit Bot 2010-06-25 17:22:43 PDT
Comment on attachment 59650 [details]
patch v2

Rejecting patch 59650 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
e
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19223 test cases.
fast/workers/storage/execute-sql-args-worker.html -> crashed

Exiting early after 1 failures. 15317 tests run.
633.17s total testing time

15316 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3341799
Comment 11 Adam Barth 2010-06-25 17:44:53 PDT
Comment on attachment 59755 [details]
patch v3

ok
Comment 12 WebKit Commit Bot 2010-06-25 22:40:01 PDT
Comment on attachment 59755 [details]
patch v3

Clearing flags on attachment: 59755

Committed r61941: <http://trac.webkit.org/changeset/61941>
Comment 13 WebKit Review Bot 2010-06-25 23:05:46 PDT
http://trac.webkit.org/changeset/61941 might have broken GTK Linux 64-bit Release
Comment 14 Adam Barth 2010-06-25 23:16:12 PDT
Strange.  Does GTK use V8?  Oh, I bet they just don't implemented the needed layoutTestController methods.

--- /var/lib/buildbot/build/gtk-linux-64-release/build/layout-test-results/fast/events/popup-when-select-change-expected.txt	2010-06-25 22:51:01.000000000 -0700
+++ /var/lib/buildbot/build/gtk-linux-64-release/build/layout-test-results/fast/events/popup-when-select-change-actual.txt	2010-06-25 22:51:00.000000000 -0700
@@ -1,4 +1,5 @@
-ALERT: PASSED
+CONSOLE MESSAGE: line 19: TypeError: Result of expression 'eventSender.keyDown' [null] is not an object.
+FAIL: Timed out waiting for notifyDone to be called
  If the pop-up was not blocked then there will be an PASS message. Otherwise, the test fails.
Comment 15 Johnny(Jianning) Ding 2010-06-28 04:49:31 PDT
Adam,

Seems like there are some limitations when writing event related testcases on Linux.
I changed the test case to make it work on both Mac & Linux by referring to other testcases. Please review the patch V4.

Thanks!
Comment 16 Johnny(Jianning) Ding 2010-06-28 04:51:11 PDT
Created attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.
Comment 17 Adam Barth 2010-06-28 09:17:58 PDT
Comment on attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

Huh.  Interesting.  I wonder why Linux behaves different...  Maybe there's some problem with event sender?
Comment 18 Johnny(Jianning) Ding 2010-06-28 17:14:40 PDT
Hi Adam

For error: 'eventSender.keyDown' [null] is not an object, I think it's a problem of event sender on linux. But have no time to dig in now, will take a look later.

Since the test is broken on linux, would you or someone who has commit privilege can help me land this patch?

Thanks!
Comment 19 Adam Barth 2010-06-28 17:23:54 PDT
> For error: 'eventSender.keyDown' [null] is not an object, I think it's a problem of event sender on linux. But have no time to dig in now, will take a look later.

Yes.  That function of layoutTestController isn't implemented on Gtk, which means they can't run a ton of tests.
Comment 20 Adam Barth 2010-06-28 17:25:04 PDT
> Since the test is broken on linux, would you or someone who has commit privilege can help me land this patch?

Is the test on the appropriate Skip lists?  If so, we can land it using commit-queue.  However, if it's going to make some builders red, we can't land it.
Comment 21 Johnny(Jianning) Ding 2010-06-28 19:10:24 PDT
(In reply to comment #20)
> > Since the test is broken on linux, would you or someone who has commit privilege can help me land this patch?
> 
> Is the test on the appropriate Skip lists?  If so, we can land it using commit-queue.  However, if it's going to make some builders red, we can't land it.

Yes, this test is in appropriate Skip lists, they are LayoutTests/platform/chromium/test_expectations.txt (Chromium:Linux) and LayoutTests/platform/gtk/Skipped.

Since it is only affected Linux version, I have tested this test case on my local Linux build of both Chrome and WebKit, they are all green.
So I think it's safe to land the patch 4.
Comment 22 Adam Barth 2010-06-28 20:55:30 PDT
Comment on attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

okiedokes
Comment 23 WebKit Commit Bot 2010-06-28 22:13:08 PDT
Comment on attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

Rejecting patch 59891 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
iling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19232 test cases.
fast/dom/HTMLScriptElement/script-set-src.html -> failed

Exiting early after 1 failures. 6663 tests run.
147.91s total testing time

6662 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3356037
Comment 24 Johnny(Jianning) Ding 2010-06-28 22:43:21 PDT
(In reply to comment #23)
> (From update of attachment 59891 [details])
> Rejecting patch 59891 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
> Last 500 characters of output:
> iling Java tests
> make: Nothing to be done for `default'.
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
> Testing 19232 test cases.
> fast/dom/HTMLScriptElement/script-set-src.html -> failed
> 
> Exiting early after 1 failures. 6663 tests run.
> 147.91s total testing time
> 
> 6662 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 1 test case (<1%) had stderr output
> 
> Full output: http://webkit-commit-queue.appspot.com/results/3356037

Interesting, since the patch V4 only affect a test file (fast/events/popup-when-select-change.html). I am sure that it doesn't affect  script-set-src.html. 
Anything should I do?
Comment 25 Eric Seidel (no email) 2010-06-28 22:48:14 PDT
Comment on attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

Possible it was a flaky test.
Comment 26 WebKit Commit Bot 2010-06-28 23:12:27 PDT
Comment on attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

Clearing flags on attachment: 59891

Committed r62088: <http://trac.webkit.org/changeset/62088>
Comment 27 Johnny(Jianning) Ding 2010-06-28 23:40:38 PDT
(In reply to comment #25)
> (From update of attachment 59891 [details])
> Possible it was a flaky test.

Thanks Eric!
Comment 28 Johnny(Jianning) Ding 2010-06-29 01:13:16 PDT
Make this bug as fixed