Quit clobbering people's work when they click the filter tips link

This is really a combination of a bug report and feature request, I think.

There is more discussion of the data loss bug at #153313: ckeditor input is lost when using the browser's back button. So I'm tempted to leave this one open just as the feature request... a popup or modal dialog or something seems like it would be more user-friendly.

One solution to the issue is available at #374646: Popbox (Popups Lite): Adding Modal Dialogs to Confirmations

Marked #394398: Using the help link is clearing previously entered data and #423734: More information about text formats loses text as duplicates of this issue.

The new help system might be used for this, since filter tips are help and the new system includes popup windows.

For now, how about we just do the quick fix and open the link in a new window as this issue's title requests.

It seems this same approach was proposed a long time ago at #59430: "More information about formatting options" must open in new window and marked "won't fix". Worth at least looking at the arguments there...

Not that Drupal is completely strict about this though. I checked, and currently install.php is already using a link with target=_blank.

I am tempted to mark this RTBC. Can we get another code review? And I think most arguments are pretty straight forward, but we simply have to fix this.

I'm opposed to using target=_blank as a "quick fix." You'll certainly have some folks screaming about not validating strict. I would prefer a js/window.open fix. I think we need some alternatives.

Spoke to webchick, basicly this solution is a no go! Sorry :(

Sorry, but as much as I really want this stupid bug fixed, we simply can't fix this with a one-off target="_blank" hack. I believe there was a means of displaying inline help in popups as part of the #299050: Help System - Master Patch patch. Possibly pulling that out and using it here would work.

ok, changed title

I started a drupal_dialog function in common.inc to use jQuery UI's dialog js and css and have this working for filter tips.

This problem has come up many times in training sessions and also in formal usability testing, IIRC. I'd assume that everyone is +1 to fixing with a modal. With jQuery UI in core, I'd also assume that we'd use its modal dialog library. Jody Lynn, I think your patch correctly moves in this direction.

Where things get tricky is the fact that multiple areas of core could be improved through the use of modal dialogs. What standards are to be used? Reviewing the jQuery UI demo page, you can see that there are many options to be set. For this use case, it could be helpful to allow the dialog box to be moved and re-sized and moved so that one could refer to it during content creation. If modals are used for confirmation messages (ex: node deletion), we'd want to grey out the rest of the screen and disallow resizing.

It would be helpful to have a comment from the D7UX team, Gabor or Webchick regarding the direction for implementing modal dialogs. Should there be a master patch that implements changes in multiple areas or should all the patches be separate? If patches are to be separate, should a usage guideline be created? Should there be a separate "enabler" patch that focuses on providing the helper function (drupal_dialog etc.) before individual implementations are made?

Here are a few related links/issues for background:

- http://hojtsy.hu/blog/2009-jun-17/two-alternatives-d7ux-overlay-implemen...
- #374646: Popbox (Popups Lite): Adding Modal Dialogs to Confirmations
- #193311: Ajax Popups in Drupal 7: Adding Modal Dialogs to Help, Confirmations and Filter tips (Unified)
- #218820: Popups: Adding the core modal dialog API

Yeah. it seems for a minimum drupal_dialog will need to accept settings for the dialog. I'll look more into the past issues as well.

Updated patch fixes the test in php module which failed due to the additional text displayed by the modal and cleans up filter_tips_long function which had no need for the arg() nonsense it was using.

Screenshot attached of the generic jquery ui dialog in effect from this patch.

Mmm... I like the screenshot of the modal dialog. Would this pattern be reusable elsewhere? From the patch code it looks like it would be, but I haven't actually tested the patch.

Modal dialogs might also be useful in some of the areas where people are considering #492834: Hover operations for flooded state screens, especially admin/content/node (see Dries' video in #30 of that issue).

Yeah, it adds a reusable drupal_dialog function you can call anywhere. It won't work in all cases because it uses static text for the modal, no AJAX.

So this is now the official patch to add modal dialog support to drupal 7?

Subscribe to see where this goes. A few notes on the patch so far:

- We can now get rid of the extensive drupal_add_js/css calls now and just say drupal_add_library('system', 'dialog'); which should add all the JS/CSS files needed.
- Rather than having a set of parameters such as $title and $content, I'd like to see this all just become $options instead, which map directly to the the options provided by jQuery UI's Dialog options. That way we'd have a much more generic use of this function.
- I think keeping $link_class would be a good thing, but it shouldn't be called "link_class". Maybe just something like "class", since it doesn't have to be a link that gets clicked. It's tempting to make this a full selector such as ".link-class" as the value rather than "link-class". We're getting so deep into jQuery I'm not sure CSS/jQuery selectors should scare people any more.

I'm having trouble with drupal_add_library because it seems to be ignoring all but the first usage (I want to use dialogs, draggable and droppable)

I started moving things around so now you specify two jquery selectors (the activator and the modal content selector). I need to implement the options properly still.

Ok this is working well now. You can define a new modal dialog by specifying the selector to be clicked and the selector that has the content, and an array of options.

I forgot to 'fakeadd' the new file.

Code looks good to me.

This page should really use a render() array but thats probably too much scope creep for this issue. We can do that after freeze.

The results are great, indeed, a very useful and valuable addition. However, the main concern I have now is about the hardcoded dialog dimensions.

How the dialogs dimensions/settings can be altered. Let's say for example, how a module/theme can alter settings of dialogs.

In this case, the dialog is being added inside a theme function, but in other cases, it might be added in menu callbacks, or other places, not necessarily inside theme functions.

Ok, I added a drupal_alter into the drupal_dialog function so you can alter dialogs.

Summary of this patch:
Adds a drupal_dialog function to allow creation of modal windows with settings.
Adds a corresponding misc/dialog.js that uses jQuery UI to implement the dialogs.
Changes the 'more filter tips' link to use drupal_dialog, which creates a draggable resizable modal window.

Context creep in this patch:
Fixes filter_tips_long function which had some unused cruft.
Fixes some unrelated simpletests which superficially break from this patch, due their testing for whether the word 'print' was on the page or not.

Forgot the fakeadd. again.

This patch is clean and does what it says.

I think it's RTBC, anyone care to mark it as such?

+1!

Nice dialog boxes! This does what it says and code looks good.

Soooo... I marked it as such. My bad, +1 anyone?

The big question is: how does this work with the overlay patch?

+++ includes/common.inc	30 Aug 2009 23:48:07 -0000
@@ -5188,3 +5188,34 @@ function xmlrpc($url) {
+ * @param $trigger_selector
+ *   A jquery selector which should open a dialog when clicked.
+ *     Example: 'a#modal-link'
+ * @param $content_selector
+ *   A jquery selector which contains the content for display in the dialog.
+ * @param $options
+ *   An array of dialog options keyed by option name.
+ *     See http://docs.jquery.com/UI/Dialog#options for available options.

Can you combine all this parameters into the $options parameter, and pass 'trigger_selector' and 'content_selector' in the array.

+++ includes/common.inc	30 Aug 2009 23:48:07 -0000
@@ -5188,3 +5188,34 @@ function xmlrpc($url) {
+function drupal_dialog($trigger_selector = '', $content_selector = '', $options = array()) {

We already have drupal_add_js(), drupal_add_library(), drupal_add_css(), drupal_add_html_head(), drupal_add_link(), drupal_add_tabledrag(), ..., etc.

Can you follow this common pattern an rename this function to drupal_add_dialog(), for consistency.

+++ misc/dialog.js	30 Aug 2009 23:48:07 -0000
@@ -0,0 +1,30 @@
+  attach: function (context, settings) {
+  for (var key in Drupal.settings.dialogs) {

(minor) Wrong indentation here.

+++ misc/dialog.js	30 Aug 2009 23:48:07 -0000
@@ -0,0 +1,30 @@
+      }).addClass('dialog-processed');
+    }

(minor) should go in the next line.

Doesn't comply to coding standards, improper indentation in dialog.js.

I'll test this with the overlay patch and make sure everything's copacetic.

But I think in terms of having both the overlay patch and this general simple modal implementation, that that makes since, because the overlay functionality is so much more complex and specific.

I lean towards not putting every parameter into the $options array- those are specifically jQuery UI options for the modal dialog, and sticking everything all together in one array seems more confusing to me.

Everything else I'll change and study the js coding standards.

I made dropcube's changes and hopefully that addressed cwgordon7's indentation concerns as well.

Still need to confirm this works well with overlay patch applied.

I reviewed this patch and made a couple of minor fixes to the code comments (attached). I think this is RTBC.

I also tried it with the overlay patch, and the answer to the question of how it works is "well enough". I was able to click on the filter tips link from within the overlay and get it to pop up on top of that (at least in Firefox) and look pretty much OK. If there are any details to worry about beyond that, I don't think it's the job of this patch to do that.

This is a nice usability improvement. Yet its integration into the render() model was a bit lacking. I've fixed it up. render elements may now add a dialog by setting #attached_dialogs. This goes on to call drupal_add_dialog() which was not changed at all.

I removed the more-info from a theme function since we don't do customizations like this via theme anymore. One can use the $conf['custom_strings'] feature of settings.php or hook_page_alter() to fiddle with the link.

If this patch fails testing, feel free to commit the prior one which is green right now.

those 4 exceptions have nothing to do with this patch AFAICT.

I thinks it's a better and more extensible approach integrated with render(). A few comments, though.

-function drupal_process_attached(array $libraries = array(), array $js = array(), array $css = array(), $weight = JS_DEFAULT, $dependency_check = FALSE) {
+function drupal_process_attached(array $libraries = array(), array $js = array(), array $css = array(), array $dialogs, $weight = JS_DEFAULT, $dependency_check = FALSE) {

There should be a way to 'attach' other stuff further, now it has the parameters fixed, and is not simple to attach other stuff dynamically.

+  
+  // Dialogs to the page.
+  foreach ($dialogs as $dialog) {
+    call_user_func_array('drupal_add_dialog', $dialog);
+  }
+  

The same, IMO, we should not do special case for dialogs, rather integrate it with the code we currently have:

call_user_func('drupal_add_' . $type, $data, $options);

+    // Allow modules to alter dialogs.
+    drupal_alter('dialog', $dialogs);

If the dialogs are attached to the render() page structure, we do NOT need this pecial hook any more. The structure can be altered with hook-page_alter() or other _alter hooks.

+    '#attached_dialog' => array(array(".filter-tips-modal", "#filter-tips-modal-dialog", array('width' => 600, 'height' => 500))),

- Sounds better in plural: '#attached_dialogs'
- All the parameters should be combined in one associative array, for example:

'#attached_dialogs' => array(array(
  'trigger_selector' => ".filter-tips-modal",
  'content_selector' => "#filter-tips-modal-dialog",
  'width' => 600, 
  'height' => 500,
));

This way, it's easier to read, and easier to alter the complete '#attached_dialogs' structure.

Working on a patch to implement these changes.

The following patch implements the changes commented above.

Ugh. bot says - failed to apply

I have created a separate issue to deal with the dynamic attaching to render() structures that that would be required here: #565496: Allow dynamic attaching of other types of stuff to render() structures .

@moshe: ugh, I had an outdated copy of HEAD, let's re-roll.

Updated patch against HEAD implementing the changes commented in #53 plus other fixes. Note that this patch includes for now the patch from #565496: Allow dynamic attaching of other types of stuff to render() structures, to allow attaching of stuff dynamically instead of doing a special case for dialogs.

Not cool, someone ban that spammer.

(Please?)

Updated patch, now using #attached.

Thats some nice rendering ... Please wait for green before commit.

I think this needs a usability review - to make sure it matches the overlay style somewhat. And not create a new style, that will confuse the user.

I am totally for committing this by the way, just want to make sure its done well - so we don't encounter issues down the road. Sadly I don't have a local machine here, but I will review later.

No worries about RTBC, this is a usability bug.

Tagging

Looks like wrong status.

Hmmmmm..... I don't want to block this because of overlays. http://drupal.org/node/517688 -- the issue looks troublesome, and it strikes me as funny to shelf a perfectly good usage of a dialog widget - as well as standard code to get them up in running (the first is it in drupal core?!) jQuery UI is such a slick library, and this particular usage is basically by the book: http://jqueryui.com/demos/dialog/#modal-confirmation (minus dropping an overlay).
The patch applied fine, as did playing with it. I've attached screenshots of how it looks. Will wait for 24 hours before i mess with webchick's mellow by marking RTBC

I think it's ready. If there are minor issues with style matching the overlays (it's using all default jQuery UI style now) that can be sorted out later.

I would like to remind everyone, that "now" is later.

I agree this is RTBC.

Note that in #49 above I tested a previous version of this patch with the overlay and it did not look horrible or create any big explosions or anything. I just tried it again with the latest versions now, and the same is true (although the overlay patch currently sets the z-index of the toolbar to a gigantic value, which causes the filter tips to appear underneath the toolbar, but that's a minor conflict that would be easy to resolve).

I don't see any reason to hold this patch up for another patch which does not seem like it's particularly close to being committed. Minor tweaks can be made if and when necessary.

I guess this can be considered RTBC, but it needs a re-roll to fix a few minor issues.

@sun: can you specify what issues should be fixed so that we can fix them ?

+++ modules/php/php.test	6 Sep 2009 20:23:24 -0000
@@ -56,7 +56,7 @@
     // Make sure that the PHP code shows up as text.
     $this->drupalGet('node/' . $node->nid);
-    $this->assertText('print', t('PHP code is displayed.'));
+    $this->assertText('print "SimpleTest PHP was executed!"', t('PHP code is displayed.'));
@@ -66,7 +66,7 @@
     // Make sure that the PHP code shows up as text.
-    $this->assertNoText('print', t('PHP code isn\'t displayed.'));
+    $this->assertNoText('print "SimpleTest PHP was executed!"', t('PHP code isn\'t displayed.'));

Introduced in #37.

Please remove these changes from this patch, unless they're strictly necessary.

+++ modules/filter/filter.module	6 Sep 2009 20:23:23 -0000
@@ -635,10 +632,18 @@
+    'trigger_selector' => '.filter-tips-modal', 
+++ misc/dialog.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,31 @@
+        $(dialog.content_selector).dialog({ 
+          autoOpen: false 

Trailing white-space here.

+++ modules/filter/filter.module	6 Sep 2009 20:23:23 -0000
@@ -635,10 +632,18 @@
+  $form['format_help'] = array_merge($form['format_help'], filter_tips_more_info());
@@ -736,12 +741,16 @@
+function filter_tips_more_info() {

Can we remove this function and just add those elements to the form?

+++ modules/filter/filter.module	6 Sep 2009 20:23:23 -0000
@@ -736,12 +741,16 @@
+    '#prefix' => '<div id="filter-tips-modal-dialog" class="ui-helper-hidden" title="' . t('Filter Tips') . '">',

"Filter tips" (lower-case t)

+++ includes/common.inc	6 Sep 2009 20:23:15 -0000
@@ -5478,3 +5478,40 @@
+ *   - 'trigger_selector'
+ *     A jQuery selector which should open a dialog when clicked.
+ *     Example: 'a#modal-link'.
+ *   - 'content_selector'
+ *     A jQuery selector which contains the content for display in the dialog.
+ *   - 'options'
+ *     An array of dialog options keyed by option name.
+ *     See @link http://docs.jquery.com/UI/Dialog#options for available
+ *     options.

The key description should follow after a colon + space (: ) right after the key name.

Indentation is correct here, though.

The proper syntax for @link is

@link [url] [title] @endlink

+++ misc/dialog.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,31 @@
+Drupal.behaviors.dialog = {
+  attach: function (context, settings) {

There should be a sanity check in here that $.dialog() is actually available.

+++ misc/dialog.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,31 @@
+      if (!$(key).hasClass('dialog-processed')) {
...
+        .addClass('dialog-processed');

Please use Drupal's new $.once() here.

+++ misc/dialog.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,31 @@
+        // Other default settings are defined by http://docs.jquery.com/UI/Dialog.

Such basics can be removed.

This review is powered by Dreditor.

+  $build['long'] = array(
+    '#prefix' => '<div id="filter-tips-modal-dialog" class="ui-helper-hidden" title="' . t('Filter Tips') . '">',
+    '#markup' => filter_tips_long(),
+    '#suffix' => '</div>',
+  );

t('Filter Tips') needs to be check-plained.

I don't see how this is a bug, reclassifying.

Sub

@Damien: uhm, no. As of now, we "trust" user interface string translations. Only user-supplied data needs to be escaped. We have many examples in core, so I hope one arbitrary is sufficient to convince you: http://api.drupal.org/api/function/contact_form_user_profile_form_alter/7

Also, this is a usability problem, not really a feature. Since you lose all your form data when clicking on that filter tips link, it can certainly be considered as a bug. However, let's just treat it as a task. ;)

Incorporated issues from #87.

Taking account for #583400: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable.

Screenhots for Garland and Seven attached.

Forgot dialog.js and those php test changes actually seem to be required (not sure why).

This abstracts dialog.js into jqueryui.js and adds drupal_add_jqueryui() which allows the registration/binding of jQuery UI elements.

  // Present the more information about text formats link.
  $form['format_help']['more'] = array(
    '#markup' => l(t('More information about text formats'), 'filter/tips', array('attributes' => array('class' => array('filter-tips-modal')))),
  );
  $form['format_help']['long'] = array(
    '#prefix' => '<div id="filter-tips-modal-dialog" class="ui-helper-hidden" title="' . t('Filter tips') . '">',
    '#markup' => filter_tips_long(),
    '#suffix' => '</div>',
  );

  // Create a dialog box for the filter tips.
  $form['#attached']['drupal_add_jqueryui'][] = array('dialog', '#filter-tips-modal-dialog', array(
    'width' => 600,
    'height' => 500,
    'dialogClass' => 'filter-tips',
    'autoOpen' => FALSE,
  ));
  // When the user clicks on the more information link, present the dialog box.
  $form['#attached']['drupal_add_jqueryui'][] = array('dialog', '#filter-tips-modal-dialog', 'open', 'click', '.filter-tips-modal');

As you can see, we create the dialog box, in the first call, and then register the click event to show it. The good thing about this is that it will work on any bindable event, as well as any jQuery UI tool we have available, which gives us quite a bit of power.

Couple side notes:

• I've kept ui.theme.css in as that problem belongs in a separate issue: #583400: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable.
• Pushing this to critical as it's a release blocker.
• Additional improvements: Load the dialog content via AJAX? Add an accordion to the filter tips? etc.

Cleaned up drupal_add_jqueryui() with some documentation. Also added a $base parameter, so that we're not restricted to "ui" libraries. We can now also use drupal_add_jqueryui() to add UI Effects as well. Yay screenshot!

Great work on this. From a usability/accessibility point of view I really think a drop shadow would add great separation between the content in the dialog and the content behind it.

However I'm not sure what the state of Drop Shadows is in jQueryUI I heard at one point they were removed because they needed more work. I'll have a look at this if I get some time.

Ok, so first things - first. Lets remove the scrollbar - just expand the browsers scroll bar. Then onto the style, as far as I can see the < li >'s cause a lot of indentation - is there anyway we can fix this? (Perhaps some screenshots of other help text's.

Also, can someone run this in sync with overlay to see how it looks, what we change about it visuals.

One thing that the popup is missing is draggable and resizable. Right now once the dialog is presented, you can't move it out of the way and continue writing. We need draggable and resizable jQuery UI libraries so that the user can move it without loosing their place.

Bojhan at #99:

as far as I can see the < li >'s cause a lot of indentation - is there anyway we can fix this? (Perhaps some screenshots of other help text's.

Do you think modifying the actual filter tips content should be in a different issue? Here we're just sticking it in a popup dialog box. Sun brought up the idea of sticking it in jQuery UI tabs/accordion, which might help.

SeanBannister at #98:

From a usability/accessibility point of view I really think a drop shadow would add great separation between the content in the dialog and the content behind it.

Yeah, unfortunately jQuery UI Drop Shadow is still in planning phase of jQuery UI 1.8. We could look at bringing in jQuery Drop Shadow....

Bojhan at #99:

Lets remove the scrollbar - just expand the browsers scroll bar.

That is the browser's scroll bar ;-) .

Bojhan at #99:

Also, can someone run this in sync with overlay to see how it looks, what we change about it visuals.

Good idea!

subscribing.

@sun in #94, That php filter test was testing for whether or not the word 'print' was appearing on the screen (not the most robust test). The filter tips contain the word 'print', so it broke the test.

I updated the filter_tips_long code (looks like there was a recent change to theme_filter_tips that affected it), and added draggable and resizable as dependencies to the dialog js, which makes the dialog draggable and resizable. (Which is in agreement with http://docs.jquery.com/UI/Dialog#overview which lists them as dependencies)

I think the theming recommendations are out of this issue's scope. We haven't done any theming at all to the dialog, it's just jQueryUI out of the box. Theming dialogs or the filter tips content should be new issues.

Forgot my own advice in #104

Rerolled Jody's last patch to HEAD without the PHP test update. I really like it with the resizable and draggable.

Here's a Screenshot of it with #583400-19: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable :-) .

Bojhan at #99 asked to see how this works with the Overlay, so I took the steps I outlined at #583400-19: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable, and then applied the latest patch in #517688: Initial D7UX admin overlay, and the attached screenshot is the result. Seems to work quite well!

Through #583400: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable, we could give Seven's jQuery UI theme a look that more matches the administrative stuff. Theme roller and theme_hook_alter to the rescue! Yay! :-)

Anyone know why the PHP test failed?

Anyone know why the PHP test failed?

The code discussed in #104 and #107 needs to be added back to the patch to make the tests pass again.

Looking great with #583400. Seems like we just need to reroll with that php test adjustment and we're golden.

Hmmm, bizarre.

Jody in #113 says that everything checks out. #111 addresses the way forward and compatibility. All tests pass in #114. Is this RTBC?

@RobLoach: Very nice job!

+++ includes/common.inc	14 Oct 2009 16:57:21 -0000
@@ -3673,6 +3673,55 @@
+ * @param $tool
+ *   The name of the tool to add (dialog, accordion, resizable, etc).
...
+ * @param $base
+ *   (optional) The base name of the tool that is being added. Specify "effect"
+ *   for any jQuery UI effect, and "ui" for widgets and interactions.
...
+function drupal_add_jqueryui($tool, $selector = NULL, $options = array(), $event = 'ready', $binded_element = NULL, $base = 'ui') {
...
+    drupal_add_library('system', "$base.$tool");

The order of arguments looks very unnatural to me. I'd expect $base, $tool, and both being required.

+++ includes/common.inc	14 Oct 2009 16:57:21 -0000
@@ -3673,6 +3673,55 @@
+ * @param $selector
+ *   (optional) The CSS selector of which to apply the tool.

s/of which//

+++ includes/common.inc	14 Oct 2009 16:57:21 -0000
@@ -3673,6 +3673,55 @@
+ * @param $event
+ *   (optional) On what binded event the tool should be applied to the element.
+ *   Defaults to document ready.

"document ready" ?

Perhaps, "document.ready()".

Or, "document's 'ready'".

+++ includes/common.inc	14 Oct 2009 16:57:21 -0000
@@ -3673,6 +3673,55 @@
+ * @param $binded_element
+ *   (optional) When binding on an event other then ready, will be the element
+ *   that the event is binded to.

s/then/than/

+++ misc/jqueryui.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,45 @@
+Drupal.behaviors.jqueryui = {
...
+    if (settings.jqueryui || false) {
...
+              // Apply the jQuery UI's effect.
+              $(selector, context).once('jqueryui-' + tool + '-ready')[tool](options.options);
...
+              // Apply the jQuery UI's effect on a binded event.
+              $(options.item, context).once('jqueryui-' + tool + '-' + event).bind(event, function(e) {

Not 100% sure, but I'd prefer if core would take away the "ui" behavior namespace (i.e. skipping the "jquery" prefix).

So also this file would be renamed to ui.js.

Because, we still don't know about the future of the contributed jquery_ui module...

This review is powered by Dreditor.

Thanks for the review. I've included the notes sun made in this patch.

• Renamed to drupal_add_ui(), along with renamed jqueryui.js to ui.js and the namespace change
• Touched up the documentation and removed the "optional" note for the $base argument. You're right in this not really being optional.
• Fixed the other documentation notes you made

... Got an idea regarding $base, we could remove the base part from the related jQuery UI libraries in system_library so we're left with $libraries['dialog'] instead of $libraries['ui.dialog'], and $libraries['explode'] instead of $libraries['ui.explode']. Then we wouldn't need this $base argument at all. Thoughts?

After a quick discussion with tha_sun and DamZ, we ended up going with:

  drupal_add_ui($module, $library, $selector, $options, etc)....

  // Create a dialog box for the filter tips.
  $form['#attached']['drupal_add_ui'][] = array('system', 'ui.dialog', '#filter-tips-modal-dialog', array(
    'width' => 600,
    'height' => 500,
    'dialogClass' => 'filter-tips',
    'autoOpen' => FALSE,
  ));
  // When the user clicks on the more information link, present the dialog box.
  $form['#attached']['drupal_add_ui'][] = array('system', 'ui.dialog', '#filter-tips-modal-dialog', 'open', 'click', '.filter-tips-modal');

This matches the function definition of drupal_add_library for added bonus consistency.

Forgot to remove the $base PHP docs.

I love it.

This may even be usable for Overlay.

I like this, but I think it is secondary to the main overlay patch at #517688: Initial D7UX admin overlay. I'd like to get the main Overlay in first, and then see how both play together. I think that is the best, and most natural order, that would give us the biggest bang.

Rerolled. And yup, it works alongside the Overly! :-) http://drupal.org/files/issues/OverlayAndFilterTips.png from #583400-19: jQuery UI libraries are loading too much CSS that needs to be reverted / Allow jQuery UI themes to be swappable.

The theme_filter_tips_more_info function was removed, but it was still being used in filter.admin.inc. I fixed that, but we probably want to just put a modal dialog in the filter admin as well.

I added the modal for the filter admin pages (e.g. /admin/config/content/formats/1)

Yup!

Just created a follow up issue: #609094: Popups for the "More Help" links.

Any update here? We're now a week past freeze #2 and overlay's status is unclear. I would hate to see a usability improvement like this one passed over while waiting for a mega-patch that may or may not land (...and I do hope it lands).

Brought back to HEAD.

Index: modules/system/system.module
===================================================================
RCS file: /cvs/drupal/drupal/modules/system/system.module,v
retrieving revision 1.826
diff -u -r1.826 system.module
--- modules/system/system.module	25 Oct 2009 19:52:47 -0000	1.826
+++ modules/system/system.module	26 Oct 2009 04:04:57 -0000
@@ -1153,6 +1153,8 @@

Please roll with -p

+++ modules/system/system.module	26 Oct 2009 04:04:57 -0000
@@ -1153,6 +1153,8 @@
Index: modules/php/php.test
Index: modules/php/php.test
===================================================================
===================================================================
RCS file: /cvs/drupal/drupal/modules/php/php.test,v
RCS file: /cvs/drupal/drupal/modules/php/php.test,v
retrieving revision 1.18
retrieving revision 1.18
diff -u -r1.18 php.test
diff -u -r1.18 php.test
--- modules/php/php.test	11 Oct 2009 03:07:19 -0000	1.18
--- modules/php/php.test	11 Oct 2009 03:07:19 -0000	1.18
+++ modules/php/php.test	26 Oct 2009 04:04:57 -0000
+++ modules/php/php.test	26 Oct 2009 04:04:57 -0000
+++ modules/php/php.test	26 Oct 2009 04:04:57 -0000
@@ -60,7 +60,7 @@

do you really need these changes?

+++ includes/common.inc	26 Oct 2009 04:04:57 -0000
@@ -3882,6 +3882,58 @@
+ * @param $module
+ *   The name of the module that registered the desired library. For most jQuery
+ *   UI elements, this is usually "system".

What jQuery UI elements would not be defined by system? none! Please remove $module altogether

+++ includes/common.inc	26 Oct 2009 04:04:57 -0000
@@ -3882,6 +3882,58 @@
+ *   An array representing all elements added to the page so far.
+ */
+function drupal_add_ui($module, $library, $selector = NULL, $options = array(), $event = 'ready', $binded_element = NULL) {

use a big $options array for all but the first two items please. Also, it's bound, not binded

+++ modules/filter/filter.module	26 Oct 2009 04:04:57 -0000
@@ -674,10 +671,28 @@
+
+  // Create a dialog box for the filter tips.
+  $form['#attached']['drupal_add_ui'][] = array('system', 'ui.dialog', '#filter-tips-modal-dialog', array(
+    'width' => 600,

what about just a #attached[ui] ?

Dmitri: Comment #104 explains why we need that change to php.test

Thanks for giving it a look, Dmitri! I like the options array idea, as well as [#attached][ui].....

  // Create a dialog box for the filter tips.
  $form['#attached']['ui'][] = array(
    'library' => 'ui.dialog',
    'selector' => '#filter-tips-modal-dialog',
    'options' => array(
      'width' => 600,
      'height' => 500,
      'dialogClass' => 'filter-tips',
      'autoOpen' => FALSE,
    ),
  );
  // When the user clicks on the more information link, present the dialog box.
  $form['#attached']['ui'][] = array(
    'library' => 'ui.dialog',
    'selector' => '#filter-tips-modal-dialog',
    'options' => 'open',
    'event' => 'click',
    'bound_element' => '.filter-tips-modal'
  );

Cleaned up the code a bit.

Obviously I agree with Dries that Overlay should go in first, but given the progress on that issue - I think its smart to commit this anyway. Since it is almost always already demonstrated with the Overlay, we know the visual issues are fixable and no bugs have been encounterd yet.

+++ includes/common.inc	27 Oct 2009 17:53:46 -0000
@@ -3882,6 +3889,66 @@
+  // Add the settings so that the behaviors are attached to the elements.
+  if (isset($options['selector'])) {
+    // Use the explicit index to avoid conflicts when the settings are merged.
+    $index = count($elements[$library]);
+    // Remove items that the JavaScript doesn't need and add the settings.
+    unset($options['module'], $options['library']);
+    $settings['ui'][$tool][$index] = $options;
+    drupal_add_js($settings, 'setting');
+  }

My mistake. This should add an item to $elements[$library][$tool], and then get the count of $elements[$library][$tool] instead so that $index keeps growing correctly. As it is now, $index will stay on how many tools you add, as oppose to how many jQuery UI elements you invoke. With this, when the settings are merged, you'd hit conflicts.

This review is powered by Dreditor.

Braindump.....

Conditional checks

In #613654: update.manager.js is missing, they want to display a dialog warning when a checkbox shouldn't be checked. It looks at this.checked, so it would be nice to somehow work that into this API.

"Library" is buggy

Although jQuery UI Highlight is effect.highlight, its function is $.show(effect, [options], [speed], [callback]);.

Abstract from jQuery UI

This could be applied to any jQuery function, and is not restricted to jQuery UI. We might want to make "library" a way to optional add jQuery UI libraries through drupal_add_library, and switch it to "function" or "callback", or something, as then we could do things like call slideDown on elements straight from PHP.

Argument list

The "options" are passed into the function call as a single argument. Instead, we could switch this to "arguments", and have it as an array of arguments. Then we could pass the speed option in jQuery UI Show. This would use the apply JavaScript function.

I'm slightly worried about calling the new function drupal_add_ui(). I'm worried novice developers will think that means "output the form elements" or something. ;) I'd rather it was called drupal_add_jqueryui() or something, since it's entirely specific to jQuery UI, right?

Also, I wish I knew why we need to do this twice, more or less:

  // Create a dialog box for the filter tips.
  $form['#attached']['ui'][] = array(
    'library' => 'ui.dialog',
    'selector' => '#filter-tips-modal-dialog',
    'options' => array(
      'width' => 600,
      'height' => 500,
      'dialogClass' => 'filter-tips',
      'autoOpen' => FALSE,
    ),
  );
  // When the user clicks on the more information link, present the dialog box.
  $form['#attached']['ui'][] = array(
    'library' => 'ui.dialog',
    'selector' => '#filter-tips-modal-dialog',
    'options' => 'open',
    'event' => 'click',
    'bound_element' => '.filter-tips-modal'
  );

Isn't there some better way to handle this? Can't we merge the definition of the dialog and defining how it's attached to various triggers into the same step? Even if jQuery UI itself can't handle that, and jQuery UI really thinks of this as 2 actions, couldn't we handle that ourselves in drupal_add_jqueryui() to avoid the duplication at every call site? Roughly:

  // Create a dialog box for the filter tips.
  $form['#attached']['ui'][] = array(
    'library' => 'ui.dialog',
    'selector' => '#filter-tips-modal-dialog',
    'definition' => array(
      'width' => 600,
      'height' => 500,
      'dialogClass' => 'filter-tips',
      'autoOpen' => FALSE,
    ),
    'actions' => array(
      // When the user clicks on the more information link, present the dialog box.
      array(
        'options' => 'open',
        'event' => 'click',
        'bound_element' => '.filter-tips-modal',
      ),
    ),
  );

Also, is there a good way to associate actions with the "okay" and "cancel" buttons on the dialog? E.g. if I want to trigger something when the user clicks "cancel" in the dialog, how would I do that?

Otherwise, this is looking very cool. I applied the patch and played with the filter help tip popup -- super yummy!

Thanks,
-Derek

This patch adds a couple things:

• Argument arrays through a "arguments" parameter allows us to call a function with more then just one parameter
• Conditional checks via an "is" argument
• Ability to call any jQuery function through the "action" parameter to let us call jQuery functions like "hide" or "show" on elements

  // Create a dialog box for the filter tips.
  $form['#attached']['ui'][] = array(
    'library' => 'ui.dialog',
    'selector' => '#filter-tips-modal-dialog',
    'arguments' => array(
      array(
        'width' => 600,
        'height' => 500,
        'dialogClass' => 'filter-tips',
        'autoOpen' => FALSE,
      ),
    ),
  );
  // When the user clicks on the more information link, present the dialog box.
  $form['#attached']['ui'][] = array(
    'library' => 'ui.dialog',
    'selector' => '#filter-tips-modal-dialog',
    'arguments' => array('open'),
    'event' => 'click',
    'bound_element' => '.filter-tips-modal',
  );

It would be great to have the ability to call multiple "actions" on a given element like Derek suggested above.

Whoops, wrong patch.

Adds a "functions" associative array, which allows declaration of JavaScript functions for use in the "arguments" array. This is handy when you want to have a function callback passed into, say a jQuery UI Dialog button, like in #613654-11: update.manager.js is missing.

Interesting...... #617730: UX: Use modal dialogs for confirmation forms..

Added in this patch

• A optional "return" argument so that you can change the default return value. FALSE was the default because we wanted to override what clicking on a link did. Sometimes though, you'd want to return TRUE here instead, like when acting when dialog boxes close and stuff.
• Cleaned up the documentation a bit
• Switched the other filter dialog to use [#attached][ui].
• Made the dialog boxes use "title" option instead of the coding the title in the markup

Things to consider

drupal_add_ui()

Since this can be applied to any jQuery function through the "action" argument (show, hide, css, etc), we could name this to drupal_add_jquery(), or something similar. Remember that it has to reflect the same namespace as [#attached][ui] and ui.js .

"actions"

dww brought up a great idea in #144 to allow acting on a single element with multiple actions. Should it be part of this patch?

Bikeshed

The sooner we get this in, the more fun stuff we can do with it. So let's get it in soon! Thanks.

Awesome!

Not sure what to do about the multiple invocation thing. Basically, each invocation seems to add cruft to the settings array, so it seems to be worth to consider multiple things for the same module + library + selector for bandwidth reasons alone.

+++ includes/common.inc	30 Oct 2009 19:12:33 -0000
@@ -3882,6 +3889,88 @@
 /**
+ * Adds a jQuery UI element to the page, and invokes its behaviors.
+ *
...
+function drupal_add_ui(array $options = array()) {

We need at least one example usage in the PHPDoc here.

+++ modules/filter/filter.admin.inc	30 Oct 2009 19:12:33 -0000
@@ -160,7 +160,30 @@
+        'library' => 'ui.dialog',
+        'selector' => '#filter-tips-modal-dialog',
+        'arguments' => array('open'),
+        'event' => 'click',
+        'bound_element' => '.filter-tips-modal',

It seems like the more natural order is:

library
selector
event
bound_element (optional)
arguments

The @param description of drupal_add_ui() should use the very same order, having any other (totally optional) parameters appended last.

+++ modules/filter/filter.module	30 Oct 2009 19:12:34 -0000
@@ -674,10 +671,40 @@
+  // Present the more information about text formats link.
+  $form['format_help']['more'] = array(
+    '#markup' => l(t('More information about text formats'), 'filter/tips', array('attributes' => array('class' => array('filter-tips-modal')))),
+  );
+  $form['format_help']['long'] = array(
+    '#prefix' => '<div id="filter-tips-modal-dialog" class="ui-helper-hidden">',
+    '#markup' => filter_tips_long(),
+    '#suffix' => '</div>',
+  );
+
+  // Create a dialog box for the filter tips.
+  $form['#attached']['ui'][] = array(
+    'library' => 'ui.dialog',
+    'selector' => '#filter-tips-modal-dialog',
+    'arguments' => array(
+      array(
+        'width' => 600,
+        'height' => 500,
+        'autoOpen' => FALSE,
+        'title' => t('Filter tips'),
+      ),
+    ),
+  );
+  // When the user clicks on the more information link, present the dialog box.
+  $form['#attached']['ui'][] = array(
+    'library' => 'ui.dialog',
+    'selector' => '#filter-tips-modal-dialog',
+    'arguments' => array('open'),
+    'event' => 'click',
+    'bound_element' => '.filter-tips-modal',
+  );

Looks like we need a helper function now. Makes no sense to have the identical code.

+++ misc/ui.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,89 @@
+/**
+ * @file
+ * Provides the jQuery UI Drupal behaviors.
+ */
+
+  /**
+   * The base UI namespace.
+   */
+  Drupal.ui = {

Strange indentation.

+++ misc/ui.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,89 @@
+      jQuery.each(elements, function(index, value) {
...
+        jQuery.each(settings.ui, function(action, effects) {
+          jQuery.each(effects, function(index, effect) {
...
+              jQuery.each(effect.functions, function(name, code) {

We can use $ here.

+++ misc/ui.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,89 @@
+    attach: function(context, settings) {

(and elsewhere) Anonymous functions should have a space between 'function' and the opening parenthesis.

I'm on crack. Are you, too?

I hit some of the tha_sun's comments, as well as added dww's "actions" parameter. I find it helps a bit with the developer experience.....

  // Create a dialog box for the filter tips.
  $form['#attached']['ui'][] = array(
    'library' => 'ui.dialog',
    'selector' => '#filter-tips-modal-dialog',
    'arguments' => array(
      array(
        'width' => 600,
        'height' => 500,
        'autoOpen' => FALSE,
        'title' => t('Filter tips'),
      ),
    ),
    'actions' => array(
      // When the more information link is clicked, open the dialog box.
      array(
        'event' => 'click',
        'bound_element' => '.filter-tips-modal',
        'arguments' => array('open'),
      ),
    ),
  );

.... Is it just me or does $(this)[action].apply($(this), effect.arguments); not work on Safari or Internet Explorer? It works fine in Firefox.

I just ran into this issue on 6.14, or I should say my customer just did and was very unhappy about it. I can't wait for 7.X to fix it, and I don't want to go around patching up the core.

I have a menu item I needed to put in a popup, so I used the same idea. I created a block that is in the footer of every page. The block has this in it

<script type="text/javascript">
$(document).ready(function() {
   $(".menu-234 > a").each(function() {
     $(this).attr("href", $(this).attr("href") + "?format=popup");
     $(this).attr("target", "_blank");
     });
});
$(document).ready(function() {
   $("a[href*='filter/tips']").each(function() {
     $(this).attr("href", $(this).attr("href") + "?format=popup");
     $(this).attr("target", "_blank");
     });
});
</script>

The first part takes care of the menu item, the second part fixes all the filter tips links. The format is just a slightly different format in teh theme that has no headers, menus, sidebars, just the content. Maybe someone better at Jquery can cleanup the code a bit.

So for someone finding this thread and needing a quick fix before 7.x comes out give this a try.

I did some more of sun's comments and fixed it for Safari (Safari didn't like "effect.return" as I guess return is a reserved word, so I switched return to 'return_value'.)

I don't understand the conditionals in ui.js that are like if (settings.ui || false) { Why not just if (settings.ui) ?

Is this patch going to have a shot to make it in under the 'usability extension' category? It's some pretty important jquery UI integration stuff.

Thanks for the Safari fix, I was having troubles with that one. Seems to be missing ui.js :-) .

Regarding if (settings.ui || false), I have a habit of strict typing code. We could just use settings.ui no problem, I was just concerned what would happen if settings.ui didn't exist.

Here it is with ui.js.

I guess the conditionals should just be however the rest of core does it.

Sub.

Overlay (which seemed to be a blocker here) is now committed. Hoping that there is still time to get this in.

Good point, rickvug! Having a look at $form['#ajax'], is there anyway it could fit in here? Maybe not since they both are trying to accomplish different things....

Lets get this in. It was ready before freeze and was told to sit and a corner and wait for overlay. It did so without complaint. Whats left?

Agreed, guess this is RTBC then.

Rerolled. There's an unrelated issue in the Filter module that displays a warning and doesn't show the Allowed Tags. Should we fix that here?

@Rob Loach, well, if you want the patch to be committed ;) of course could be fixed in a separate issue if it's a big / complicated one but a minor one should be just fine here.

subscribing

Subscribe

@Rob_Loach in #162, what issue are you exactly talking about? Could you give us a reroll?

Lets get this patch in. Especially the [#attachted][ui] stuff...

1. Switched it to drupal_add_action() as this is not restricted to "UI". You can call any jQuery method using this.
2. Removed the function callbacks as that's not needed for this.
3. Switched to a "link" form type, as well as a parenting "container" type instead of the theme function. Yay Drupal 7!

@casey: The warnings I was referring to is big red messages you see at the top of the screenshot. Unrelated to this patch.

Forgot to update the PHP test.

This doesn't work with the overlay. Clicking the filter tips link within the overlay gets me out of the overlay and on the regular filter tips page.

@Rob Loach #162: "There's an unrelated issue in the Filter module that displays a warning and doesn't show the Allowed Tags"
I had those too. This comes from a previous patch in the filter config UI. You need to resave your filters config.

Frando at #176:

This doesn't work with the overlay. Clicking the filter tips link within the overlay gets me out of the overlay and on the regular filter tips page.

This is actually because the Overlay goes through each link and changes its behavior. Telling the Overlay not to change the link's behavior fixed it. It would be nice if the Overlay had a hook_overlay_no_process(), or something so that we could tell it how to behave or how not to behave on any given link. Like hook_admin_paths(), but to tell the Overlay to not process X links....

Telling the Overlay not to process it through the class seems the easiest solution around this for now.

yched at #177:

"There's an unrelated issue in the Filter module that displays a warning and doesn't show the Allowed Tags"
I had those too. This comes from a previous patch in the filter config UI. You need to resave your filters config.

Hmmm, do you know where that issue is? It would be nice if the upgrade path didn't require us to visit that configuration.

Well, here's an updated patch that works with the Overlay, along with a screenshot of it in the Overlay. #614494: Give Seven a nice looking default jQuery UI theme would make it prettier, but that's unrelated to this patch.

subscribing

#178

This is actually because the Overlay goes through each link and changes its behavior. Telling the Overlay not to change the link's behavior fixed it. It would be nice if the Overlay had a hook_overlay_no_process(), or something so that we could tell it how to behave or how not to behave on any given link. Like hook_admin_paths(), but to tell the Overlay to not process X links....

#668104: Make overlay respect other click handlers should fix that.

['#attached']['action'] and drupal_add_action(): I am not sure about the name 'action'. Action is already being used elsewhere. We need a name that makes clear this is about adding scripts/interaction to the webpage/document/ui clientside. I think I would stick to 'ui'. Not by means of jQuery UI, but in general; a dialog or a collapsing event is part of the user interface, right? UI doesn't always mean GUI.

oops

I tested it and reviewed and it looks good to me with and without overlay. I'd be very glad to see this make it in.

I'm opening up a new issue for the php test adjustment that comes along with this issue, in the hopes that can go right in and avoid any further confusion here.

+++ includes/common.inc	1 Jan 2010 16:44:59 -0000
@@ -4193,6 +4200,99 @@
+ *   - return_value: (optional) The value which should be returned after the action is
+ *     taken on the bound element. Defaults to FALSE.

Exceeds 80 chars.

+++ modules/filter/filter.module	1 Jan 2010 16:45:03 -0000
@@ -711,11 +708,44 @@
+      'attributes' => array('class' => array('filter-tips-help-link', 'overlay-displace-no-click', 'overlay-processed')),

Can we please remove Overlay from core? How much more ugly does it need to be?

+++ modules/filter/filter.module	1 Jan 2010 16:45:03 -0000
@@ -711,11 +708,44 @@
+  $form['format_help']['dialog'] = array(

Speaking of dialog... did you test whether this entire action handling actually works for Dialog module?

Powered by Dreditor.

#183 Those classes ( 'overlay-displace-no-click', 'overlay-processed') aren't necessary any more when #668104: Make overlay respect other click handlers gets committed.

It sounds like webchick wants to get #668104: Make overlay respect other click handlers in before this one.

Also she committed #673252: Adjust a test in php.test for issue #87994 so we need to pull the change to php.test out of this patch.

The Overlay has dramatically slowed the progress on this. The issue was RTBC during the summer and was postponed due to it. Is there anywhere else we could apply this instead so we can at least get some seeable core jQuery UI elements into one of the required Drupal modules?

jQuery UI Tabs on the Filter Tips page?

Off topic under the context of D7, I know. But maybe someone landing here could be interested in this kind of things for D6. Here's Filter Tips (and other usability enhancements for D6) with the help of the Modal Frame API:

http://drupal.org/project/modalframe_contrib

#668104: Make overlay respect other click handlers has landed.

I retested this because i think its gonna fail
And it needs a lil tweak as the overlay click handler is fixed now

I think the initial width of the dialog is too small since most of the filter tips contain tables with fairly wide content. See screenshot.

Pushing this again, cause i think this can be fixed before alpha and need to be fixed.. (before or fast after alpha)

Thank you to jacine for the .js-show class that solved an issue that was popping with when the filter tips dialog was in the overlay.

@mrfelton:

I think the initial width of the dialog is too small since most of the filter tips contain tables with fairly wide content.

Increased it to 600, which helped it a bit. We'll be re-theming the filters page after this one, so hopefully that will make it a bit thinner.

@markus_petrux:

Off topic under the context of D7, I know. But maybe someone landing here could be interested in this kind of things for D6. Here's Filter Tips (and other usability enhancements for D6) with the help of the Modal Frame API: http://drupal.org/project/modalframe_contrib

The Modal Frame API is a great module. This is doing a bit more then just providing one modal box though. It gives us an API that allows calling any jQuery method on elements from PHP through #attached ui and drupal_add_ui(). This small filter tips dialog is just one application of it. We could use #attached ui to apply tabs to filter tips page, for example, without having to put together an additional JavaScript file.

The .js-show class needed display: static.

subscribing.

reviews anyone? this is marked critical.

+++ modules/filter/filter.module	13 Jan 2010 08:27:19 -0000
@@ -711,11 +708,44 @@
+    'selector' => '.filter-tips-long-dialog',

This needs an ID. Maybe we should make 'selector' optional and when not provided ensure that element has an ID and use that ID as selector.

+++ modules/filter/filter.module	13 Jan 2010 08:27:19 -0000
@@ -711,11 +708,44 @@
+        'bound_element' => '.filter-tips-help-link',

Same here. Can't use ID of element however. "$element_id . ' .filter-tips-help-link'" should do however.

+++ modules/system/system.module	13 Jan 2010 08:27:30 -0000
@@ -1194,6 +1194,8 @@
Index: modules/php/php.test

+++ modules/php/php.test	13 Jan 2010 08:27:20 -0000
@@ -82,7 +82,7 @@

@@ -82,7 +82,7 @@
     $this->assertRaw(t('Basic page %title has been updated.', array('%title' => $node->title)), t('PHP code filter turned on.'));
 
     // Make sure that the PHP code shows up as text.
-    $this->assertNoText('print "SimpleTest PHP was executed!"', t("PHP code isn't displayed."));
+    $this->assertNoText('print "SimpleTest PHP was executed!"', t('PHP code isn\'t displayed.'));
     $this->assertText('SimpleTest PHP was executed!', t('PHP code has been evaluated.'));
   }
 }
@@ -110,7 +110,7 @@

@@ -110,7 +110,7 @@
 
     // Make sure that the PHP code shows up as text.
     $this->drupalGet('node/' . $node->nid);
-    $this->assertText('print', t('PHP code was not evaluated.'));
+    $this->assertText('print "SimpleTest PHP was executed!"', t('PHP code is displayed.'));
 
     // Make sure that user doesn't have access to filter.
     $this->drupalGet('node/' . $node->nid . '/edit');

Not part of this issue?

Powered by Dreditor.

Has this modal dialog had an accessibility review?

Thanks for giving it a look, Casey. I addressed your concerns here. Also noticed that we could use a #theme property call for one of the elements.

Regarding the test, Jody outlined why we need that change at #104:

That php filter test was testing for whether or not the word 'print' was appearing on the screen (not the most robust test). The filter tips contain the word 'print', so it broke the test.

I would be happy to give a brief accessibility review of this patch if someone can point me to a place where I can test it out. I can install head / the patch myself, but would strongly prefer not to.

Clearly everyone would prefer not to install and patch head themself, but with the limited time I have to contribute, and with the limited number of individuals involved in the community with accessibility skills I find this to be the best use of available resources for ensuring that d7 is as accessible as possible.

This would be pretty cool if it actually worked. On a fresh checkout of head + #201 I didn't get any usable tips.

See...
http://img.skitch.com/20100202-funp9wjc9a7nf7knw7s8rx3gu.jpg
http://img.skitch.com/20100202-k53ckwrgiyijsdfx1m8krr4k9i.jpg

The other burning question I have is, since we are replicating jQuery UI type functionality in PHP... what happens when a module updates jQuery / UI to newer versions that have API changes? How does altering drupal work there?

Hmmm, Matt, are you using an old patch? The reason this happens is because the content is set to display:none. But then when the dialog hits it, it should be show()-ing it. I don't get those results on Chrome, Firefox or IE.

• Here the patch with -p.
• A screenshot
• Works with or without the Overlay
• Doesn't make you completely loose your node edit when you click the link

It seemed that for some reason, WebKit was thinking that .ui-helper-hidden was taking priority, even though it shouldn't. In this patch, we use a .js-show class (now alongside .js-hide) that will show the element only when JavaScript is enabled.

While I am not entirely sure about the ickiness of a pop-up inside an overlay it does work now.

http://img.skitch.com/20100202-bthp657t4pg2r8rquqdtc26wnb.jpg

There is an issue with having a horizontal scrollbar. The vertical one I understand but we shouldn't have the horizontal one.

Here is a more detailed review....

+++ includes/common.inc	2 Feb 2010 15:47:37 -0000
@@ -4033,6 +4034,12 @@ function drupal_process_attached($elemen
+  // Add any jQuery actions.

What is a jQuery action? Reading through I understand what it does. But, where does this name come from? Is it another drupalism?

+++ includes/common.inc	2 Feb 2010 15:47:37 -0000
@@ -4189,6 +4196,100 @@ function drupal_get_library($module, $na
+    // Make sure to only add the library once.

drupal_add_library makes sure a library has not already been added. Why are we adding a check here as well?

+++ includes/common.inc	2 Feb 2010 15:47:37 -0000
@@ -4189,6 +4196,100 @@ function drupal_get_library($module, $na
+    // Remove items that the JavaScript doesn't need, and add it to the settings.

doesn't should be does not.

+++ modules/php/php.test	2 Feb 2010 15:47:38 -0000
@@ -82,7 +82,7 @@ class PHPFilterTestCase extends PHPTestC
+    $this->assertNoText('print "SimpleTest PHP was executed!"', t('PHP code isn\'t displayed.'));

isn't should be is not. Not sure how that conjunction got in there in the first place.

+++ misc/ui.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,52 @@
+          // element isn't provided, we will use the original selected item.

isn't should be is not.

Powered by Dreditor.

Changed the width of the dialog so that the horizontal bar doesn't show up.

subscribe.

It would be great to do some more work on the code comments. Mfer is right in "action" being the wrong word.

Decorator? Or just function?

tagging

Fixing tag spelling.

I took a quick look at the filter tips dialog with four different screen-readers: JAWS 10 and Firefox 3.6, JAWS 6 and IE6, NVDA 0.6p2 and Firefox 3.6, and VoiceOver (Leopard) and Safari 4.

The dialog is using ARIA (Accessible Rich Internet Applications) markup which does improve accessibility for those user agents and assistive technologies that properly implement the draft W3C recommendation. Note that this is a draft recommendation and although pages will still validate (as the ARIA markup is injected with JS) it does theoretically break the validity of the DOM.

JAWS10: When activating the "More Information" link the screen-reader moves to the top of the dialog and begins reading. The user must then move back to the top of the dialog (if they realize that they are in a dialog) to close the dialog. This leaves the user at the bottom of the Add Content page. I can imagine users hitting the back button here because they do not realize that the content is in a modal JS dialog.

JAWS 6: Nothing appears to happen. If the user moves down to the Powered by Drupal footer of the page they can find the content. Nothing directs the user to look here and they may anticipate that the link has not functioned correctly. I believe that this behaviour will be present in JAWS versions up to JAWS 8 or possibly JAWS 9.

NVDA: The user is taken to the first link in the dialog. Same issues as in 1. above with JAWS 10.

VoiceOver: Same as NVDA.

** Note: this was a very fast preliminary assessment of the patch from the perspective of only one user, only testing with screen-reader technology, with no structured QA methodology, and does not constitute a thorough accessibility review.

I would suggest that this functionality would be at best confusing to most screen-reader users and would, depending on where the functionality is implemented, substantively break some functionality on the site.

@Everett Zufelt thanks for the feedback. This is great feedback that we need more of.

How about calling actions 'interactions', 'widgets' or 'effects' instead, as that's what's seen in the jQuery UI demos? Or maybe just "[jQuery] methods" as all this is done to 'call', or at least queue a call to, some method on a jQuery object?

I just tested the patch in #209 and I like it!

My english is not perfect, but:

+++ includes/common.inc	2 Feb 2010 18:22:47 -0000
@@ -4189,6 +4196,91 @@
+ *   - event: (optional) On what binded event the tool should be applied to the

and

+++ includes/common.inc	2 Feb 2010 18:22:47 -0000
@@ -4189,6 +4196,91 @@
+ *     be the element that the event is binded to. Defaults to what was passed

and

+++ misc/ui.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,52 @@
+          // Apply the jQuery UI's effect on the binded event. If a bound

s/binded/bound/ ?

+++ includes/common.inc	2 Feb 2010 18:22:47 -0000
@@ -4189,6 +4196,91 @@
+ * any associated JavaScript. Selected handlers can be bound by events (like
+ * click) or be executed on once the element is ready.

"... Selected handlers can be bound to events (like click) or be executed once the element is ready." ?

A few side notes:
When testing this I had a pretty high CPU load (mostly due to other things) and I got a pretty annoying 'trail/tear' effect when moving the dialog. I happened to catch one of the more extreme ones in a screenshot. The trail/tear is not what I'm most concerned about tho (don't think it can be avoided), if you look at the top of the same screenshot, you'll notice I've got CKEditor loaded as well, and as soon as the dialog is over it's iframe and lags behind even a pixel, it stops dead until the cursor is outside the frame again. This also applies to resizing. The problem also exists in the jQuery UI demos if you inject an iframe there, so I'm not asking for a fix here. I'll just inform those who care that temporarily putting a div over any iframe will keep the mouse events inside the document and allow for normal dialog functionality.

The resize handle itself obscures and blocks the scroll-down button on the scrollbar, which also happens in the official demos. I guess that's a task for those jQuery UI theming issues mentioned above.

I know nothing about screen readers, so I'm afraid I can't help test that. :/ But the rest looks great to me!

Powered by Dreditor.

Switched to TwoD's suggestion of "effect". This gives us drupal_add_effect(), which does reflect what the function does. Also touched up the documentation a bunch.

Related issue about accessibility of jQuery UI modal dialog used in overlay #716612: Overlay is not accessible to screen reader users. At this time modal dialogs are impossible to implement for at least the following screen-readers (JAWS, VoiceOver, NVDA). Dialogs can be implemented, modality is not possible to control.

I give up.

It's a shame that the overlay blocked this /actual/ usability improvement getting fixed while making the original bug worse with #655388: Many ways to lose data on form input in the overlay.

Rob Loach: No way, you're not allowed to :)

This patch was essentially RTBC six months ago and held up for the overlay, which at the time was still three months from being committed. That's just plain wrong.

Surely, somewhere among the 15 patches above that were marked RTBC above, there is something committable here?

@catch: This is a feature/task, not a bug. The bug you're thinking of is #153313: ckeditor input is lost when using the browser's back button, I think.

@David, no, this is very different from clicking random other links on the screen, or even closing the overlay, because the filter tips are embedded help text in the form itself linked directly to the process of content creation, so you fully expect not to be messed with when clicking on them. At Baltimore usability testing, someone clicked one of the links, lost all their form data, then said "I'll never click a help link again". IMO the expectations are very different from clicking on a menu item, home icon or wherever else which might take you away from the node from. If you can't complete your form without clicking on a help link, then a warning that you might lose context by visiting the help page does you absolutely no good whatsoever.

In other words this is a special kind of WTF, which even if we don't fix the overall form data loss elsewhere ought to be fixed here, and an (albeit huge) hovertip or modal popup for help text makes sense here, whereas a warning dialog or disabling clicking links, or any of the other suggestions in other issues, would make no sense at all.

I haven't done any reviewing on this issue, but I have followed it very closely because this is one of the biggest "small" usability improvements that I think could be made in Drupal 7.x. I am sorry that the scope of it crept to include an entire jQuery UI API - I was just hoping for a popup window... I think David_Rothstein is right at #221 - I am very busy right now, but I might be able to give a layman's usability review sometime this week, if I know which patch to apply to the latest D7 alpha.

So... what needs to be done here? To me it looks like we're stuffing all kind of other issues into this one, making this one unfixable.

At least overlay doesn't block anything of this patch any more (#668104: Make overlay respect other click handlers)

Regarding modal dialogs and accessibility for screen-reader users. I have written a blog article Can a modal dialog be made to work properly for screen-reader users on the web?. The short answer is kind of, but not really. I would have posted the entire article here, but it is a lttle to long for that. It explains the objective problem, as well as the subjective experience.

Would it be sufficient simply to make the filter tips show up in a new window or tab by setting the link to have target="_blank"?

I think that would address the problem of "I just lost my work by clicking a help link."

sadly (and inexplicably) target="_blank" is not XHTML compliant

I had no idea. Huge bummer :(

Another reason, imo, that XHTML should not be the goal in D8. HTML5 FTW!

Guess I'll have to create the "Filter tips in target blank" contrib module then...