Lightbox #7129
Lightbox #7129
Conversation
Sometimes the `doHydration` function will abort - e.g.: if it has already been run - and in these circumstances we do not want to dispatch the fake mouse event because the real one will work by itself
…he page
This code assumes that there is not a use case where you would want to execute the same island multiple times in different contexts.
If there were, you could use a prop like { hydrateOnce: boolean } to control this
This is clearly a sub optimal solution but it provides a solution to the race condition where the island code sets the click event listener which needs to exist prior to any simulated click being fired. 50ms won't be noticed by readers but it is a long time to wait for code to execute so this solution is likely to be sufficient in most cases. Where it isn't a second click would be needed.
This solves a problem where images in liveblogs only had smaller sources
|
I was just looking at the CODE example of this - do you think it opening full screen on desktop is a bit much? I'd say it would feel more natural to first fill your browser and then allow users to take over the full screen if they want? |
It’s definitely needed/good on mobile: frontend Lightbox was/is infinitely buggy with browser chrome showing unexpectedly, image jumping around and many more bugs. I would still leave proper fullscreen on desktop too. It makes the immersive experience complete. But indeed I was thinking on a possible addition to accommodate folks who would prefer having control over it: a new keyboard shortcut Fullscreen is great. Check it out on a telly! (65in looks awesome ;-) |
This reverts commit b7e0367.
|
|
||
| ## Useful links | ||
|
|
||
| The [frontend version of lightbox](https://github.com/guardian/frontend/blob/main/static/src/javascripts/projects/common/modules/gallery/lightbox.js) works in a similar way and was used as the basis for DCR's version |
There was a problem hiding this comment.
Pointing to a specific commit ensures that this link cannot rot.
|
|
||
| ## How it gets hydrated | ||
|
|
||
| On the server, we only insert a small amount of html and zero javascript. This html can be found in LightboxLayout. On the client hydration is trigged when the page url contains a hash that martches an expected pattern, namely, `img-[n]`. Using the `deferUntil='hash'` feature of Islands, we only execute the Lightbox javascript at the point the url changes and lightbox is requested. |
There was a problem hiding this comment.
Could we add a link to the LighboxLayout.tsx file?
Elsewhere lightbox is not capitalised. Can we ensure consistency?
|
|
||
| Lightbox works by using the url as the source of truth. If and when the url contains a hash in the form `img-[n]` then hydration is triggered (if it hasn't already happened) and the lightbox is opened. As soon as the url does not contain this hash, the lightbox is closed. | ||
|
|
||
| You open and close the lightbox by changing the url. When a reader clicks the 'close' button inside the lightbox there is some javascript that fires the `history.back()` function. Manually editing the url or pressing the back button in the browser is therefor identical to clicking 'close'. |
There was a problem hiding this comment.
I'm not sure that closing is the same as navigating back. Closing is a move forward.
|
|
||
| ### `LightboxLink` | ||
|
|
||
| This file is used on the server to co er each article image with an <a> tag. No javascript is used here, only platform features and css. |
There was a problem hiding this comment.
co er ➡️ cover
Maybe we can clarify which platform feature: navigation to a hash does not cause a reload.
| What is the edge case? | ||
| Because we use the url as the source of truth for lightbox it means we close the lightbox by using `history.back`. This works fine when the reader started on an article and then clicked an image because it means the place they go 'back' to is the article url without the hash. But if you directly load a url with a hash already on it then the place you will go 'back' to is probably a blank page (or just where you were when you entered the url). | ||
|
|
||
| The fix here is to mutate the history state by adding a new entry so that when `history.back()` gets fired the reader ends up on the article. |
There was a problem hiding this comment.
If closing was a forward action, would this edge case disappear?
| const data = { | ||
| ...validated, | ||
| mainMediaElements, | ||
| blocks, | ||
| }; |
|
Added comments so we have some pointers of things we might want to fix-forward once this is merged. They are not blockers as the PR has been approved, but you may want to address some of them. |
|
Hi @mxdvl I can't be sure if you're aware of this or not, but your comments come over as aggressive and are hurtful to me. I'm sure some of your points are valid but your timing and the demanding tone feels inappropriate, especially given I am an external contributor and am not being paid for this work. I presented this feature to the team yesterday and have now made the final commit as agreed with @jamesgorrie , @arelra and @paperboyo so I don't think continuing code style debates here is constructive. If you think there are any important points that would block deployment then these should be addressed but, if not, I suggest we put these debates to bed, move forward with the agreed plan and then any nitpicks can be picked up by the Dotcom team later. |
|
Thanks for everybody's help and contribution to this PR. @mxdvl - your suggestions are very astute and leave a good record for where we could improve and standardise things, let’s go through these soon. The plan stands that this will go in behind the 0% switch, and a roll-out plan made once it’s prioritised. There’s been a bit of heat on this PR and we are thus locking it. This is an exception to how we do things. Whilst the conversation above has been constructive and points out some very valid considerations, we’re locking this as it is no longer focussed on the work and is not in line with the culture we try to follow at the Guardian. For future travellers to this PR - there is already the great PR recommendations doc which we will definitely make more discoverable. |
Following an A/B testing experiment we have confirmed that this change does not have any negative impact on our Core Web Vitals and have therefore been able to roll out this long awaited feature to all our users in #10329. Thank you @oliverlloyd for your major contribution to this feature, I hope you’ll be pleased to know it has made its way into PROD. |
gu-lightboxThis PR adds a lightbox to DCR. Finally
It's mostly a copy of the existing frontend lightbox, but it also steals some ideas from this PR by @OllysCoding and pays attention to some useful comments from @paperboyo
Things it does
You can use the following shortcut keys
[i]- Toggle info[esc]- Close[q]- Close[ArrowLeft]- Show previous image[ArrowRight]- Show next image[ArrowUp]- Show info[ArrowDown]- Hide infoJavascript is deferred
Based on the work from @OllysCoding, the lightbox javascript is not downloaded until the reader decides to open it
Uses Fullscreen API
When you open the lightbox it triggers a request to the browser to open the
lightboxdiv in fullscreen mode. Pressing escape or clicking close will remove the lightbox and close fullscreen mode.The use of this API means it is not possible to use a dialog element as this element is not well supported with Fullscreen mode.
If you use a permalink to an image (eg.
http://...#img-4) then the lightbox will open but not in fullscreen mode. Permission is being denied in this scenario, most likely because there hasn't been a click eventEmploys native scrolling
Images are rendered side by side as a horizontal ul/li list and can be scrolled using native scroll controls as well as using the javascript powered buttons and shortcuts
Traps focus
Focus trapping is a requirement to make dialogs accessible. It is achieved using javascript where the tab event is taken over and instead of letting the browser chose the next item to focus we manually decide based on which image is selected. This approach ensures focus isn't lost from the dialog and also prevents the browser from trying to scroll to offscreen tabable elements
Optimises image loading
When an image is clicked upon to show it in the lightbox it is immediately downloaded. We also eager load its immediate siblings ahead of any navigation left or right. We continue to eager load siblings on each navigation event.
De-duplicates images
Some articles repeat the main media in the article body so duplicates are removed during the
buildLightboxImagesstepAllows permalinks to images
When lightbox is open the url is modified to include a reference to the image, e.g.
#img-4. Urls containing this modifier can be used to hotlink into the lightbox. The fileLightboxHashexists to support these permalinks.Browser navigation back / forward will also dynamically open or close the lightbox based on this hash state. This is achieved by listening to the
popstateevent. This listener is only set after the lightbox has been hydrated, however, so in some scenarios navigating back / forward does not load the lightbox but I think this is preferable to having this listener set on every page load.Links to liveblog posts
Liveblog images show a link to the post where they appeared below the caption
Scales images to the viewport
Depending on orientation, images will scale to fit the viewport vertically or horizontally
Remembers display preferences for the caption
User preference is persisted in local storage.
But, but...
Another Cypress lib?
Cypress support for tabbing is poor. The recommended workaround is to use cypress-plugin-tab which we already have installed, but even that has issues with simulating the escape key. I think for lightbox, and for most areas where we want to test interactivity, it's good to drive tests using keyboard actions because this helps prevent loss of keyboard accessibility through regression. To ensure such tests are as realistic as possible when being driven by the keyboard, I added the cypress-real-events library.
The down side to this lib is that it only supports Chrome and so these scripts will fail if run on Firefox.