933040 - Warn for showModalDialog uses

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

showModalDialog is a terrible API, both to implement and to use. And it's relatively little used on the web.

We should try to remove the last remaining usage by warning in the developer console whenever it is used. Chrome just did the same so we're not going alone.

Comment 1

[Johnny Stenback (:jst)]

This should be trivial, we just need to call ReportUseOfDeprecatedMethod() in nsGlobalWindow::ShowModalDialog() here, and set the message up for localization etc.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We also have WarnOnceAbout() which appear to have the same purpose as ReportUseOfDeprecatedMethod(). I filed bug 933563 on consolidating them :)

Comment 3

[Ritesh]

I would like to work on it.

Comment 4

[Vendelin Ruzicka [:vendo]]

Attachment: bug933040

patch

Comment 5

[Vendelin Ruzicka [:vendo]]

ups I am sorry Ritesh. I have done it meanwhile.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 826031 [details] [diff] [review]
bug933040

Stealing review, and thanks vendo! r=jst

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 826031 [details] [diff] [review]
bug933040

>+# LOCALIZATION NOTE: Do not translate "Window.showModalDialog()"
>+ShowModalDialogWarning=Use of Window.showModalDialog() is deprecated. There is no short answer what to use instead, basically all JavaScript frameworks provide an alternative. For more help https://developer.mozilla.org/en-US/docs/Web/API/Window.showModalDialog

"Window" should be lower cased (except inside the URL).

Comment 8

[Vendelin Ruzicka [:vendo]]

Attachment: bug933040

lower case applied

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 826031 [details] [diff] [review]
bug933040

I'd say simply refer to window.open instead. showModalDialog() doesn't provide much value over what window.open does, other than that it's synchronous, and the synchronous part is not something that any frameworks can help with.

Comment 10

[Vendelin Ruzicka [:vendo]]

Attachment: bug933040

ok, patch updated

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW, I also filed bug 933042 on making WarnOnceAbout collect telemetry so that we know if/when it's safe to remove the implementation.

Comment 12

[mjh563]

I guess this patch is ready to be checked in then.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e495c0d3c240

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e495c0d3c240

Comment 15

[Kohei Yoshino]

https://developer.mozilla.org/en-US/docs/Web/API/Window.showModalDialog
https://developer.mozilla.org/en-US/Firefox/Releases/28/Site_Compatibility