fix: prevent list items from overflowing Markdown content

[julien-deramond]

Description

Context

This PR is an attempt to fix an issue reported in this Discord > starlight > Text overflow issue thread.

On iPhone XR, OP could see on a 414px width screen the following (at https://0d6fe6ee.chart-docs.pages.dev/charts/dependency/kube-state-metrics/) which results in a horizontal scrollbar:

It can also be observed, at least on macOS with Safari and Chrome.

Changes

As you can see in the deployed documentation, I've tested some long links in the regular content, but also in ordered and unordered lists. Regular content is fine, while the long links in the lists are overflowing.

The idea here is to rely on the same fixes that happened in #756 and #814 based on the use of overflow-wrap: anywhere.

Not knowing in detail the codebase of Starlight, I suppose that reducing the scope to .sl-markdown-content li is acceptable (I've added a reddish background for visual manual regression testing). Indeed, adding this rule directly in the reset.css might have some unexpected repercussions.

Here's the rendering before the fix.

Here's the rendering after the fix (directly accessible at https://starlight-git-fork-julien-deramond-main-jd-bab67d-astrodotbuild.vercel.app/guides/authoring-content/):

Non-regression testing

I'm sorry but I haven't tested yet some impacts that would maybe need a stricter CSS rule, and I'll be off for 1 week tomorrow.

Some use cases that could be impacted:

• https://starlight-git-fork-julien-deramond-main-jd-bab67d-astrodotbuild.vercel.app/getting-started/: tabs if there are too much of them
• https://starlight-git-fork-julien-deramond-main-jd-bab67d-astrodotbuild.vercel.app/guides/project-structure/#example-project-contents: directories list with some long names or too many sub-directories
• https://starlight-git-fork-julien-deramond-main-jd-bab67d-astrodotbuild.vercel.app/guides/css-and-tailwind/#custom-css-styles: complex lists with graphical elements in them
• https://starlight-git-fork-julien-deramond-main-jd-bab67d-astrodotbuild.vercel.app/guides/customization/#add-your-logo: combo of previous mentioned use cases
• https://starlight-git-fork-julien-deramond-main-jd-bab67d-astrodotbuild.vercel.app/resources/community-content/#astro-videos: list of cards

[changeset-bot]

🦋 Changeset detected

Latest commit: b82f34d

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

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
starlight	✅ Ready (Inspect)	Visit Preview	May 16, 2024 8:48pm

[delucis]

Thank you for jumping into this so quickly @julien-deramond!

I did some quick testing across the Starlight docs to investigate the potential for regressions (hard to be sure for all user content, so I’m hoping our range of content is a good enough test case).

I did find one specifically in our <Tabs> component that should never wrap and instead scroll horizontally when they overflow:

I guess we’ll need to override the overflow-wrap: anywhere for that component. It also gives a small indication that this could have an impact on user components in theory.

It makes me wonder if our global rule setting this should be scoped to the Markdown content and use :not(:where(.not-content *)) like the other rules to make it easier for users to escape that styling if needed. This rule here:

https://github.com/withastro/starlight/blob/main/packages/starlight/style/reset.css#L35-L44

[julien-deramond]

I guess we’ll need to override the overflow-wrap: anywhere for that component. It also gives a small indication that this could have an impact on user components in theory.

Yes, if it is difficult to measure the impact on Starlight side, we can be sure that it will have an impact on users' components.

It makes me wonder if our global rule setting this should be scoped to the Markdown content and use :not(:where(.not-content *)) like the other rules to make it easier for users to escape that styling if needed. This rule here:

https://github.com/withastro/starlight/blob/main/packages/starlight/style/reset.css#L35-L44

My first version was adding li to this rule, but it was a little bit "scary" not to be able to measure the impact and the potential regressions for the users, not knowing all the details of the codebase.

I'd be in favor to scope it to the Markdown content. It would seem to be the most maintainable approach for the Starlight core team, and maybe also less opinionated and/or impactful for the users and their components, yeah.

[HiDeoo]

Thanks for the great contribution. We reviewed this PR with a lot of eyes/devices/browsers today in Astro Talking & Doc'ing session.

To do so, we had everyone browse random pages on various Starlight websites (Starlight Docs, Astro Docs, SST Ion docs and Knip Docs) where I implemented ahead of time the suggested fix from this PR.

With exactly the suggested changes from this PR, no extra issue except the one spotted by Chris regarding the <Tabs> component was found.

We tried a slight variation of the selector (.sl-markdown-content li:not(:where(.not-content *))) which prevents this issue as the tabs list wrapper uses the not-content class. Altho, as spotted during the session, this would not work in the case of the <Tabs> component nested in the <Steps> component for example.

Did not play more than that yet with the selector, but I guess it will need a bit more tweaking to handle those cases.

[astrobot-houston]

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

Locale	File	Note
en	getting-started.mdx	Source changed, localizations will be marked as outdated.
en	guides/authoring-content.md	Source changed, localizations will be marked as outdated.
en	guides/customization.mdx	Source changed, localizations will be marked as outdated.

Warnings reference

Icon	Description
🔄️	The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

[julien-deramond]

Sorry for the delay, I was off and also overwhelmed by other OSS repos.

Based on #1736 (comment) and #1736 (review), I've reset the overflow-wrap value for tab items via d5fae7d#diff-07167ef1e6f2c6cadc53adf10a29f1e6057bcfad18c10b956fbfa6626bf43f15.

This can be tested out here:

• Start Here > Getting Started with the long "Yarnnnnn[...]" string in a basic <TabItem>
• Guides > Customizing Starlight > Set up fonts > Set up a Fontsource font with the long "Yarnnnnn[...]" string in a <Steps> + <TabItem>

With these modifications, the PR should cover all use cases.

[delucis]

Thank you for following up on this @julien-deramond! Just played around with it again and I think this is indeed the best approach.

I do wonder if our generic set-up for this in reset.css ought really also to be moved to markdown.css:

starlight/packages/starlight/style/reset.css

Lines 35 to 44 in d33bbc7

	p,
	h1,
	h2,
	h3,
	h4,
	h5,
	h6,
	code {
	overflow-wrap: anywhere;
	}

In a quick check, I think the only element this currently applies to which is outside of the Markdown content is the page title.

But that can happen as follow-up work to keep this PR focused on the specific fix.

[delucis]

Agree that this makes sense given how overflow-wrap is inherited 👍

[julien-deramond]

I do wonder if our generic set-up for this in reset.css ought really also to be moved to markdown.css:
In a quick check, I think the only element this currently applies to which is outside of the Markdown content is the page title.
But that can happen as follow-up work to keep this PR focused on the specific fix.

Yes, I can take a look in a follow-up PR to ease the review and evaluate the impacts.

Whenever this PR is accepted, feel free to ping me so that I can remove the extra testing data before merging :)

[delucis]

I think everything looks good to me!

We can remove the background colour and the test content I think.

[delucis]

Perfect — this looks good to me!

Would you want to write a changeset for the changelog (you can use the link in the bot comment above to add one), or would you prefer me to take care of that?

[julien-deramond]

Tried to add one thanks to your doc via b82f34d.
Chose "patch" bump.
Feel free to modify it so that it's consistent with the other changesets :) I'm not that aware of your architecture and release management.

[delucis]

Thanks again for the thoroughness working out the solution @julien-deramond and @HiDeoo for running it through the Talking & Doc’ing testing machine 💖