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

If there is no<a> wrapper, the image cannot be opened #2051

Open
otary opened this issue Jul 30, 2023 · 7 comments
Open

If there is no<a> wrapper, the image cannot be opened #2051

otary opened this issue Jul 30, 2023 · 7 comments

Comments

@otary
Copy link

otary commented Jul 30, 2023

<div id="test3">
            <img class="img2"
                 src="https://img2.baidu.com/it/u=3329909248,3799019568&fm=253&fmt=auto&app=138&f=JPEG?w=889&h=500"
            />
            <img class="img2"
                 src="https://img2.baidu.com/it/u=3329909248,3799019568&fm=253&fmt=auto&app=138&f=JPEG?w=889&h=500"
            />
            <img class="img2"
                 src="https://img2.baidu.com/it/u=3329909248,3799019568&fm=253&fmt=auto&app=138&f=JPEG?w=889&h=500"
            />
 </div>
const photoSwipeLightbox = new PhotoSwipeLightbox({
                 gallery: '#test3',
                children: '.img2',
                pswpModule: () => PhotoSwipe
});
photoSwipeLightbox.init();

As shown above, clicking on the image can open it, but it cannot display the image!

I don't want to add wrapper, is there any way to achieve the goal??

@arnowelzel
Copy link

Without an wrapper there is nothing to click on and users won't expect that clicking an element which is not a link to something or a button element will do something. Especially since the mouse pointer won't indicate this as well. You may need to build a collection of all images first yourself and add a click handler for each image which will then call the respective method in PhotoSwipe to open a lightbox for the specific image.

@KnIfER
Copy link

KnIfER commented Aug 15, 2023

you can bypass the "A"===t.tagName check inside of photoswipe.umd.min.js and photoswipe-lightbox.umd.min.js

@arnowelzel
Copy link

arnowelzel commented Aug 15, 2023

And by the way - it is not the visible images which are displayed in the lightbox it is the the links. The visible images should just be smaller thumbnails and not the final images.

Usually the final images displayed in the lightbox are much bigger then what you see on the website.

See here for example: https://wordpress-demo.arnowelzel.de/lightbox-with-photoswipe-5/

@Anton-Evstigneev
Copy link

Such tight coupling with anchor tag results in bad UX when user tries to click on image while page's still loading. So instead of opening zoom overlay, user is navigated away from the site to an empty page with just one image.

@arnowelzel
Copy link

arnowelzel commented Jan 26, 2024

About the "bad UX" comment: when the user clicks on an image before the loading process has finished, the script is not loaded yet. That's the reason why the browser will show the image instead of opening the lightbox. But I would like to see your alternative how it should be handled instead. Any live example? Also keep in mind, that event handlers need to be added to the images for PhotoSwipe. I'd like to learn how to do this while the document is still loading and the DOM is not built completely yet.

And in general - maybe there is misunderstanding how image galleries on websites work. Except a very few execptions, the images which are visible in the website are not the ones which are displayed in the lightbox.

So the usual pattern is:

<a href="full-resolution-image.jpg"><img src="preview-image.jpg" alt="alternative text" /></a>

Also keep in mind the semantic here: the anchor tells the browser "here is something the user can open". This is important! By adding an anchor, the browser will also display a different pointer when hovering with the mouse and makes the element focusable. When the user is not able to use a mouse but navigates by switchting from one focusable element to the next, this is also important: an image itself is not focusable - only anchors with a reference and input elements are.

Also keep in mind: how to distinguish between images which are just decorative and others which are intended to be clicked by the user? This is also indicated by adding an anchor element - as an indication "this image links to something you can open".

In my opinion I would close this issue as "won't fix" since the way how PhotoSwipe works is the logical and expected behaviour: only those images will be used for the lightbox which would be opened anyway when them, even without JavaScript and PhotoSwipe. If you want to have a custom solution, you still can create your own frontend script which adds all images to a collection and pass that to PhotoSwipe and add event handlers to the collected images. You can also generate the script code in the backend and pass that along with "onclick" handlers hardcoded in the image tags.

@tennox
Copy link

tennox commented Mar 26, 2024

I also would prefer to be able to do something like this:

<div class="gallery-image cursor-pointer" role="button">
  <img ... >
</div>

(role=button docs)

...but I get your point with using a semantic tag like <a>, so here is a way to address the

bad UX when user tries to click on image while page's still loading

<a href="javascript:void(0);" data-pwsp-src="imageUrl">

this makes the link just do nothing when gallery code is not loaded yet

@arnowelzel
Copy link

arnowelzel commented Apr 3, 2024

...but I get your point with using a semantic tag like <a>, so here is a way to address the

bad UX when user tries to click on image while page's still loading

<a href="javascript:void(0);" data-pwsp-src="imageUrl">

this makes the link just do nothing when gallery code is not loaded yet

Yes, this could solve the issue that images can be clicked before the DOM is completely loaded.

But I am not sure if this is "good UX" since it creates a new problem: If JavaScript is disabled - maybe because the user does not want this for security reasons or there are security policies which forbid JavaScript by default - image links don't work at all.

What you describe as "bad UX" is just the fact, that images may not be opened in the PhotoSwipe lightbox if a user clicks too early. However images will be opened in any case, even if there is no JavaScript at all - either in a lightbox or as a single image. This is still better than keeping everybody from opening an image if there is no JavaScript available. Yes, having JavaScript avialable is expected nowadays, but good web design should always keep graceful degradation in mind which is a very important thing for accessibility too.

I also doubt, that handling the consequences of slow loading websites is something which should be addressed in PhotoSwipe. If a website takes significantly longer than a few seconds to load under normal conditions (which means at least 10 MBit/s downstream which should be the baseline for most users), then there a more serious issues that should be fixed in the server side.

Edit:

If you don't want the user to click anything at all before the DOM is completely loaded, you may also add some additional layer on top of it which get's all click events and remove that layer in a script when the DOM is ready. However, this would also prevent anyone from using the website if no JavaScript is available.

In addition - anchors fire "onlick" events when you use the keyboard to navigate, elements with the "button" role seem to behave different:

<p><a href="#" onclick="alert('link 1 was clicked!')">Focusable link</a></p>

<div role="button" tabindex="0" onclick="alert('DIV was clicked!')">
Click me!
</div>

<p><a href="#" onclick="alert('link 2 was clicked!')">Second focusable link</a><p>

The two links can be opened by navigating the focus with the keyboard and then using the enter key. However this is not possible with the DIV element - eventhough you can click it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants