Prevent scroll on `body` when search is open #715

itsmatteomanf · 2023-09-14T22:54:42Z

What kind of changes does this PR include?

Changes to Starlight code

Description

This PR resolves the scroll on body when the search is open.

From my tests, it works fine even on mobile, with the menu open.

changeset-bot · 2023-09-14T22:54:45Z

🦋 Changeset detected

Latest commit: 724292a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@astrojs/starlight	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2023-09-14T22:54:47Z

✅ Deploy Preview for astro-starlight ready!

Name	Link
🔨 Latest commit	`724292a`
🔍 Latest deploy log	https://app.netlify.com/sites/astro-starlight/deploys/650a3d56dbd433000758aed5
😎 Deploy Preview	https://deploy-preview-715--astro-starlight.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 100 (no change from production) Accessibility: 100 (no change from production) Best Practices: 100 (no change from production) SEO: 100 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

itsmatteomanf · 2023-09-14T23:03:34Z

packages/starlight/components/Search.astro

@@ -72,13 +72,15 @@ const pagefindTranslations = {

 			const openModal = (event?: MouseEvent) => {
 				dialog.showModal();
+				document.body.toggleAttribute('data-search-modal-open', true);


I added the force option here, just to never get in a situation where it's the wrong way around for any weird reason.

This could work, too. Do note L82 that does the same thing.

Suggested change

document.body.toggleAttribute('data-search-modal-open', true);

document.body.toggleAttribute('data-search-modal-open');

kevinzunigacuellar · 2023-09-15T02:44:10Z

Hey @itsmatteomanf thanks a lot for the PR. I was testing the deploy preview and It seems that closing the modal using the "esc" key doesn't remove the attribute data-search-modal-open.

dreyfus92 · 2023-09-15T18:08:31Z

Hey @itsmatteomanf thanks a lot for the PR. I was testing the deploy preview and It seems that closing the modal using the "esc" key doesn't remove the attribute data-search-modal-open.

I think that it would be worth adding a KeyboardEvent as well to the arguments of both functions. https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent

itsmatteomanf · 2023-09-15T19:02:18Z

Thanks for the review, @kevinzunigacuellar, @dreyfus92.

I think that it would be worth adding a KeyboardEvent as well to the arguments of both functions.

I didn't want to change the actual behaviour of the search modal, but I see now that it actually does need to be intercepted as it actually sticks as locked. Gonna do the change immediately.

itsmatteomanf · 2023-09-15T19:07:07Z

Done, added a preventDefault() as it does exit out of fullscreen when pressing esc to close the modal.
Now it closes the modal, and then exits fullscreen on the next press.

dreyfus92

LGTM ✅

HiDeoo

I tested the Escape key workaround in Chrome and Firefox and it does not seem to work for me. Did not really investigate why but maybe it could be because keypress is now deprecated and no longer recommended?

I guess this could be handled in the existing keydown event listener just below the keypress one.

One alternative solution could be the the HTMLDialogElement close event:

The close event is fired on an HTMLDialogElement object when the it represents has been closed.

We could handle everything in a code like this I assume:

dialog.addEventListener('close', () => {
  document.body.toggleAttribute('data-search-modal-open', false);
});

This would be perfect in this case but maybe the browser support is not good enough yet?

itsmatteomanf · 2023-09-19T11:46:44Z

I guess this could be handled in the existing keydown event listener just below the keypress one.

You could actually just replace it, it does the same thing, as far as I know.
Replacing it, seems to work fine.

One alternative solution could be the the HTMLDialogElement close event:

Not sure if we can classify it as supported enough, this allows you to support basically any browser. Can be added, too. But not sure it's worth doing that twice.

kevinzunigacuellar · 2023-09-19T14:07:11Z

This would be perfect in this case but maybe the browser support is not good enough yet?

I am not sure if its the same case but we already use the dialog element and its browser support is similar to the close event. Hence I think it should be safe to use.

HiDeoo · 2023-09-19T14:11:51Z

I am not sure if its the same case but we already use the dialog element and its browser support is similar to the close event. Hence I think it should be safe to use.

Indeed, great catch, I imagined <dialog/> being usable before Safari 15.4 but I guess I dreamed about that ^^

In this case, I guess it would make sense to use the HTMLDialogElement close event for all document.body.toggleAttribute('data-search-modal-open', false);?

itsmatteomanf · 2023-09-19T16:16:21Z

@HiDeoo @kevinzunigacuellar, moved to using that event.

I left the closeModal() function as it's used by the mobile button (maybe a desktop button would be good, too), but moved the removal of the attribute in the close event, for all cases.

kevinzunigacuellar · 2023-09-19T18:00:36Z

Tested it in Chrome and on my mobile device (Galaxy fold). Works beautifully. Thanks a lot @itsmatteomanf

delucis

This looks great to me @itsmatteomanf! Thanks for tackling and thank you to everyone reviewing along the way to polish this. Arrived at a nice solution I think.

I left one suggestion for a tiny refactor here, but overall this looks great 🙌

packages/starlight/components/Search.astro

delucis

Thanks again @itsmatteomanf! LGTM 🚀

itsmatteomanf added 2 commits September 15, 2023 00:52

feat: implement scroll lock body on search

e1518c7

feat: add changeset

d470714

github-actions bot added the 🌟 core Changes to Starlight’s main package label Sep 14, 2023

itsmatteomanf commented Sep 14, 2023

View reviewed changes

fix: handle keypress for escape key

d382f11

dreyfus92 approved these changes Sep 15, 2023

View reviewed changes

HiDeoo reviewed Sep 19, 2023

View reviewed changes

fix: event not firing on Chrome/Firefox

230f0f1

itsmatteomanf added 2 commits September 19, 2023 18:14

feat: move to close event

2049d86

fix: remove typo

475ddfb

delucis reviewed Sep 19, 2023

View reviewed changes

packages/starlight/components/Search.astro Show resolved Hide resolved

delucis added the 🌟 patch Change that triggers a patch release label Sep 19, 2023

fix: condense function

724292a

delucis approved these changes Sep 20, 2023

View reviewed changes

delucis merged commit e726155 into withastro:main Sep 20, 2023
9 checks passed

astrobot-houston mentioned this pull request Sep 20, 2023

[ci] release #730

Merged

itsmatteomanf deleted the i/lock-scroll-search branch October 2, 2023 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent scroll on `body` when search is open #715

Prevent scroll on `body` when search is open #715

itsmatteomanf commented Sep 14, 2023

changeset-bot bot commented Sep 14, 2023 •

edited

Loading

netlify bot commented Sep 14, 2023 •

edited

Loading

itsmatteomanf Sep 14, 2023 •

edited

Loading

kevinzunigacuellar commented Sep 15, 2023

dreyfus92 commented Sep 15, 2023

itsmatteomanf commented Sep 15, 2023

itsmatteomanf commented Sep 15, 2023

dreyfus92 left a comment

HiDeoo left a comment

itsmatteomanf commented Sep 19, 2023

kevinzunigacuellar commented Sep 19, 2023

HiDeoo commented Sep 19, 2023 •

edited

Loading

itsmatteomanf commented Sep 19, 2023

kevinzunigacuellar commented Sep 19, 2023

delucis left a comment

delucis left a comment

	document.body.toggleAttribute('data-search-modal-open', true);
	document.body.toggleAttribute('data-search-modal-open');

Prevent scroll on body when search is open #715

Prevent scroll on body when search is open #715

Conversation

itsmatteomanf commented Sep 14, 2023

What kind of changes does this PR include?

Description

changeset-bot bot commented Sep 14, 2023 • edited Loading

🦋 Changeset detected

netlify bot commented Sep 14, 2023 • edited Loading

✅ Deploy Preview for astro-starlight ready!

itsmatteomanf Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

kevinzunigacuellar commented Sep 15, 2023

dreyfus92 commented Sep 15, 2023

itsmatteomanf commented Sep 15, 2023

itsmatteomanf commented Sep 15, 2023

dreyfus92 left a comment

Choose a reason for hiding this comment

HiDeoo left a comment

Choose a reason for hiding this comment

itsmatteomanf commented Sep 19, 2023

kevinzunigacuellar commented Sep 19, 2023

HiDeoo commented Sep 19, 2023 • edited Loading

itsmatteomanf commented Sep 19, 2023

kevinzunigacuellar commented Sep 19, 2023

delucis left a comment

Choose a reason for hiding this comment

delucis left a comment

Choose a reason for hiding this comment

Prevent scroll on `body` when search is open #715

Prevent scroll on `body` when search is open #715

changeset-bot bot commented Sep 14, 2023 •

edited

Loading

netlify bot commented Sep 14, 2023 •

edited

Loading

itsmatteomanf Sep 14, 2023 •

edited

Loading

HiDeoo commented Sep 19, 2023 •

edited

Loading