Edit should use core-provided Dialog (instead of its own)

Because it was not (and AFAIK still is not) ready. Is that no longer correct?

See http://drupal.org/node/1852224 and http://drupal.org/node/1853388

#3: thanks, but I was aware of those :) I guess I have the impression that it's not yet ready because nothing in core is using it yet. Or is that just because we have only rarely the need to use a dialog?

No idea, we sometimes commit stuff and do nothing with it .. :)

.

Now that the dialog API has matured and #1678002: Edit should provide a usable entity-level toolbar for saving fields has landed, we should start working on this!

So it is unfortunately impossible to create a consistent experience right now: #2071801: Client-side generated dialogs provide an inconsistent experience.

But, that doesn't mean we can't move forward here. We can at least get rid of Edit's custom modal, because at least we'll be using the same dialog system.

Another related, but out-of-scope bug is #2067285: The entity in-place editing toolbar appears above the Drupal dialog.

Before

After

30 insertions, 163 deletions!

We're tracking issues to address the z-indexing issues of the Drupal core dialog. Those bugs should not be addressed in this issue which is just to get Edit using the sanctioned dialog in core, problems and all.

#2067285: The entity in-place editing toolbar appears above the Drupal dialog
#2072037: Drupal dialog modal background z-index is set too low to reliably occlude core UI components

One would think that the close X button in the upper right of the dialog would also invoke the close callbacks on the dialog, but this is not the case, so Wim had to hide the close button in order to prevent a user from exiting the modal without making a decision about whether to keep to throw away the changes. Hooking into the dialog close flow is proving to be really messy, so I agree with the decision to just hide the button. Oddly, the jQuery UI API docs explain how to do this as well.

Hiding the close button

In some cases, you may want to hide the close button, for instance, if you have a close button in the button pane. The best way to accomplish this is via CSS. As an example, you can define a simple rule, such as:

.no-close .ui-dialog-titlebar-close {
  display: none;
}

Then, you can simply add the no-close class to any dialog in order to hide its close button.
http://api.jqueryui.com/dialog/#option-closeText

I jsHinted the code changes and found three missing semi-colons. This updated patch reflects just those additional semi-colons and no other changes. Other than that, this is straightforward port to a standard UI pattern in core. I've manually tested the change and the dialog is working as expected.

As soon as the bot comes back green, we can RTBC.

Bot came back green and I made no significant changes with the patch in #12 to the patch from #9, so I'm setting this to RTBC.

I have no clue whatever the heck is going on this issue :-), but I can certainly approve the latest interdiff.
RTBC++

Just removed the use of the global event. It was added to the API to alter things that all dialog would share, since here which dialog is closing is checked and the same thing can be done without the listener, I just moved a few things around.

The official HTML5 API doesn't provide those global events so we shouldn't rely on them to build dialog-specific functionality.

rerolled. As far as I can see, everything still works. bear with me until I find how to interdiff on windows…

here it is

now in the right way…

#12: hah! :) Thanks :)

#15: If it must be done this way, then the documentation in both the code (there is none) and the change notice is majestically bad. The change notice lists 4 events (and doesn't even say they occur on the window), and never indicates these MUST NOT be used unless for altering *all* dialogs.
Also: if the events are only supposed to serve "global alterations", then why do we even bother to pass a return value to the dialog.close() method and why do we even have dialog.returnValue? Global code can't/shouldn't look at dialog.returnValue, and most code will look like the one in this patch, having no need whatsoever for dialog.returnValue.

The patch in #15 also no longer deletes the image file *nor* the ModalView.js file.

Reroll attached that slightly simplifies/clarifies things, and includes the deletion of the two above files again.

returnValue is from he HTML5 spec. It's not that they must not, it's more like it isn't necessary in that case.

Sorry about the bad reroll, that's what I get for working on windows :p

#18++

Als per #13, #14, #15 and #19, back to RTBC!

8 files changed, 50 insertions, 182 deletions.

Well, that's a lovely diffstat!

The main user-facing change seems to be a darker background on the modal.

Before:

After:

Not having to maintain two copies of dialog code seems like a pretty major thing to me.

Committed to 8.x. Can't push atm because of https://drupal.org/node/2075775 but hopefully will be able to tomorrow.

Ahem.

.

d.o tag monster