Problem/Motivation
There are multiple places in drupal core where you cannot use the easy to use #ajax element in forms to do your custom logic, but you
need actual support by custom javascript OR an ajax command. Some examples are a page manager (#1841584: Add and configure master displays)or the views edit interface.
Proposed resolution
Add an openDialogCommand/closeDialogCommand (bikeshed the name) that takes a title, the content and potentially even the dialog settings.
Additional to the PHP level you need on the JS level the actual commands and additional binding for the form element.
Remaining tasks
Postponed on this
- #1851414: Convert Views to use the abstracted dialog modal
- #1909202: Use modals in operations column of language settings config page
- #1842036: [META] Convert all confirm forms to use modal dialog
Related
- #1818142: [meta] Unified Blocks and Layouts (SCOTCH+WSCCI+Spark+Field UI)
- #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases (which is postponed on #1884530: Complete modal dialog tests.)
- #1909144: Allow #ajax['dialog'] to contain options other than 'modal' (a spin of of this to speed up unblocking issues that might be waiting on this issue)
- #1807692-10: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget
Comment | File | Size | Author |
---|---|---|---|
#102 | interdiff.txt | 1.21 KB | quicksketch |
#102 | ajax_dialogs-1870764-102.patch | 38.97 KB | quicksketch |
#94 | ajax-dialog-1870764.92.wip_.interdiff.txt | 7.52 KB | larowlan |
#86 | interdiff.txt | 2.83 KB | quicksketch |
#86 | ajax-dialog-1870764-86.patch | 38.94 KB | quicksketch |
Comments
Comment #1
dawehnerCloned some code of #1841584: Add and configure master displays and #1851414: Convert Views to use the abstracted dialog modal
Comment #2
dawehnerThis still certainly needs more ideas copied from the original code at http://privatepaste.com/e1bbddc718
Comment #3
dawehnerTracking some work.
Comment #4
dawehnerSo this fixes the closing dialog.
Comment #5
gddI definitely think it would be useful to add this. There's a ton of use cases for various dialog settings that are difficult to use now (for instance we could use this on #1821548: Add a "diff" of some kind to the CMI UI)
Comment #6
nod_Holds up a major Views and CMI task .
Comment #7
dawehner@nod_
Did you made progress here? You told something in IRC.
Comment #8
effulgentsia CreditAttribution: effulgentsia commented#1851414: Convert Views to use the abstracted dialog modal is a critical task postponed on this, so raising this to critical.
Comment #9
effulgentsia CreditAttribution: effulgentsia commented#4 uses these names, but hard-codes functionality to just modal dialogs. So I think the names should be openModalCommand/closeModalCommand or the currently named commands should be abstracted to support non-modal dialogs.
I agree about it being useful, but I think a challenge is that dialog.js is currently modeled on http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.htm..., so that it is abstracted from jQuery UI (leaving open the possibility to remove that dependency), and AFAICT, there's no spec there for settings. @nod_: any thoughts on this?
Comment #10
Crell CreditAttribution: Crell commentedI think the HTML5-ish way to handle settings would be data-* attributes on whatever the main element is, no?
Comment #11
sunWe have a mismatch here — a modal is a specific incarnation of a dialog.
If we call the Ajax commands "Dialog", then I expect to be able to use them for dialogs; i.e., potentially multiple on the same page.
The sub-facility of being able to deliver modal functionality should either be an optional parameter/setting for the Ajax commands, or we should introduce two separate Ajax command sets for dialogs and modals.
drupalSettings isn't used here.
Comment #12
YesCT CreditAttribution: YesCT commented@sun gave a review in #11
So, should this be needs work to address those things?
Or, if it's still at needs review because it needs more review, what kind of review is additionally needed, or are there a couple people (@nod_?) in mind folks are hoping will review this specifically?
Comment #13
frega CreditAttribution: frega commentedPlease find a patch attached. Caveat: it is *not* a direct improvement/descendant of #4 (I've conferred w/ @dawehner about this). Instead it tries to bring together the different strands and issues. It is still WIP and more of a prototype as it cobbles together bits and pieces from dawehner's code in comment #4 above, #1667742: Add abstracted dialog to core (resolves accessibility bug) (in particular larowlan's patch in http://drupal.org/node/1667742#comment-6734556) and also some notions from ctools_modal-7.x. If the general direction is ok, I would refactor the code (proper AjaxCommands-objects).
The patch *deliberately* adds a module to modules/dialog_example/ in order to allow for *easy* testing and speccing of this functionality. Please Install the the dialog_example module after applying the patch add visit the dialog_example/ to see the various "use cases".
Comment #14
dawehnerI appreciate the fact that we have commands for both pure dialogs and modals.
You better expose the full functionality or people will start to get inconsistent again.
Just in general: I'm wonderning whether and how we can explain in documentation the difference
between a dialog and a modal.
It seems to be that this could be somehow moved into fapi directly if we can already know whether the request asks for ajax?
Totally not sure whether it makes sense to put such specific functionality deeply into the system.
I guess we can skip that, drupal_build_form will always return a render array.
Should we put that into the AjaxRender class, as it seems to be something fundamental like css/js?
Wouldn't this maybe even fix the problem that some JS errors lead to json responses? This is a huge WIN from my perspective. Also the DX seems to be improved a lot. I'm wondering whether this should be a method on the content_negotation service, but it seems to be to specific.
Do people know that an "ajax" request uses xhr requests internally? Maybe we could improve the name.
nitpick: So it's not a xhr request, let's return FALSE.
In general it seems to be that we should split up the ajax.js file to have all that dialog logic in a separate file.
Do we use Drupal.theme for something like that?
drupall, what could that be ;)
We seem to miss an equivalent for closing a modal
Comment #15
frega CreditAttribution: frega commentedRerolled the patch to address a few of the points above, adding & adjustings tests as well converting the "old" style ajax commands to D8's Command objects/AjaxResponse functionality.
A modal is a singleton blocking the UI whilst (normal) dialogs stack and do not block the UI.
Re: moving ajax_dialog_form_wrapper deeper into FAPI - i feel the same and am not sure what the best way to proceed is. The current approach here is similar to the ctools_modal one which is sort of proven to work.
Fixed.
Good point, but maybe in a follow up?
Fixed.
Leaving the dialog "insertion" logic in ajax.js seemed to be something effulgentsia wanted. I am not sure about it myself. But the form-"wiring" certainly should be moved to a separate function.
CloseModalDialogCommand and CloseDialogCommand are *very* similar so i felt it was overkill but am happy to add it.
Comment #17
frega CreditAttribution: frega commented#15: 1870764-15.patch queued for re-testing.
Comment #18
Wim LeersFirst: the included dialog_example module is a godsend for reviewing this patch — thanks :)
Overall: works well. Except for once, where I had opened the "Complex Form [modal]" case, clicked "Okayish" and it went to the non-AJAX fallback page. But I can't reproduce that, so maybe I'm imagining things.
Initial round of feedback, a bunch of nitpicks, but also a bunch of bigger questions/concerns.
ajax_is_ajax_request()
is a better name because you're checking forajax
, notxhr
?(This is related to what effulgentsia said at #1851414-6: Convert Views to use the abstracted dialog modal.)
How will this be used? Well, you'll send some data to the server (typically a form), and based on the result, it is decided whether the dialog should be closed (e.g. no form validation errors, so the dialog's job is done).
But why send back an extensive AJAX command, when a small JSON response would be sufficient for the JS to figure out whether to close the dialog or not?
I definitely see there are use cases for this (when you don't want to write any custom JS, for example), but it still feels very strange to see this kind of thing.
We can improve those things down the road, right now we need to move forward. I just wanted to note that I share effulgentsia's concerns.
Newline before this. That's the convention.
"any any"
Do we really need to pass settings here? It makes sense for
InsertCommand
and subclasses, because new HTML content is being added, which may very well contain "behavioral HTML".But for a *close* command?
s/an/a/
Broken comment.
bool?
dialog-specific
If
$dialog_settings
is about "JavaScript settings", then it's a subarray of$settings
?If we want to keep
$dialog_settings
, then it should really be data that gets passed as a parameter to the JS function that instantiates a dialog, otherwise we're duplicating/confusing things.Delete newline.
Why have a separate AJAX command? Why not have a
$(is_)modal
parameter forOpenDialogCommand
?Drupal
Set up
Comment #19
dawehnerSo what views does is to store a list of forms it wants the user to fill out. This is used for example if you add multiple fields in a row.
As the php side of views stores the forms which still have to be filled out, it also has to control to close the actual form at some point.
I think an extra ajax command is the best and clean way to handle that.
Comment #20
nod_in
dialog.ajax.js
:Drupal.ajax.prototype.commands.openDialog(ajax, response, status);
should beajax.commands.openDialog(ajax, response, status);
since you could override the command for a specific ajax object, same for insert.And yeah the option merging needs to be looked closer, I got the order wrong :p
Comment #21
dawehnerIf you want to allow altering you should implement getter/setters and then use request subscribers to alter it.
I kept it for better implicit documentation what is going on.
Fixed the problems from the review, and fixed some TODOs.
Comment #22
dawehnerForgot to include some files.
Comment #23
dawehnerdawehner--
Comment #25
frega CreditAttribution: frega commenteddawehner was kind enough to take care of most of the nit-picks.
This patch fixes a small issue in the JS of the patch #23 and (hopefully) allows the tests to run again.
I reworked some of the function signatures to be a cleaner and also added a CloseModalDialogCommand (but no corresponding JS function). I would prefer to actually only have *one* OpenDialogCommand and CloseDialogCommand with "modal" being passed in, but will only do so, if people agree :)
As @dawehner already pointed out in #21 we'll need this approach for complex situations and to be able to add additional AjaxCommands after a submission has taken place (that's what the dialog_example module does by "replacing" the title).
Comment #26
frega CreditAttribution: frega commentedForgot to change status.
Comment #28
frega CreditAttribution: frega commented#25: drupal-1870764-25.patch queued for re-testing.
Comment #29
dawehnerJust some pieces of cleanup, we certainly need feedback from effulgentsia
Comment #31
frega CreditAttribution: frega commented#29: drupal-1870764-29.patch queued for re-testing.
Comment #33
dawehner#29: drupal-1870764-29.patch queued for re-testing.
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedAssigning to myself, so I remember to review this as soon as I get a chance. Am shooting for later this afternoon or tomorrow.
Comment #35
effulgentsia CreditAttribution: effulgentsia commentedThanks frega, dawehner, and others for working on this.
My biggest concern is the coupling between forms and dialogs in all of the new code added in ajax.inc and ajax.js. I can see that core's FAPI/AJAX is lacking in its ability to integrate complete forms being returned by ajax that then need to trigger more ajax, and given that Views needs that, I support adding that integration. But I don't yet see why that should be coupled to dialogs. Maybe it's better UX for form wizards to happen in dialogs rather than other kinds of AJAX containers, but I don't like those things being coupled at the API level. I'll try to work on decoupling that, but let's not hold up Views progress on that.
For what it's worth, I agree with that.
Comment #36
effulgentsia CreditAttribution: effulgentsia commentedOops. Didn't mean to change status. I think this patch could use more work, but I didn't leave enough specific feedback to justify the status change.
Comment #37
effulgentsia CreditAttribution: effulgentsia commentedI opened a spin-off for just the settings/options portion: #1909144: Allow #ajax['dialog'] to contain options other than 'modal' in order to help unblock #1821548: Add a "diff" of some kind to the CMI UI. I may try to spin-off the commands from the FAPI portion as well, to aid the remaining discussion and review process.
Comment #38
YesCT CreditAttribution: YesCT commentedI updated the issue summary with a postponed on this and related issues section
Comment #38.0
YesCT CreditAttribution: YesCT commentedadded postponed on this and related issues section
Comment #39
webchickSince #1818142: [meta] Unified Blocks and Layouts (SCOTCH+WSCCI+Spark+Field UI) isn't happening, untracking from Spark. Also since #1909144: Allow #ajax['dialog'] to contain options other than 'modal' was committed, not sure this is critical anymore?
Comment #40
webchickWhat?
Comment #41
rfayThat was pretty impressive :-) And giving me the easy stuff too :-)
Comment #42
sun#1851414: Convert Views to use the abstracted dialog modal is still a critical task that is postponed on this, so unless that task's priority is demoted, this one needs to be critical, too.
The code here definitely needs work though. It contains plethora of @todos that we'll have to resolve prior to commit. Also, the ajax.inc as well as ajax.js changes look a bit fishy to me. Can we try to simplify and clean this up as much as possible?
Comment #43
quicksketchThis issue may also end up blocking #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms. For now I may take the views-ish approach and write a dialog-close AJAX command as part of editor.module, but then we'll end up with two places in core that are providing specialized closing AJAX actions and CSS styling for dialogs. On the other hand, it'll make it clear what requirements we have for making something that actually works in both places
Comment #44
frega CreditAttribution: frega commentedRerolled the patch from #29, minor manual merge, minor cleanup und updating example module dialog_example (.info -> info.yml).
Comment #45
Crell CreditAttribution: Crell commentedComment #46
DamienMcKennaComment #48
frega CreditAttribution: frega commentedAnother reroll - sorry missed a duplicate "use"-statement in a merged test. Attached interdiff is to #44.
Whilst it would be possible to rewrite views' current modal implementations (or quicksketch's in ckeditor), dawehner and I agreed that this issue needs some architectural feedback before doing that; having a modal/dialog cater for more complex use-cases will inevitably straddles FAPI, routing and JS - so feedback welcome :)
Comment #49
quicksketchThis looks like a great patch @frega! I'll give this a review later today and try to utilize it in #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms and see if I run into any shortcomings. This patch seems like it was accidentally marked RTBC; there are still a number of @todos in the code and hasn't gotten enough review yet.
Comment #50
quicksketchI'm taking a stab at rerolling this patch and I'm finding a lot of areas for improvement. I don't think the changes will be drastic code-wise, but we have some strange workarounds happening between the AJAX framework and the dialog framework. This probably happened because the dialog API happened first, without the AJAX commands.
The root of the problem stems from trying to define the response behavior inside of $element['#ajax']. Properties on the #ajax element beyond 'callback' is a legacy approach. When the D6 AJAX framework was written, we intended everything to be controlled through settings on the #ajax property. However, the D7 revamp of the AJAX framework introduced by @merlinofchaos shifted the functionality from being defined in the element to being defined in the AJAX response. The new approach is drastically more flexible, because you can tell the client-side to do *anything* you want, and it's not all bound up in a never-expanding set of FAPI properties. This newer post-D7 version of the framework provides lots of functionality that isn't otherwise supported using the #ajax property such as removing HTML in addition to adding it, adding CSS/JS, restriping tables, etc (basically everything in /core/lib/Drupal/Core/Ajax). We don't have FAPI properties for any of those things, and we shouldn't. The same thing applies to dialog functionality.
So... what should happen here is that we should remove the #ajax['dialog'] property entirely. Trying to have the build element control the response behavior is a huge mess, and it requires a bunch of modifications to ajax.js. If we switch to using AJAX commands only, we can remove all these nasty hacks we've inserted into ajax.js that deal with dialogs. There's an uncomfortable amount of interdependency right now that can be remove entirely.
Anyway, working on it now. I'll post when I get everything working again. I chatted with @dawehner in IRC and he seems to support the idea also. Considering AJAX commands is the way both Views UI and Editor module are using right now, so far 100% of our core dialog implementations prefer AJAX commands over #ajax properties.
Comment #51
larowlanWhich brings us right back to where we were in November in #1667742-45: Add abstracted dialog to core (resolves accessibility bug) (patch at comment 45)- using ajax commands instead of FAPI properties. Oh well, sometimes its the journey as much as the destination.
Comment #52
sunHm.
We need to differentiate between 1) triggering/opening a dialog, and 2) interacting within a dialog.
For 1), what we currently have with #ajax][dialog seems to be most appropriate, since it's all about triggering the thingie only.
The needs for 2) may vary in multiple ways. I'm not sure whether our current toolset is sufficient for that.
What you're essentially trying to do is to implant a Command & Conquer™ facility to trigger and control dialog operations into... server-side form/render arrays and/or Ajax commands. Regardless of how we design it, that's a daunting task.
Since all operations within a dialog apparently mean that we're operating in a JS environment already, one would normally assume that this is something the front-end should primarily take over (e.g., a backbone micro-app that's able to properly set and track state from the client-side, and which sends and receives data to/from the backend), since realistically, the backend is not able to reliably track the dialog's state.
I'd recommend to explore in that direction instead. @frega, @nod_, and others are most likely happy to help with the frontend plumbing.
Comment #53
larowlanFurther to @sun's points at #52, option (1) (triggering a dialog) is being used by CMI to display the diff in their UI so I don't think it is an option to remove it. i.e. we need both.
I agree that using the client-side to track dialog state makes more sense. One issue I can see with that is how would we go about making it degrade gracefully for non-js users?.
Comment #54
quicksketchI can assure you that using AJAX-style commands will work in every situation that #ajax-based elements worked. This new version will also still not make dialog.js dependent upon ajax.js, so if you have a non-AJAX situation, you won't need to use AJAX commands at all.
The same way that Views works already. Every single page has a "nojs" version that is used by default. It's only when ajax.js goes through and replaces all "use-ajax" links on the page that "nojs" is replaced with "ajax" in the URLs, so every link that is visited immediately knows if it need to return an AJAX command or just normal page content.
We're not really doing anything new here. We're just adjusting the Views/CTools approach to dealing with modals and using the same thing in core.
One huge advantage I've seen with this approach is that the dialog.js file doesn't need to be loaded on the page until a dialog is necessary. Because ajax.js already dynamically adds necessary JS/CSS, the dialog.js file is loaded *on demand* when a dialog command comes back. Better yet, this means that if your initial page load doesn't have jQuery UI yet, even *that* is all lazy-loaded only when you need it. The bandwidth/requests savings is fantastic.
I have everything working already at this point, I'm just revising our example module for testing purposes to demonstrate the full gamut of use-case scenarios.
Comment #55
quicksketchFollowup to my own comment:
The patch in #48 also includes ajax_is_ajax_request() which determines whether the page is an AJAX request by the header, but I haven't fully investigated/implemented that into my approach yet. It's probably a good idea. It'd be nice not to have "nojs" or "ajax" in all your AJAX URLs.
Comment #56
dawehnerComment #57
quicksketchOkay here's the new patch so far. I didn't get through updating all the previous examples in the dialog_example module. I did however get through all of the ajax_test.module, so to see more examples comments out the
hidden: true
in the ajax_test.info.yml to get more working examples.The basics of this patch:
Git has been giving me trouble generating the interdiff for this one, so here are the most important summary points.
For links, instead of using #ajax (which was never supported on links before as far as I know, maybe it was?) you set the 'use-ajax' class on the link. You can optionally include the special "nojs" string in your URL, which will be replaced with "ajax" by ajax.js. This is no different than any other AJAX link in use today, we're just adopting the current behavior for modals also.
For buttons, we do the same thing. Instead of making up a new #ajax['dialog'] property, we use the existing #ajax['callback'] property:
However by shifting all this code from the build stage to the response, menu handlers and AJAX callbacks are going to need to specifically state what functionality should be done upon clicking/submitting. There is no magically ability to just point at any form and say "put this in a dialog". Each form has to be updated to specifically handle what happens if it's been put into a modal by adding a #ajax['callback'] property on its buttons. This is more slightly more tedious, but can be mitigated by eventually updating the confirm_form() function to globally support modals. Since confirmation forms are one of our most likely candidates, after updating it, enabling modal confirmation forms will be a 1-3 line affair on each place where we want to use a confirmation modal.
Removing the magic behavior also makes it so that our modals work in more situations. The previous approach (as far as I could tell) hijacked ALL the buttons in the form. This meant that if you had existing buttons that had different AJAX behaviors, such as a file upload field or the "Add another" button in a modal, it wouldn't be supported by the magic behavior. Now that every button in the form is specifically targeted with a different #ajax['callback'] property, we don't have these conflicts any more. The #ajax['dialog'] approach also wouldn't have worked for Editor module, since we don't actually want to submit the form in a traditional sense: we need the specific values in the form to be inserted into the editor rather than redirecting the user to some other location.
This patch still needs some work and will definitely fail tests. I'm setting to needs review just to get an idea of the damage to tests, and some general review would also be greatly appreciated.
Comment #58
quicksketchOne thing that *did not* change from previous patches: ajax_is_ajax_request(). This handy utility function can detect if you're in an AJAX request directly without using the swap "nojs"/"ajax" trick. So if you don't want to update your menu paths, you can still use this method to detect an AJAX/dialog request. This additional bit of flexibility is really handy, though not entirely a necessity.
Comment #60
larowlanShould fix tests
Comment #61
larowlanTurns out we don't need ajax_is_ajax_request.
Comment #62
quicksketch@larowlan pointed out to me in IRC that Drupal::service('request')->isXmlHttpRequest() already provides this functionality. So the new patch removes the function because it's not necessary. IMO at this point we should go ahead and remove the demo module, moving what's testable into the ajax_test.module and what's not potentially into the examples project. However Views is probably the most complicated situation we'll experience, it should provide an extensive example itself. Other areas like the filter tips would also be easy wins.
Comment #63
jibranTagging.
Comment #64
quicksketchMostly the same page as #62 but tidying up:
- dialog_example module removed.
- Changed dialogSettings to dialogOptions for consistency (dialog.js and jQuery UI use the term options, not settings).
- Removed changes to confirm_form(). Confirm form modifications will be its own issue.
- Minor docblock updates.
Comment #65
quicksketchHm, missed a few settings => options renaming.
Comment #67
larowlanAdded followup for cleaning up drupal_get_js to use Drupal::service('request')->isXmlHttpRequest() too #1941288: Regression: ajaxPageState not being updated with AjaxResponse, assets (js/css) being added twice
Comment #68
quicksketchFixes the two broken tests.
Adds new test coverage for dialog commands. Hats off to @effulgensia and @sun for the creation of WebTestBase::drupalPostAJAX(). That actually makes it so that we can get a reasonably accurate emulation of an AJAX response for opening a dialog. Unfortunately it only works for POST requests sent by a form (rather than a link) so we can only use it for the dialog-opening buttons. The links on the page use drupalGetAJAX(), which isn't quite as accurate a simulation, but still suitable for checking our specific commands.
The previous test for dialogs was kind of comical: A full WebTestBase installation of Drupal that only went to a single URL then a @todo for "add some tests". :)
Comment #69
quicksketchDang, a few comment typos. Sorry testbot.
Comment #70
twistor CreditAttribution: twistor commentedLooking good, and with that, nitpicks:
Why is this being added?
Missing period.
(Optional)?
All of these properties need to be declared.
Trailing comma, there are lots of these.
Trailing period.
(Optional)?
Returning the constructor?
Empty line.
Don't we set #drupal-modal as the default already?
Trailing commas.
Comment #71
quicksketchThanks @twistor! I think you're right on all counts. Will reroll.
Comment #72
quicksketchEverything in #70 addressed. Expanded out docblock a little bit and code style fixes.
Comment #73
twistor CreditAttribution: twistor commentedI guess I didn't do a very good job the first time.
The references to CloseDialogCommand are incorrect. Also, if we're hardcoding #drupal-modal here, why are we setting it as a default in the parent class? It's odd that CloseDialogCommand would set its default to #drupal-modal.
Perhaps the other settings could be documented?
CamelCase.
Could $dialog_options be documented?
CamelCase.
Title isn't a string?
Comment is 81 characters.
Document $settings?
SetDialogOptionCommand not CloseDialogCommand.
Arguments trailing a default argument. Is that kosher? Also, (optional).
SetWindowLocationCommand not OpenDialog.
manipulating
Comment #74
larowlancan't this be
$this->selector = $selector
then make the default '#drupal-modal' instead of NULL?Nitpick, should include 'defaults to array()', same story for $settings
setDialogOptions instead?
these should be camel case
same
Same here, can't we use a sensible default in the arguments?
Apart from that and the points from @twistor, I think this is RTBC, plus with actual tests now too ;)
Comment #75
quicksketchFixes everything from #73 and #74. Some exceptions:
Unfortunately if you pass in a NULL value in PHP, it blows away any default specified in the function declaration. So using
$this->selector = $selector ? $selector : '#drupal-modal';
is the only way to do this. However there's no point in making the first argument "optional" if you have to fill out the second parameter, so I removed the argument default and updated the PHPdoc to specifically state that you may pass in an empty value to affect the modal dialog.This doesn't seem to be a coding standard. Existing counts of such references in core:
- Defaults to array() - 2 counts
- Defaults to empty array - 2 counts
- Defaults to an empty array - 10 counts
- Functions that contain a default of an empty array (estimated by regex search) - 287 counts
This is inline with our documentation on coding standards: http://drupal.org/coding-standards/docs#param
Comment #76
larowlanYou're right on the coding standards points at #75.
I think this is RTBC but tagging as JavaScript to get more eyes on it.
Comment #77
larowlanDidn't mean to do that until after reviews from JS team.
Comment #78
larowlanComment #79
nod_Nice that seems to work well :)
Few things:
options = options || {};
Couple of comments:
it appears that the ajax stuff doesn't update ajaxPageState anymore and the dependencies get loaded over and over. Don't think it's related to this patch.
Comment #80
quicksketchJust from looking at the code, it was screaming at me: this is an object. Drupal.dialog() was nothing but a wrapper around several variables and two callback functions. If this is intended to be a polyfill of sorts for HTML5, any native-DOM based dialog is definitely going to be a real object of a particular class, not a generic object that is used as a closure around a couple of functions. The API is still simple, it's just using a defined object instead of a generic one.
Looks like this latest patch undoes those changes. I'm not particularly a fan of the reverting, but since it turned out storing the element as part of the dialog object isn't really necessary after all, it's an unnecessary change for this patch.
Location seemed too vague to me, so I used WindowLocation to be clear. I didn't think of Redirect, which would be better and shorter. I'll reroll.
Comment #81
quicksketch- Renamed SetWindowLocationCommand to RedirectCommand.
- Fixed a few dialog.js whitespace indentations introduced in #79.
- Reformatted the comment on Drupal.behaviors.dialog.attach() to fit in the 80 character bar.
Comment #82
nod_Looking RTBC to me :)
Comment #83
larowlanAgreed
Comment #84
podarok+1RTBC
very nice code with a good documentation
happypush )
Comment #85
effulgentsia CreditAttribution: effulgentsia commentedconfig_admin_sync_form() needs to be fixed then. Currently, clicking on "View differences" on the admin/config/development/sync page results in the AJAX spinner starting and stopping, but the contents not being displayed anywhere.
Comment #86
quicksketchThanks @effulgentsia. I didn't know that had happened already. Fixed their usage. Although more verbose, I think it demonstrates customizing the page based on if it's shown in a modal or not. For example we use the title element as the modal title and only add the "dialog-close" class if it's applicable. I can make a shorter implementation if needed, but now that we have this ability to adjust contextually for the dialog we should encourage people to use it.
Comment #88
effulgentsia CreditAttribution: effulgentsia commentedWhat I don't like about this is that this changes config_admin_diff_page() to:
a) Care about whether it's being returned as an Ajax response or an HTML page.
b) Decide that if an Ajax response, then as a dialog.
I see that as regressing a separation of concerns that we worked hard to achieve in D7 and improve in D8: that page callbacks (and the _content controllers they're slowly being turned into in other issues) just worry about the content to return, and other code (that can be reused across many page callbacks / _content controllers) deals with packaging for HTML, Ajax, or whatever other format is wanted, and if Ajax, whether to put it in a dialog or in some other wrapper.
I understand that in some complex cases like Views, we may need the individual page callback to make these decisions, but I'm concerned about making that the pattern for all the simple cases like the config diff ui.
Comment #89
quicksketchEven in this simple case of the diff page, the contents of the page *should* be different based on whether its shown in a modal. Although in this particular example, the only thing that needs be changed is the addition of the "dialog-close" class. However, it's a change nonetheless. By explicitly modifying the form that is returned to function in a dialog correctly, we're removing a lot of magic and interdependencies. After this patch, jQuery UI is only added after clicking on the "View Differences" link, whereas before the page required jQuery UI (base, draggable, dialogs, etc) as well as dialog.js all before a dialog was even used. Now it only requires ajax.js and the rest is loaded on demand. Although it's slightly more PHP code, we have a huge boost in client-side efficiency.
We could add additional checking that specifically passed in some "isDialog" variable as part of the GET request so AJAX wouldn't necessarily mean "dialog", but in this situation I think it's entirely unnecessary. If something is being returned via AJAX, it's only a JSON response that gets returned containing all the necessary information for the page. If you wanted some other kind of AJAX-response system for viewing diffs, you could easily ignore the JSON commands that weren't relevant and retrieve the needed information only.
I'm not really sure which part of your comment to address. It sounds like you're arguing for implementation simplicity in the guise of purity of the menu callbacks. If in complicated situations it's okay to modify content based on the response, why would it be considered a regression in simple cases? In both situations we're tailoring the response to the way the data is going to be displayed. This is just removing the magic (note the removed @todo for inappropriately using the 'title' property to determine dialogs in the response) and making it explicit.
If we end up converting all menu callbacks to routers/content controllers (I admit I don't really know what this would mean, I only have a rudimentary understanding of most D8 concepts), it sounds like we could move the manipulation one level up and separate the content of the page from the AJAX commands. It seems like that would be an improvement towards separation and avoid all the magic at the same time. However in the interest of avoiding a rewrite of config.module's callbacks, this patch demonstrates the cleanest implementation we can have without conversion.
Comment #90
quicksketch@larowlan pointed out in IRC #1833440: Implement partial matcher based on content negotiation MIME type, which indicates that the new router system can set up different _content callbacks based the requested content type. This seems like what we're looking for: the AJAX request would get a different handler (which would most likely call the "normal" handler and then manipulate it). Then we get the explicit behavior and the clean separation.
Comment #91
effulgentsia CreditAttribution: effulgentsia commentedHm. I'll think about #89 a bit and will post back here tomorrow.
Comment #92
jibran#1884530: Complete modal dialog tests. is marked as duplicate.
Comment #92.0
jibranUpdated issue summary added a postponed issue
Comment #93
nod_Just want to underline that to me, this justify any amount of workaround or weirdness on the PHP side. As quicksketch said, it's pretty huge.
Comment #94
larowlanHere's a wip on how it would be done with routes, doesn't work for the ajax callback yet but might helps someone see what we're talking about
Comment #95
podarok#1875086: Improve DX of drupal_container()->get()
http://drupal.org/node/1929006
this needs update to latest DX
Comment #96
quicksketch@podarok the patch in #94 is not intended to be part of this patch (at least not yet), it's merely demonstrating the concept that could be used instead of the approach in the current patch, which is #86. #94 is a complete patch, #86 is only a demonstration and would need to be incorporated into the existing patch or (hopefully) moved to a followup that converts all the content handlers in config.module at the same time.
Comment #97
larowlanRe #95, I believe that is the correct approach when the container is injected, this is in the controller create method and is consistent with the recent changes.
Comment #98
effulgentsia CreditAttribution: effulgentsia commentedThanks for #94! I like that direction, and furthermore, would like the diffDialog() controller further genericized into a dialog() controller that can be reused for many simple dialog routes. I'm okay with us proceeding with #86 here, and doing #94 in a follow up, if that would help unblock progress on Views. But if we do that, I think it would make sense to keep #1842036: [META] Convert all confirm forms to use modal dialog postponed until after that follow up.
Comment #99
quicksketchYeah this route would allow any path in Drupal to be "dialogified" simply by adding a second route at the same URL and enabling AJAX on the link that should open the dialog. @effulgentsia and I talked about adding some kind of ability at the time that you specify the AJAX link to also specify the request type, so the link could specifically request a dialog back from Drupal and use that particular route/content handler. This way we'd get pretty much everything "good" and nothing "bad" about the new system: complete separation between the original content handler and the dialog handler, easy enabling of any link to become a dialog (using the generic "wrapper" content handler), and the ability to use your own specialized content handler for advanced dialogs. Plus we get consistency and lazy-loading of the dialog libraries in both simple and complex use-cases.
However all of that isn't included in this patch. We're not entirely sure the routing system can handle this situation at present, and it's a lot to try to bundle all together into a single issue. This issue does what we set out to do: add AJAX commands for dialogs. However, the server-side handling could be improved by using routes to separate the page content from the dialog and providing a generic content handler that could wrap any page in Drupal. Let's open a followup and add a @todo that can provide a tip for users who look at this for a particular example, since by the time we finished the current approach would only apply to non-routed menu callbacks.
Comment #100
quicksketchI also looked into using the #ajax property on links again instead of the 'use-ajax' class, but taking a look at drupal_pre_render_link and ajax_pre_render_element, these functions do not appear to be good candidates for handling links that have no #ajax properties and simply want a class. We don't need any Drupal.settings added to the page for these links. Additionally using #ajax introduces an unnecessary #attached for jquery.form.js, which is needed for form elements but not links. Refactoring these functions would be yet another ball of yarn, so for now just specifying the class 'use-ajax' and the required ajax.js #attached seems significantly more straightforward.
Comment #101
quicksketchNew issue to implement this fancy-pants generic handler: #1944472: Add generic content handler for returning dialogs
Comment #102
quicksketchHere's a minor reroll that references our follow-up issue so that users who look to config.module for an example can be pointed to the issue that will provide our recommended approach (once we get that working).
If the router system provides the functionality advertised, we should be able to convert Views right away. Regardless of the router system, I do know this at least provides everything Editor module needs to move forward with image/link handling; as it won't provide a non-JS version of WYSIWYG functionality and so doesn't need multiple content handlers at the same path.
Comment #103
quicksketchWell I didn't quite understand how deep Views went with its custom form handling. It'll take some significant work to undo Views' custom form handling mechanisms and use the approaches @larowlan is investigating in #1944472: Add generic content handler for returning dialogs. However, directly replacing the modal system without rewriting the form handling in Views is almost trivial. I did the full conversion in #1851414-19: Convert Views to use the abstracted dialog modal. We can now knock off two critical tasks. Any takers on RTBC?
Comment #104
larowlan#1944472: Add generic content handler for returning dialogs now works cleanly on top of this, providing a proof of concept for the new routing approach. Good to go
Comment #105
webchickAWESOME. So great to see this at RTBC! The code is actually much less complicated than I feared, too. And as a bonus, it fixes the CMI dialog that we accidentally broke in #1843220: Convert form AJAX to new AJAX API.
Committed and pushed to 8.x. Thanks!
Not sure if this needs a change notice or not, since I don't see any changes in the calling code..?
Comment #106
quicksketchClosed out #1863478: Split Dialog API and optional integration helpers into separate files as duplicate of this one.
This change notice no longer applies: https://drupal.org/node/1853388. We removed #ajax['dialog'] in this patch. Should we update that change notice, delete it, or both update and make new one?
Comment #107
jibranhttp://drupal.org/node/1852224 and http://drupal.org/node/1853388 need update.
Comment #108
effulgentsia CreditAttribution: effulgentsia commentedI didn't change http://drupal.org/node/1852224, because I think everything there is still correct. Am I wrong on that?
I updated http://drupal.org/node/1853388 to indicate it has been reverted (do we have a more standard way of doing that?).
We still need a new change notice for the command-based API, so leaving this issue open for that.
Comment #109
larowlanDraft notice is here: http://drupal.org/node/1989646
Comment #110
Crell CreditAttribution: Crell commentedTweaked the language a little. Looks good now. Yay!
Comment #111
jibranI think we need to add all the cases.
1. form link element.
2. theme_links/form operations element.
3. buttons/form action element.
4. Difference between using #ajax and #attribute.
is same as
Ensure the path
ajax-test/dialog-contents
is handled by a route.Comment #112
Crell CreditAttribution: Crell commentedjibran: Go ahead and do so, please.
Comment #113
jibranI have updated change notification http://drupal.org/node/1989646/revisions/view/2679778/2681396.
Comment #114
larowlanLooks good, thanks @jibran
Comment #115
larowlanComment #117
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #117.0
xjmUpdated issue summary.