Bug 151885 - Make showModalDialog a runtime enabled (on-by-default) feature
Summary: Make showModalDialog a runtime enabled (on-by-default) feature
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-12-04 14:35 PST by Domenic Denicola
Modified: 2016-11-29 09:01 PST (History)
22 users (show)

See Also:


Attachments
Patch (93.72 KB, patch)
2016-01-28 13:02 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (15.21 KB, patch)
2016-11-18 17:04 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (15.19 KB, patch)
2016-11-28 13:34 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Revised to leave 'showModalDialog' on by default. (15.21 KB, patch)
2016-11-28 13:48 PST, Brent Fulgham
mitz: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Domenic Denicola 2015-12-04 14:35:23 PST
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.
Comment 1 Brent Fulgham 2016-01-28 10:23:28 PST
<rdar://problem/16747181>
Comment 2 Brent Fulgham 2016-01-28 13:02:07 PST
Created attachment 270140 [details]
Patch
Comment 3 Chris Dumez 2016-01-28 13:11:32 PST
Whaaaat?
Comment 4 Ryosuke Niwa 2016-01-28 13:12:20 PST
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.
Comment 5 Brent Fulgham 2016-01-28 14:16:11 PST
Dialog element work is documented under Bug 84635, but nobody appears to be working on it at present.
Comment 6 Brent Fulgham 2016-01-28 14:54:36 PST
(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?
Comment 7 Darin Adler 2016-01-30 11:06:04 PST
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.
Comment 8 Alex Christensen 2016-08-09 10:43:07 PDT
Firefox has removed showModalDialog.  I think we should, too.
https://www.fxsitecompat.com/en-CA/docs/2016/window-showmodaldialog-has-been-removed/
Comment 9 Darin Adler 2016-08-12 11:50:56 PDT
Back when I implemented showModalDialog, the SAP apps using it included apps from SAP that did *not* support Firefox.
Comment 10 Brent Fulgham 2016-11-18 17:04:35 PST
Created attachment 295226 [details]
Patch
Comment 11 Chris Dumez 2016-11-18 19:16:08 PST
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.
Comment 12 Brent Fulgham 2016-11-28 13:34:13 PST
Created attachment 295523 [details]
Patch
Comment 13 Brent Fulgham 2016-11-28 13:35:22 PST
(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'.
Comment 14 Chris Dumez 2016-11-28 13:39:03 PST
(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.
Comment 15 Brent Fulgham 2016-11-28 13:48:47 PST
Created attachment 295526 [details]
Revised to leave 'showModalDialog' on by default.
Comment 16 mitz 2016-11-28 14:33:24 PST
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 17 mitz 2016-11-28 14:34:43 PST
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.
Comment 18 Chris Dumez 2016-11-28 14:51:34 PST
(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).
Comment 19 Brent Fulgham 2016-11-29 08:56:33 PST
(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?
Comment 20 Darin Adler 2016-11-29 09:01:52 PST
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.