Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Astro 3.5.5 compiler regression for whitespace #893

Open
1 task
LER0ever opened this issue Nov 20, 2023 · 4 comments
Open
1 task

Astro 3.5.5 compiler regression for whitespace #893

LER0ever opened this issue Nov 20, 2023 · 4 comments
Labels
- P4: important Violate documented behavior or significantly improves performance (priority)

Comments

@LER0ever
Copy link

LER0ever commented Nov 20, 2023

Astro Info

Astro                    v3.5.5
Node                     v18.17.1
System                   macOS (arm64)
Package Manager          pnpm
Output                   static
Adapter                  none
Integrations             @astrojs/tailwind
                         @astrojs/react
                         @astrojs/sitemap
                         astro-compress

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

Astro v3.5.5 (or specifically @astrojs/compiler 2.3.1) generates additional whitespace for the same code compared to previous versions, presumably due to different handling of new lines in source code.

For the following example, taken out of astro-paper:

<nav class="breadcrumb" aria-label="breadcrumb">
  <ul>
    <li>
      <a href="/">Home</a>
      <span aria-hidden="true">&nbsp;&gt;&nbsp;</span>
    </li>
    ...

Locking to Astro 3.5.3 and @astrojs/compiler 2.3.0 would give the following breadcrumbs, which I believe is the correct behavior

image

But the latest Astro 3.5.5 (@astrojs/compiler 2.3.2) generates additional whitespaces around the <span>, which can only be removed if I manually format the file where the <a> and <span> above are on the same line. The whitespace is visible as 1 character wide, and also shown in the FireFox DevTools.

image

Using Astro 3.5.5 with compiler 2.3.0 works fine, and is my current workaround. This narrows the problem to @astrojs/compiler rather than the astro main package. And the above can be reproduced with a fresh clone of https://github.com/satnaing/astro-paper after running npm upgrade, and the workaround can be verified by locking
"@astrojs/compiler": "2.3.0"

What's the expected result?

I would expect the old version to be correct, i.e. behavior from @astrojs/compiler 2.3.0 and before. But I don't know much about web dev to be sure if this is a regression or just a previously wrong behavior fixed.

Link to Minimal Reproducible Example

https://github.com/satnaing/astro-paper

Sorry, I'm not able to figure out what's wrong to put it inside a minimal example, and I was hoping devs working on @astrojs/compiler would know from the description above. Also the astro-paper project can also be used to reproduce the issue.

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Nov 20, 2023
@bluwy
Copy link
Member

bluwy commented Nov 20, 2023

It's likely caused by #879. While I'm inclined that we're preserving the whitespace as before compacting, which feels like the more correct behaviour, the compressHTML option does document that:

... By default, Astro removes all whitespace from your HTML, including line breaks, from .astro components. This occurs both in development mode and in the final build.

So I wonder if my fix isn't actually right. Perhaps @natemoo-re is able to make the call here.

@natemoo-re
Copy link
Member

@bluwy I believe the change you merged is mostly correct! This seems like a case where the element whitespace inside the BLOCK element should be completely removed but whitespace inside the INLINE elements should be preserved.

Adjusting this isWhitespaceInsensitiveElement check to also return true for BLOCK elements should fix it.
https://github.com/withastro/compiler/pull/886/files#diff-acb773b2f6cf89261440e9e0b04cb34b7e62086194256b09abfaad172fd56403R280

@bluwy bluwy transferred this issue from withastro/astro Nov 21, 2023
@bluwy
Copy link
Member

bluwy commented Nov 21, 2023

I think the part I'm unclear of is how we decide what elements are block vs inline. We can pick their default states but wouldn't users be able to change block->inline & inline->block with CSS? I had head in isWhitespaceInsensitiveElement because head can never be inline.

@florian-lefebvre
Copy link
Member

It also happens for anchors in 3.6

sanman1k98 added a commit to sanman1k98/www that referenced this issue Dec 15, 2023
Links on the resume page had additional whitespace surrounding `<span>`
elements, making the underline on the anchor elements look weird.
Similar issue with the `<summary>` elements.

Since `@astrojs/[email protected]`, Astro preserves whitespace before
compacting; see withastro/compiler#893 for more details and links to
other issues.

To address this change without refactoring all the HTML onto a single
line to avoid creating whitespace-only text nodes inside an
inline-formatting context, set the display to `flex`. For information on
how whitespace is handled by HTML and CSS, see this article on MDN:
https://developer.mozilla.org/en-US/docs/Web/API/Document_Object_Model/Whitespace
reupen added a commit to reupen/reupen.uk that referenced this issue Dec 19, 2023
withastro/compiler#893

Astro is now preserving more white space than before, causes extra spaces in links in particular.

This applies some CSS workarounds to hide that white space.
@MoustaphaDev MoustaphaDev added - P4: important Violate documented behavior or significantly improves performance (priority) and removed needs triage Issue needs to be triaged labels Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

No branches or pull requests

5 participants