Bug 55110 – fast/forms/input-file-not-open-without-gesture.html passes when a file picker dialog is opened without a gesture.

• Status: ASSIGNED
• Component: Tools / Tests

[Andy Estes]

I was thinking about how to make this test more obvious, and I think the right thing to do is to have DumpRenderTree implement the UI delegates for the open panel (webView:runOpenPanelForFileButtonWithResultListener:allowMultipleFiles: and webView:runOpenPanelForFileButtonWithResultListener on the Mac) and have them log a message. Layout test expectations could then be based around the presence/absence of this log message.

That technique could be useful for this test as well as for the tests that will be necessary for the two dependent bugs.

[Andy Estes]

Maybe this is only a problem for some ports (it is on Apple's Mac port). I didn't check if Chromium, for instance, implements these UI delegates.

[Johnny(Jianning) Ding]

(In reply to comment #1)
> Maybe this is only a problem for some ports (it is on Apple's Mac port). I didn't check if Chromium, for instance, implements these UI delegates.

It's good idea, I like it. Chromium's implementation is similar, I have implemented delegate for WebViewImpl::runFileChooser to log a message in Chromium DRT.

Due to the super slow network speed today, I haven't finished my webkit client sync, but will upload the patch tomorrow.

Thanks,

[Johnny(Jianning) Ding]

Created attachment 83835 [details]
patch v1

Log the status of opening file chooser dialog in most of ports except win. It's because WebKit doesn't use UIDelegate to open the file dialog in win port.

[WebKit Review Bot]

Attachment 83835 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Tools/DumpRenderTree/wx/DumpRenderTreeWx.cpp:86:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/wx/WebView.h:554:  The parameter name "win" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Collabora GTK+ EWS bot]

Attachment 83835 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8044356

[Johnny(Jianning) Ding]

Created attachment 83923 [details]
patch v1 w/ fixing gtk compilation error

[WebKit Review Bot]

Attachment 83923 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/wx/WebView.h:534:  The parameter name "win" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/wx/WebView.h:534:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/wx/WebView.h:544:  The parameter name "win" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/wx/WebView.h:544:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/wx/WebView.h:554:  The parameter name "win" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/wx/WebView.h:554:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Johnny(Jianning) Ding]

(In reply to comment #6)
> Created an attachment (id=83923) [details]
> patch v1 w/ fixing gtk compilation error

@Andy, any comments for the patch?

[Andy Estes]

(In reply to comment #8)
> (In reply to comment #6)
> > Created an attachment (id=83923) [details] [details]
> > patch v1 w/ fixing gtk compilation error
> 
> @Andy, any comments for the patch?

Sure, I'll take a look in just a bit. Keep in mind that I'm not a reviewer, but I'll be happy to give you some feedback :)

[Andy Estes]

Comment on attachment 83923 [details]
patch v1 w/ fixing gtk compilation error

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

I don't know enough about non-Apple ports to comment effectively, so I'll just assume you did the right thing :) It does look like you missed Apple's Windows port, though.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:560
> +        printf("Opened a file chooser dialog\n");

Perhaps this should print "OPEN PANEL" for consistency. This matches the messages for other dialogs (e.g. "ALERT", "CONFIRM",  "PROMPT", etc.). Same comment for all the other instances of this.

[Andy Estes]

Comment on attachment 83923 [details]
patch v1 w/ fixing gtk compilation error

I'm going to r- due to the lack of an Apple/Windows implementation.

[Johnny(Jianning) Ding]

(In reply to comment #11)
> (From update of attachment 83923 [details])
> I'm going to r- due to the lack of an Apple/Windows implementation.

Hi Andy,
Thanks for your feedback. Missing win part is because WebKit doesn't use UIDelegate to open the file dialog in win port.
If I am going to change the webkit win port implementation to use UIDelegate to open the file dialog in win port like what mac port does. Do you think it makes sense?

[Andy Estes]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 83923 [details] [details])
> > I'm going to r- due to the lack of an Apple/Windows implementation.
> 
> Hi Andy,
> Thanks for your feedback. Missing win part is because WebKit doesn't use UIDelegate to open the file dialog in win port.
> If I am going to change the webkit win port implementation to use UIDelegate to open the file dialog in win port like what mac port does. Do you think it makes sense?

Hi Johnny,

The Windows DRT does implementation has a UIDelegate with the following method (in Tools/DumpRenderTree/win/UIDelegate.h):

    virtual HRESULT STDMETHODCALLTYPE runOpenPanelForFileButtonWithResultListener( 
        /* [in] */ IWebView *sender,
        /* [in] */ IWebOpenPanelResultListener *resultListener) { return E_NOTIMPL; }

You should just be able to implement this method, no?

[Andy Estes]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 83923 [details] [details])
> > I'm going to r- due to the lack of an Apple/Windows implementation.
> 
> Hi Andy,
> Thanks for your feedback. Missing win part is because WebKit doesn't use UIDelegate to open the file dialog in win port.
> If I am going to change the webkit win port implementation to use UIDelegate to open the file dialog in win port like what mac port does. Do you think it makes sense?

Ohh, I misunderstood you. Yea, like you said, WebKit displays the file chooser directly on Windows instead of using UIDelegate.

It'd probably be best to file a separate bug about this. The popup tests can then be skipped on Windows pending the resolution of that bug.

[Johnny(Jianning) Ding]

> It'd probably be best to file a separate bug about this. The popup tests can then be skipped on Windows pending the resolution of that bug.

Good, will file the bug later.
So if I change to print "OPEN PANEL" based on my last patch, are you satisfied with it? I can ask tc/adam as reviewer to get approval.

[Andy Estes]

(In reply to comment #15)
> > It'd probably be best to file a separate bug about this. The popup tests can then be skipped on Windows pending the resolution of that bug.
> 
> Good, will file the bug later.
> So if I change to print "OPEN PANEL" based on my last patch, are you satisfied with it? I can ask tc/adam as reviewer to get approval.

I am! Thanks Johnny.

[Andy Estes]

Just keep in mind that any updated tests based on this patch will fail on Windows, so that new bug and associated Skipped list changes will be important to keep the bots green.

[Johnny(Jianning) Ding]

(In reply to comment #17)
> Just keep in mind that any updated tests based on this patch will fail on Windows, so that new bug and associated Skipped list changes will be important to keep the bots green.

I see, will do add the test into skip test to make bot happy:)

[Johnny(Jianning) Ding]

Created attachment 85799 [details]
patch v2

Sorry for delay.
I have changed the log content to "OPEN PANEL" according to andy's comments, already compiled and tested it on mac, chromium-mac/window, linux-gtk platform.

Adam and Andy, please take a look at the new patch.

Thanks,

[WebKit Review Bot]

Attachment 85799 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/wx/WebView.h:553:  The parameter name "win" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 85799 [details]
patch v2

I don't see an implementation for Apple's Windows port (in DumpRenderTree/win). Was that intentional?

Presumably this change affects test results, but I don't see any changes to LayoutTests.

[Martin Robinson]

Comment on attachment 85799 [details]
patch v2

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

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:620
> +    if (DumpRenderTreeSupportGtk::dumpRenderTreeModeEnabled()) {
> +        gboolean retval;
> +        WebKitWebFrame* webFrame = kit(frame);
> +        WebKitWebView* webView = getViewFromFrame(webFrame);
> +        g_signal_emit_by_name(webView, "run-file-chooser", &retval);
> +        return;
> +    }

See below.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2857
> +    /**
> +     * WebKitWebView::run-file-chooser:
> +     * @webView: the object on which the signal is emitted
> +     *
> +     * This signal is emitted when we need to open a file chooser dialog,
> +     * so DRT can have chance to log whether a file chooser dialog is opened or not.
> +     * So far this signal is only used for DRT mode.
> +     */
> +    webkit_web_view_signals[RUN_FILE_CHOOSER] = g_signal_new("run-file-chooser",
> +            G_TYPE_FROM_CLASS(webViewClass),
> +            (GSignalFlags)G_SIGNAL_RUN_LAST,
> +            0,
> +            g_signal_accumulator_true_handled,
> +            NULL,
> +            webkit_marshal_BOOLEAN__VOID,
> +            G_TYPE_BOOLEAN, 0);
> +

Instead of adding the signal and risking introducing some ABI break in a future release (even for private API), maybe it's better for now to just take the approach you took for Qt with GTK+ as well.

[Johnny(Jianning) Ding]

(In reply to comment #21)
> (From update of attachment 85799 [details])
> I don't see an implementation for Apple's Windows port (in DumpRenderTree/win). Was that intentional?

Yes, it is because WebKit displays the file chooser directly on Windows instead of using UIDelegate on mac.

Andy suggested filing a separate bug for using UIDelegate to open file chooser dialog on Windows port and temporarily skip the test on Windows port.

I just filed bug 56383 for using UIDelegate to open file chooser dialog on Windows port 

> Presumably this change affects test results, but I don't see any changes to LayoutTests.

The change to LayoutTests is in bug 55102.

[Johnny(Jianning) Ding]

> Instead of adding the signal and risking introducing some ABI break in a future release (even for private API), maybe it's better for now to just take the approach you took for Qt with GTK+ as well.

Thanks for your review,  Martin.

I was thinking the way to directly use printf to dump the log in ChromeClientGtk.cpp like what QT port does. But I didn't find similar usage, so I gave up that way.

If you reviewers are comfortable with this simple way, I am happy to do that:-)
Do I have the green light?

[Martin Robinson]

(In reply to comment #24)
> > Instead of adding the signal and risking introducing some ABI break in a future release (even for private API), maybe it's better for now to just take the approach you took for Qt with GTK+ as well.

> If you reviewers are comfortable with this simple way, I am happy to do that:-)
> Do I have the green light?

Sure, this seems fine for now. Eventually we'll want to implement this signal, but probably after we cut the branch for the next stable series.

[Johnny(Jianning) Ding]

Created attachment 85900 [details]
patch V3

Directly print out the open panel status when calling runOpenPanel in DRT mode according to Martin's comment.

[WebKit Review Bot]

Attachment 85900 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Tools/DumpRenderTree/wx/DumpRenderTreeWx.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 85900 [details]
patch V3

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

Looks like you left out Apple's Windows port. It's also weird to add this new capability but not any new tests that use it.

> Tools/DumpRenderTree/mac/UIDelegate.mm:267
> +- (void)webView:(WebView *)sender runOpenPanelForFileButtonWithResultListener:(id<WebOpenPanelResultListener>)resultListener allowMultipleFiles:(BOOL)allowMultipleFiles WEBKIT_OBJC_METHOD_ANNOTATION(AVAILABLE_IN_WEBKIT_VERSION_4_0)

No need for the annotation here.

[Andy Estes]

(In reply to comment #28)
> (From update of attachment 85900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85900&action=review
> 
> Looks like you left out Apple's Windows port.

There is commentary in this bug about why this is (WebKit on Windows doesn't consult UIDelegate for runOpenPanel() so DRT isn't able to override the default behavior). <https://bugs.webkit.org/show_bug.cgi?id=56383> tracks this issue.

[Johnny(Jianning) Ding]

> There is commentary in this bug about why this is (WebKit on Windows doesn't consult UIDelegate for runOpenPanel() so DRT isn't able to override the default behavior). <https://bugs.webkit.org/show_bug.cgi?id=56383> tracks this issue.

Andy, although the patch for bug 55102 may not be necessary, but it's still good to use the patch to check open panel status since it is more obvious.
I like to change test input-file-not-open-without-gesture.html to use the new way and add it in this patch. Does it make sense?

[Andy Estes]

(In reply to comment #30)
> > There is commentary in this bug about why this is (WebKit on Windows doesn't consult UIDelegate for runOpenPanel() so DRT isn't able to override the default behavior). <https://bugs.webkit.org/show_bug.cgi?id=56383> tracks this issue.
> 
> Andy, although the patch for bug 55102 may not be necessary, but it's still good to use the patch to check open panel status since it is more obvious.
> I like to change test input-file-not-open-without-gesture.html to use the new way and add it in this patch. Does it make sense?

Sounds good!

[Johnny(Jianning) Ding]

Created attachment 92219 [details]
patch v4

change LayoutTests/fast/forms/input-file-not-open-without-gesture.html to use new way.

[Eric Seidel (no email)]

Attachment 92219 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Tools/DumpRenderTree/wx/DumpRenderTreeWx.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Johnny(Jianning) Ding]

Created attachment 93429 [details]
patch v4 (fix style complain)

[Andy Estes]

Comment on attachment 93429 [details]
patch v4 (fix style complain)

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

This is looking good. I'm not a reviewer so I can't r+, but I do have a few comments.

> LayoutTests/fast/forms/input-file-not-open-without-gesture.html:28
> +    setTimeout("layoutTestController.notifyDone()", 100);

Are you sure a delay is necessary here? I think both the EventSender events and the synthetic click event will fire synchronously, which means the 'OPEN PANEL' message will be logged by the time openFileDialog() returns. You should be able to call layoutTestController.notifyDone() directly. If for some reason notifyDone() needs to be called on a timer, 100ms seems far too long.

> LayoutTests/fast/forms/input-file-not-open-without-gesture.html:34
> +<input type="file" name="file" id="f" onclick="this.click()">

Why is the onclick attribute necessary here?

> LayoutTests/platform/win/Skipped:1237
> +# Since WebKit displays the file chooser directly on Windows instead of using UIDelegate, we need to skip the following test on window port.

Nit: this should read "on Apple's Windows port."

> LayoutTests/platform/win/Skipped:1238
> +# See https://bugs.webkit.org/show_bug.cgi?id=55110 for more details.

I'd link to the bug that actually tracks fixing this on Windows, which is https://bugs.webkit.org/show_bug.cgi?id=56383

[Johnny(Jianning) Ding]

Created attachment 94948 [details]
patch v5

(In reply to comment #35)
Thanks for comments, all done. 

Now I moved the startTest out of onload handler to make Event::fromUserGesture grant that the mouseDown/up is a user gesture.

@Adam, would you like to review this patch?

[Adam Roben (:aroben)]

Comment on attachment 94948 [details]
patch v5

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

This patch as currently written will cause this test to fail in WebKit2. You'll need to implement the new "OPEN PANEL" dumping in WebKitTestRunner.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:607
> +        fprintf(stdout, "OPEN PANEL\n");

Something more descriptive might be nice. Even just "RUN OPEN PANEL" would be better.

> Source/WebKit/wx/WebKitSupport/DumpRenderTreeSupportWx.cpp:34
> +bool DumpRenderTreeSupportWx::s_drtRun = false;

This can just be a file-level static. It doesn't need to be a member variable.E

> Source/WebKit/wx/WebKitSupport/DumpRenderTreeSupportWx.h:36
> +class DumpRenderTreeSupportWx {
> +
> +public:

Extra newline here.

> Source/WebKit/wx/WebKitSupport/DumpRenderTreeSupportWx.h:38
> +    DumpRenderTreeSupportWx();
> +    ~DumpRenderTreeSupportWx();

No need to declare/define these. The compiler will do it for you.

> LayoutTests/fast/forms/input-file-not-open-without-gesture.html:20
> +function openFileDialog(withUserGesture) {
> +    window.console.log("Open the file dialog " + (withUserGesture ? "with" : "without") + " a user gesture.");
> +    if (withUserGesture) {
> +        eventSender.mouseMoveTo(10, 10);
>          eventSender.mouseDown();
>          eventSender.mouseUp();
> -        clickFired = true;
> -        testStage = OPEN_FILE_WITHOUT_GESTURE;
> -        // Waits for opening the file dialog and checks whether the click event can still be received by document.
> -        setTimeout("checkResult(function() { layoutTestController.notifyDone(); })", 500);
> -    }, 1);
> +    } else {
> +        document.getElementById('f').click();
> +    }
>  }

Having two separate functions might be clearer.

> LayoutTests/fast/forms/input-file-not-open-without-gesture.html:31
> +<body style="margin:0px; padding:0px" onload="setTimeout('startTest();', 0)">

Why is this delay needed?

> LayoutTests/platform/win/Skipped:1245
> +# Since WebKit displays the file chooser directly on Apple's Windows port instead of using UIDelegate, we need to skip the following test on Apple's Windows port.
> +# See  https://bugs.webkit.org/show_bug.cgi?id=56383 for more details.
> +fast/forms/input-file-not-open-without-gesture.html

It would be a little better to check in expected failure results for Windows rather than skipping the test. That way we'll notice if the test's behavior changes in any other way unexpectedly.

[Johnny(Jianning) Ding]

(In reply to comment #37)

> > LayoutTests/fast/forms/input-file-not-open-without-gesture.html:31
> > +<body style="margin:0px; padding:0px" onload="setTimeout('startTest();', 0)">
> 
> Why is this delay needed?

If we call StartTest inside the period of handling load event, since the event of window is "load", Event::fromUserGesture returns false, which causes ScriptController::processingUserGesture returns false,
then the OpenPanel will not be opened inside RenderFileUploadControl::click.

Usually there is not key/mouse input passed to webki inside the period of handling load event in normal usage, so I just simply use a timeout to trigger the test.

[Adam Roben (:aroben)]

(In reply to comment #38)
> (In reply to comment #37)
> 
> > > LayoutTests/fast/forms/input-file-not-open-without-gesture.html:31
> > > +<body style="margin:0px; padding:0px" onload="setTimeout('startTest();', 0)">
> > 
> > Why is this delay needed?
> 
> If we call StartTest inside the period of handling load event, since the event of window is "load", Event::fromUserGesture returns false, which causes ScriptController::processingUserGesture returns false,
> then the OpenPanel will not be opened inside RenderFileUploadControl::click.
> 
> Usually there is not key/mouse input passed to webki inside the period of handling load event in normal usage, so I just simply use a timeout to trigger the test.

Thanks for the explanation.