-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modernize and refactor modal dialogs #6330
Conversation
This contains a small bug fix, in that confirm() and prompt() said "return" in some cases instead of "return false" or "return null" as appropriate. Other notable changes, all editorial, are: * Factoring out repeated "cannot show modals" steps, which will likely expand over time (see e.g. #6297). * Separating out and explaining the no-argument overload of alert(). * Passing the document through to the "printing steps", instead of just having them talk about "this Window object".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
@@ -78763,7 +78763,7 @@ interface <dfn interface>Window</dfn> : <span>EventTarget</span> { | |||
readonly attribute boolean <span data-x="dom-originAgentCluster">originAgentCluster</span>; | |||
|
|||
// user prompts | |||
undefined <span data-x="dom-alert">alert</span>(); | |||
undefined <span data-x="dom-alert-noargs">alert</span>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the problem with treating this as a single method in prose? I much prefer that style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you can treat it as a single method, since there are two separate method declarations. As written before this patch, there was no definition for no-arg alert()
in the spec.
data-x="event-beforeprint">beforeprint</code> at the <span>relevant global object</span> of the | ||
<code>Document</code> that is being printed, as well as any <span data-x="child browsing | ||
data-x="event-beforeprint">beforeprint</code> at the <span>relevant global object</span> of | ||
<var>document</var>, as well as any <span data-x="child browsing | ||
context">child browsing contexts</span> in it.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is rather imprecise. Same for the after event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was wondering if you'd notice that... I didn't want to touch that here. I'll file a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we have one already: #5096
This contains a small bug fix, in that confirm() and prompt() said
"return" in some cases instead of "return false" or "return null" as
appropriate.
Other notable changes, all editorial, are:
expand over time (see e.g. Add early return to JS dialogs triggered from different origin-domain iframes #6297).
having them talk about "this Window object".
I will rebase #6297 if we merge this first.
/timers-and-user-prompts.html ( diff )
/window-object.html ( diff )