Fix language selector invalid value #2635

HiDeoo · 2024-11-25T10:43:37Z

This issue fixes an issue where the language selector could have an invalid value when the user navigates back to a page in a different language.

Repro:

Visit http://localhost:4321/
Change the language to French
Press the back button

At expected at this point, the page is in English however the language selector is still set to French.

In Chrome & Safari, a refresh of the page would fix the issue. However, in Firefox, a refresh of the page is not enough to fix the issue, you have to manually focus the address bar and press enter again.

This is not something visible with the theme selector as we have some JS code to update the picker when the page is loaded.

Setting the to autocomplete attribute to off on the select element prvent the browser to automatically select a cached value for this field based on the last user input. Our <Select> component is only used for the theme and theme selector so I applied the change to the component itself.

changeset-bot · 2024-11-25T10:43:40Z

🦋 Changeset detected

Latest commit: 2ba9d3d

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 · 2024-11-25T10:43:55Z

✅ Deploy Preview for astro-starlight ready!

Name	Link
🔨 Latest commit	`2ba9d3d`
🔍 Latest deploy log	https://app.netlify.com/sites/astro-starlight/deploys/6761c83b2240390008b8eb2b
😎 Deploy Preview	https://deploy-preview-2635--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.

delucis

Oh, nice fix! Would have assumed we needed a bit of extra JS for this.

Are we sure it works? I just tried in Chrome (v131 on macOS 15) and still saw the old behaviour:

picker.mov

HiDeoo · 2024-11-25T11:03:21Z

Oh, interesting, I did not checked the deploy preview yet but indeed, this works in dev but not in the production version. I wonder why the difference 🤔 I'll investigate a bit more as it would be nice to avoid JS for this if possible somehow.

trueberryless · 2024-11-25T11:34:36Z

I think that the production preview works properly, because it works on my machine :)

I'm using Arc v1.29.0 on Windows 11 and here you can see the comparison between current Starlight (left) and preview of this PR (right).

Production.works.I.think.mp4

PS: I'm sorry that my screen recorder didn't capture the dropdown menu, what are you guys using for recording?

HiDeoo · 2024-11-25T14:21:30Z

After investigating a bit more and finding so many cases/bug reports regarding how the autocomplete attribute is behaving, and considering the explicit value we provide can be ignored, domain-dependent, markup dependant, change based on user's settings or browser, I think it's safe to say that we sadly can't rely on it even tho, per spec, it should work…

I'll have to update this PR to use a JS based approach instead when I get the time.

PS: I'm sorry that my screen recorder didn't capture the dropdown menu, what are you guys using for recording?

Depending on the machine I use, I'll either use ScreenFlow or the built-in macOS screen recording tool.

HiDeoo · 2024-11-27T13:44:28Z

Turns out after more investigation that in this specific case, autocomplete="off" works in all browsers but what's making the difference in production is the back/forward cache (or bfcache) (that's also why autocomplete="off" is still present in the changes and works in dev where the bfcache does not work).

I updated the PR with some code to hook to the pageshow event which has a persisted property, which is true if the page was restored from bfcache. If this is the case, we ensure that the selected value is the correct one.

I made an E2E test for it while adding the code but I did not submit it:

By default, the bfcache is disabled in playwright.
We can change that but due to a Chrome limitation, this does not work in headless mode.
The new headless implementation available in Playwright v1.49.0 removes that limitation but it felt a bit too new to switch to it as there are still some issues with it.

HiDeoo · 2024-11-27T13:44:49Z

packages/starlight/components/LanguageSelect.astro

@@ -46,4 +46,18 @@ function localizedPathname(locale: string | undefined): string {
 		}
 	}
 	customElements.define('starlight-lang-select', StarlightLanguageSelect);
+	window.addEventListener('pageshow', (event) => {


A better event would be resume from the Document API but it's not available in Safari at the moment.

trueberryless

LGTM 🎉

But I have a question regarding the fallback if no selected index could be determined (see comment 👇)

packages/starlight/components/LanguageSelect.astro

delucis

Thanks @HiDeoo! I’ll admit there’s part of me that is unsure if this is even a “bug” to be fixed or just “how form elements work and are expected to work”, but the thing that we should definitely fix is that switching language should always work (which it doesn’t currently in this scenario because re-selecting a language doesn’t change anything). So I guess this is one approach to fixing that.

I left a couple of queries and also looks like this script takes us 10 bytes over our JS side limit, so that budget would need increasing for CI to pass.

packages/starlight/components/LanguageSelect.astro

packages/starlight/components/Select.astro

Co-authored-by: Chris Swithinbank <357379+delucis@users.noreply.github.com>

HiDeoo · 2024-12-02T13:39:57Z

I’ll admit there’s part of me that is unsure if this is even a “bug” to be fixed or just “how form elements work and are expected to work”

I personally think that the bfcache restoration not respecting the attribute is a bit weird in our scenario and you can only know something is wrong by hooking up to this event (or resume when properly supported) as before this event everything is fine.

I updated the branch with your suggestions and also increased the JS size limit as I was waiting to get an approval before doing so.

delucis

Ready for launch! Thanks again @HiDeoo 🙌

fix: language picker invalid value after back/forward navigation

ad22ce7

github-actions bot added the 🌟 core Changes to Starlight’s main package label Nov 25, 2024

delucis reviewed Nov 25, 2024

View reviewed changes

HiDeoo marked this pull request as draft November 25, 2024 11:03

feat: fix language picker invalid value after bfcache

210079f

HiDeoo commented Nov 27, 2024

View reviewed changes

HiDeoo marked this pull request as ready for review November 27, 2024 13:45

trueberryless approved these changes Nov 28, 2024

View reviewed changes

packages/starlight/components/LanguageSelect.astro Outdated Show resolved Hide resolved

delucis reviewed Dec 2, 2024

View reviewed changes

packages/starlight/components/LanguageSelect.astro Outdated Show resolved Hide resolved

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

HiDeoo and others added 3 commits December 2, 2024 14:27

refactor: move event listener to custom element

f5bcf82

Co-authored-by: Chris Swithinbank <357379+delucis@users.noreply.github.com>

chore: add autocomplete comment

3a08ef0

Co-authored-by: Chris Swithinbank <357379+delucis@users.noreply.github.com>

chore: increase js size limit

e87e21f

Merge branch 'main' into hd-fix-language-select-invalid-value

2ba9d3d

delucis approved these changes Dec 17, 2024

View reviewed changes

delucis added the 🌟 patch Change that triggers a patch release label Dec 17, 2024

delucis merged commit ec4b851 into withastro:main Dec 17, 2024
15 checks passed

astrobot-houston mentioned this pull request Dec 17, 2024

[ci] release #2718

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix language selector invalid value #2635

Fix language selector invalid value #2635

HiDeoo commented Nov 25, 2024

changeset-bot bot commented Nov 25, 2024 •

edited

Loading

netlify bot commented Nov 25, 2024 •

edited

Loading

delucis left a comment •

edited

Loading

HiDeoo commented Nov 25, 2024

trueberryless commented Nov 25, 2024

HiDeoo commented Nov 25, 2024

HiDeoo commented Nov 27, 2024

HiDeoo Nov 27, 2024

trueberryless left a comment

delucis left a comment

HiDeoo commented Dec 2, 2024

delucis left a comment

Fix language selector invalid value #2635

Fix language selector invalid value #2635

Conversation

HiDeoo commented Nov 25, 2024

changeset-bot bot commented Nov 25, 2024 • edited Loading

🦋 Changeset detected

netlify bot commented Nov 25, 2024 • edited Loading

✅ Deploy Preview for astro-starlight ready!

delucis left a comment • edited Loading

Choose a reason for hiding this comment

HiDeoo commented Nov 25, 2024

trueberryless commented Nov 25, 2024

HiDeoo commented Nov 25, 2024

HiDeoo commented Nov 27, 2024

HiDeoo Nov 27, 2024

Choose a reason for hiding this comment

trueberryless left a comment

Choose a reason for hiding this comment

delucis left a comment

Choose a reason for hiding this comment

HiDeoo commented Dec 2, 2024

delucis left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Nov 25, 2024 •

edited

Loading

netlify bot commented Nov 25, 2024 •

edited

Loading

delucis left a comment •

edited

Loading