Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow up for #1775530: Move picture into core
Problem/Motivation
If an AJAX callback is sending a new picture it isn't processed.
Proposed resolution
Add a listener so the polyfill is executed after every AJAX request. The listener only has to be available if jQuery and Drupal are defined, so we don't load jQuery on every page.
Remaining tasks
Needs manual testing.
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#49 | i1836860-49-ajax.patch | 1.62 KB | Jelle_S |
#47 | i1836860-47-ajax.patch | 1.6 KB | Jelle_S |
#44 | i1836860-38-ajax.patch | 1.61 KB | durgesh_singh |
#42 | i1836860-38-responsive_images.patch | 1.61 KB | durgesh_singh |
#41 | i1836860-38-responsive_images.patch.txt | 1.61 KB | durgesh_singh |
Comments
Comment #1
attiks CreditAttribution: attiks commentedComment #2
nod_Adding a script and
library_alter
drupal.ajax
to depend on the behavior for the polyfill seems like it'd work.Comment #3
nod_woops, sorry for status change.
Comment #4
nod_so yeah, would be better to have that than checking for Drupal and jQuery which doesn't guarantee anything about ajax.
Comment #5
attiks CreditAttribution: attiks commentedTrue, what was I thinking :-)
Comment #6
nod_Comment #7
attiks CreditAttribution: attiks commentedSo this means both picture.js and picturefill.js gets loaded even if there's no picture to display?
Comment #8
nod_um that's right. We could just put the behavior in a file and remove the events bindings from the polyfill to avoid executing that all over the place.
I could live with #6, the file will be rather small and if people don't like it, they should not use the module.
Either way works for me, I'd prefer this one to avoid having contrib take that has an example and do crazy things.
Comment #9
attiks CreditAttribution: attiks commented#8 the good thing about the polyfill is that it works outside Drupal as well, this guaranties us the best compatibility with the standard.
The event bindings on the polyfill or needed for resize, so they have to stay, unless we switch to our new matchmedia model.
For this issue we need to find a solution without introducing more dependencies, and avoid loading files unless we really need them.
Comment #10
tstoecklerI don't think we usually specify this fore core libraries. If we do want something we should use some documentation page, but not this issue.
Comment #11
attiks CreditAttribution: attiks commentedThis is the same as has been done for matchmedia.js, see http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
But the bigger problem to solve is what to add to solve this problem.
Comment #12
attiks CreditAttribution: attiks commentedMoving to right component
Comment #13
attiks CreditAttribution: attiks commentedWrong issue
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedAny consensus on how to proceed here?
Comment #15
attiks CreditAttribution: attiks commented#8 we can not babysit contrib code, the patch in #1 only adds the js if needed, and although it looks strange it doesn't cause overhead if not needed.
@nod_ can you give feedback as well?
Comment #16
nod_#1 +1, no better solution right now.
Don't need the closure though.
Comment #17
attiks CreditAttribution: attiks commentedI'll try to reroll later today, unless someone beats me to it
Comment #18
attiks CreditAttribution: attiks commentedPatch rerolled, closure removed
Comment #19
Jelle_S#18: picture.js is missing from the patch.
Comment #20
attiks CreditAttribution: attiks commentedlet's try again
Comment #21
RainbowArrayJust curious where this issue is at, since the last patch was in December. Has this been resolved elsewhere? Is a reroll needed?
Comment #22
RainbowArrayIt appears this patch still applies if somebody wants to review it.
Comment #23
attiks CreditAttribution: attiks commentedFYI: picture is renamed to responsive_image so this needs a reroll once #2124377-74: Rename "Picture" module to "Responsive Image" module is committed
Comment #24
Eli-TComment #25
SGhosh CreditAttribution: SGhosh commentedThe underlying code seems to have changed a lot. The patch's code doesn't match with the current branch cod eat all. Also, will need to check whether the bug still remains.
Considering the change in name to responsive_image -
*
function picture_library_info() {
doesn't exist in responsive_image.module* hence the new js file picture.js or now responsive_image.js cannot be added in the function for inclusion either
The patch needs to be recreated. Changing status to needs work.
Comment #26
attiks CreditAttribution: attiks commentedIf anybody wants to work on this, keep in mind that we can no longer change the upstream polyfill
Comment #27
RainbowArrayThe most recent patch relies upon making a change to picturefill, which we won't be able to do. So how viable is this? Is there another way to tackle this?
Comment #28
attiks CreditAttribution: attiks commentedFiles needs to be renamed, but the change to the polyfill is no longer needed.
Postponing on #2260061: Responsive image module does not support sizes/picture polyfill 2.2 since it will add the latest polyfill version.
Comment #29
Wim Leers#2260061: Responsive image module does not support sizes/picture polyfill 2.2 got committed (YAY!), hence this is now unblocked :)
Comment #30
attiks CreditAttribution: attiks commentedQuick reroll, but this will load javascript on all pages where responsive images are used, even if there's no other javascript added, maybe we should try to detect if other javascript is added (especially ajax related).
Comment #31
Wim LeersComment #32
attiks CreditAttribution: attiks commented#31 @Wim Leers off course use alter.
The problem is that it is needed if a new picture tag is added so it gets processed.
Comment #33
attiks CreditAttribution: attiks commentedComment #35
Wim LeersSo:
That seems like a huge flaw in the polyfill?
Comment #36
attiks CreditAttribution: attiks commented#35 Not sure, I don't even know if you could listen to newly added DOM elements and I think most polyfills work like this.
Comment #37
attiks CreditAttribution: attiks commentedComment #38
Wim LeersThis will actually cause it to run upon loading the page also. Which is unnecessary.
I wonder if it'd be better to override
when there is no work to be done, but there are actual
<picture>
elements on the page.Profiling this at http://scottjehl.github.io/picturefill/, it doesn't seem like there's any significant cost. Can you confirm that, @attiks?
Let's s/drupal.responsive_image/ajax/.
Because, this is really only necessary for AJAX requests; the current name implies its' more generally useful, but it's definitely not.
Comment #39
Vikas.Kumar CreditAttribution: Vikas.Kumar commentedNeeds review.
Comment #40
attiks CreditAttribution: attiks commentedPatch looks good, having this executed on page load is not going to be a performance problem.
@vks7056 can you do the rename to ajax instead of drupal.responsive_image as @Wim Leers asked in #38?
Comment #41
durgesh_singh CreditAttribution: durgesh_singh commentedComment #42
durgesh_singh CreditAttribution: durgesh_singh commentedComment #43
attiks CreditAttribution: attiks commented#42 Thanks for the patch, but the rename is not done:
drupal.responsive_image
should beajax
Comment #44
durgesh_singh CreditAttribution: durgesh_singh commentedyou means file name should be i1836860-38-ajax.patch
Comment #45
dcam CreditAttribution: dcam commented#44 applies to HEAD. Removing the "Needs reroll" tag.
Comment #46
attiks CreditAttribution: attiks commentedComment #47
Jelle_SNew patch:
Comment #48
attiks CreditAttribution: attiks commentedrename to responsive_image.ajax
responsive_image.ajax.js
Comment #49
Jelle_SNew patch based on #48
Renamed the behavior to responsiveImageAJAX since the standard seems to be camelCase for behaviors.
Comment #52
attiks CreditAttribution: attiks commentedThis is good to go
Comment #53
alexpottCommitted 3473bd5 and pushed to 8.0.x. Thanks!