In #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases, we added generic AJAX commands for controlling dialogs. In that issue, we discovered that having a single menu callback (or router _content callback) generate both AJAX and non-AJAX content introduced callbacks that were doing multiple things based on the request headers. One of the primary selling points of the new router system is the ability to send back different content based on the specific type of content requested *at the same URL*, using different _content handlers.
We should utilize the new router system for cleanly separating _content handlers that provide actual content, and those that utilize the original content handlers and return the content in some different way (such as in a dialog). In order to facilitate simple "dialogification" of existing paths, we should add a generic _content handler that can wrap around any existing handler. This will make simple situations that use dialogs possible with the following:
- Adding a second router item at the path to be displayed in the dialog.
- Add the "use-ajax" class in the link to the URL to be opened in the dialog.
- Attach the drupal.ajax library so that the use-ajax link is detected.
Preferably, we could combine use-ajax and the drupal.ajax library requirements together, but how that should be implemented isn't yet decided.
The config.module was converted in #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases to use the new AJAX commands, but would benifit from using separate router items as described above. We may convert that module as part of this issue as a demonstration, or separate it out into another issue.
Remaining tasks
Postponed on this
- #1842036: [META] Convert all confirm forms to use modal dialog
- #1979038: Convert views_ajax_autocomplete_taxonomy() to a Controller
- #1979036: Convert views_ajax() to a Controller
- #1978894: Convert file_ajax_progress() to a Controller
- #1978932: Convert overlay_ajax_render_region() to a Controller
- #1978892: Convert file_ajax_upload() to a Controller
Related
Comment | File | Size | Author |
---|---|---|---|
#68 | dialog-route-1944472.68.patch | 29.61 KB | larowlan |
#65 | dialog-route-1944472.65.interdiff.txt | 8.2 KB | larowlan |
#65 | dialog-route-1944472.65.patch | 29.61 KB | larowlan |
#61 | config-diff.png | 41.38 KB | effulgentsia |
#56 | dialog-route-1944472.56.interdiff.txt | 798 bytes | larowlan |
Comments
Comment #1
quicksketch@larowlan posted this patch in #1870764-94: Add an ajax command which makes it easy to use the dialog API in complex cases to demonstrate the concept. It's not a working patch, more a proof of concept on how we could convert config.module's "Show differences" button.
Comment #2
larowlanI'll keep kicking this along, been up to my eyeballs in symfony routing trying to make this work
Comment #3
quicksketchThanks @larowlan, I'll give this as much attention as I can, hoping I can be a help on some front. I can help be your "reroll support". ;)
Comment #4
larowlanOriginal patch was so close - the ajax one had to use _controller instead of _content.
Comment #6
larowlanWill re roll tomorrow
Comment #7
Crell CreditAttribution: Crell commentedRelated: #1938980: Fix Ajax system; the last remnants of the old API must be swept away and #1945024: Remove subrequests from central controllers and use the controller resolver directly.. Note that currently the Ajax-controller-detection code is wrong, so the first issue there is probably a blocker for this one.
Otherwise, ++ on generalizing the ability to Ajax load an arbitrary page body. That's exactly the sort of coolness we want to enable.
Comment #7.0
jibranUpdated issue summary.
Comment #8
jibranTagging
Comment #9
larowlanSo this is the patch from #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases that should apply cleanly now that is in.
However, we want a more generic approach that provides a dedicated controller that just forwards to the fallback version and wraps it in a dialog command.
Comment #10
larowlanAnd this time with the patch
Comment #11
Crell CreditAttribution: Crell commentedI think technically we need a line break between the description and @var line. Same for $sourceStorage.
We don't really have a standard for prefixing docblocks of controller methods yet, but "Page callback" is not it. :-) Probably we can just rip that part off and leave the rest.
Probably off topic here, but... why is DrupalDiffFormatter in the global namespace???
Ibid.
I really like what we're able to do here with the routing, though. :-) Is there a way to genericize that behavior, perhaps?
Comment #12
larowlanYeah that is the ultimate goal here, we need a \Drupal\Ajax\DialogController and \Drupal\Ajax\ModalDialogController then mark the routes to use that in your yml, setup the 'use-ajax' class on the link and you're done.
Yep, missed that - thanks
My thoughts exactly -definite wtf.
Comment #13
larowlanTagging for nod_ and JS team.
This is the generic implementation.
In order to make a link load its contents in a dialog you need to a) use the new routing system and b) make sure your link has the following attributes
If you don't want a modal, use 'application/vnd.drupal-dialog'
This just works. You cannot begin to comprehend how fully awesome the new routing system is until you see stuff like this.
At the moment all that I've converted is the config module's diff dialog.
Next step is to convert the ajax_test module's dialogs.
Then we need to wait for #1938980: Fix Ajax system; the last remnants of the old API must be swept away and rework the bit in the RouteProcessorSubscriber to use a Route Enhancer instead.
Comment #14
larowlanLets see if it breaks anything else
Comment #15
larowlanLost tags
Comment #16
larowlanNeed to add tests for the new accepts behaviour
Comment #17
mkadin CreditAttribution: mkadin commentedThis looks quite good to me and falls in line with what already exists for Ajax. The idea of hitting node/1 with a custom mime type and getting back a modal has me drooling over D8! Looks like #1938980: Fix Ajax system; the last remnants of the old API must be swept away is RTBC, so maybe we should wait until that's in so we can refactor and then write tests.
As a side note, I wonder if there's a way to do something a bit more general so that some of the code that's repeated between Ajax, Modal, and Dialog controllers can be consolidated...I don't think it's particularly important for the moment, but if we start to add more of these mime-type based controllers...it might make our lives easier. Or if it was a generalized class, it could make it easy for modules to add their own custom controller for a custom mime type. Just a thought.
Comment #18
larowlanTurns out this works for existing menu links (hook_menu) too.
I just created an article node, then a second article node, added a link with that class and data-accepts in the second node pointing to the first node and it loaded node/1 in a modal!!!!
Comment #19
larowlanscreencast http://www.youtube.com/watch?v=-0GRVADbawc&feature=youtu.be
Comment #20
larowlanThings to consider: we should be able to control modal size from data- attributes
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedAgreed. How about if a data- attribute could hold default dialogOptions that Drupal.ajax.prototype.commands.openDialog() extends with response.dialogOptions instead of using only response.dialogOptions?
Comment #22
quicksketchI think in most cases the size of the dialog would be determined by the content being returned. If you were opening the same URL from multiple places, it would probably be the same size in all of them (maybe not though). I think I'm basically saying that we should keep the settings server-side rather than making our generic handler provide all kinds of options that are based on client-side specifications. If we go down that road I won't be offended, but I'd like to see client-side specifications be a secondary option rather than the de facto approach.
Speaking of, this code seems a bit too rigid:
It basically looks to me like the controller is forcibly set based on the request content type. Does this mean that a module couldn't (or shouldn't) set up a different route/content handler specifically for dialogs? Is that still an option with this approach, and what would it look like?
As a guess, if you wanted to have a customized dialog, would you you use data-accepts="application/vnd.drupal-ajax" and return a set of custom AJAX commands? Or would you still use "application/vnd.drupal-dialog"?
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedThis looks great. Just some minor nits.
This can be improved with #1830588: [META] remove drupal_set_title() and drupal_get_title(), but we shouldn't hold up this issue on that.
Per #21, let's not have this here.
#drupal-modal doesn't make sense as the selector for non-modals. I suppose if we think it's useful to have a generic DOM element for non-modals, we can simply rename it to 'drupal-dialog'. However, the nature of non-modals is you can have multiple open at a time, so that's why I originally thought that for non-modals we need to require the specific use case (response or link data- attribute) to explicitly specify the selector.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedIt's not forcibly set, it's only defaulted if the route doesn't have an explicit _controller (look again at the if statement). So, you can have a drupal_dialog (application/vnd.drupal-dialog) route explicitly define a _controller in the routing file (in addition to the _content) if you want a specific one for that route.
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedOh, and if a module wanted to define a different default _controller to use for all routes that don't have an explicit one (i.e., swap out Drupal\Core\Ajax\DialogController with something else entirely), it could do that by defining a higher priority RouteProcessorSubscriber that would run before this one.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedGood point. First of all, I hope in #1918744: Define some dialog CSS classes for common dialog sizes, we move away from pixel sizes, and define things like "small", "medium", and "large". But whether the link should have precedence or the server response should have precedence is a little tricky. To set it from the server, but not hard-code it in the generic controller, we could add some info to the route. For example:
That only handles size though. Alternatively, instead of _content_size, we could define something like _dialog_options?
Comment #27
mkadin CreditAttribution: mkadin commentedRE: module's defining their own module controllers...they don't need to use the mime-type at all...they can just use the Ajax commands to open and close dialogs as they wish. The purpose of the mime-type routing is to take content that wasn't pre-determined to be in a modal to be modalizable.
I disagree (but won't freak out if it goes the other way) with the server-side implementation for sizing. Because this mime-type approach gives us the ability to put any content into a modal, it makes sense to me to handle its presentation after the fact. For instance, if I want to pull up nodes in a modal by linking to /node/{nid} with the custom mime-type, I don't think core should predetermine (in the node module's YAML routing file) the size of that modal.
Comment #28
Crell CreditAttribution: Crell commentedSide note: The screencast in #19 makes me drool.
If a contrib module wants to change the default-controller logic, it can rip out the existing enhancers and add its own if it wants to, via a Compiler Pass. There's a lot of flexibility there. Depending on how things go I could be persuaded to move controller selection from a series of enhancers to a series of objects that a single enhancer uses, much the way we are currently handling parameter conversion. I don't know if that makes sense or not, but I'm open to discussing it later if it would be more performant/deterministic.
I don't have a strong opinion on server-side vs. client-side sizing, since I don't understand the problem space as well as I'd like. Couldn't we have some site-wide default, overridden by a per-route default, overridden by a client-side specification? (That seems to be the Drupally answer, anyway.)
Comment #29
quicksketchOn dialog sizes, I posted to #1918744: Define some dialog CSS classes for common dialog sizes basically saying we shouldn't have any sizes at all other than what's defined in CSS, assuming we can figure out the technical details of that approach. A requirement of that working though is giving each dialog a class that it can be targeted by. If we can figure out a good generic way of handling that here too, that would be great.
Comment #30
falcon03 CreditAttribution: falcon03 commentedQuestion from a novice point of view: is this a blocker for converting Overlay to use the new dialog API to solve
#890288: Improve Overlay accessibility by using modal dialogs
Comment #31
larowlanSo this patch supports
To allow the front-end to pass in options.
Not sure if there is an XSS threat vector here, looked into the ajax.dialog.js, looks like the selector ends up getting straight into $() but has to be prefaced with a # so I think that is safe.
Some screenshots
The markup showing three dialogs of varying sizes, targets and modal state
Screenshot of two of the non-modals open with different widths
Screenshot of the modal at a nominated width
So to-do here is tests.
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedI don't think so. Just like the config diff dialog that's in HEAD works fine without this, I think Overlay related work can proceed without this as well. Possibly, once this lands, some Overlay integration code can be cleaned up to leverage it, but even if that's the case, it represents a small fraction of the total work needed in rethinking Overlay based on dialogs. I only recommended postponing #1842036: [META] Convert all confirm forms to use modal dialog on this, because that issue is practically nothing but marking URLs/links that should appear in dialogs, and this issue changes the way that's done.
Comment #33
falcon03 CreditAttribution: falcon03 commented@effulgentsia#32: thank you very much for this information. I was wondering if this was a blocker for the overlay conversion while I was looking at #1842036: [META] Convert all confirm forms to use modal dialog.
Comment #34
larowlanSo this refactors the tests to use this method.
Also some minor coding standards cleanups in DialogTest and documents WebTestBase::drupalGetAJAX as it took me ages to work the headers out.
So this gives us
*automatic route enhancement to dialog/modal based on data-accepts attribute for links (with test coverage)
*pass through of dialog options via data-dialog-options attribute for links (with test coverage)
We're now stuck waiting for #1938980: Fix Ajax system; the last remnants of the old API must be swept away
Shots of config module using it at different widths
Comment #35
larowlanSo now that route enhancers are in, switched this to use those instead of the route subscriber.
Lets see what breaks.
This is ready for serious reviews/consideration now the blocker is out of the way.
Comment #37
larowlan#35: dialog-route-1944472.35.patch queued for re-testing.
Comment #39
larowlanMissed the return
Comment #41
larowlanWe need to handle legacy routes too (for now).
Also had controller instead of _controller.
Comment #42
effulgentsia CreditAttribution: effulgentsia commentedOnly for the AjaxEnhancer do we currently have this nonsensical if statement. For the other enhancers, we only set _controller for routes that *don't* already have one and *do* have _content (or _form). AjaxEnhancer does it this backwards way to support legacy routes, but why do we need to support legacy routes for the new dialog controller? I'd be fine with saying you need to convert routes to the new routing system in order to benefit from the generic dialog controller.
Comment #43
larowlanWorks for me - gives folks an incentive :)
Means I have to move the test module over to make it work but no big deal.
Comment #44
larowlanRemoves legacy route support.
Comment #45
larowlanOk, back to green, ready for serious reviews.
Comment #46
effulgentsia CreditAttribution: effulgentsia commentedThis removes the ViewSubscriber changes that are being handled in a separate issue (#1594870: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply).
And this makes a couple other cleanups related to #42.
The tests here look good, so removing the "Needs tests" tag.
I haven't finished reviewing everything, so am not quite ready to RTBC it yet, but reviews or RTBC from others welcome.
Comment #47
Crell CreditAttribution: Crell commentedRoutes won't be all converted to return render arrays. They'll be returning PageFragment or whatever it gets called. Same impact (we can get rid of drupal_set_title()), but the @todo should be accurate.
Interesting. It may make sense to generalize this further. I've been pondering if we're going to end up with more enhancers than we want this way, and some kind of standardized registration would work better. Not something to change in this patch, but something to consider.
Overall this looks sensible. If I read it right, it means that we can modal-ify *any* _content-using route, right? If so, that makes me warm and fuzzy inside.
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedJust for clarity, per #1594870-26: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply (which is my stance, not Crell's), I agree with Crell that the result of a forward() call of a _content using route will be a standardized object like PageFragment. My only disagreement in that issue is that I think the _content controller itself should generally return a render array (or possibly string, if we want to continue supporting that for 'hello world' controllers), so that the creation of a PageFragment object is centralized in a View subscriber.
This code will need to be touched when PageFragment lands anyway. In the meantime, let's keep the @todo accurate for HEAD as it is now. If someone comes up with a way to word it so that it's both accurate now, and gives people a heads up on what the future will be, then that's ok too.
Agreed. larowlan++.
Comment #49
larowlan<cheeky>So does that mean we're RTBC?</cheeky>
Comment #50
effulgentsia CreditAttribution: effulgentsia commentedAlmost. I haven't tested manually, but from looking at the code, I think my only concerns are:
How does this end up working? I don't see whose responsibility it is to get this element into the DOM.
I think this means that for nojs requests, that the HTML response lacks a title and breadcrumb. Should we keep a stub hook_menu() entry around for that, like we've been doing for other menu links whose route info we move to the new system?
Comment #51
larowlanYes you're right, setting to needs work.
Its in the existing js from #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases
Comment #52
larowlanAdds back the hook_menu entry for breadcrumbs etc
Comment #53
nod_Seems like there are some redundant checks in ajax.js:
can't we just have
In the first piece of code?
Also
ajax.options.data['dialog_options'] = element_settings.dialog;
ought to bedialogOptions
if we follow convention. In this case it should beajax.options.data.dialog_options = element_settings.dialog;
at least, no need for the brackets.Comment #54
larowlanThis fixes issues identified by nod_
Comment #56
larowlanMissed tests for dialog options.
Comment #58
larowlan#56: dialog-route-1944472.56.patch queued for re-testing.
Comment #59
larowlanWhat else needs to be done here?
All issues from reviews addressed.
Comment #60
nod_All good for me. The patch makes Crell "warm and fuzzy inside" too :þ
I'll let the RTBC to someone who knows about the whole routing thing though.
Comment #61
effulgentsia CreditAttribution: effulgentsia commentedHere's a screenshot of the config diff modal:
Did you intentionally reduce this to 500 from 700?
This turns out to never be true, because the subrequest's ViewSubscriber::onView() calls
$event->setResponse(new Response(drupal_render($page_result)));
and should because the signature of Response is that $content must be a string. As a result, the screenshot above shows 2 titles rather than the desired one as the dialog title. Any ideas on how to quick fix the title problem while deferring (and adding a @todo link to) a complete solution to #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox]?Comment #62
tim.plunkettCode looks good, needs some cleanup though
First word should end in 's'
@param \Symfony\Component\HttpFoundation\Request $request
Typehint:
Request $request
Second line of @todo is indented with 2 more spaces
s/id/ID
Neither of these are used?
Out of scope
Let's get a real line here
Utility is spelled wrong
Missing docblock
Comment #63
xjmSome issues for converting AJAX callbacks:
Comment #64
larowlanWe're only tackling dialogs here. There is another issue for converting system/ajax to a route.
Typing on my phone, sorry if terse.
Comment #65
larowlanFixes issues identified at #61 and #62
Comment #66
larowlanstill green, love those ones
Comment #67
effulgentsia CreditAttribution: effulgentsia commentedThanks!
Comment #68
larowlanre-roll
Comment #68.0
larowlanUpdated issue summary.
Comment #69
jibranA lot of postpone issues on this normal task it should be major.
Comment #70
webchickAlex walked me through this patch line by line, looks really great! I love the cool trick with using the mime type to trigger dialog behaviour, rather than mooshing it into the route definition.
There were two classes missing docblock but I just fixed that on commit, with help from Alex:
For DialogController: "Defines a default controller for dialog requests."
For ConfigController: "Returns responses for aggregator module routes."
Please eyeball those for accuracy. Other than that, looks great!
Committed and pushed to 8.x. Thanks! :D
Also, we don't escalate things to major for blocking normals, so undoing that priority change. I think it's been established that those route conversions are not in fact blocked on this patch.
One follow-up we'll need is the ability to use this on _form as well as _content, because we'll need that to use this in Views and on confirm forms.
Comment #71
quicksketchWhee, yay thanks @larowlan, @effulgentsia, et. all. Can't wait to see this in action.
Comment #72
jibranhttp://drupal.org/node/1853388 needs update and we also have to fix #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases as well.
Thank you for explaining.
Comment #73
webchickno probs, and thanks for catching the need for a change notice here. total brain-fart. :)
Comment #74
aspilicious CreditAttribution: aspilicious commentedHmm, when the dialog is long (needs scrolling) and the toolbar header is in horizontal mode the top of the dialog gets rendered under the toolbar.
See image: https://www.diigo.com/item/p/cdrqprqzbpdporpddzbacbasre/abb2954c5bca72db...
Is there an issue for this?
Comment #75
jessebeach CreditAttribution: jessebeach commentedI can fix the placement issue quickly here. No need for a follow-up. What path should I look at?
Comment #76
aspilicious CreditAttribution: aspilicious commentedI went to the config sync page. Copied a view from active storage to staging. Changed some values (5 should be enough). When viewing the diff you will get a large modal.
Comment #77
jessebeach CreditAttribution: jessebeach commentedThe patch in #68 does apply at all for me. Can someone try to apply it to HEAD and verify if I'm going crazy or not?
Comment #78
mkadin CreditAttribution: mkadin commentedI think #68 is already committed.
Comment #79
tim.plunkettYep. http://drupalcode.org/project/drupal.git/commit/22df596
Comment #80
jessebeach CreditAttribution: jessebeach commentedThis has been committed. Changing the status from active to fixed.
Comment #81
jessebeach CreditAttribution: jessebeach commentedAdded #1986884: New AJAX dialog is positioned behind the Toolbar when the content of the dialog is very long to address the dialog positioning.
Comment #82
jibran@jessebeach this is active because it needs change notification. :)
Comment #83
larowlanDraft notice is here: http://drupal.org/node/1989646
Comment #84
Crell CreditAttribution: Crell commentedTweaked the language a little. Looks good now. Yay!
Comment #85.0
(not verified) CreditAttribution: commentedUpdated postponed issues.