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
7

@davidb It seems that one of the recent commit to the lightbox component has a bug preventing the component from starting in discourse solved and other items like that, after my PR get merged.

The problem may be related to this commit.

FEATURE: allow quoting an image from the lightbox by SamSaffron · Pull Request #36156 · discourse/discourse · GitHub.

I’m not quite sure whether this PR actually breaks that.

The repro can be done here right now.

Like clicking the [IMG_6085] from the onebox quote, or open the link and click the image of the answer in discourse solved component of the given page. The original behavior is open a lightbox, but now nothing happened.

The console shows an error: Uncaught TypeError: Cannot read properties of null (reading 'getAttribute')

Related code is:

// this ensures that cropped images (eg: grid) do not cause jittering when closing
data.thumbCropped = true;

data.src = data.src || el.getAttribute("data-large-src");
-> data.origSrc = imgEl.getAttribute("data-orig-src");
data.title = el.title || imgEl.alt || imgEl.title;

Maybe turning the code into the following will work:

data.origSrc = imgEl?.getAttribute("data-orig-src") || el.getAttribute("data-orig-src") || null;

data.base62SHA1 =
    imgEl?.getAttribute("data-base62-sha1") || el.getAttribute("data-base62-sha1") || null;

data.targetWidth =
    el.getAttribute("data-target-width") || imgEl?.getAttribute("width") || null;
data.targetHeight =
    el.getAttribute("data-target-height") || imgEl?.getAttribute("height") || null;

A PR fix has been applied here:

1 Like