Bug 153436

Summary: First parameter to window.showModalDialog() should be mandatory
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar, WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2016-01-25 12:29:38 PST 
First parameter to window.showModalDialog() should be mandatory.

This matches the behavior of Firefox.

Also, having it optional causes the W3C HTML test suite to hang because it mistakenly pops up a modal dialog during the test.
Comment 1 Chris Dumez 2016-01-25 12:52:10 PST 
Created attachment 269780 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2016-01-25 12:53:00 PST 
<rdar://problem/24331306>
Comment 3 Radar WebKit Bug Importer 2016-01-25 12:53:26 PST 
<rdar://problem/24331317>
Comment 4 youenn fablet 2016-01-25 23:42:10 PST 
Comment on attachment 269780 [details]
Patch

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

> LayoutTests/imported/w3c/ChangeLog:10
> +        the workaround but this is fixed now.

Great to see that fixed!

> LayoutTests/imported/w3c/web-platform-tests/html/dom/interfaces.html:2299
> +  any showModalDialog(DOMString url, optional any argument);

This method is taking two parameters, one being optional.
But JSDOMWindow::showModalDialog is accessing argument 0 and argument 2, argument 1 being not accessed in the custom code.
This seems inconsistent.
Comment 5 Chris Dumez 2016-01-26 09:23:47 PST 
Comment on attachment 269780 [details]
Patch

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

>> LayoutTests/imported/w3c/web-platform-tests/html/dom/interfaces.html:2299
>> +  any showModalDialog(DOMString url, optional any argument);
> 
> This method is taking two parameters, one being optional.
> But JSDOMWindow::showModalDialog is accessing argument 0 and argument 2, argument 1 being not accessed in the custom code.
> This seems inconsistent.

WebKit does not do anything with this second argument (just ignores it). However, we support a third optional argument for window options, like Firefox:
https://developer.mozilla.org/en-US/docs/Web/API/Window/showModalDialog

At least we are consistent with Firefox.
Comment 6 Chris Dumez 2016-01-26 10:04:33 PST 
Comment on attachment 269780 [details]
Patch

Clearing flags on attachment: 269780

Committed r195594: <http://trac.webkit.org/changeset/195594>
Comment 7 Chris Dumez 2016-01-26 10:04:39 PST 
All reviewed patches have been landed.  Closing bug.