Astro 3.5.5 compiler regression for whitespace

LER0ever commented 1 year ago

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

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.

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.

bluwy commented 1 year ago

It's likely caused by https://github.com/withastro/compiler/issues/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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

It also happens for anchors in 3.6

withastro / compiler