feat(html): add normalizeUrlAttributes and widen two attribute rewrites - #12
Conversation
…ites Source PR: webpack#21960 Source head: eaddc87
⛔ Shipwright · BlockedRecommendation: do not merge PR #12 · Tier
Findings (6)
Fireworks usage: 41,045 input · 794 output · 41,839 total tokens · $0.0096 · 15s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| if (COMMA_LIST_ATTRIBUTES.has(name)) return _normalizeCommaList(raw); | ||
| } | ||
| const urlOn = URL_ATTRIBUTES.get(name); | ||
| const urlOn = _transforms.normalizeUrlAttributes |
There was a problem hiding this comment.
Shipwright · CRITICAL
'normalizeUrlAttributes' is enabled by default and trims ASCII whitespace around URL-valued attributes.
Impact: 'normalizeUrlAttributes' is enabled by default and trims ASCII whitespace around URL-valued attributes. The change assumes URL resolution always skips surrounding whitespace, but this is not universally true for all URL attributes, custom elements, or non-browser consumers. Trimming can alter 'getAttribute' results and potentially change how URLs are parsed by downstream systems, creating a security-relevant behavio…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| ? writeStyle(declarations !== null ? declarations : value) | ||
| : _writeAttribute(attributeName, value, rawValue, spellable); | ||
|
|
||
| // Offered whatever the built-in made of it: a list already at its |
There was a problem hiding this comment.
Shipwright · HIGH
The deferred embedded style handling now computes 'delimiter' as 'open[valueStart - tagStart - 1]' and builds 'asWritten' from raw source offsets.
Impact: The deferred embedded style handling now computes 'delimiter' as 'open[valueStart - tagStart - 1]' and builds 'asWritten' from raw source offsets. If 'valueStart' or 'tagStart' is at the beginning of the buffer, or the attribute is unquoted, this indexing can read the wrong character or produce an invalid reconstructed attribute. The previous code did not perform this raw-offset reconstruction, so this is a new fail…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| @@ -0,0 +1,6 @@ | |||
| import "./page.html"; | |||
|
|
|||
| it("should write a boolean attribute bare whatever its value said", () => { | |||
There was a problem hiding this comment.
Shipwright · LOW
The new test 'minimize-collapse-boolean-attributes-all/index.js' contains a tautological assertion ('expect(true).toBe(true)') and relies entirely on snapshot/afterExecute checks.
Impact: The new test 'minimize-collapse-boolean-attributes-all/index.js' contains a tautological assertion ('expect(true).toBe(true)') and relies entirely on snapshot/afterExecute checks. This makes the test's intent opaque to a new contributor and provides no direct behavioral assertion in the test body.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
Summary
Grew out of reviewing webpack#21730 by @aryanraj45, whose four proposed switches are mostly on
mainalready; this lands what is still missing and two bugs found while checking.normalizeUrlAttributesgates the whitespace trim around a URL value — the one rewrite of that family with no way to turn it off, though it changes the stringgetAttributehands back like its three siblings.collapseBooleanAttributesgains the"all"tierlib/html/htmlMinify.js's own TODO asked for:truestill rewrites only the spelling the spec canonicalizes,"all"any value, including thechecked="false"that already means checked.Two things were broken rather than missing.
sortTokenListssilently did nothing whenevernormalizeListAttributeswas off, though its own description calls it separate. And astyleattribute needs no switch of its own — it is an embedded body, so a renderer answering with the list it was given keeps it — but that worked only synchronously: where the answer is deferred, an answer nothing beats was read as no answer and the built-in's text stood, which is the path webpack's own minimizer takes.Neither new option is here for bytes: over the 68,253 documents in web-platform-tests,
"all"changes 27 files (184 bytes) and the URL trim fires on 38 (114 bytes). They exist so a consumer that compares those bytes has a way out, which is why the rest of the family exists.What kind of change does this PR introduce?
feat
Did you add tests for your changes?
Yes — a
SourceProcessor — attribute rewrites as switchesblock intest/HtmlSyntax.unittest.js, and two config cases,minimize-collapse-boolean-attributes-allandminimize-sort-token-lists-alone;minimize-transforms-offgained the new switch. Every added line and branch arm is covered.Does this PR introduce a breaking change?
No. Both options default to what webpack already did, and the two fixes only make an option and a renderer's answer take effect where they were dropped.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
The
normalizeUrlAttributesoption andcollapseBooleanAttributes's"all"tier, plus the note added tosortTokenListsthat it applies whatevernormalizeListAttributessays.Use of AI
Claude Code was used to compare webpack#21730 against
main, to write this change and its tests, and to verify it: the wpt measurement above,yarn lintin full, the html unit and config suites, andsyntax-equivalenceover the wpt corpus were all run locally. I reviewed every change.Generated by Claude Code
Summary by CodeRabbit
normalizeUrlAttributesto trim surrounding whitespace from URL attributes when enabled.collapseBooleanAttributes: "all"to collapse boolean attribute values into bare attributes.Source merge-base:
a03c24a180ed6ebfa5b38b23cf1148a4cc3378c7Source head:
eaddc87476c00d02e7bb6e05c9b5c9529d8dee4b