This patch is a continuation of the following patches:
Add dynamic AHAH submission properties to Forms API - http://drupal.org/node/154398
Add javascript 'more' button to Poll (AHAH enable) - http://drupal.org/node/155870
This patch extends the abilities of the new ahah.js to apply to most types of form elements, not just buttons. Now on any javascript event to a form element (click, change, blur) Drupal can perform a request in the background, retrieve additional HTML and inject it directly onto the page. With this patch, all this will be available for form elements without writing a line of javascript. For a better description of AHAH, read the initial patch description.
Changes in this patch include:
- Adding the standard jQuery forms library to Drupal. The javascript is uncompressed in this version, but in the final version we'll likely compress it, since this is a standard distribution like jQuery itself.
- The addition of this library allows us to remove redirectFormButton(), createIframe(), and deleteIframe() from drupal.js.
- upload.module's attachments now actually work in Safari again.
- The blocks page is the entry point for the new features. Upon changing a weight or region, the page is dynamically updated reflecting the new regions and weights.
- New formAPI property #ahah_selector. Allows the AHAH replacement to be attached to multiple items on the page, not just the one on which the Form property is applied. This property was necessary to add in a a scenario such as the blocks page, where there could be hundreds of AHAH enabled elements. A single selector can get them all at once, rather than adding a javascript setting for every unique ID.
- New "special" javascript class 'ahah-new-content'. If content within the new HTML contains this class, the #ahah_effect will only apply to this content, not the entire chunk of new HTML.
Demo video: http://quicksketch.org/node/504
The video demonstrates nearly all the new features. The select elements are all AHAH enabled, by using the #ahah_selector property. On #ahah_event = 'change', a request is made to Drupal for an updated form. The entire table is replaced with the updated version, but the changed row has the 'ahah-new-content' class, so it receives the #ahah_effect = 'fade'.
This patch is dependent upon jQuery 1.1.3.1, where improvements were made when applying effects to table rows and cells. The update to jQuery 1.1.3.1 patch must be applied first.
This patch makes changes to both theme functions and menu hooks, so cache tables must be cleared and menu_rebuild() executed before testing.
Comment | File | Size | Author |
---|---|---|---|
#86 | drupal_ahah2_14.patch | 883 bytes | scor |
#77 | +1.jpg | 89.74 KB | webchick |
#76 | drupal_ahah2_13.patch | 46.74 KB | quicksketch |
#72 | drupal_ahah2_12.patch | 44.9 KB | recidive |
#68 | drupal_ahah2_11.patch | 45.5 KB | quicksketch |
Comments
Comment #1
webchickD'oh. No patch...
Comment #2
profix898 CreditAttribution: profix898 commentedThis is really amazing stuff. Subscribing. Any chance to get this into D6 post-freeze?
Comment #3
quicksketchMwahahah patches are good :)
Comment #4
mikey_p CreditAttribution: mikey_p commentedLoading admin/build/block results in white page, logs:
PHP Fatal error: Cannot unset string offsets in /Users/mdp/www/drupal-6.x/includes/form.inc on line 359
this is with patch #56 from http://drupal.org/node/146462
Comment #5
profix898 CreditAttribution: profix898 commented1. function block_theme():
'block_admin_display_form' => array('arguments' => array('form' => NULL, 'blocks' => NULL, ...),
is missing the two additional parameters (blocks, theme). Thats should solve mikey_p's issue.2. function block_admin_display_js(): variable $changed_block is not necessarily defined for use in '$form[$changed_block]['attributes']['class']'. This part should be wrapped in
if (isset($changed_block)) {...}
.Otherwise it works like charm :) I'm not enough js guru to comment on the js part, but from a visual inspection the php part looks fairly well designed. I didnt have time to play with other fapi elements (e.g. textfields, ...) in this context. Your patch adds ahah properties to more elements than are actually used in the blocks 'demo'. Did you write a tiny module to test the additional element's behaviour? The password strength validator doesnt seem to be using the new textfield/paasword ahah property, right? Shouldnt that be changed as well?
Comment #6
profix898 CreditAttribution: profix898 commentedPassword strength indicator is client-side only stuff. Ignore that.
Comment #7
Frando CreditAttribution: Frando commentedFunctionality review:
Great! It works like a charm. Shuffling blocks between regions and assigning different weights both work as expected. This is a huge usability enhancement.
The issue in #4 was solved for me after clearing the menu cache and rebuilding the menu. Can't reproduce afterwards, so this is not a bug.
Quick code review:
The implantation looks really clean and neat. Replacing the Drupal.redirectFormButton and all this iframe code with the jQuery forms plugin sounds good - I never really liked this iframe code, and the jQuery forms plugin appears to be really clean and greatly documented. I've been reading the patch pretty much from top to bottom, and couldn't find anything I didn't like. Great work.
I don't think profix's first issue is valid. AFAICS, block_theme is correct the way it's in the patch.
And regarding $changed_block, I think that is OK too, as the Ajax submit handler only is called when the form is being changed. So, there is always a changed block, AFAICS.
Comment #8
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedFirst of all, this is a very nice usability enhancement and should be noted as such. (Perhaps then, it could make it into drupal 6 also)
Something I do not like, is that the progressbar is displayed after the "Region"-column. i think it would be better to have it displayed on top of the table, or to:
- fade the current table row out;
- display the progressbar;
- hide progressbar when AJAX-reorganisation is done;
- display the "new" tablerow, where it is supposed to appear;
Another possibility is to use craqbox, to display a modal-window for the progressbar with some descriptive message (we could use this and the new AHAH-system through a lot of other area's through drupal too!).
Another nice thing would be, that the layout is directly updated to display the (de)activated blocks in order in their respective region inside the drupal site. but perhaps, that is out of the scope for this patch.
Very nice work after all.. Now if only jQuery would work consistently across the browsers, this would truly rock..
Comment #9
profix898 CreditAttribution: profix898 commented@Frando: You are right about block_theme, but the isset($block_changed) still seems required for me. Trying to set a block from RegionX to none generates the following warning: notice: Undefined variable: changed_block in /modules/block/block.module on line 359. without the isset wrapper.
I think we should think about the progress indicator. It somehow doesnt look right. Not sure it is the position it occurs at or the image itself. There should be a different indicator for this purpose than the progressbar IMO. There is no 'real' progress to track here, its only two states 'working/updating' or 'ready', but no position to visualize. How about something like this or the this?
Comment #10
quicksketchThanks much for the reviews all! This isn't nearly as much an "API" change as UI, so my hopes are high for inclusion in Drupal 6.
@profix898 Re: #5 above.
Yep, thanks! Fixed in this patch.
Ah yes. $changed_block should always be defined. This explains the weird fade effect when disabling a block. Thanks for pointing this out. It's fixed in this patch so $changed_block is always set.
@Stefan Nagtegaal Re: Progress Bar. I'd like the progress bar to be there, but as unobtrusive as possible. Switching out for a throbber seems like a possibility. Fading out and in the entire block could actually take longer than the request itself. I'd prefer to keep the effects to a minimum. We could move the progress bar to another location, but keeping it next to the element changed makes the most sense to me, especially considering this will need to be a generic solution throughout all of Drupal (see the current upload.module implementation and the ahah poll patch).
This patch fixes the two issues raised by profix898, and compresses the updated forms.jquery.js 1.0.1 for a more realistic patch for core.
Comment #11
profix898 CreditAttribution: profix898 commentedI completely agree with this statement. After thinking about this a bit more its seems like the right location to display the update indicator as close as possible to the changing element. And I also agree that it shouldnt trigger any modal overlay windows or fadein/out the whole block. We should however try to replace the progress bar with the throbber image as it better fits here (see #9) IMO.
Otherwise the new patch looks good. No more errors/warnings/notices. This is a really huge usability improvement and it makes it a lot easier for developers to use ahah stuff in contrib as well.
Comment #12
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedI think, the best way to get this in, is to split your patch into 3, so:
1) for jQuery updating to 1.1.3.1;
2) for the AHAH FAPI extension;
3) Your regions/blocks improvements;
The first, and third could be considered usability, and the second a bug/inconsistency.
Comment #13
quicksketchThis patch does touch on a lot of code, but it's already been dependent on several other patches. The patch includes a minor API change (though large underhood changes), and one example of how the new API functions (the blocks page). I figure with an example it's hard to convince it's a usability improvement. If requested I'll gladly split this up into separate patches for the changes to javascript and the changes to the blocks page.
jQuery 1.1.3.1 upgrade is already a separate patch: http://drupal.org/node/146462
Comment #14
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedQuicksketch, can you pls join #drupal for a moment, I want to discuss it with you... I'm there...
Comment #15
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedNice, as I type I'm testing the patch in IE 6/7/Safri 3b/FF 2 on Windows Vista and Safari 2/3b/, FF 2, opera 9 and Camino on OS X.
Comment #16
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedDoes not apply anymore, needs updating...
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentednice video! would love to get this in. subscribe
Comment #18
rickvug CreditAttribution: rickvug commented+1 - This would be a very noticeable usability improvement for end users.
Comment #19
BioALIEN CreditAttribution: BioALIEN commentedI like this, subscribing. Will mess around with this when I get some free time.
Comment #20
merlinofchaos CreditAttribution: merlinofchaos commentedThis is very important to another usability patch I am working on; subscribing.
Comment #21
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedQuicksketch, are you planning on updating this issue? Many people would like to see the patch updated, and have it hit the trunk. the usability improvements are very nice!
Comment #22
Frando CreditAttribution: Frando commentedAny news here?
Comment #23
Dries CreditAttribution: Dries commentedThis patch needs to be re-rolled because http://drupal.org/node/162871 went in. Thanks. :)
Comment #24
quicksketchYes, I'm super busy but I'll get to it soon. Steven has some outstanding complaints about the way AHAH is written, but since I haven't seen them written anywhere I'll keep them separated from this patch. We can tweak the code once the functionality is in place.
I haven't yet heard any debate about the inclusion of the jQuery Forms library. It's certainly better than our current per-jquery, super-rigged redirectFormButton, but if there is opposition (or support), they should speak now.
Comment #25
merlinofchaos CreditAttribution: merlinofchaos commentedSteven's objections that I know of:
The use of eval() is incorrect and unnecessary.
The iframe is needed for upload.js because normal js form handling can't handle files for security reasons. I don't know if this limitation escapes forms.js or not, but it needs to be investigated.
Comment #26
moshe weitzman@drupal.org CreditAttribution: moshe weitzman@drupal.org commentedany progress here? nate?
Comment #27
KarenS CreditAttribution: KarenS commentedSubscribing. Once working it would be great to get something like this into CCK's Manage Fields screen so you can dynamically change field weights and groups.
Comment #28
quicksketchThat's right folks, the new patch is here!
Compatibility-wise this patch:
- Updates everything to the latest HEAD, included the sweeping changes that happened to block module's templating.
- Renames block-admin-display.tpl.php to block-admin-display-form.tpl.php, since the name of that theme function changed.
- Makes Safari work with upload.module for the first time since 4.7! Unfortunately, it also causes Safari to crash on the blocks page. Cause is TBD.
- Upload module is fully compatible with these new changes, with the exception that upload module isn't saving files correctly right now anyway: issue #168813.
Code-style:
- Removed all instances of eval() from ahah.js. Instead doing
$object[method_name](param1, param2);
to execute dynamic function names.- Remove all hard-coded css from ahah.js, now classes are assigned and defined in system.css to handle the default styles.
- Once again, redirectFormButton() is removed entirely from drupal.js. The jQuery forms library in a compressed form is added in it's place. Now at version 1.0.3
- jquery.forms.js switches to using to an iFrame if a file is being uploaded, though special considerations are necessary, such as the returned string cannot be an XMLObject (see the jQuery forms library code samples). This patch takes the returned string and runs it through Drupal.parseJson() if necessary to convert it to JSON.
I've left the blocks part of the patch intact for ease of testing. I'll be glad to split if necessary. Thanks very much for all the positive feedback. I've been very busy and the consistent attention made this too hard to resist. Everyone please give this patch a good testing!
Comment #29
anders.fajerson CreditAttribution: anders.fajerson commentedTested block and upload in Opera 9, Safari 3 and Firefox 2.
Blocks:
Blocks doens't work in Safari (Windows/3.0.3):
notice: Undefined index: region in W:\www\drupal-cvs\modules\block\block.admin.inc on line 128.
notice: Undefined index: region in W:\www\drupal-cvs\modules\block\block.admin.inc on line 130.
notice: Undefined index: weight in W:\www\drupal-cvs\modules\block\block.admin.inc on line 129.
Add block page doesn't work for all browsers, and gives this notice: notice: Undefined index: block_add_block_form in W:\www\drupal-cvs\includes\theme.inc on line 48.
Upload:
When pressing Attach with no file selected it's still possible to submit. "0 0" is added after the Attach-button or under a previously added file.
UI:
I also aggree that the blue "throbber" would be prefered as an indicator for block changes. I would also be cool if the there could be *some* kind of effect on the block that appears in a new place. Usually I'm negative to effects but here it would serve as a visual indicator to the user where the block actually ended up. *Fast* fade in or something like the "yellow fade in" tecnique could be used for that specific block, and it wouldn't interrupt the user.
It would be nice if the progressbar didn't cause the colums to be pushed left when displayed. It's a bit visually distracting.
Comment #30
quicksketchThanks fajerstarter, what browsers does the upload module present these issues? I don't see it in Safari or Firefox (Mac). I mentioned that the blocks page doesn't work in Safari, but looks like you got more information than I could. Mac Safari just crashes when trying to change a block. :P
This patch fixes the Add Block page, which was caused by a change in the menu handlers. Thanks!
Comment #31
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedPerhaps it is wise to enable Safari's hidden 'Debug' menu? When Safari is not running, type in the Terminal the following line:
% defaults write com.apple.Safari IncludeDebugMenu 1
Then start Safari, and enjoy the new Debug-menu.
To disable the menu (why would you want that?):
% defaults write com.apple.Safari IncludeDebugMenu 0
Hope I gave a helping hand here...
Comment #32
quicksketchThe Safari crashing bug has identical symptoms to the post described in the jQuery forums here: http://www.nabble.com/jQuery-1.1.3.1-Safari-Crashes-t4228093s15494.html
The crashing problems should be resolved in 1.1.4, which is due to be released any day now. Could we intentionally overlook Safari for the time being?
I also forgot to respond to fajerstarter's last comment on throbbers and effects. The fade effect is already being used on new content on the blocks page. It happens quickly, but it's definitely there. On throbbers, I'm hesitant to use the circle because it would mean either using it for all AHAH effects, or making an option in the FAPI for one or the other. Take into consideration file uploads are included in this group. Which would make the most sense?
Comment #33
quicksketchComment #34
anders.fajerson CreditAttribution: anders.fajerson commentedThe upload problem (see attached screenshot) appears in all tested browsers (Windows: Opera 9, IE6/7, Safari, Firefox2). To reproduce simply press the Attach button without selecting a file first.
Here is the firebug response for attaching "no file":
A fix could be to check if the file field is filled in before submitting (not tested):
That would also hinder the progress info to appear when no file is selected.
I noticed that the block page works in nightly builds of Safari (WebKit-r24872). Seems wise to wait for 1.1.4 though and hope for a fix for current versions...
The effect on new content is only visible in Safari nightly in my tests. In Firefox the speed has to be decreased, try
$(".ahah-new-content", new_content)[this.showEffect](1100); // ahah.js line 149
. In Opera9 and IE7 it never works. IE6 is kind unpleasant all over (table is too wide (probably related to the new tableheader) and everything jumps).Comment #35
alippai CreditAttribution: alippai commentedSubscribing.
Comment #36
alippai CreditAttribution: alippai commentedjQuery 1.2 is out (and it's attached to drupal), so you can continue with this issue (I found this patch today, but IMO it's now RTBC - I didn't test it, only read the comments - with the value checking)...
Comment #37
quicksketchI found the problem with Safari crashing. This line:
won't work in Safari because it cannot handle regular expressions on strings greater than 8K, see http://dev.jquery.com/ticket/1152.
To fix, I'm using this line of jQuery:
though I'd like to clean that line up a bit.
Another fix for Safari requires the parseJson function be added back into drupal.js. Sorry for the troubles here, but it looks like it'll be necessary when using the jquery.form.js library and Safari. The problem reported by fajerstarter above for the upload module has been fixed in another patch. Please review and post any problems!
Comment #38
quicksketchAfter some face-to-face feedback from Drupalcon, here's an updated patch with a much improved UI.
- The message to remind user's to click the "Save" button has been changed from 'message' to 'warning', making it yellow for greater attention.
- The progress bar image has been changed to the rotating circle throbber.
- The progress bar has been moved to be next to the element the user changed.
Comment #39
scor CreditAttribution: scor commentedI applied the patch on a subdirectory drupal install, and the throbber.gif doesn't show up because it's fetching /misc/throbber.gif instead of /subdirectory/misc/throbber.gif.
url(../../misc/throbber.gif)
should be used in the block.css file instead ofurl(/misc/throbber.gif)
.Once this is fixed and I select a value in a list, the listbox is disabled, and the throbber keeps spinning... nothing happens. I checked my logs and I got a POST request to /subdirectory/
Is there another patch to install, or is it supposed to work out of the box?
Comment #40
quicksketchThanks scor for the CSS bug. Here's an updated patch with the correct patch.
The problem you're seeing is likely because the menu system has not recognized the new menu path. Did you try rebuilding the menu by placing
menu_rebuild()
in your index.php? You can also enable/disable a module to clear the menu system.Comment #41
scor CreditAttribution: scor commentedI actually applied the patch before installing drupal. throbber ok now.
But I still have the same problem, and I found that if I activate the clean URLs, then it works. can you try with clean URLs off?
Comment #42
quicksketchThanks again scor for the thorough review. I'm seeing the same behavior with Clean URLS turned off. This turned out to be a problem with jQuery's $.ajax() method trying to help us out with the submitted data. If you passed in a url like '/?q=admin/build/block/etc', it would try to help you out by changing the url to '/', and adding '&q=admin/build/block/etc' to your data variable.
Conveniently, this 'feature' was added recently and has been removed again in the latest release of jQuery, 1.2.1. After applying this patch http://drupal.org/node/176222, which is currently help up by Konquerer, everything works as expected. Though I'll work on fixing the Konquerer problem in that patch, let's not hold up this one because of that problem also.
Comment #43
alippai CreditAttribution: alippai commentedhttp://drupal.org/node/176222 - fixed ;)
Comment #44
quicksketchConfirmed that the latest head version of jQuery makes the blocks page work correctly without clean urls.
Comment #45
webchickI did a functionality run-through in Safari, Firefox, and Opera on the Mac. Tested the blocks admin page, file uploads, and update system, with and without clean URLs enabled. Seems to work. I find the way the things just "appear" in their new spots in Opera/Safari a bit jarring. But this is something we could tweak later, methinks.
However, I think we need someone more skilled at JS, like kkaefer or Steven, to look this over for code-level review.
Comment #46
webchickBtw, I had better results testing this patch from a clean install... even with the menu_rebuild() and cache clearing, there were still some problems with display, etc. Might've just been my head being out of date.
Comment #47
quicksketchOn visibility, we can add a class to mark new rows with a separate background or text color. We could also keep a history of all the changed rows if wanted. But as webchick suggested, let's work on getting it in first then we can talk about recommended colors and clarity.
Comment #48
kkaefer CreditAttribution: kkaefer commentedDisclaimer: I have not applied the patch and tested the functionality.
I looked through the code, especially the JavaScript parts. Most things look pretty sane and clean. However,
var new_content = $('<div></div>').html('<div>' + response.data + '</div>');
needs some cleaning up: We don’t need that additionaldiv
inside thehtml()
method.I think the additions are overall a good idea, however I’m inclined to not vote for a commit, given that we are that late in the release cycle. The patch also renames a
tpl
file. I’m not sure if that’s a good idea. Is this change required? What I like, though, is the removal of vast.css()
statements and the use of classes instead.Comment #49
quicksketchKkaefer pointed out to me that
adds a redundant div around the added content. This patch now is just
so that we have a single wrapper div around newly added content. It also cleans up that line to a state that I'm now happy with it.
Comment #50
quicksketchI'd like to have a core committer go over this request, so I'm bumping up in priority for D6. I personally consider this to be a critical task, as the current implementation of ahah.js was meant to be a test bed, not the final version in Drupal. If it ships as-is, I would be very disappointed in my ability to get an adequate version shipping with Drupal 6.
The changes to the blocks system are purely a UI improvement, but not necessary for this patch to go in. The changes to the ahah framework however, are so very important because our current implementation is difficult to read, contains several inline styles, and doesn't work in Opera, and only under certain circumstances in Safari. The new implementation removes our dependence on the browser-incompatible and crufty redirectFormButton(). It also has the convenient side-effect of making upload module work in Opera (since it is dependent on AHAH framework now).
Comment #51
eaton CreditAttribution: eaton commentedI'm pretty unqualified to talk about the jQuery and CSS aspects of this patch. I noticed two specific things that might be worth tweaking:
With those nitpicks out of the way, and the caveat that I'm not any kind of jQuery expert, I think the patch really opens up a lot of potential for form interaction in D6. I'm hoping it gets in.
Comment #52
quicksketchThanks for the review! Responses to 1-3 in Eaton's review.
Comment #53
Gábor HojtsyStill discussed and developed => not RTBC.
Comment #54
bennybobw CreditAttribution: bennybobw commentedLooks like the latest patch isn't applying well. It's adding files that are already there.
Comment #55
scor CreditAttribution: scor commentedI notice the same thing, I also get 2 hunks failing on system.module
are you sure you attached the right patch, this one seems to be coming from the ahah part 1 (Add dynamic AHAH submission properties to Forms API). the numerotation is not consistent, it should logically be something like drupal_ahah2_7.patch, not drupal_ahah_9.patch :)
Comment #56
quicksketchOh dear, I did attach my patch from part 1 rather than the updated one for this issue. Here's the correct patch.
Comment #57
Dries CreditAttribution: Dries commentedIf this is considered to be an important clean-up, I encourage you to clean this up as part of this patch.
Comment #58
profix898 CreditAttribution: profix898 commentedI'm with Eaton that we should combine all #ahah properties in a single array, eg.
$element['#ahah']['event'] = 'foo'
. The various #ahah_* properties (ie. effect, method, wrapper) are actually options required for AHAH rather than true properties of the form elements IMO.Otherwise this patch is really awesome, it cleans up the crappy JS (redirectFormButton, ...) and open the door for amazing applications in D6.
Comment #59
quicksketchMuch thanks to Dries and Gábor for their reviews.
It seems like we're pretty much at a consensus that #ahah should be a single array type property. I'll reroll this patch with those changes (it shouldn't be difficult at all).
As for renaming, I don't have a problem with it really, but it's going against the jQuery file-naming convention that I've seen. Most libraries that add new methods to the jquery library are given this sort of file name including jCarousel, Accordion, Forms, and others. But then again interface and thickbox are just called interface.js and thickbox.js. I guess it's a matter of whether or not we think renaming files from other projects is a good idea in the future.
Comment #60
quicksketchHere's a patch with a single #ahah property as an array of options. I'll wait to hear confirmation on renaming the file before updating for that change.
Comment #61
quicksketchI found a side effect of the single array structure. It means that user's will need to set an event for the javascript to be triggered. Previously, we had set #ahah_event defaults for every form element (such as 'change', 'click', 'blur', etc). Now with the single array format, most definitions for AHAH will look like:
I removed the default event properties to make usage consistent. I think it makes #ahah more clear, since developers will be immediately made aware of the #ahah[event] property. Previously it was a property that would rarely need to be changed since a reasonable default was set. With a single #ahah property however, setting default #ahah[event] wouldn't be as helpful since developers will probably prefer to set the entire #ahah property at once with a new array.
Comment #62
webchickYeah, I don't think we should rename jquery.form.js, as that's the file name from the original source: http://www.malsup.com/jquery/form/#download
Comment #63
kkaefer CreditAttribution: kkaefer commentedNate, just add an array with the default values (
+=
operator) to the#ahah
property at some point. This will add the array properties which are not yet present without overwriting already present values.Comment #64
quicksketchThanks kkaefer for that suggestion. I put in the defaults into a switch statement in form_expand_ahah(), so a default event is added if $element['#ahah']['path'] is set, but $element['#ahah']['event'] is not. This patch is now functionally equivalent to the version before #ahah was split into an array.
Comment #65
bennybobw CreditAttribution: bennybobw commentedAfter applying this patch in head, when I try to go to admin/build/block. I get:
But if I add a module, and go to admin/build/block it works.
I'm thinking it might have to do with rebuilding the menu?
Comment #66
webchickYes, please note to test this patch you'll need to clear your cache tables and also place a menu_rebuild() in index.php (for one page load only). That string offset things happens when one of these two steps isn't followed.
It might be easier to test on a fresh install.
Comment #67
anders.fajerson CreditAttribution: anders.fajerson commented1. Rows change place even if not changed weight. To reproduce: 1. Set primary to "left region". 2. Change the weight for any of the "disabled blocks". The left region blocks then randomly changes place with eachother.
Minor stuff:
2. A rare one, related to tableheader: if you change a block and instantly resizes the browser window (tested in Firefox), a JS error is thrown. Removing tableheader.js "fixes" this.
3. Usability: On a page with many blocks and a small screen, the yellow status text will be scrolled out of the viewport. I like that it pops up (makes it more noticable), but it might be better to always show it so there is at least a chance to see it once when scrolling down a long page.
4. Usability: Make "Save button" italics in the status text?
5. CSS: The padding on the status page is now 2px, it should not override the standard .messages padding.
I'd love to test the actual API of this as well, are there any documentation available yet?
Comment #68
quicksketchGreat feedback all, fajerstarter. Replies:
Fixed in this version. It turns out enabled blocks have never been sorted by weight and then title, like one might assume. The order has always been random with 2 blocks at the same weight, though it was "consistently random". I'm not sure how this is, because they're sorted with usort(). According to the PHP docs for that function,
This seems to be a problem with tableheader.js trying to use it's behavior on content that is no longer on the page. I wrote http://drupal.org/node/179937 to patch the problem, but I think this might be indicative of a larger problem we'll need to solve in D7.
I'd like to handle this in an entirely different manner. I'd like to have a behavior for 'savechanges'. This behavior would alert a user when leaving a page without saving the changes. It would present the user with the option to save the changes, do not save changes, or cancel and not leave the page at all. Anyway, I'll say we post this as a separate patch also.
Fixed.
I didn't change anything with the status message CSS. I think this is the intentional styling when using the messages div outside of the messages area. You get similar message divs when your system is checked for Clean URL support during the install.
I put some up in the original patch: http://drupal.org/node/154398#comment-266514, though looks like it's not very accessible any more. The patch version should still work. It'll need to be updated to include new functionality in this version of course.
Comment #69
scor CreditAttribution: scor commentedbennybobw: to force the rebuild of the menu, you can also just enable or disable any module, it may be easier than editing index.php and adding menu_rebuild().
Nate: in block_menu(), since you don't use drupal_get_form on admin/build/block any longer, you need to add
'page callback' => 'drupal_get_form',
to the child items admin/build/block/configure and admin/build/block/delete menu items. As it is, you can't access the configure or delete page of the blocks.
Comment #70
Dries CreditAttribution: Dries commentedI tested this patch with Firefox and it looks good.
The blocks in the 'Disabled' region of the block administration page didn't seem to be "moving". That looks like a bug -- if it is not, the behavior is confusing and inconsistent with the other regions. All the other regions seem to work.
This comment wasn't 100% to me. It would be great if you could explain (in the code comments) _why_ you are attaching the AHAH events to the button.
I think this patch should go into Drupal 6 beta2 ... after it has been confirmed to work on nearly all browsers.
Comment #71
scor CreditAttribution: scor commentedI applied the last patch ahah2_11 and then installed drupal. On Ubuntu Firefox 2.0.0.6, I notice a slight jump of the height of the row when the throbber appears. It first appears on the left under the select field, and then floats to the right.
See this screencast: http://openspring.net/files/issues/ahah2_11-jumpy.ogg
I noticed a similar problem in IE7.
No such problem on firefox/windows, opera/ubuntu and konqueror/ubuntu.
Comment #72
recidive CreditAttribution: recidive commentedI've re-rolled the patch trying to solve the problem described on #71.
I've changed the css from:
To:
Now the throbber shows up direct on the right, but the column is still being stretched a little bit. This is mainly due to that margin being set on the select element. Moving this to the div:class=form-item should solve the problem, but this would require changes in js, e.g. to set the class progress-disabled on that element instead. Despite this minor issue, I think the patch is ready to go.
Comment #73
scor CreditAttribution: scor commentedWhich browser do you use? this doesn't seem to solve the problem on ubuntu firefox.
if the throbbers were displayed when the page loads but hidden with the
visibility: hidden
CSS rule, they'll take the room they need. all we'd have to do to display them is to switch tovisibility: visible
when necessary.Comment #74
recidive CreditAttribution: recidive commentedHi, I use firefox 2.0.0.5 on OpenSuse (Mozilla/5.0 (X11; U; Linux i686 (x86_64); pt-BR; rv:1.8.1.5) Gecko/20061023 SUSE/2.0.0.5-1.1 Firefox/2.0.0.5).
The throbber is not being hidden/unhidden, they are actually added/removed.
Comment #75
scor CreditAttribution: scor commentedOh, yeah I know that ;) but I just made a suggestion to fix the issue: display them hidden so that their space is already allocated, and make visible when needed.
Another idea may be just to use absolute positioning.
Comment #76
quicksketchHere's a very good shot at full browser compatibility, plus a few changes requested.
I totally agree. Since we've touched on the sort function now anyway, this is a very logical change. It's fixed in this version.
I've replaced with this more verbose comment:
I think it's a great change, as people will often go hunting for examples in core.
As for browser compatibility, I added CSS to make the throbber appear properly and nearly identical in IE6/7, Safari, Firefox (Mac/Windows) 1.5 and 2.0, and Opera 9 (Mac/Windows/Ubuntu). There is no shift in column width or row height in any of these browsers. Unfortunately I couldn't find a fix to the jumpy issue in Firefox on Ubuntu reported by scor in #71.
Because I couldn't bear to remove the nice fade effect from other browsers, I put in a special case for Safari when it comes to using effects on table rows:
This makes it so Safari no longer does the improper table resizing when showing the new row. I think it's important to include this special case for table rows, since we'll probably want to add the fade effect back to upload module now that we have the ability to do that through the AHAH framework.
To help the effects-challenged browsers (nearly everything except Firefox it seems), I've added color highlighting on rows. In #45, webchick suggested that the addition of new rows was a bit jarring. Though in #47 I said I'd include this in a later patch, perhaps adding it now will put to rest any concerns about losing track of changes. It should also help with people leaving the page without saving, though I still plan on implementing a sort of javascript alert I described in #67 in a separate patch.
Comment #77
webchickOh. My. GOD!
I LOVE this!!
For those who don't know how to apply patches, I've attached an annotated screen shot so you can see how awesome this is.
I've tested the blocks interface thoroughly, as well as file uploads, in OS X Safari 2.0.4, Firefox 2.0.0.7, and even Opera 9.0.2 for kicks. Also tested both with and without JS enabled. It seems like to me that everything is working.
If we can get someone to chime in on IE 6/7, I think this sucker is RTBC.
Comment #78
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedTested on IE 6/7: Everything works..
Nice patch and usability boost, wish we had this before. I can think of *a lot* of other places where this would definatly be needed to ensure our usability gets to a higher level.
This is RTBC
Comment #79
anders.fajerson CreditAttribution: anders.fajerson commentedI'm not willing to remove the RTB, but some nitpicking (to use Webchick words ;) ):
* I really can't see any fade effect (WindowsXP: IE6, IE7, Safari, Opera 9, Firefox 2). It sort of flashes in Firefox 2, which usually means jQuery is trying to fade in/out, but sort of fails. Or my eyes are just too slow...
* I would add the (very nice) star to the status message: "* Your settings will not be saved..."
* Opera 9 is really jumpy. Not sure what's causing it, seems random. Sometimes the whole screen flashes, sometimes the row is just increased in height. NOT a show stopper to me.
The color and star makes all the difference, great!
Comment #80
quicksketchfajerstarter, the fade is something that is a little hit or miss. I can clearly see the fade in Firefox (Mac), and a sort of delayed appearance in Opera. Either way, it's not actually breaking the layout so I'm not heavily concerned. I get the feeling that it's something that will improve with time, through updates to jQuery and various browsers. Increasing the effect interval time seems to have no effect on the browsers that don't show the fade currently.
It's better to include the effect for those browsers that can display it and since it doesn't cause a problem in other browsers, I'd like to keep the patch as-is. If a more effective way of fading in a table row comes to light, I'd love to apply it to the AHAH implementation.
Comment #81
Gábor HojtsyThe interface improvements look great, and it also seems that you fixed all concerns Dries had, so committed. Thanks for all the hard work.
Comment #82
eaton CreditAttribution: eaton commentedHooray for cool UI effects that degrade gracefully! Thanks for all the awesome work on this. :D
Comment #83
anders.fajerson CreditAttribution: anders.fajerson commentedSorry for not seeing this earlier, but I thinks it sneaked in a bug:
Clicking "configure" on the block admin page gives this error and no config page:
notice: Undefined index: block_admin_configure in W:\www\drupal-cvs\includes\theme.inc on line 48.
I'll open a new issue if it's not related to this (or should I have done it anyway?).
Comment #84
Dries CreditAttribution: Dries commentedGreat job people. I also like the '+ 1' picture that Angie threw in -- made me smile. :)
Comment #85
Gábor Hojtsyfajerstarter: indeed, I can reproduce the bug and it is most probably caused by this change. Let's fix the issue!
Comment #86
scor CreditAttribution: scor commentedI mentioned this bug in #69 already.
the patch fixes the configure and delete page (for custom blocks).
(don't forget menu_rebuild() before testing)
Comment #87
anders.fajerson CreditAttribution: anders.fajerson commentedGreat, works.
Comment #88
Gábor HojtsyThanks, committed.
Comment #89
anders.fajerson CreditAttribution: anders.fajerson commentedSo, drag and drop in Drupal 7? ;)
http://drupal.org/node/181066
Comment #90
quicksketchNext targets for AHAH in core:
http://drupal.org/node/157752 (poll module, code needs review)
http://drupal.org/node/181125 (book module, stub issue)
http://drupal.org/node/181126 (menu module, stub issue)
These patches shouldn't need to change the AHAH framework any further, though it's likely that they'll expose small issues with ahah.js as we use it in more places. The poll module patch I posted this morning includes one such fix. So for the ahah-excited, please go review. :)
Comment #91
scor CreditAttribution: scor commentedfor the first link I guess you meant
http://drupal.org/node/155870 (poll module, code needs review)
Comment #92
(not verified) CreditAttribution: commented