Bug 113777 – Actions that require user gesture don't work in window.showModalDialog in Chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Keishi Hattori]

It's because showModalDialog isn't modal in chromium.

Can you give an example?

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.

[Keishi Hattori]

Created attachment 196140 [details]
Patch

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.

[Adam Barth]

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.

[Keishi Hattori]

Created attachment 196291 [details]
Patch

[Keishi Hattori]

(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.

[WebKit Review Bot]

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.

[Keishi Hattori]

Created attachment 196293 [details]
Patch

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

(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 :-/

[Keishi Hattori]

Created attachment 196335 [details]
Patch

Comment on attachment 196335 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 196335 [details]
Patch

Clearing flags on attachment: 196335

Committed r147554: <http://trac.webkit.org/changeset/147554>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey L. Lakutin]

Thanks, all!
Are there any workarounds for this issue?
This feature is very important for our customers.