date_popup outputs a separate inline JS block to initialize each popup-enabled text field. This requires a separate jQuery() scan through the DOM for each one, which quickly gets unacceptably slow if you have a multiple date field (or several!).

The attached patch reduces this to one inline block/DOM scan for each unique combination of function/arguments to be called.

I wrote this last night and, this morning, I'm thinking there are at least two better approaches:

1. Attach the function(arguments) call (e.g. timeEntry({ ... settings ... })) to the DOM element directly as an event handler via onload or similar (or maybe even onfocus). This way, no scans through the DOM are required as each invoked event handler automatically identifies the DOM element to operate on.

2. If for some reason that does not work, output a single CSS class for all elements that should be initialized via this system, search for them all once, then invoke the correct function(arguments) on each one based on other CSS classes assigned to the element (the whole set of required function/arguments data having been output in a single call to drupal_add_js, probably by just adding it to Drupal.settings).

Anyway, here's my patch to get started.

CommentFileSizeAuthor
#17 date-popup-id-244025-17.patch9.28 KByched
#15 date-popup-id-244025-15.patch9.04 KByched
#14 date-popup-id-244025-14.patch9.17 KByched
#11 date-popup-id-244025-11.patch3.86 KBbjaspan
date-popup-settings.patch3.21 KBbjaspan

Comments

bjaspan’s picture

bjaspan commented
Status: Active » Needs review
karens’s picture

karens commented
Status: Needs review » Fixed

I knew there was a better way to do this and was hoping someone would provide a patch :)

I'm sure there are lots of other ways to make this work better, but I'm no jquery expert so I'm not sure of the best approaches and am open to ideas.

I'm committing this much, if you have more ideas, please let me know. Thanks!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

karens’s picture

karens commented
Version: 5.x-2.0-rc » 6.x-2.x-dev
Status: Closed (fixed) » Active

This works fine in the D5 version but breaks in D6 when new fields are added using the AHAH 'add more' button. Now when you add a new field, the jquery calendar completely stops working.

I need ideas on how to fix this to work in D6 with AHAH.

karens’s picture

karens commented
Title: date_popup outputs too many inline JS blocks » JQuery Calendar quits working when you add a new field

Changing title.

karens’s picture

karens commented
Priority: Normal » Critical

This is pretty critical - you can't use popup dates with the AHAH add more button.

yched’s picture

yched commented

Karen, this sort of bug often means the JS code has to be implemented as a "Drupal.behavior", allowing it to be automatically re-executed by the AHAH API whenever new content is updated.

http://www.raincitystudios.com/blogs-and-pods/katherine-bailey/the-lowdo...
and http://drupal.org/node/114774#javascript-behaviors have good info about it.

karens’s picture

karens commented

Thanks yched, I knew there must be some way. Now, if I only understood how it works :)

I'm hoping someone will provide a patch to get this working right. If not, I'll try to bulldoze my way through it later. If anyone does look at this, be sure to pick up code with today's commits. I found I had left out part of Barry's original patch initially and just got that fixed this morning.

yched’s picture

yched commented

I'll try to look into it, but it might not be before 2-3 days :-/

bjaspan’s picture

bjaspan commented

It's good that this issue got re-opened because it reminded me of something I learned about it 6 weeks after submitting the patch.

Performing a complete DOM scan in JS is very slow, and every jQuery top-level search for a class (e.g. $(".classname")) performs such a scan. Performing lots of scans on a single page is EXTREMELY slow. The date_popup module performs a DOM scan for every unique set of settings (with this patch) and for every date element (without the patch). On a node edit form with many date fields, the date_popup DOM scans can cost tens of seconds by itself.

Retrieving elements by id is *much* faster. I guess the browsers keep an internal dictionary of ids so the DOM method getElementById() is fast (I've heard that getElementByClass() will exist eventually but does not yet).

So, I backed out the patch I posted for this issue and replaced it with a different one that sets a unique id for every date element, and outputs a separate set of settings for each element (like the code did before my patch), and uses the id to figure out which settings to apply to each element. It is MUCH faster.

An even better approach would be to combine the two solutions. Output a unique id for every date element, a single block of JS for each unique combination of date_popup settings, and another array indicating which block of settings should be used for each date element id.

Does this make sense?

bjaspan’s picture

bjaspan commented
StatusFileSize
new date-popup-id-244025-11.patch3.86 KB

Here's my latest patch. It is not RTBC as is but shows the intermediate approach of using ids instead of classes.

karens’s picture

karens commented

Actually most of the settings are the same always and only a few settings need to change for each unique field or element. I know there is a way to set up some global settings for the calendar that will never change, then only worry about settings which are different in the individual ids/classes. I don't know how much that helps things, but that's another change to think about. Actually almost everything but the date format and the year range will never change.

I also need to bump up to the newer version of the jQuery calendar popup. I haven't so far because it has API changes so the settings are different, but it adds lots of new things, like the ability to select a date range as well as an individual date. And both the current and the new code have ways you can have the from and to dates restrict each other so you can't select a from date greater than a to date.

But the starting point is to get this much working.

yched’s picture

yched commented

Barry, the patch in #11 doesn't seem to be generated against latest 6.x-2.x ?

yched’s picture

yched commented
Status: Active » Needs review
StatusFileSize
new date-popup-id-244025-14.patch9.17 KB

Attached keeps barry's approach in #11 and
- updates it to D6 (his patch was for D5),
- implements the JS calls as 'behaviors', so that they can be reattached after a JS-'add new'
(this also works thanks to jpetso's patch in http://drupal.org/node/275192, that got committed recently)
- drops the static $id count in favor of ids derived from the actual FAPI #id of the enclosing element, so that the same form element always gets the same settings no matter how the form gets AHAH-updated.

Seems to work OK for 'calendar' popups, but I couldn't get any 'timeEntry' stuff to display - dunno if it's normal in the current state of date.module.
Also, I'm not sure what Barry referred to when saying "It is not RTBC as is" in #11 above, so this patch might not be as well :-)

Patch is against 6.x-2.x-beta3 (I couldn't get HEAD to work at the time I started this).

yched’s picture

yched commented
StatusFileSize
new date-popup-id-244025-15.patch9.04 KB

Previous patch had some leftover cruft.

yched’s picture

yched commented
Status: Needs review » Needs work

Hm, looks like I posted too soon. The patch actually doesn't completely work with JS-'add new', I must've broken something with a last minute change.
I'll come back to it tomorrow.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new date-popup-id-244025-17.patch9.28 KB

This was breaking because of http://drupal.org/node/273539, that now runs an additional form_builder() on js-'add more', so this was messing with js settings getting attached to form id's twice, resulting in invalid js settings array. I am not 100% sure the fix for #273539 is the right one, but the case also happens when using 'Preview' (the form gets built twice in the request)

Attached patch works around this by appending a static index to the #ids generated by date_popup_js_settings_id().
(note : this differs from Barry's #11 because we need those ids to be specific to a given field - if not, when a form has several date fields, you could end up with non-unique ids when playing with js-'add more')

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Status: Needs review » Needs work

Patch no longer applies against -dev, 2 hunks failed. Tried to sort it out but it looks like some stuff has moved around.

karens’s picture

karens commented
Status: Needs work » Fixed

I got everything working except for a problem with the timepicker which I'm going to open as a separate issue. I can now get the calendar popup working in multiple fields.

Thanks yched :)

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

alan d.’s picture

alan d. commented

Just a side note, while this patch automatically means that 99% of the work is done for you, you must include drupal_get_js in your AHAH response for this to work.

Eg:

<?php
$output = theme('status_messages') . drupal_get_js() . drupal_render($target_form);
?>