Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Carousel: error thrown in originalDimensions function #11191

Open
keoshi opened this issue Jan 24, 2019 · 52 comments · Fixed by #20010, #20101 or #20149
Open

Carousel: error thrown in originalDimensions function #11191

keoshi opened this issue Jan 24, 2019 · 52 comments · Fixed by #20010, #20101 or #20149
Assignees
Labels
Customer Report Issues or PRs that were reported via Happiness. aka "Happiness Request", or "User Report" [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Pri] Normal [Status] In Progress [Type] Bug When a feature is broken and / or not performing as intended
Milestone

Comments

@keoshi
Copy link
Contributor

keoshi commented Jan 24, 2019

Steps to reproduce the issue

  1. Activate recommended features in Jetpack.
  2. Create new post and add a 1) Tiled Gallery and 2) a Gallery block in Gutenberg.
  3. Upload these two images. Add a long caption to the last one.
  4. Publish post and click one of the images to display the carousel.

What I expected

  • The clicked image to be displayed correctly in the carousel.

What happened instead

  • Carousel came up empty (both the image, caption, exif data).
  • Site is halted and clicks on the carousel don't work because of a JS error.
  • Console shows this JS error:
Uncaught TypeError: Cannot read property 'split' of undefined
    at a.fn.init.originalDimensions (jetpack-carousel.min.js?ver=20190102:4)
    at a.fn.init.e.fn.jp_carousel (jetpack-carousel.min.js?ver=20190102:4)

image

Which seems to originate from the originalDimensions function:

var splitted = $(this).data('orig-size').split(',');

Screenshots

gallery-carousel

@keoshi keoshi added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. labels Jan 24, 2019
@jeherve jeherve added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 24, 2019
@jeherve
Copy link
Member

jeherve commented Jan 24, 2019

I can't seem to reproduce right now, with the same images.

screen recording 2019-01-24 at 12 36 pm

Was there any other plugin installed on the site? Can you reproduce with other images? Were there any issues when you uploaded the images?

@keoshi
Copy link
Contributor Author

keoshi commented Jan 24, 2019

@jeherve No other plugins installed. The post I was testing this on had a tiled gallery on it as well, but that's it.

You can see it here: https://unaware-water-10.jurassic.ninja/2019/01/24/testing-carousel/

@keoshi keoshi removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 24, 2019
@keoshi
Copy link
Contributor Author

keoshi commented Jan 24, 2019

And boom, site is gone. Did you manage to test it out before it went dark, @jeherve ? That's why I didn't add a link on it on the original issue.

@jeherve
Copy link
Member

jeherve commented Jan 24, 2019

oh sorry, I was looking at another issue! :(

Can you reproduce with a brand new site now?

@keoshi
Copy link
Contributor Author

keoshi commented Jan 24, 2019

No worries, @jeherve — will try to reproduce and report back.

@keoshi
Copy link
Contributor Author

keoshi commented Jan 24, 2019

@jeherve Yep, managed to reproduce here: https://marxist-caribou.jurassic.ninja/2019/01/24/testing-galleries/

It seems that this error is only triggered when a simple gallery is used in tandem with a tiled gallery block. As you can see it doesn't happen with just the gallery block: https://marxist-caribou.jurassic.ninja/2019/01/24/testing-carousel-with-simple-gallery/

@jeherve
Copy link
Member

jeherve commented Jan 24, 2019

Excellent, nice catch!

Related: #8033, #6638

@getdave
Copy link
Contributor

getdave commented Jul 8, 2019

So I've been looking into this as it's been causing issues (see p58i-7RD-p2#comment-42220). It turns out that we're missing a check for the data-orig-size attribute here:

origSize = current.data('orig-size').split(',' ),

This is causing a JS error to be thrown which causes the UI to freeze up.

This attribute appears to be added here

https://github.com/Automattic/jetpack/blob/master/modules/carousel/jetpack-carousel.php#L503

...which if you trace it back gets its data from a call to wp_get_attachment_metadata() here

https://github.com/Automattic/jetpack/blob/master/modules/carousel/jetpack-carousel.php#L455

However, if the image is not stored on the site there is no attachment. How can this happen? If you take a set of blocks and paste them into the editor (or insert them via code) then it's perfectly possible to have a gallery referencing images from another site.

You should also read p58i-7RD-p2#comment-42252.

So

  1. We should add a check for origSize in the JS to avoid the crash.
  2. Should/could we use a fall back set of dimensions?
  3. How do we handle images that are not stored on the site directly?

@TomNaessens
Copy link

Got the same problem here. Externally hosted images, the only one working in the carousel is the wider image at the bottom. Others don't show up and show the same Javascript error mentioned above.

@katiebethbrown
Copy link

Another report in #16986623-hc possibly related to externally hosted images.

@aheckler
Copy link
Member

User in 2792313-zen gets this same error but does not seem to have externally hosted images.

@matticbot matticbot added the Customer Report Issues or PRs that were reported via Happiness. aka "Happiness Request", or "User Report" label Mar 16, 2020
@lakellie
Copy link

Another report in #3244831-zen

@zdenys
Copy link
Contributor

zdenys commented Sep 5, 2020

I noticed the same issue on https://docs.woocommerce.com/document/woocommerce-blocks/#section-2 where you can click any of the images in the gallery.

The only way I was able to reproduce it at this point in time is by adding a Gallery Block with one image, switching to Code Editor, and removing this set of 4 attributes for the <img> element (here is the video):

data-id="158" data-full-url="https://mystore123456789.wpcomstaging.com/wp-content/uploads/2020/09/Best-Selling-Products-Block-Settings-1.png" data-link="https://mystore123456789.wpcomstaging.com/?attachment_id=158" class="wp-image-158"

So possibly this has been somehow addressed in the latest versions of Gutenberg (which updated the Gallery Block)? I noticed that the issue is present when Gallery Block has no Image size section in the sidebar in the Editor. When that section is present (which is the case for any Gallery Block you add with the most recent version of Gutenberg), then the issue cannot be reproduced.

Old Gallery Block Current Gallery Block

For Gallery Blocks added in the past, <img> didn't have the attributes data-link and data-full-url. Here's how a Gallery Block looks in the Code Editor when added today versus last year:

Old Gallery Block Current Gallery Block
<!-- wp:gallery {"ids":[945]} --><figure class="wp-block-gallery columns-1 is-cropped"><ul class="blocks-gallery-grid"><li class="blocks-gallery-item"><figure><img src="https://atrumtest03.files.wordpress.com/2019/10/wp-image-1419139768jpg.jpg?w=800" alt="" data-id="945" class="wp-image-945"/></figure></li></ul></figure><!-- /wp:gallery --> <!-- wp:gallery {"ids":[158]} --><figure class="wp-block-gallery columns-1 is-cropped"><ul class="blocks-gallery-grid"><li class="blocks-gallery-item"><figure><img src="https://mystore123456789.wpcomstaging.com/wp-content/uploads/2020/09/Best-Selling-Products-Block-Settings-1-844x1024.png" alt="" data-id="158" data-full-url="https://mystore123456789.wpcomstaging.com/wp-content/uploads/2020/09/Best-Selling-Products-Block-Settings-1.png" data-link="https://mystore123456789.wpcomstaging.com/?attachment_id=158" class="wp-image-158"/></figure></li></ul></figure><!-- /wp:gallery -->

I hope these bits help in any way.

@donalirl
Copy link

donalirl commented Oct 1, 2020

Encountered another case of this on a WP.com site in #24528170-hc / #3364177-zen

Recreated on this test page by copying the code over.

Is there a workaround to get it working now, or any way to avoid this?

Console error:
Markup 2020-10-01 at 08 15 41

@eduardozulian
Copy link
Member

eduardozulian commented Oct 1, 2020

Also reported in 23458609-hc

The workaround we used was to disable Carousel in admin.php?page=jetpack#/writing. Not ideal, but it worked for this particular case.

@GeoJunkie
Copy link

@GeoJunkie
Copy link

An additional note on this one ^

The carousel was deactivated pending this bugfix, but when it's activated, it works fine if you're logged in to the site, but not if you're logged out.

@msilbers
Copy link

msilbers commented Nov 1, 2020

A customer reported this issue this week and I was able to reproduce it here:

https://melissasgutenbergteststuff.blog/gallery-variations/

The very first gallery image with the caption "lone gallery image"
Note that it is not a long caption and no externally hosted images.

I see this issue was reported in January 2019 almost 2 years ago, do we have any fix coming or a workaround? I notice this does not happen with the Tiled Gallery block.

@carladoria
Copy link

Another report in 25432752-hc
Disabling carousel fixed the issue

@kaitohm
Copy link

kaitohm commented Feb 19, 2021

Another report in 3740078-zd-woothemes

@goblinartificer
Copy link

If it helps any, we have permission from that user to make minor changes to their site at any point while figuring things out. If you'd like me to go in and see if editing and fixing will work at a later point, or ask the user to do so, we can make that happen. Thank you for your hard work!

sgomes added a commit that referenced this issue Jun 21, 2021
The carousel currently fails if some crucial data properties are missing
in the images it attempts to load.

This will make it more robust, allowing it to fall back to other ones.
In addition, it restores the pointer cursor in some situations.

Fixes #11191.

Co-authored-by: Brandon Kraft <public@brandonkraft.com>
@sgomes
Copy link
Contributor

sgomes commented Jun 21, 2021

Thank you, @goblinartificer!

This should now be fixed on wpcom! 🙂 It looks like the test site I was using was changed to the Gutenberg slideshow, however, so I can't directly test.

@msilbers
Copy link

@sgomes I checked my old test page for this: https://melissasgutenbergteststuff.blog/gallery-variations/#jp-carousel-63

I no longer see the same errors in console, but the issue persists
We also had a report of this in Automattic/wp-calypso#53882

Should this be a new issue if the errors are different? Seems like the exact same problem visually, and I spot-checked a few reports above and found this unfixed, including:

another report here: 3998202-zen

@msilbers msilbers reopened this Jun 22, 2021
@goblinartificer
Copy link

goblinartificer commented Jun 23, 2021

@msilbers I forgot to post here, but it's still persistent even on the case I reported earlier ⁠— same Slack thread as above, but later comment: p1624337699095100-slack-C03TY6J1A

@sgomes
Copy link
Contributor

sgomes commented Jun 23, 2021

Thank you, @msilbers! It looks like the images in your gallery are yet another variation in how data can go missing. I'll try to account for those as well 👍

@sgomes
Copy link
Contributor

sgomes commented Jun 23, 2021

The issue appears to be that there are situations where images contain absolutely no width and height information in the markup. This makes it impossible for us to display them in the current setup.

@jeherve : what do you think we should do here? The only solution I see is to preload all of the images that don't have sizing info, so that we can know their size ahead of opening the carousel. Do you think that's acceptable?

@jeherve
Copy link
Member

jeherve commented Jun 23, 2021

Would it be acceptable / possible to not trigger any Carousel modal for such images? If width and height is missing, I assume there was a problem during upload. This may happen on posts with lots of big images, maybe, in which case we wouldn't want to start pre-loading images on such posts since that would potentially be a lot of images.

@sgomes
Copy link
Contributor

sgomes commented Jun 23, 2021

The issue is that some versions of Gutenberg generated galleries without sizing info, which is what happened here. The upload worked well and the images exist in a good state, but the gallery is simply missing the info because it wasn't part of the markup that Gutenberg produced. That's since been fixed, but the new markup doesn't get applied unless the user updates Gutenberg and then goes in to edit their post and (presumably) click the buttons to fix the mismatches in markup.

I think we have one last option that doesn't involve preloading, which is to use the thumbnail natural size; we already measure that, so we can just display the tiny gallery image in the carousel, as we're already doing in other cases. I'll give that a try.

@sgomes
Copy link
Contributor

sgomes commented Jun 23, 2021

Using the thumbnail natural size appears to work. Created #20149 with the fix.

@sgomes
Copy link
Contributor

sgomes commented Jun 23, 2021

Both the test cases from @msilbers and @goblinartificer appear to be working well with the latest fix! 🙂 (live in wpcom; scheduled for Jetpack 9.9)

As such, I'm closing this issue again, but please do reopen it if you find any other examples! We'll get rid of all of those edge cases yet! 😄

@sgomes sgomes closed this as completed Jun 23, 2021
@jeherve
Copy link
Member

jeherve commented Jul 7, 2021

The problem seems to happen on this site, whether they run Jetpack 9.8.1 or Jetpack 9.9:
https://ayudawp.com/optimizar-slider-revolution/

I am not sure why this happens there, though I do note that they use WP Rocket and its defer JS feature. That said, the reporter mentioned that excluding the Carousel's script from being defered does not appear to solve the issue.

Reference:
https://wordpress.org/support/topic/cannot-read-property-split-of-undefined-at-jetpack-carousel-min-js/

@sgomes
Copy link
Contributor

sgomes commented Jul 7, 2021

@jeherve : thanks for the link! It looks like that's a separate issue. The carousel doesn't behave too well when there are exactly two slides, apparently. I'll look into it.

@sgomes
Copy link
Contributor

sgomes commented Jul 7, 2021

Update: I'm not able to test things with this exact site, but I implemented a 2-slide carousel (using the carousel in master) on my test site and it seems to work correctly. Perhaps the use of swiper ended up fixing the issue?

@jeherve
Copy link
Member

jeherve commented Jul 7, 2021

Perhaps the use of swiper ended up fixing the issue?

It would appear that it didn't on that specific site; the owner experiences the same issue when updating to 9.9.

I cannot reproduce the problem on my end, though. :\

@fernandotellado Would you be able to copy the HTML you have in the editor for that post, and send it to us, either in a Gist or privately via our contact form?

Thank you!

@fernandotellado
Copy link

Perhaps the use of swiper ended up fixing the issue?

It would appear that it didn't on that specific site; the owner experiences the same issue when updating to 9.9.

I cannot reproduce the problem on my end, though. :\

@fernandotellado Would you be able to copy the HTML you have in the editor for that post, and send it to us, either in a Gist or privately via our contact form?

Thank you!

Of course, here it is: https://github.com/fernandotellado/fernandotellado/blob/master/jetpack-9-9-issue

@jeherve
Copy link
Member

jeherve commented Jul 7, 2021

Thank you! Any chance you could copy from the HTML view in the editor as well, like so?

Screen.Recording.2021-07-07.at.17.22.10.mov

@fernandotellado
Copy link

Ah, of course, here it is: https://github.com/fernandotellado/fernandotellado/blob/master/html-post-slider

I can even give you admin access to the site if you want to make the test by yourself ;)

@kerrynicl
Copy link

Hi Team, I see the issue is still occurring on this site When a picture is selected I get a black screen and the page becomes unresponsive.

@kerrynicl kerrynicl reopened this Jul 20, 2021
@sgomes
Copy link
Contributor

sgomes commented Jul 20, 2021

Thank you, @kerrynicl !

This is the most extreme case of missing info I've seen so far 😕 In this case, the attachment ID property is present but empty, so there isn't anything meaningful we can do with the image.

The only reasonable course of action in these situations is probably to simply refuse opening the carousel. @jeherve, what do you think?

@sdixon194
Copy link
Contributor

@sgomes Jeremy is AFK for a bit, but I did some extensive testing and definitely seems something is up with the images. Trying to do some testing and they are being saved as webp images? I'm not sure what's up with that.

@kerrynicl I think I would have them try and re-upload the images, or download them and re-upload them and see if that fixes things. Are they able to use the carousel with other images/galleries, or does it happen for all of them? Or, like Sergio suggested, try disabling the carousel.

@jeherve
Copy link
Member

jeherve commented Mar 23, 2022

@Thelmachido I'm afraid you're running into a different problem entirely. This issue is for the Carousel feature, triggered when opening gallery images. You seem to be having issues with the Post Carousel block, a block that's developed and maintained in this repo. I would recommend opening an issue there directly.


Note: When you create that new issue, I would encourage you to upload your screenshots and screencasts to GitHub directly instead of using Droplr. See this for more info:
pbg9X-fWF-p2

@Thelmachido
Copy link

Thelmachido commented Mar 26, 2022

@Thelmachido I'm afraid you're running into a different problem entirely. This issue is for the Carousel feature, triggered when opening gallery images. You seem to be having issues with the Post Carousel block, a block that's developed and maintained in this repo. I would recommend opening an issue there directly.

Note: When you create that new issue, I would encourage you to upload your screenshots and screencasts to GitHub directly instead of using Droplr. See this for more info: pbg9X-fWF-p2

Thanks posted on the correct repo, I will remove the above comment, to avoid confusion.

@kelasante
Copy link

kelasante commented Jun 10, 2022

Got a chat in 35760928-hc where Images in Carousel are black when you click on images in Gallery block. I see this when I inspect:

Markup 2022-06-10 at 16 28 54

However, I took a look at the image URL and it looks to be an import file URL. The user mentioned they just imported from Wix so in this case, the issue seems to be because it is an import file so uploading the file directly should fix it but commenting here just in case it is related to this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customer Report Issues or PRs that were reported via Happiness. aka "Happiness Request", or "User Report" [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Pri] Normal [Status] In Progress [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet