search

thegetty / quire

A multi-package repository for the Quire multiformat publishing framework
https://quire.getty.edu/
BSD 3-Clause "New" or "Revised" License
93 stars 12 forks source link

Fullscreen mode not working in image viewer for `essay` and `splash` pages. #849

Open Erin-Cecele opened 12 months ago

Erin-Cecele commented 12 months ago

Before proceeding, make sure there isn’t an existing issue for this bug.

Expected Behavior

No matter the layout type, when an image is opened in the image viewer, and you click the fullscreen button in the upper left corner, it should open the image in fullscreen mode.

Actual Behavior

On the essay and splash pages, when the image is opened in the viewer and the fullscreen button is pushed, nothing happens. The image does not go fullscreen.

Important to note: you can open images in fullscreen mode on the entry type pages.

Steps to Reproduce

  1. Start a new quire project
  2. Navigate to any of the images on the Introduction (splash) or American Photographs (essay) pages, open the images in the image viewer and click the fullscreen button

Version Numbers

[test-project] quire-cli 1.0.0-rc.10 quire-11ty 1.0.0-rc.14 starter https://github.com/thegetty/quire-starter-default@2.6.0 [System] quire-cli 1.0.0-rc.10 node v18.16.0 npm 9.5.1 os Darwin 21.6.0

Web Browser

I tested with Chrome 114 and Firefox 117, and @cbutcosk tested on Safari 16.6 and Chrome 116.

Relevant Terminal/Shell Output

No response

Supporting Information

Special thank you to @cbutcosk for bringing this bug to our attention in #815. As noted by @cbutcosk "I suspect your developers also need to add a Shadow DOM polyfill for Safari < 16.5 (mobile and desktop), which in this issue breaks in a slightly different way from the Chromium-based browsers."

So when fixing this issue be sure to test in multiple browsers!

cbutcosk commented 10 months ago

I had a chance to peer into this, it turns out this line is the culprit on browsers that should otherwise support the fullscreen API: https://github.com/thegetty/quire/blob/e36d4cb100a81fd6ecd90a894e78f14e47e1b11b/packages/11ty/_includes/web-components/lightbox/index.js#L80

Optional-chaining the wrapper and classList here lets the function succeed and open fullscreen as you'd expect. I didn't have a shadowDOM-unsupporting browser available but fullscreen did still work when I disabled shadowDOM on Safari 17.0 for whatever that is worth.

Erin-Cecele commented 9 months ago

Hi @cbutcosk, Thanks for finding the root cause of this issue. Do you have a fix you could possibly submit as a PR? You can find instructions via our Contributing Guidelines: https://github.com/thegetty/quire/blob/main/CONTRIBUTING.md#submit-your-contribution.