Bug 153436 – First parameter to window.showModalDialog() should be mandatory

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 269780 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/24331306>

[Radar WebKit Bug Importer]

<rdar://problem/24331317>

[youenn fablet]

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.

[Chris Dumez]

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.

[Chris Dumez]

Comment on attachment 269780 [details]
Patch

Clearing flags on attachment: 269780

Committed r195594: <http://trac.webkit.org/changeset/195594>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.