Bug 151885 – Make showModalDialog a runtime enabled (on-by-default) feature

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Platform

[Domenic Denicola]

showModalDialog has been deprecated in the HTML spec and we would like to remove it: https://github.com/whatwg/html/pull/374 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=26437.

The only modern user agents that support it are desktop Safari and desktop Firefox. (It has been completely removed from Chrome and Edge, and mobile Safari and mobile Firefox both do not support it.)

The corresponding Gecko bug is https://bugzilla.mozilla.org/show_bug.cgi?id=981796.

[Brent Fulgham]

<rdar://problem/16747181>

[Brent Fulgham]

Created attachment 270140 [details]
Patch

[Chris Dumez]

Whaaaat?

[Ryosuke Niwa]

Comment on attachment 270140 [details]
Patch

I don't think we should remove this feature until dialog element is supported.  There are a lot of enterprise websites that rely on this feature.

[Brent Fulgham]

Dialog element work is documented under Bug 84635, but nobody appears to be working on it at present.

[Brent Fulgham]

(In reply to comment #4)
> Comment on attachment 270140 [details]
> Patch
> 
> I don't think we should remove this feature until dialog element is
> supported.  There are a lot of enterprise websites that rely on this feature.

Really? Chrome's usage data says less than 0.006% of page visits use this feature.

Can you list any sites that you think would be affected?

[Darin Adler]

In the past there were many web apps in corporate environments, not ones out on the "open internet", that depended on this, web apps that were designed with IE in mind. For example, at one time SAP products used inside Apple relied on it. In 2003, we did not have support for showModalDialog and multiple enterprise software developers including SAP asked us to add support for it, and that’s why I originally implemented the feature in 2005. People inside Apple can see the some of that background in <rdar://problem/3166090>.

I have never seen much use of the feature on "normal internet websites".

I have no idea how much enterprise web apps may have changed in the intervening 10 years since we added this. From a "what web browsers are used on these intranet web apps" point of view, it's not all that interesting that Chrome has removed it, because many of those enterprises may require the use of IE on Windows and Safari on Mac and would not support use of Chrome, but it is interesting that Edge has removed it. Those corporations are likely able to use IE rather than Edge for a while, but longer term they will have to change their products.

[Alex Christensen]

Firefox has removed showModalDialog.  I think we should, too.
https://www.fxsitecompat.com/en-CA/docs/2016/window-showmodaldialog-has-been-removed/

[Darin Adler]

Back when I implemented showModalDialog, the SAP apps using it included apps from SAP that did *not* support Firefox.

[Brent Fulgham]

Created attachment 295226 [details]
Patch

[Chris Dumez]

Comment on attachment 295226 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Make showModalDialog an option (off-by-default) feature

I still do not think we should disable showModalDialog() until we support the HTML dialog element. Yes, other browsers have dropped this API but they support the dialog element and suggest it as an alternative. There are also polyfills based on the dialog element.

[Brent Fulgham]

Created attachment 295523 [details]
Patch

[Brent Fulgham]

(In reply to comment #11)
> Comment on attachment 295226 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295226&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Make showModalDialog an option (off-by-default) feature
> 
> I still do not think we should disable showModalDialog() until we support
> the HTML dialog element. Yes, other browsers have dropped this API but they
> support the dialog element and suggest it as an alternative. There are also
> polyfills based on the dialog element.

Would you support the current change if the default state was "on"? Then we could at least make the decision to toggle it off more easily.

This bug would then become" make 'showModalDialog' a runtime-enabled feature'.

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #11)
> > Comment on attachment 295226 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=295226&action=review
> > 
> > > Source/WebCore/ChangeLog:3
> > > +        Make showModalDialog an option (off-by-default) feature
> > 
> > I still do not think we should disable showModalDialog() until we support
> > the HTML dialog element. Yes, other browsers have dropped this API but they
> > support the dialog element and suggest it as an alternative. There are also
> > polyfills based on the dialog element.
> 
> Would you support the current change if the default state was "on"? Then we
> could at least make the decision to toggle it off more easily.
> 
> This bug would then become" make 'showModalDialog' a runtime-enabled
> feature'.

Definitely. And if other people feel strongly about disabling showModalDialog despite the fact we still do not support the HTML dialog element then I obviously won't stop you. I personally do not think this would be a good situation to be in though. It is always good to have a have an alternative we can suggest to Web authors when we drop a web-facing feature.

[Brent Fulgham]

Created attachment 295526 [details]
Revised to leave 'showModalDialog' on by default.

Isn’t the feature already runtime-enabled and effectively off by default? I think that unless the client chooses to implement -webView:createWebViewModalDialogWithRequest: and -webViewRunModal:, the feature is off.

Comment on attachment 295526 [details]
Revised to leave 'showModalDialog' on by default.

r- because I believe this is redundant. Feel free to change back to r? if I am mistaken.

[Chris Dumez]

(In reply to comment #17)
> Comment on attachment 295526 [details]
> Revised to leave 'showModalDialog' on by default.
> 
> r- because I believe this is redundant. Feel free to change back to r? if I
> am mistaken.

Mitz is right that there is some overlap. It looks like we call a client would need to implement WKPageUIClient.runModal for showModalDialog() to do anything. However, one issue is that even when showModalDialog() does nothing, it is still exposed to the Web via window.showModalDialog.

I think that ideally, window.showModalDialog would be exposed only when the client actually implements runModal (i.e. ChromeClient::canRunModal() returns true).

[Brent Fulgham]

(In reply to comment #18)
> (In reply to comment #17)
> > Comment on attachment 295526 [details]
> > Revised to leave 'showModalDialog' on by default.
> > 
> > r- because I believe this is redundant. Feel free to change back to r? if I
> > am mistaken.
> 
> Mitz is right that there is some overlap. It looks like we call a client
> would need to implement WKPageUIClient.runModal for showModalDialog() to do
> anything. However, one issue is that even when showModalDialog() does
> nothing, it is still exposed to the Web via window.showModalDialog.
> 
> I think that ideally, window.showModalDialog would be exposed only when the
> client actually implements runModal (i.e. ChromeClient::canRunModal()
> returns true).

Right. Currently, 'showModalDialog' will appear as a valid JavaScript property, even if the User Agent does not implement the delegate methods. The existence of 'showModalDialog' might be enough to prevent sites from using polyfills, which is one of the things we wanted to test by adding this flag.

In fact, since it is effectively off by default, perhaps my original plan of having the feature flag off by default is actually correct, and I should revert to the "off-by-default" version of my patch?

[Darin Adler]

I believe Dan’s point is that this feature is already off by default; it does not use the feature flags system to turn it on, but rather client function, which turns into a delegate call on Cocoa platforms.

I suggest we investigate making the existing mechanism work more thoroughly so the JavaScript property is not even present when the feature is off. Making that refinement would be preferable to adding a new runtime switch.

[Tim Nguyen (:ntim)]

Created attachment 452745 [details]
WIP

[Tim Nguyen (:ntim)]

*** This bug has been marked as a duplicate of bug 243099 ***