Bug 113777

Summary: Actions that require user gesture don't work in window.showModalDialog in Chromium
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: UI EventsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alakutin, esprehn+autocc, jochen, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Keishi Hattori 2013-04-02 06:48:12 PDT
It's because showModalDialog isn't modal in chromium.
Comment 1 jochen 2013-04-02 06:54:45 PDT
Can you give an example?
Comment 2 jochen 2013-04-02 07:07:52 PDT
Saw the example on the crbug entry. Hum hum.

The real solution is to actually make the modal dialog modal, and render it in a different process.

Until then, I guess we need a hack, maybe something like clearing the token in showModalDialog and resetting it once the nested message loop exits.
Comment 3 Keishi Hattori 2013-04-02 07:51:20 PDT
Created attachment 196140 [details]
Patch
Comment 4 jochen 2013-04-02 07:56:29 PDT
Comment on attachment 196140 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196140&action=review

> Source/WebCore/ChangeLog:13
> +        No new tests. Impossible to tell if a file dialog is open in layout test.

you could use the eventSender to click somewhere and try to open a new window from the event handler (with testRunner.setCanOpenWindows(), testRunner.setPopupBlockingEnabled(true);). This should return a non-NULL if the user gesture was correctly updated.
Comment 5 Adam Barth 2013-04-02 10:39:40 PDT
Comment on attachment 196140 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196140&action=review

> Source/WebCore/page/DOMWindow.cpp:2002
> +    UserGestureIndicator* savedIndicator = UserGestureIndicator::topmostIndicator();
> +    UserGestureIndicator::forceResetTopmostIndicator(0);

If we're going to use this approach, we should have a RAII object to control it.
Comment 6 Keishi Hattori 2013-04-03 00:14:33 PDT
Created attachment 196291 [details]
Patch
Comment 7 Keishi Hattori 2013-04-03 00:20:23 PDT
(In reply to comment #4)
> (From update of attachment 196140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196140&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        No new tests. Impossible to tell if a file dialog is open in layout test.
> 
> you could use the eventSender to click somewhere and try to open a new window from the event handler (with testRunner.setCanOpenWindows(), testRunner.setPopupBlockingEnabled(true);). This should return a non-NULL if the user gesture was correctly updated.

I tried to write a test but eventSender.mouseDown/eventSender.keyDown don't seem to run inside a showModalDialog window.
Comment 8 WebKit Review Bot 2013-04-03 00:33:34 PDT
Attachment 196291 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/UserGestureIndicator.cpp', u'Source/WebCore/dom/UserGestureIndicator.h', u'Source/WebCore/page/DOMWindow.cpp']" exit_code: 1
Source/WebCore/dom/UserGestureIndicator.cpp:136:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Keishi Hattori 2013-04-03 00:36:31 PDT
Created attachment 196293 [details]
Patch
Comment 10 jochen 2013-04-03 01:17:18 PDT
Comment on attachment 196291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196291&action=review

> Source/WebCore/dom/UserGestureIndicator.h:50
> +class UserGestureIndicatorDisabler {

Please make this class non-copyable

> Source/WebCore/dom/UserGestureIndicator.h:56
> +    UserGestureIndicator* m_savedIndicator;

We should also store the s_state
Comment 11 jochen 2013-04-03 02:03:18 PDT
(In reply to comment #7)
> (In reply to comment #4)
> > (From update of attachment 196140 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=196140&action=review
> > 
> > > Source/WebCore/ChangeLog:13
> > > +        No new tests. Impossible to tell if a file dialog is open in layout test.
> > 
> > you could use the eventSender to click somewhere and try to open a new window from the event handler (with testRunner.setCanOpenWindows(), testRunner.setPopupBlockingEnabled(true);). This should return a non-NULL if the user gesture was correctly updated.
> 
> I tried to write a test but eventSender.mouseDown/eventSender.keyDown don't seem to run inside a showModalDialog window.

ah yes, the eventSender always sends the events to the main window :-/
Comment 12 Keishi Hattori 2013-04-03 05:42:15 PDT
Created attachment 196335 [details]
Patch
Comment 13 jochen 2013-04-03 07:00:53 PDT
Comment on attachment 196335 [details]
Patch

r=me
Comment 14 WebKit Review Bot 2013-04-03 08:02:54 PDT
Comment on attachment 196335 [details]
Patch

Clearing flags on attachment: 196335

Committed r147554: <http://trac.webkit.org/changeset/147554>
Comment 15 WebKit Review Bot 2013-04-03 08:02:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Alexey L. Lakutin 2013-04-03 21:59:44 PDT
Thanks, all!
Are there any workarounds for this issue?
This feature is very important for our customers.