The 'use-ajax' class allows you to have a link that loads content via ajax when clicked. One possible use case for this is tabbed content, where clicking on a tab loads the tab's content via ajax. However, one would expect that once the content has been loaded the first time, it should just stay there, and in the case of tabs it would just be shown or hidden based on subsequent tab clicks. There is a handy new method in jQuery called .one() which works similarly to .bind() except that the attached behavior is removed after it has been invoked the first time. It would be nice to have the flexibility to tell the ajax system to use this instead of .bind() by just adding an extra class to your link, e.g.
<a href="mypath/nojs" class="use-ajax ajax-once">My Tab</a>
The attached patch achieves this flexibility.
Comment | File | Size | Author |
---|---|---|---|
#48 | ajax-bind-once-769258-48.patch | 2.03 KB | driki_ |
#46 | ajax-bind-once-769258-46.patch | 2.03 KB | driki_ |
#39 | 769258-ajax-bind-once.patch | 2.36 KB | katbailey |
#36 | 769258.ajax-bind-once.35.patch | 2.3 KB | rfay |
#35 | 769258.ajax-bind-once.35.patch | 2.3 KB | katbailey |
Comments
Comment #1
sunThese should be proper sentences, starting with a uppercase letter and ending in a period.
Hm. Isn't there some nifty dynamic method callback construct we could use here? I.e. define "bind" as default bind method, override it via the CSS class, and call something along the lines of:
Trailing white-space here.
107 critical left. Go review some!
Comment #2
katbailey CreditAttribution: katbailey commentedHeh, that's exactly the way I tried it at first! The problem is that element_settings.bindMethod is a string not a function, so there's no way to do that without using eval somewhere along the way. Unless you mean there's some way I can create a pointer to the jQuery bind() method..? Anyway, I'll make those other changes...
Comment #3
rfaysubscribing
Comment #4
katbailey CreditAttribution: katbailey commentedMade those punctuation changes, will see if I can come up with anything better for that other issue...
Comment #5
rfay@katbailey, could you please post a boiled-down module that demonstrates this use-case and allows easy testing of the patch? Since we don't have testing for javascript, we need easy ways to experiment with things like this.
Comment #6
rfayComment #7
katbailey CreditAttribution: katbailey commentedOK, for now I'm just attaching a version of ajax_example module where I've changed the ajax link example to render two links, one that uses the ajax-once class and one that doesn't. So, there are no actual tabs here. However, I almost have jQuery UI Tabs and the Ajax framework working in perfect harmony together to create the ultimate D7 tabs solution (Quicktabs ;-)), and it needs this change! (See here for more info on that: #553070: Merge with tabs module for D7?)
Comment #8
katbailey CreditAttribution: katbailey commentedHere's a demo of my use case: jQuery UI Tabs + Ajax framework = awesome :-) http://174.143.25.221/
The point is that once we've used the ajax framework to load the tab content via ajax, jQuery UI tabs can take over and just show/hide as needed.
It should be noted that using this "ajax-once" option will require another click event handler that at least just does
return false;
so that the browser doesn't follow the link now that the original event handler has been detached. In the case of the tabs example, jQuery UI Tabs' click handler does this for us. However in the ajax_example module above, I needed to add an extra js file to provide this basic click event handler.Comment #9
RobLoachAwesomeness! We need more jQuery.one in Drupal.
Comment #10
katbailey CreditAttribution: katbailey commentedI've been thinking some more about the possibility of calling functions whose name is stored as a string in a variable, as per Sun's comment above. Another use of that would be the ability to specify a js callback function in your ajax commands so you could run your own js after ajax.js has worked its magic, which would also increase the flexibility of this mechanism. I remembered that ajaxsubmit module does such a thing and had a look at the code that invokes callback functions - here it is:
Looks like eval is the only way.
So, for the purposes of the problem I mention above, I think the patch as it stands is the best way to go - no need for eval here. As to whether we want to add the custom callback option, I'll leave that for another issue. I will try to get more eyes on this one - I guess the main problem is lack of use cases as not many people are using this mechanism yet.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedsubscribe
Comment #12
katbailey CreditAttribution: katbailey commentedThis is just a slight change to the patch, separating out the 'return false' behaviour into its own click handler that always gets bound using .bind() so that it's only the actual ajax behaviour that would be bound using .one(). This way it's not necessary for a module that uses this mechanism to attach its own click handler in order to prevent the browser following an ajax link that's been bound using .one().
Comment #13
Sid_M CreditAttribution: Sid_M commentedI apologize for not rolling a patch, but I'm not set up for that on this machine. Nonetheless, here's how to implement a dynamic callback function as discussed in #1 and #2. As Sun suggests, create element_settings.bindMethod which can equal 'bind', 'one' or any other jquery method name as appropriate. Then use js's syntax for tokenizing a string, which is to put it in brackets. So these two method calls:
get replaced by this one
Note that the same approach can be used to eliminate the eval statement in #10.
In case the above isn't clear, here's a bit more. Anywhere there's a dot, it can be replaced with brackets, and the brackets can contain any expression (including function calls) which evaluates to a string which is the name of a property or method contained by the object which precedes the opening bracket.
In cases where a variable or function is global, precede the brackets with window. For example:
Comment #14
katbailey CreditAttribution: katbailey commentedSweet! I remember seeing js code like this before and not being able to make head or tail of it - now it all makes sense! Thanks, Sid_M :-)
Updated patch attached.
Comment #15
sunShouldn't we support .ajax-once for other integration methods than .use-ajax, too?
1) We do not have to split the function into 'func'; just replace the original line containing bind().
2) To make any sense of the "return false", the second event handler has to be bound after the AJAX event handler, not before. I'd additionally suggest to only conditionally add it, if bindMethod != 'bind'.
3) Anonymous functions should have a space after "function", and there should also be a space before the opening curly brace {.
4) Trailing white-space.
Powered by Dreditor.
Comment #16
katbailey CreditAttribution: katbailey commentedThanks for the feedback, Sun. I had wondered about your first point, i.e. whether to allow this option for elements using #ajax, when I was doing the last iteration of the patch. I can certainly go ahead and add that in. As to the other points:
1) Yes, this was a holdover from when I had to do the if/else statement and had two versions of the function call - will change it back to an anonymous function.
2) I'll change the order. I had actually moved 'return false' out of the other click-handler so then you would need it regardless of .one or .bind. You think it should be in the ajax click handler anyway and just bound again separately if bindMethod != 'bind'? I guess that might be slightly better.
3) Yessir
4) Yessir
;-)
Comment #17
katbailey CreditAttribution: katbailey commentedRe-rolled as per #16.
Comment #18
sunExceeds 80 chars. (can be shortened)
Can we move the inline comment above the if condition? It's easier to read the code flow that way.
Additionally, the second comment just repeats what can be easily read from the following code lines. It should explain the usual "why?" and "WTF?" questions instead. ;)
96 critical left. Go review some!
Comment #19
katbailey CreditAttribution: katbailey commentedBetter?
Comment #20
sun"By default, behaviors are bound with .bind()."
Can we move the comment above the condition?
"If the behavior was only bound once, the browser's default behavior needs to be prevented on subsequent events."
("prevented" is not JS jargon in this context, but I just have a blackout, so there's probably a better term.)
Powered by Dreditor.
Comment #21
sunBetter title.
Comment #22
sunLooks ready to fly for me.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedWhy do we need to check for the class here? This is the code block that has #ajax bindings. Shouldn't the PHP code be responsible for adding bindMethod to it?
Any reason not to remove the first "return false" and always bind the function that just returns false?
Powered by Dreditor.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedAlso, for the code that must check for class (because links don't have #ajax bindings), what about using a class of ajax-bind-method-METHOD (i.e., ajax-bind-method-one)?
[EDIT: or even crazier, ajax-setting-SETTING-VALUE, so something like ajax-setting-bindMethod-one. Seems like there might be other use-cases for needing settings on links.]
[EDIT: sorry, ignore this comment. Advanced use-cases can use #attached to add settings. Here, we want something easy for a common use-case. Given that, "ajax-once" makes sense.]
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedMaybe it's just bike shedding, but how about this?
Comment #26
sunomg - let's not make this even more complicated than it is already, please! :)
(also, I actually hoped for a RTBC here finally as this is just one of the issues that fix ajax with links... ;)
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedDiscussed with sun. The differences between #22 and #25 only start mattering if and when other AJAX patches land. #22 solves this issue completely adequately given current HEAD. Tweaks to the logic can be rolled into the other patches. So, re-uploading #22 and RTBC'ing it.
Comment #28
aspilicious CreditAttribution: aspilicious commented#27: drupal.ajax-bind-once.22.patch queued for re-testing.
Comment #29
aspilicious CreditAttribution: aspilicious commented*Retesting old patches*
Comment #30
katbailey CreditAttribution: katbailey commentedbump - not sure why this is still stagnating here :-(
Comment #31
sun#27: drupal.ajax-bind-once.22.patch queued for re-testing.
Comment #32
sun#27: drupal.ajax-bind-once.22.patch queued for re-testing.
Comment #34
pfrenssenI tried rerolling this but the patch can not be applied anymore since #939568: Enable finer control for ajax-using modules of the trigger response in the ajax object got in.
Comment #35
katbailey CreditAttribution: katbailey commentedHere's a re-roll.
Though I've just been looking through the issue mentioned in #34 which I was oblivious to at the time and realising that if that patch had just gone a little further, then we wouldn't need this one so much. That patch enabled other modules' js to overwrite the event response because it is now a named function. But it's being called by an anonymous function which is actually what gets bound to the element. If we bind a named function as the event handler then we can unbind it, i.e.
Thoughts?
Comment #36
rfayHas to go into D8 first. Shucks. Attaching #35's patch to get it tested on D8.
Comment #38
katbailey CreditAttribution: katbailey commentedOK, I'll try and do a reroll this evening...
Comment #39
katbailey CreditAttribution: katbailey commentedLet's see if this one applies...
Comment #40
blup CreditAttribution: blup commented+1
Comment #41
RobLoachHerro, patch bot?
Comment #42
katbailey CreditAttribution: katbailey commentedThanks Rob! Was wondering why it was being ignored... duh
Comment #43
driki_ CreditAttribution: driki_ commentedJust tested this patch on drupal 7 for some use case we have. Looks to work fine to me.
Comment #44
driki_ CreditAttribution: driki_ commentededit : changed core version, category.
Comment #45
aspilicious CreditAttribution: aspilicious commentedThis is a bug and we need to fix bugs first in D8. When it's committed to D8 it will be committed soon to D7.
Comment #46
driki_ CreditAttribution: driki_ commentedthis is the patch from comment #30 with the path corrected ( core/misc )
Comment #47
aspilicious CreditAttribution: aspilicious commentedtrailing whitespace, in 2 places and there is an unneeded newline.
0 days to next Drupal core point release.
Comment #48
driki_ CreditAttribution: driki_ commentedremoved newline and whitespace
Comment #49
nod_This need to use
e.preventDefault()
instead ofreturn false
.Also I'm not really getting the use of
.one
if you need to do a.bind
on it later on. Can't you just do a.bind
and have your if inside that?Better yet have a
.bind
canceling events for every element and add either a.one
or a.bind
depending on the class? You can have several listeners for one event.Comment #52
heddnI think this needs some significant attention. Marking as postponed since we need to confirm this is still an issue as a lot of time has passed in the past 5 years.
Comment #61
quietone CreditAttribution: quietone at PreviousNext commented@katbailey, Thank you for reporting this problem. We rely on issue reports like this one to resolve bugs and improve Drupal core.
Is this issue still a problem?
There has been no activity on the patch for 10 years and more information was asked for 5 years ago.
Since we need more information to move forward with this issue, I am keeping the status at Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks
Comment #63
Kristen PolThanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.
As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" more seven year ago and there has been no activity here since that time other than @quietone asking for more information.
Per #61, I'm marking the issue "Closed (cannot reproduce)". If anyone can provide complete steps to reproduce the issue (starting from "Install Drupal core"), document those steps in the issue summary and set the issue status back to "Active" [or "Needs Work" if it has a patch, etc.].
Thanks!
Comment #64
Kristen PolWhoops. Forgot to close but... @klonos noticed that this could be moved to D7 queue.