Problem/Motivation
As part of the Views in Drupal Core initiative (VDC) this issue is to track abstracting the Views modal widget from views ui module into core. This is identified as one of the early dependencies of the VDC initiative.
Proposed resolution
Abstract the Views Modal widget/pattern from views ui module and add it to core.
Remaining tasks
Re-roll as per @quicksketch's feedback
User interface changes
Delete forms for nodes from links at admin/content/node display in modals for users with JavaScript
Delete forms for forum containers and forums display in modals for users with JavaScript
API changes
Modules wishing to display form content in modals should change their page callback to ajax_modal_form instead of drupal_get_form.
Note that the new routing system has a proposal to do format based routing so this will become more intuitive
If modules wish to trigger these callbacks via links, they should add 'modal' => TRUE to the options passed to l() when generating the link.
Modules wishing to display modal forms using the ajax api should attach an ajax behavior to their button and attach the modal library
$form['actions']['delete'] = array(
'#type' => 'submit',
'#value' => t('Delete'),
'#attached' => array(
// Attach the modal library.
'library' => array(array('system', 'drupal.ajax.modal'))
),
'#ajax' => array(
// Declare the ajax callback.
'callback' => 'forum_confirm_delete_ajax'
)
);
// ..
/**
* Ajax callback for deleting a forum.
*
* Calls ajax_render_modal_form passing the $form_id and the form arguments.
*/
function forum_confirm_delete_ajax($form, &$form_state) {
return ajax_render_modal_form('forum_confirm_delete', array(
$form_state['values']['tid']
));
}.
Comment | File | Size | Author |
---|---|---|---|
#125 | Diff dialog.png | 215.38 KB | Dean Reilly |
#110 | core-js-modal-1667742-110.patch | 16.56 KB | effulgentsia |
#110 | interdiff.txt | 4.6 KB | effulgentsia |
#108 | core-js-modal-1667742-108.patch | 13.38 KB | jessebeach |
#108 | interdiff.txt | 1.23 KB | jessebeach |
Comments
Comment #1
larowlanAdding tags
Comment #2
nod_See what's currently missing from ctools modal #1397370: Make modal.js use jQuery dialog it's not really useful to views but it it for the modal functionality.
The abstracted part in the issue is important. Modal is not a good thing on mobile, we need to be able to change the way it work on the fly meaning we can't go blindly with jQuery UI.
(edit) Also there are two parts to this issue. The actual abstract modal API and the ajax commands. You might want to split one or the other in a follow-up issue.
Comment #3
nod_For reference, core issues somewhat related:
#1175830: Update to jQuery UI 1.10.2
#114546: Confirmation alerts done with JS/AJAX (delete confirmation page and any others)
Comment #4
larowlanAlso:
http://bugs.jqueryui.com/ticket/7861
http://bugs.jqueryui.com/ticket/7862
If this is going to fly, need to get these resolved upstream to get through the a11y gate
And:
#617730: UX: Use modal dialogs for confirmation forms.
Tagging for mobile because we need fallback on smaller devices.
Comment #5
nod_Well scott gonzalez from the jquery team assured us our blocking accessibility issues will be fixed for 1.9.
Comment #6
larowlanSee: https://github.com/jquery/jquery-ui/pull/684, fixes tabbing issues with jquery UI Dialogs
Comment #7
larowlanwip: http://www.youtube.com/watch?v=GxDdOPGuqug&feature=youtu.be
Comment #8
larowlanPatch for discussion
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedPerhaps it's premature, but at some point (later than now, but likely earlier than all of Views being committed to core), it will be helpful to have a more complete use case in core than a simple confirmation dialog. In #1175830-6: Update to jQuery UI 1.10.2, yched suggests Field formatter settings as a good candidate. There's also #1101600: Users need to be able to select from list when adding menu items to a menu, though some later comments in that issue argue against a modal for that. Adding d8ux tag for usability feedback for what would be an accepted use case.
Comment #11
tim.plunkettTagging.
Comment #12
Bojhan CreditAttribution: Bojhan commentedWhy don't we also move the overlay to this?
Comment #13
tim.plunkettAs of now, this works while in an overlay. Not sure how Inception-y we want to get.
Comment #14
Bojhan CreditAttribution: Bojhan commented@tim.plunkett I mean, the technology. The visual styling obviously has to be different, but given that this modal is accessible and overlay isn't we shouldn't have two similar technologies in core.
Comment #15
nod_See #1175830-79: Update to jQuery UI 1.10.2 html5 is working on something, we should keep an eye on it.
Comment #16
tim.plunkettThis doesn't actually have any CTools dependencies.
Comment #17
larowlanThis attempt makes use of the request object's content-negotiation and does away with the need for a change to the menu callback paths.
Patch includes change to make the 'delete' links on admin/content use a modal.
With this patch, making a form display in a modal requires:
You change the menu callback from drupal_get_form to ajax_modal_form.
You change the calling link to add
$options['modal'] = TRUE
when calling l().Comment #18
yched CreditAttribution: yched commentedThe rest of the code is way over my head, but :
This looks a little too common for a class name that's supposed to trigger some specific modal-related behavior. Could we namespace/specialize that a bit ?
6 days to next Drupal core point release.
Comment #19
larowlanFixes suggestion from #18.
Comment #20
swentel CreditAttribution: swentel commentedGave it a quick spin, looks nice overall. The only weird thing is that there's a border around the delete button.
Chrome Version 21.0.1180.89
- edit - seems like it's some sort of autofocus on it - clicking somewhere removes that.
Comment #21
larowlanThe border is because the delete button gets the focus, making this an accessible modal. Yay for jquery.ui.
Comment #22
nod_Nice start. The JS needs some work. Lots of JSHint issues to solve and a few other things that are not following current core practice. Like the
$('selector', context)
that should be$(context).find('selector')
. Missing a docbloc for the dismissModal JS command.There seems to be a lot of JS considering we're using jquery ui to do it for us. Care to have a look at #1397370: Make modal.js use jQuery dialog and see if there is something that can be reused from there?
Also it kinda doesn't work well with overlay, probably an easy fix.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedThis is required for VDC, so raising to major.
Comment #24
tsvenson CreditAttribution: tsvenson commentedAs pointed out in #10 by @effulgentsia:
I'm also curious to learn how simple/advanced this one is planned to be. The modal Page manager uses, which comes from CTools I believe does much more and would be so much more useful to have in core.
Is there plans to allow the modal to contain more complex forms, AJAX and multistep wizards even? Or, is that better suited for a more advanced variant...
I can think of tons of use cases all over core where this would vastly improve usability. Such as adding/editing fields in the Field UI in a modal instead of replacing the field list tab as now is the case.
Sorry if this is a bit off-topic, but I feel it is at least to some extent relevant.
Comment #25
larowlanCan I get a review of the php/form api stuff too?
Will re-roll once for both js/php.
Comment #26
seutje CreditAttribution: seutje commentedAlways provide a radix for parseInt.
And I don't think those padding values could possibly change in between those 2 calls, so it'd prolly be nice to cache the return value of those
css()
callsOther stuff, like
===
vs==
should be flagged by JSHint.Comment #27
effulgentsia CreditAttribution: effulgentsia commentedSorry I haven't gotten around to reviewing this yet. I plan to soon. Reviews from others, of course, are still very much welcome.
Comment #28
webchickTentatively tagging Spark. We might need this for the layout/block UI.
Comment #29
User Advocate CreditAttribution: User Advocate commented@webchick – glad to see you add that connection to Spark. I think a core modal form capability can be very relevant to usage contexts such as the ones that Spark is addressing. In fact it can potentially have a profound change on the UX for all of Drupal. It’s worth taking some time to consider the dimensions of this so we don’t miss the opportunity.
Bringing a modal dialog capability into core is consistent with the general shift in UX strategy we’re going through for D8 - and that reflects a change going on across the entire web. That change comes from an increase in user expectations for web applications. That is to say, web apps are more and more going to have to provide rich user experiences that were previously only possible in desktop applications. In that regard, this simple confirmation modal dialog is the tip of a very big iceberg.
@effulgentsia and @tsvenson indicated that it would be useful to consider more use cases. I think that’s a good idea but I want to mention that doing so could be a long process. To begin that process I want to just go over a few use cases that apply to even a simple confirmation dialog and share a few key concepts that come from UI design standards for desktop applications.
A confirmation dialog should be based on a base class (or equivalent) that can be configured for different purposes. In the Windows API (which I’m most familiar with) this is done using a ‘Message Box’ window. When one is created, the specific type of dialog can be controlled by flags passed into the create function. These flags will create the UI buttons that the given dialog type requires to carry out its specific use cases. These bitwise flags of course are defined as semantic constants. Here are some examples taken from the Win API:
So it’s all about being able to control from a high level what the meaning of the dialog box should be for a given use case.
There are other aspects to consider as well, such as the proper use of the ‘caption bar’ titles. In the example shown in comment #20, this area contains the primary prompt which asks the question “Are you sure you want to delete testing?” The Windows (and other OS) convention is to use this space to identify something higher level through a title such as ‘Confirmation Required’ or ‘Warning’ etc. Keep in mind also that many users will not be expecting to find the primary prompt in that location.
There are other factors to consider such as the optional inclusion of a graphic component (e.g. an exclamation mark for warning dialogs). These can also be determined by flags or perhaps optional arguments to the builder function. Use of these icons can help the user rapidly understand what task they are to carry out in the modal.
More significant in the evolution of the Drupal user experience are the use cases where more complex tasks can be carried out through a modal form. There are three key UX benefits from this strategy:
If we could achieve these benefits throughout Drupal we would be light years ahead of the current user experience. For now, it is probably practical to shoot for a fully functional confirmation dialog API. In the next round maybe we can tackle the more complex sub tasks.
Comment #30
Jelle_SShould be a '.' at the end, I think.
Todo
Todo
Should be a '.' at the end, I think.
Punctuation
No '.' at the end of @see statements.
Double space between $type and ==.
use drupal_add_library('system', 'drupal.ajax');
Also, this feels strange to mee. Looks like this could be a library (defined in hook_library_info) on its own.
Same here, if it's a library, you can call drupal_add_library('system', 'drupal.modal'); or something similar.
Todo & commented out code.
Not 100% sure if it's a coding standard or personal preference... I'd have to check. But I always put a comma after every element in an array (including the last one).
Edit: seems like it is a coding standard after all.
Sorry for nitpicking :-)
Comment #31
Jelle_SForgot the status.
Comment #32
MustangGB CreditAttribution: MustangGB commented#5: Just noting that jquery ui 1.9 has landed, but the accessibly fixes were not included.
Comment #33
scott.gonzalez CreditAttribution: scott.gonzalez commentedCorrect; #5 was either a miscommunication or a typo. After talking to nod_, I asked my team if they were ok with splitting the 1.10 release into 1.10 and 1.11 specifically so that dialog would be released in time for Drupal 8. That day we shuffled stuff around and came up with our new roadmap. Jörn Zaefferer, the other development lead for jQuery UI, is actually at my house right now (visiting from Germany) and we'll be focusing on dialog all week. The goal is to have a beta with all of the dialog changes available before your feature freeze and a stable release before your code freeze.
Comment #34
mgiffordThanks Scott. This is encouraging.
Comment #35
larowlanhoping to get back to this once I've finished with #731724: Convert comment settings into a field to make them work with CMI and non-node entities which is getting close...
Comment #36
dasjoi'd like to link this video of the newly created Drupal SiteBuilding Usability initiative:
Comprehensive Modal Dialogs System in Drupal
video link: http://youtu.be/lOXiqYu50pE
announcement link: http://groups.drupal.org/sbui-announcement
Comment #37
larowlanWe have been asked to review http://view.jqueryui.com/dialog/tests/visual/dialog/form.html by the jquery.ui team they've continued to work on a11y issues and have https://github.com/jquery/jquery-ui/pull/794/files ready to go - just needs testers.
Comment #38
larowlanRe-roll against latest head, just seeing how tests go, no interdiff - only merges from 8.x branch
Comment #39
nod_At BADCamp working on it for a couple hours.
First, it works, awesome :D
But it's too mushed together. We need to have :
The actual JS looks very much like the ctools code, and it's not really good, jQuery ui has a position utility that could help us out here.
I'll work on it for a while and send an updated patch.
Comment #40
nod_Still a WIP but to show a simpler approach for this.
There is no setForm/dismissForm thing anymore. There is just 2 commands to open and close a modal. The content of the modal is handled by the existing AJAX insert command (since that's what we want it to do).
The replacement of the callback in node.module makes me think we'd want to change the drupal_get_form method to make it possible to have any form in a modal (or at least all form will give you back ajax commands).
A few things might be missing from the previous patch, let me know what and I'll be happy to put it back.
Haven't looked to much into the PHP, might need some simplification. Since I changed the way it works there are some naming inconsistencies introduced.
Comment #41
larowlango bot go!
Comment #43
larowlannod_ patch doesn't apply, missing file core/misc/ajax.modal.js - please ping me on irc so we can coordinate efforts, I've a branch in my sandbox for this and can give you commit access. I'm working on porting the views code in views/include/ajax.inc to the generic api.
Comment #44
nod_Updated. Needs the patch in #1833678: Ajax is messing with overlay to work properly with the overlay.
Comment #45
larowlanThis one adds image style effects using modals.
Screencast here: http://www.youtube.com/watch?v=VySgJjlkutU&feature=youtu.be
Comment #46
quicksketchOoooOOOo. Nice @larowlan. I like how the use of modals in image styles actually increases consistency. You're right that some actions (i.e. desaturate) do not have settings. The use of modals to add other actions without a redirect is a nice pairing that keeps the user on the page all the time.
That said, maybe integrations like this should be done a-la-carte in separate issues? The image action modal could be cleaned up by styling the interior forms to take up less vertical space (thus fitting on the screen more easily). Considering modals are using iframes and this viewport is passed forward to the child page, we should even be able to add CSS media queries to make the interior forms have more appropriate layouts when the modal is displayed at different sizes, such as on a mobile device.
Similar to that, it looks like the modal pushes down the page but the underlay doesn't stretch with it. Could we make sure that overlay scrolls with the page, or (probably better) position the modal in such a way that it will use a higher position on the screen if needed? Ideally, the modal would attempt to use more vertical space if it were available, up to a certain limit. For example if you're on a mobile device, the modal would likely fill the whole screen. On screens larger than 768 wide (or thereabouts), I'd expect the modal to be centered to fit, up to 80% of the page height, then scroll the modal (not the window) when needed, a la Views. Then the underlayment isn't a problem because you're never scrolling the page.
Accessibility-wise, last I heard is that we're okay with putting in modals now and working under the assumption that jQuery UI 1.10 will fix the remaining issues before D8 is released. Is that still the case that we can push this forward and assume accessibility will be worked out before release?
Great work so far! Generic modals open a huge door of possibility for us.
Comment #47
larowlanand I fooed up with a dsm() in the patch, same as 45 less that.
@quicksketch the plan is to get this in then decide on places where it should be used.
Comment #48
tstoecklerAwesome!!!
Comment #49
scott.gonzalez CreditAttribution: scott.gonzalez commentedjQuery UI 1.10 will use
position: fixed; height: 100%; width: 100%
for the overlay, so whatever is causing that issue should no longer be a problem when you upgrade. Hooray for no longer supporting IE6.Comment #50
quicksketchI'm an idiot. These aren't iFrames at all (at least in current implementations). Crazy! I'll try actual code reviews before I post more feedback.
Comment #51
quicksketch.
Comment #53
sunGlad to see progress here.
I think this should be renamed to
ajax_render_modal_form()
.It's not clear to me why the first condition checks for 'executed' but the second checks for 'submitted'. Normally, all code external to Form API should check for 'executed' only.
Just the last value without an array performs exactly the same thing in a much simpler way. :)
I'm relatively sure that this is not the right approach. I'll have to dive a bit deeper into the code to verify this.
This somewhat confirms my suspicion — most of the properties being specified here should be considered internal to Form API.
Comment #54
Bojhan CreditAttribution: Bojhan commentedI would like us to separate the technical implementation from where we apply this pattern. As suggested by quicksketch, and like we did with dropbutton. Confirmation forms, should be the primary place where we do this.
I noticed most of the styling is still pretty off, something that needs work but could also be split off.
Comment #55
larowlanSounds good
This is largely borrowed from views, might need to ask @timplunkett et al about why they did this
Yes, but there is some specific stuff views needs here which I've removed from the generic implementation, the plan being to port views to use this instead of it's own - so it will need these additional suggestions here to know when to react.
Happy to know if this can be handled cleaner so we don't need something similar for every implementation. Basically the code as it stands is an ajax equivalent of drupal_get_form, perhaps we need another util function in the middle.
@bohjan, so I'm assuming you want to see the patch at #44 with sun's suggestions from #53 and a meta for decide where to use modals with the image formatter code attached with a .do-not-test so others can see how to implement it?
Please let me know
Lee
Comment #56
Bojhan CreditAttribution: Bojhan commented@larowlan Yes, I would remove all cases where you use this except the "Are you sure" forms. We are 100% sure we want to use them there.
Comment #57
larowlanSo this patch
From here I see two follow ups
What do we think?
Comment #59
yched CreditAttribution: yched commentedTagging JS too
Comment #60
larowlanYep, that wasn't going to work.
More cleanup of putting the $form_state logic out of arms reach.
Comment #62
larowlandamn wrong branch, interdiff above is right, patch should have been this one:
Comment #63
larowlanComment #64
larowlanok, green again
follow-ups as per #57
what else needs to be done?
Comment #65
quicksketchGreat work @larowlan! I'm really looking forward to this excellent feature. I'm hinging on it for modal dialogs for WYSIWYG image and link editing. Though a different use-case, this still looks like it's going to provide some needed infrastructure.
Okay here come the nitpicks (sorry!):
give => given
drupal => Drupal
drupal_goto => drupal_goto()
non javascript => non-JavaScript
Document the hook.
non-javascript => non-JavaScript
Non javascript => Non-JavaScript.
behaviour => behavior (Damn Americans! ;)
hook_menu () => hook_menu()
This makes me want to use a more familiar function name that is easier to find. If the function were named "drupal_get_form_modal" then it would be easy to find via the api.drupal.org site and remember, because I just need to add the word "modal" to the function I would usually call. However this makes ajax.inc not have its nice little "every function starts with ajax_" pattern. IMO, DX is more important here than making our .inc file have 100% consistent prefixes.
Comment #66
larowlanthanks for the review @quicksketch, I agree with you on the name of the function
Comment #67
Fabianx CreditAttribution: Fabianx commentedLooks really good.
Comment #68
larowlanaddresses issues identified by @quicksketch
Comment #69
larowlanOk, so green again, @quicksketch and @sun's feedback resolved.
Follow-ups created as per instruction from @bojhan
#1842040: [meta] Decide on where to use modal dialogs
#1842036: [META] Convert all confirm forms to use modal dialog
What's next? I think this is very close, plus as it's a major task - getting this in helps with thresholds :)
Comment #70
yoroy CreditAttribution: yoroy commentedWith reviews from sun and quicksketch & bojhan's UX guidance in the bag I think the current state deserves an rtbc. You know, that version of rtbc where we leave it for a couple of days for any final reviews and nitpicks before actually committing :-) Great work all.
Comment #71
sunSorry, but I only reviewed this once and never signed it off.
The code looks slightly better now, but still not ready from an architectural and DX perspective.
All of these should not exist.
90% of the extremely complex logic in this function are undocumented. It will take me some time to study it.
For now, all I can say is that the amount of custom processing going on there indicates that something is architecturally wrong.
This still looks too much for me.
We might need to postpone this on #1238484: Ability to mark the form buttons to be of a specific type (so that they can be styled differently) or find another, similar, but ultimately much more automated way to indicate that a certain form button or link should open a modal.
Why is this a separate callback?
Why is this needed?
Comment #72
larowlanThese are only here so we can use this api with views, views ui does extra stuff with it's modals with titles and highlighting sections. So the plan being to port views ui modals to use this api. Without these we have two near identical systems. (see views/include/ajax.inc)
We have to pass the form_state['values']['tid'] as an argument to the forum_confirm_delete form builder.
Because drupal_build_form will set #action to the current url, which because this is ajax is system/ajax, so the modal ends up with
without this.
Please advise, happy to discuss on irc if needed.
Comment #73
Fabianx CreditAttribution: Fabianx commentedWe can maybe learn something from ctools_automodal for how to trigger things. Especially the github fork() has an interesting approach.
Comment #74
effulgentsia CreditAttribution: effulgentsia commentedGreat to see such progress here.
IMO, what's wrong is that we have a ajax_command_set_modal() command that is different from a ajax_command_insert() command. This requires the PHP code to care whether something is to be shown in a modal vs. any other kind of AJAX insertion. Whereas it seems to me that this could be made entirely client-side logic, since all Drupal.ajax.prototype.commands.modalOpen() does is run some JS code and invoke the insert command anyway, on the #drupal-modal wrapper. I'm guessing we can instead make a client-side 'modal' behavior that can be applied to links and buttons and that can wrap the Drupal.ajax invocation with the equivalent stuff, allowing the PHP code to not have to do anything different for a normal AJAX response vs. a "AJAX to be shown in a modal" response.
Meanwhile, for normal AJAX responses, we already have Drupal\Core\EventSubscriber\ViewSubscriber::onAjax() handling them. Quite possibly, we haven't covered the "return an entire form" as an AJAX response use case yet, and that some of the code in this patch is required to handle that, but it would be nice to move those fixes directly into the Form API or the ViewSubscriber::onAjax() pipeline, and not be so tied up with modals.
If we do that, we should be able to remove all the stuff mentioned in #71.
The above refactoring might take some time to hash out though, as it involves the intersection of JavaScript, Form API, and Drupal's server-side AJAX system. And to complicate matters, the server-side AJAX system is being significantly refactored in #1812866: Rebuild the server side AJAX API, and the client-side portion is being discussed in #1533366: Simplify and optimize Drupal.ajax() instantiation and implementation. Meanwhile, though, basic modal functionality is wanted for WYSIWYG and Layout UI work before feature freeze.
So, I wonder if we can add a few todos to #68, but otherwise expedite it, and then work on cleaning up the PHP side in follow ups. I can help write some of those todos if sun and others here agree to that suggestion.
Comment #75
sunThat's a very far stretch to ask for. I assume it is caused by feature freeze. What constitutes a feature is:
Post feature-freeze, that's extended by:
In light of that:
What I'm currently missing is a clear definition of "the feature"; i.e., what is explicitly supposed to be supported and what is not. I.e., when, exactly, are modals supported, what is the user interface pattern for that, where is it documented, and perhaps most importantly, which (possible) usage of modals is explicitly not supported?
Furthermore, as I already alluded to in my review above, the feature being introduced here ought to be much easier for module developers.
So in terms of @todos and a (borderline) "acceptable" initial feature, I'd much rather expect hefty hacks and ugly workarounds within Form API and Ajax API internals, along with corresponding @todos, but in completely different locations, which can then be cleaned up and resolved post feature freeze.
And implementation-wise, I'd expect literally nothing more than this:
In other words, circling back into what I just mentioned above:
I don't care at all how Views or whichever contrib module implemented this for D7. Product features need to be designed in a proper way; i.e., defining how users (here: developers) are going to benefit from a feature and how they are going to use it. If a feature is too complex to use, it just simply won't get used, or it won't be used properly. Consequently, completely failing on its goal.
This is further manifested by the fact that modal dialogs imply a user interface pattern, and user interface patterns must be used in a consistent fashion, so as to not horribly confuse actual end-users. Thus, if this is too complex to implement, only fraction of modules will do it, leaving end-users behind with a very inconsistent user interface. In that case, the end result would actually be worse compared to what we have today, so I'd strongly object to that.
I hope this provides sufficient reasoning and a clear path forward.
Comment #76
Bojhan CreditAttribution: Bojhan commentedCan we get some actual todo's? This seems to be going down the exact same route as all the other modal issues, because people are keen on re architecting everything every time for each UI issue. @larowlan addressed some of @sun's concerns, and sun mentioned we need to add todo's in the accurate places - so lets do that, and then push this in.
@sun Your comment on features, is completely unrelated to this issue. Yes it is a problem, that our whole "code freeze" is largely undefined. But lets not try to solve that here. Also I want to keep "how to use it" discussion out of this issue, as it totally side-tracks correct implementation. We will similar to dorpbutton open an appropriate followup.
Comment #77
rvilarI want to help on #1842036: [META] Convert all confirm forms to use modal dialog, but I see that I can't start on that until this issue is closed. Can I help on this?
Comment #78
falcon03 CreditAttribution: falcon03 commented@sun#75: I can understand your important concerns. But we must strongly keep in mind that we need modals in core to enhance views and other features' accessibility.
If feature freeze can block this issue, can we ask for an exception to core maintainers to let us work on this issue until code freeze?
Comment #79
effulgentsia CreditAttribution: effulgentsia commentedI'm gonna work on cleaning up the PHP part of this patch today.
Comment #80
effulgentsia CreditAttribution: effulgentsia commentedI got distracted by some other issues, so apologies for not getting to this yet. Will try to this weekend.
Comment #81
Dries CreditAttribution: Dries commentedSorry, but I only reviewed this once and never signed it off.
@sun: I've noticed a trend in your comments across different issues, and it is time for me to speak up about it. You appear to increasingly bash ideas or people. I love your passion for Drupal and respect your technical depth, but your approach and communication style turns people off and doesn't foster a collaborative culture. The truth is people don't need formal sign off from you. More importantly, you'll want to change your style and create an environment were people look forward to a "sun review" (remember that time?) versus creating an environment where people loath a sun review because it makes them feel like crap. Good leaders don't make other people feel like crap. It's not right. You can't be proud of yourself if you do that. Good leaders respect and motiviate people, build consensus, and empower them to shine. Again, I love your passion and your technical depth, but I really need you to be a better leader here. Thanks!
Comment #82
larowlan@sun, apologies if I've misinterpreted you're comments here.
Given that we were down to nitpicks at #65 I genuinely thought that I'd addressed your concerns and this was close to ready.
Clearly this was not the case.
I too respect your technical knowledge and passion for Drupal, a passion I share.
Given #1812866: Rebuild the server side AJAX API completely refactors the way ajax responses are built, there is some significant re-working of this patch required as is, so at the same time I feel it would be an ideal opportunity to address your concerns.
Can you articulate further what you consider to be the way forward here, as it's not completely clear to me. Are you proposing building this functionality into the form api at the ground level via process callbacks to attach required logic/libraries and content-type handling logic in drupal_build_form? I don't really see any other way to implement the functionality you've described (eg the #modal => TRUE) in a generic way without such low-level integration. But that said, I could be missing something obvious. Please let me know.
As well as this being a killer feature for Drupal 8, I've invested a fair bit of time in it and hence have a vested interest in seeing it make the cut, in whatever form that may take.
Thanks
Lee
Comment #83
effulgentsia CreditAttribution: effulgentsia commentedOk, here's a minimal solution. I replaced the forum.module use case with the Delete button on the node edit form, because forum_form_main() is a mess that needs to be refactored to use Form API correctly.
There are some problems with modal submissions busting the base page out of the Overlay, but I think that can be fixed by someone with better JS skills than I have.
@larowlan: the basic architectural difference with your patch is that this allows modal submissions to happen to the same URL as if it weren't in a modal. In D8, we have proper content negotiation, so some of the D6 and D7 CTools workarounds related to submitting to a different URL aren't needed, and that simplifies things a lot.
However, this patch does have a @todo related to content negotiation not working on redirects from IframeUpload submissions.
@larowlan: since this was assigned to you before #79, I'm assigning it back to you, in case you're using assignment to track your issues. I'm happy to continue helping on this though until we get it in.
Comment #84
effulgentsia CreditAttribution: effulgentsia commentedBy the way, turns out it doesn't. That patch added some nice new classes, but didn't change Drupal to use them yet. There are follow up issues listed on that issue to do so.
Comment #86
effulgentsia CreditAttribution: effulgentsia commentedIn sun's defense, the first sentence of #71 was a direct response to him being used as one of the factors in #70's RTBC. However, I still agree with Dries's feedback in general.
I just want to attest to this not being taken lightly. I alone have received hundreds of reviews from sun over the past 3 years, and have learned a ton from many of them. For Drupal to succeed, we need contributors continually improving their skills, and reviews from sun have done this for me and many other contributors. But, sun, that doesn't happen when the tone is too antagonistic, condescending, or controlling. It's not always easy to walk the line between pointing out problems constructively vs. destructively, and to be honest, I don't even know if #71 crossed it, but some of your comments on other issues have. My hunch is that it's a symptom of burnout. If I'm right, then, please, please, take a little time to unwind and take care of yourself. Drupal will survive while you're resting, and be better off in the long term if you get the rest you need.
Comment #87
larowlanThanks @effulgentsia, (I think) this clarifies what @sun was looking for above and confirms my speculation in #82 was closer to the mark than I thought.
In @sun's defence, I don't think a line was crossed at #71, however I think perhaps the tone at #75 could have been more constructive; I can't speak to other issues.
Along with @effulgentsia I have learnt a lot from @sun's reviews, both of my work and of others and Drupal is definitely the better for it.
In fact, I was stoked when @sun chimed in at #53 and (I felt) the changes required were minimal, it gave me confidence that this was close to being accepted.
I think everyone is feeling the pressure at the moment given the looming feature freeze and I hope we have a way forward with this issue.
Thanks again
Comment #88
nod_Updated patch, there was a little problem in the $.extend of the modal file, wasn't extending the commands object. Overlay doesn't close when clicking the dismiss link but still closes when the form is submitted. Apparently the behaviors on the child page aren't triggered properly.
Haven't fixed the PHP warning either.
Comment #89
effulgentsia CreditAttribution: effulgentsia commentedThis fixes the PHP warning. I like the JS cleanup in #88 except:
Per #74, I had actually intentionally changed that in #83 to extend Drupal.ajax, not Drupal.ajax.prototype.commands. I don't consider modalOpen to be a command for PHP to send back. PHP should only send back an insert command, and the JS code decides under what conditions to open a modal. So, can we change this back to not be a command? I didn't do so in this patch, cause wanting nod_'s feedback on that first.
Comment #90
nod_Oh ok, sorry, didn't get it.
If that's the case, better make a
Drupal.modal
object and not tie it to the Ajax framework at all. I changed it back since the patch didn't work for me. On thing we should discuss then is if we allow several dialogs open on the page? (since dialogs don't have to be modals, can just be popins).I'll mess around with that for a bit and I'll update the patch after dinner.
Comment #91
xjm#1806308: Review Views JavaScript + generic modals for accessibility is a critical bug that is probably resolved by this, so escalating the priority of this issue. (The other may be a duplicate, but it is postponed until this is resolved, one way or the other.)
Comment #92
mgifford@xjm I was trying to see how it resolves the accessibility bug or who did the testing for that. Can you give me a bit more information on this.
Not sure how it's tied, but would love to get it fixed. Modals aren't easy.
Comment #93
larowlanHi Mike
This doesn't solve issues with views with the current patch (no views ported in this issue), but could.
Nor does it solve accessibility in general until jquery.ui 1.10 is released, which are the github pull requests you and falcon03 have been testing.
That's where the accessibility review is taking place, but in a general nature - not specifically of views modals.
Hope that helps.
Lee
Comment #94
mgiffordThanks for the clarification Lee.
Comment #95
nod_All right after my 6 day dinner updated patch.
Dialog thing not dependent on Ajax commands. Works well. I pretty much implemented http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.htm... saving the position stuff.
Renamed modal-dismiss by modal-cancel. No reason to introduce different terms here. to use the dialog API:
There are 4 events fired on window to allow contrib to change settings, DOMElement or whatever. The ajax file is using a couple of events to bind the cancel link closing so that we keep that out of the modal.js file.
Comment #96
nod_Comment #97
nod_modal.js
should be nameddialog.js
really, updated patch.Comment #98
jessebeach CreditAttribution: jessebeach commentedThe code in #97 looks good. I applied it, read through it and understood how I would use a modal in other contexts.
This issue has a lot of history. Based on #97 I would RTBC. If someone else feels so as well, let's do it.
Comment #99
sunThe latest patch looks way better and achieves a deep integration into the various subsystems. I'm not even able to spot any major subsystem hacks in it, so it's even better that we don't even need any workarounds to make this possible.
Overall, I'm not sure whether the processing/handling of #ajax][modal] shouldn't be converted into an Ajax command, but that's the exact kind of internal refactoring that we can happily perform post feature freeze, so there's no reason to discuss this detail in this issue and hold up this feature for it.
The essential feature and API that we're adding to D8 is this:
Whereas the expectation is that this works for any form button, and also, for any kind of link (though using a different syntax for a link that goes through
theme_links()
.)There do not seem to be any new tests to verify the expected behavior and functionality, but we generally lack tests on the Ajax front, so I guess that confirmations based on manual testing will be sufficient for this issue. We can investigate whether it is possible to add more sophisticated/automated test coverage in a separate follow-up issue.
Thus, we're down to final reviews. Here's mine:
Can we clarify why we're unconditionally adding a page title here? (in code)
Can we clarify why the path should only default to 'system/ajax' when there is a callback? (and not always, like previously?)
Wouldn't it make sense to just check for $link['ajax'] and conditionally execute this instead?
The naming of classes, behaviors, libraries, dependencies, and functions appears suboptimal to me.
Wouldn't it make sense to
1) rename all instances of "modal" into "dialog"
2) rename all instances of "drupal[.-]modal" into "ajax[.-]dialog"
?
Ideally, I'd think we should try to stay as close to jQuery Dialog as possible — while we're implementing a modal dialog here, I can't see a reason for why there should not be a direct follow-up issue to this that asks for non-modal dialog support. Thus, by renaming things to "dialog", we keep all doors wide open to that. A modal is really just a subset of a dialog, and that's a semantic that most dialog libraries (including jQuery Dialog) follow and implement very well.
That said, the HTML ID #drupal-modal should probably be just #ajax-modal and not #ajax-dialog, since there can only be a single modal by design.
--
Lastly, I think that some of the earlier comments were unfair, off-topic, and inappropriate. I fundamentally disagree with some of them. But that's off-topic for this issue. Let's make sure to get this feature into core instead. It looks like we're close. :)
Comment #100
effulgentsia CreditAttribution: effulgentsia commentedI'm working on implementing sun's feedback.
Comment #101
effulgentsia CreditAttribution: effulgentsia commentedThis addresses #99, and in the process moves some functions from ajax.js to dialog.js and changes indentation in dialog.js, so the interdiff appears bulkier than it really is.
It would need to be drupal_pre_render_link() in order to get the id attribute in there, but I fail to see how that makes more sense than calling drupal_render(). I actually think we should make it always call drupal_render(), and not have the
else
part, but we first need to fix Views to not set 'ajax' on links with different semantics. There's a todo comment about that already in the patch.Comment #102
nod_Not having any behavior or event listener in
dialog.js
was on purpose. I want to be able to usedialog.js
without any assumption, it doesn't need to create adrupal-modal
element to work, the ajax framework does. That's not a biggie, I'll move them to their own file later on.Comment #103
effulgentsia CreditAttribution: effulgentsia commentedOwn file would be fine, but no need for it to be in ajax.js. Non-AJAX use cases of #drupal-modal and .dialog-cancel are also possible (e.g., wysiwyg).
Comment #104
effulgentsia CreditAttribution: effulgentsia commentedI thought I was being clever here, but I subsequently realized that this violates nod_'s intent of sticking to the API of http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.htm.... This corrects that.
Comment #105
Bojhan CreditAttribution: Bojhan commentedIt looks like @sun his concerns are addressed and it was his final review - it looks like we are in good shape for getting this into core. Given that its a huge change, which always lead to follow ups and we need to get this major thing in this week. I am going to go ahead and mark it RTBC.
For A11y followers, this does not actually resolve the accessibility bug of the modal. The jQuery UI team is working intensely on that, and we expect to update to that version as soon as its released - see https://github.com/jquery/jquery-ui/pull/794. Although its not common practive to commit before resolving a bug like this, we are tied to code freeze and this is the third release we try to get a modal into core and its a great intimidate step till jQuery UI updates are released.
Comment #106
catchJust a note that since this blocks a critical bug it's not necessarily blocked by feature freeze at all.
I'm not sure why the node delete is included in the patch, is that just to have an implementation for testing? If so can we drop it from the patch and add it in a new issue? It seems like it'd be either worth doing for all entities or not at all, and I'm not sure about introducing that new interface pattern along with the rest of the patch.
Comment #107
Bojhan CreditAttribution: Bojhan commented@catch We can drop it from the patch.
Comment #108
jessebeach CreditAttribution: jessebeach commentedI pulled out the node delete operation code and put that in its own file for posterity (core-js-modal-1667742-node-delete-modal-example-do-not-test.txt).
I agree with nod_ that dialog.js should be void of implementation code (the attached events), but looking at it, I can't figure the appropriate place to move the event attachments to right now. I'm reluctantly willing to let it go as something we can improve later since no better options present.
I agree with sun that tests are lacking. This is debt we knowingly take on as part of the test writing work that looms on our nearing horizon for most front end code.
Comment #109
jessebeach CreditAttribution: jessebeach commentedSetting to needs review to get this into people's queues. Should be set back to RTBC once the small change to remove the node delete operation code is verified and accepted.
Comment #110
effulgentsia CreditAttribution: effulgentsia commentedRipped out one other hunk missed in #108, and added demonstration use cases to ajax_test.module. I also added a starter DialogTest, though it's thin at the moment, because all the interesting stuff happens client-side.
Comment #111
effulgentsia CreditAttribution: effulgentsia commentedBack to RTBC. I think catch is a sufficient reviewer for the interdiffs in 108 and 110.
Comment #112
sunThis looks great now.
I'm not sure I understand why this is not perceived as a feature, but anyway, would be great to get this in before feature freeze.
Thanks all!
Comment #113
nod_Views modals are not accessible, this will in time replace all views modal and is aimed at being accessible, hence not really a feature. But anyway it's ready now so no pressure :)
Comment #114
jessebeach CreditAttribution: jessebeach commentedHere's the followup to convert confirm forms to modals, just to get it at the bottom of the issue:
#1842036: [META] Convert all confirm forms to use modal dialog
Comment #115
webchickYeah, this feels a little borderline between feature/bug (a "fug"? ;)) but the accessibility work done here is important for resolving critical Views accessibility problems, and is also coming up over and over as a fundamental UX element in various other major/critical UX patches (CMI UI, Entity Translation UI, etc.). Looks like it's been pretty well picked over by several smart folks, so that's great.
I'm not sure if all of the use cases outlined by User Advocate are covered here... looks like there's an option to show / show as a modal as well as an arbitrary class that can be added to any form button to enact "cancel" type behaviour, so that's a good start. We might want to expand this patch in a follow-up to create some nice wrapper functions / properties / something for creating standardized "OK" / "Yes / No" / "OK / Cancel" type of dialogs so that we don't end up with inconsistencies in the way various modules implement these patterns. But most likely, the easiest way to spot these will be to convert various forms to use it and such patterns will likely naturally emerge as a result.
Committed and pushed to 8.x. Great work on this, folks! :D
Let's get a change notice with some instructions on how people can use this in their modules.
Comment #116
quicksketchWoohoo! Yay great work all.
Comment #117
xjmFiled #1851414: Convert Views to use the abstracted dialog modal.
Comment #118
MustangGB CreditAttribution: MustangGB commentedHas this been implemented anywhere yet?
Comment #119
nod_nope.
Change notice, I covered the JS part, someone up for the PHP side? https://drupal.org/node/1852224
Comment #120
nod_Comment #121
effulgentsia CreditAttribution: effulgentsia commentedI edited the JS change notice and added a #ajax one.
Comment #122
nod_thanks! Looks good to me, may want a confirmation that there is enough information to understand from someone else though.
Comment #123
tstoecklerThe change notice looks good. With its help I just tried manually replacing the node delete button by a modal link. Awesome!!!
One thing that might be worth mentioning is what the 'modal' => TRUE/FALSE actually does, specifically what the difference is if you *don't* set 'modal' to TRUE. (I would have edited myself, but I don't actually know the answert to that question.) That is by no means critical, though so leaving open, but marking normal for now. Should be back to 'critical' when it's marked fixed.
Comment #124
nod_Some clean-up in #1863478: Split Dialog API and optional integration helpers into separate files.
Comment #125
Dean Reilly CreditAttribution: Dean Reilly commentedHow would I go about setting the size of a dialog window? I'm currently working on adding a diff to the configuration synchronization page #1821548: Add a "diff" of some kind to the CMI UI and noticed the default dimensions are too small (see the attached screen shot).
Comment #126
jibranComment #127
jibranCreated a follow up issue for this @todo #1884530: Complete modal dialog tests.
Comment #128
webchickDear 71 people following this issue, #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases is apparently blocking anything in core from actually using this nice new dialog API, so if you could take a look, that'd be swell. :)
Also, would someone please be able to give a final review of the change notice so we can mark this puppy fixed?
Comment #129
larowlanChange notice from #121 looks good to me
Comment #131
jibranTagging.
Comment #132
andypostDoes this code could be possible to backport to D7?
Comment #133
klonosThe way I see it from reading the "API changes" section of the issue summary, there has been API addition - not an actual change (as in "alteration") per se. I think if that's true and we don't break things, we should consider backporting.
Comment #134
effulgentsia CreditAttribution: effulgentsia commented#1870764: Add an ajax command which makes it easy to use the dialog API in complex cases changed the implementation of this issue considerably, so once a change notice is written there, let's use that issue to discuss/do backporting.
Comment #135
effulgentsia CreditAttribution: effulgentsia commentedActually, one thing that might be worth discussing here though is just a question of whether a backport is even on the table, considering that D7 uses jQuery UI 1.8, and we need jQuery UI 1.10 for the dialog to be accessible.
Comment #136
webchickI don't understand how a D7 backport that includes an upgrade of jQuery UI to 1.10 could possibly be on the table? We know jQuery UI 1.8 => 1.10 broke at least Edit module in core.
Comment #137
scott.gonzalez CreditAttribution: scott.gonzalez commentedFor the D7 backport, dialog would be the least of your concerns. 1.10 removed a lot of 1.8 APIs: http://jqueryui.com/upgrade-guide/1.10/ Backporting would not be a good idea.
Comment #138
effulgentsia CreditAttribution: effulgentsia commentedRight. So the question is: is a backport of the dialog API on the table, if we don't use it in D7 core, but just provide the API for contrib modules to use. Then contrib modules can then decide to either make jQuery Update a dependency, or accept a dialog with accessibility problems.
My gut feeling at this time is that we should not do that, and leave it to http://drupal.org/project/dialog or some other contrib module to backport what's been done for D8.
Comment #139
quicksketchIncluding it in CTools is probably the best bet. Here's the issue for switching CTools to jQuery UI: #1397370: Make modal.js use jQuery dialog.
Comment #140
effulgentsia CreditAttribution: effulgentsia commentedGiven #137 and #139, restoring issue attributes to pre #135. If someone feels strongly about this being backported to core, you can reopen.
Comment #140.0
effulgentsia CreditAttribution: effulgentsia commentedsummary update
Comment #141
jason.fisher CreditAttribution: jason.fisher commentedFYI, I have updated the CTools JQuery Dialog patch here against latest 7.x-1.x-dev release: https://drupal.org/node/1397370