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.

Sign up for GitHub

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

Jump to bottom

Using Parcel 2 SVG attributes are all getting lower cased #5004

Closed
dgkimpton opened this issue Aug 10, 2020 · 10 comments · Fixed by #6593
Closed

Using Parcel 2 SVG attributes are all getting lower cased #5004

dgkimpton opened this issue Aug 10, 2020 · 10 comments · Fixed by #6593
Labels
🐛 Bug ✔️ Confirmed Bug ✨ Parcel 2
Milestone
v2.0.0-rc.0

Comments

@dgkimpton
Copy link

🐛 bug report

Using Parcel 2 when I build, my inline SVG graphics get their attributes lowercased. SVG is a case sensitive specification, so this is not desired.

There are several other bug reports here about SVGO stripping out tags (like viewBox) which I believe is happening exactly because of this attribute lowercasing.

It feels like this is something that could be solved if I could get the correct configuration, but I've spent half a day on it and gotten nowhere... maybe someone here knows how to solve it?

(apart from this niggle I'm loving Parcel 2 so far)

🎛 Configuration

cli-command: npx parcel build index.html

package.json

{
  "name": "parcel2-svg-test",
  "private": true,
  "devDependencies": {
    "parcel": "^2.0.0-beta.1"
  }
}

.posthtmlrc

{
	"xmlMode": true,
	"recognizeSelfClosing": true,
	"lowerCaseAttributeNames": false,
}

.htmlnanorc

{
	"minifySvg": false
}

🤔 Expected Behavior

Attributes in the specification should not be changed to lower case.

😯 Current Behavior

The svg gets all its attribute names converted to lower case:

<svg viewbox="1.8 2.4 2 2" preserveaspectratio="xMinYMin meet"> <defs> <filter id="blur"> <feGaussianBlur in="SourceGraphic" stddeviation="0.01"></feGaussianBlur> </filter> </defs> <g> <path d="M2 3C3 2 4 3 3 4" stroke-width="0.1" stroke="blue" fill="none" filter="url(#blur)"></path> </g> </svg>

💻 Code Sample

index.html

<!DOCTYPE html><html lang="en"><head></head><body>

	<svg viewBox="1.8 2.4 2 2" preserveAspectRatio="xMinYMin meet">
		<defs>
			<filter id="blur">
				<feGaussianBlur in="SourceGraphic" stdDeviation="0.01" />
			</filter>
		</defs>
		<g>
			<path d="M2 3C3 2 4 3 3 4" stroke-width="0.1" stroke="blue" fill="none" filter="url(#blur)"/>
		</g>
	</svg>

</body></html>

🌍 Your Environment

Software Version(s)
Parcel 2.0.0-beta.1
Node v14.4.0
npm 6.14.5
Operating System Windows 10
@mischnic
Copy link
Member

It's actually not htmlnano (because it also happens with --no-minify), but:

parcel/packages/transformers/html/src/HTMLTransformer.js

Lines 20 to 22 in 46e8ffa

program: parse(await asset.getCode(), {
lowerCaseAttributeNames: true,
}),

@wbinnssmith Any chance you remember why you added this?

@dgkimpton
Copy link
Author

Apart from SVG, and possibly React custom attributes, this is generally a good choice. Even JS's Element.getAttribute() lowercases names before looking them up.

Chrome also doesn't seem to care about lowercase attributes in SVG, but a) the minimiser does and b) transforming valid code into technically invalid code just seems like a bad idea.

Is there a way to make this transformation be applied to everything except SVG?
(I've no idea what should be done about React and it's weird case sensitivity, make it optional I guess?).

@dgkimpton
Copy link
Author

Although lowerCaseAttributeNames is forced to be true in the HTMLTransformer I'm still not convinced this is actually a Parcel issue. The fact that the htmlparser2 library converts svg attributes to lower case seems to be wrong behaviour. I've submitted an issue to that project here: fb55/htmlparser2#539

I propose no-one change this Parcel 2 behaviour until we hear back from the maintainer of the parser. If the fix is applied at that level then upgrading the libraries will be enough.

@FRSgit
Copy link
Contributor

FRSgit commented Oct 12, 2020

Seems that htmlparser2 issue got closed. What would be the follow-up here?

@AndrewKvalheim
Copy link
Contributor

AndrewKvalheim commented Feb 25, 2021 •
edited

I looked everywhere and tried everything to resolve this with no luck

Per above this is caused by the use of lowerCaseAttributeNames. For a workaround it's sufficient to preempt the default HTML transformer with an empty one that parses without that option.

@devongovett devongovett added this to the v2.0.0-rc.0 milestone Jul 12, 2021
@devongovett
Copy link
Member

I just tested and lower case attribute names seem to work for me in browsers. This makes sense as the HTML spec says

In the HTML syntax, attribute names, even those for foreign elements, may be written with any mix of ASCII lower and ASCII upper alphas.

See https://html.spec.whatwg.org/multipage/syntax.html#syntax-attribute-name

Can someone explain what kind of problems they've seen with this?

@AndrewKvalheim
Copy link
Contributor

SVGO is opinionated about attribute case and deletes viewbox.

@devongovett
Copy link
Member

Hmm interesting. I'm not sure what we should do about this. We can't simply remove that parser option because we need to handle attributes of any case for other parts of HTML. I think ideally SVGO would have an option to indicate that the SVG is embedded inside HTML, because this is completely valid - i.e. someone could author their HTML that way and SVGO would be broken.

@devongovett devongovett mentioned this issue Jul 13, 2021
Closed
@devongovett
Copy link
Member

Maybe we should just disable removeUnknownsAndDefaults for now?

@AndrewKvalheim
Copy link
Contributor

That sounds reasonable; better to have some pending optimizations than corrupted images.

@devongovett devongovett mentioned this issue Jul 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
🐛 Bug ✔️ Confirmed Bug ✨ Parcel 2
Projects
None yet
Milestone
v2.0.0-rc.0
5 participants
@devongovett @AndrewKvalheim @mischnic @dgkimpton @FRSgit