Unexpected scaling in experimental PhotoSwipeLightbox component breaking the ratio of the original image

UX
1

The new experimental PhotoSwipeLightbox (siteSettings.experimental_lightbox) contain bugs when the image link exists in Discourse Solved or the upload component in admin/config/logo and somewhere without a rendered [img] html mark.

The root cause is that PhotoSwipeLightbox need width and height from the already-loaded image, while Discourse Solved and the upload component don’t have these attributes.

Without these attributes, the image will be stretched to fill the browser window, causing distortion, as shown in the figure attached.

This commit add a preload mechanism for images without dimensions to get their dimensions, so that they can be displayed normally.

2 Likes
2

Thank you for reporting this and sharing a fix.

I’ve added some comments/suggestions on the PR but overall I think we need a fallback like this to catch the edge cases.

4

OK thanks, I’ll cast insight into the comments accordingly when free.

  context "when missing data attributes" do
    it "preloads images and sets dimensions" do
      upload_1.update(width: 400, height: 300)
      post.update(
        cooked:
          "<p><a href=\"#{upload_1.url}\" class=\"lightbox\" data-download-href=\"#{upload_1.url}\">[image]</a></p>",
      )

      topic_page.visit_topic(topic)
      lightbox_link = find("#post_1 a.lightbox")
      lightbox_link.click

      expect(lightbox).to be_visible

      expect(lightbox_link["data-target-width"]).to eq(upload_1.width.to_s)
      expect(lightbox_link["data-target-height"]).to eq(upload_1.height.to_s)
    end
  end

I’m adding this to the lightbox_spec.rb as a unit test for those images without dimensions, so does that seem okey?

1 Like
5

Also, I have made some alterations to the discrimination logic, to handle those with meta but without the meta fails to contain the size info. (i.e., the upload component)

const missingMetaData = !item
    .querySelector(".meta")
    ?.textContent.trim()
    .split(/x|×/)
    .every((v) => v && +v > 0);
1 Like
6

Thank you for working on this and following up on the PR review. I’ve merged this now.

1 Like