Skip to content

Commit

Permalink
fix #1776: only merge duplicate ie7 selectors
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Nov 16, 2021
1 parent 231bf7f commit 6e724a1
Show file tree
Hide file tree
Showing 4 changed files with 207 additions and 4 deletions.
27 changes: 27 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -10,6 +10,33 @@

Up until now, esbuild used 13.2.0 as the lowest supported version number to avoid generating dynamic `import` expressions when targeting node versions that don't support it. But with this release, esbuild will now use the more accurate discontiguous version range in this case. This means dynamic `import` expressions can now be generated when targeting versions of node 12.20.0 up to but not including node 13.0.0.

* Avoid merging certain qualified rules in CSS ([#1776](https://github.com/evanw/esbuild/issues/1776))

A change was introduced in the previous release to merge adjacent CSS rules that have the same content:

```css
/* Original code */
a { color: red }
b { color: red }

/* Minified output */
a,b{color:red}
```

However, that introduced a regression in cases where the browser considers one selector to be valid and the other selector to be invalid, such as in the following example:

```css
/* This rule is valid, and is applied */
a { color: red }

/* This rule is invalid, and is ignored */
b:-x-invalid { color: red }
```

Merging these two rules into one causes the browser to consider the entire merged rule to be invalid, which disables both rules. This is a change in behavior from the original code.

With this release, esbuild will now only merge adjacent duplicate rules together if they are known to work in all browsers (specifically, if they are known to work in IE 7 and up). Adjacent duplicate rules will no longer be merged in all other cases including modern pseudo-class selectors such as `:focus`, HTML5 elements such as `video`, and combinators such as `a + b`.

## 0.13.13

* Add more information about skipping `"main"` in `package.json` ([#1754](https://github.com/evanw/esbuild/issues/1754))
Expand Down
6 changes: 3 additions & 3 deletions internal/css_ast/css_ast.go
View file
Expand Up @@ -519,7 +519,7 @@ type NamespacedName struct {
// If present, this is an identifier or "*" and is followed by a "|" character
NamespacePrefix *NameToken

// This is an identifier or "*" or "&"
// This is an identifier or "*"
Name NameToken
}

Expand Down Expand Up @@ -565,9 +565,9 @@ func (ss *SSClass) Hash() uint32 {

type SSAttribute struct {
NamespacedName NamespacedName
MatcherOp string
MatcherOp string // Either "" or one of: "=" "~=" "|=" "^=" "$=" "*="
MatcherValue string
MatcherModifier byte
MatcherModifier byte // Either 0 or one of: 'i' 'I' 's' 'S'
}

func (a *SSAttribute) Equal(ss SS) bool {
Expand Down
160 changes: 159 additions & 1 deletion internal/css_parser/css_parser.go
View file
Expand Up @@ -326,7 +326,8 @@ skipRule:
// "a { color: red; } b { color: red; }" => "a, b { color: red; }"
if i > 0 {
if r, ok := rule.Data.(*css_ast.RSelector); ok {
if prev, ok := rules[i-1].Data.(*css_ast.RSelector); ok && css_ast.RulesEqual(r.Rules, prev.Rules) {
if prev, ok := rules[i-1].Data.(*css_ast.RSelector); ok && css_ast.RulesEqual(r.Rules, prev.Rules) &&
isSafeSelectors(r.Selectors) && isSafeSelectors(prev.Selectors) {
nextSelector:
for _, sel := range r.Selectors {
for _, prevSel := range prev.Selectors {
Expand Down Expand Up @@ -361,6 +362,163 @@ skipRule:
return rules[start:]
}

// Reference: https://developer.mozilla.org/en-US/docs/Web/HTML/Element
var nonDeprecatedElementsSupportedByIE7 = map[string]bool{
"a": true,
"abbr": true,
"address": true,
"area": true,
"b": true,
"base": true,
"blockquote": true,
"body": true,
"br": true,
"button": true,
"caption": true,
"cite": true,
"code": true,
"col": true,
"colgroup": true,
"dd": true,
"del": true,
"dfn": true,
"div": true,
"dl": true,
"dt": true,
"em": true,
"embed": true,
"fieldset": true,
"form": true,
"h1": true,
"h2": true,
"h3": true,
"h4": true,
"h5": true,
"h6": true,
"head": true,
"hr": true,
"html": true,
"i": true,
"iframe": true,
"img": true,
"input": true,
"ins": true,
"kbd": true,
"label": true,
"legend": true,
"li": true,
"link": true,
"map": true,
"menu": true,
"meta": true,
"noscript": true,
"object": true,
"ol": true,
"optgroup": true,
"option": true,
"p": true,
"param": true,
"pre": true,
"q": true,
"ruby": true,
"s": true,
"samp": true,
"script": true,
"select": true,
"small": true,
"span": true,
"strong": true,
"style": true,
"sub": true,
"sup": true,
"table": true,
"tbody": true,
"td": true,
"textarea": true,
"tfoot": true,
"th": true,
"thead": true,
"title": true,
"tr": true,
"u": true,
"ul": true,
"var": true,
}

// This only returns true if all of these selectors are considered "safe" which
// means that they are very likely to work in any browser a user might reasonably
// be using. We do NOT want to merge adjacent qualified rules with the same body
// if any of the selectors are unsafe, since then browsers which don't support
// that particular feature would ignore the entire merged qualified rule:
//
// Input:
// a { color: red }
// b { color: red }
// input::-moz-placeholder { color: red }
//
// Valid output:
// a, b { color: red }
// input::-moz-placeholder { color: red }
//
// Invalid output:
// a, b, input::-moz-placeholder { color: red }
//
// This considers IE 7 and above to be a browser that a user could possibly use.
// Versions of IE less than 6 are not considered.
func isSafeSelectors(complexSelectors []css_ast.ComplexSelector) bool {
for _, complex := range complexSelectors {
for _, compound := range complex.Selectors {
if compound.HasNestPrefix {
// Bail because this is an extension: https://drafts.csswg.org/css-nesting-1/
return false
}

if compound.Combinator != "" {
// "Before Internet Explorer 10, the combinator only works in standards mode"
// Reference: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Selectors
return false
}

if compound.TypeSelector != nil {
if compound.TypeSelector.NamespacePrefix != nil {
// Bail if we hit a namespace, which doesn't work in IE before version 9
// Reference: https://developer.mozilla.org/en-US/docs/Web/CSS/Type_selectors
return false
}

if compound.TypeSelector.Name.Kind == css_lexer.TIdent && !nonDeprecatedElementsSupportedByIE7[compound.TypeSelector.Name.Text] {
// Bail if this element is either deprecated or not supported in IE 7
return false
}
}

for _, ss := range compound.SubclassSelectors {
switch s := ss.(type) {
case *css_ast.SSAttribute:
if s.MatcherModifier != 0 {
// Bail if we hit a case modifier, which doesn't work in IE at all
// Reference: https://developer.mozilla.org/en-US/docs/Web/CSS/Attribute_selectors
return false
}

case *css_ast.SSPseudoClass:
// Bail if this pseudo class doesn't match a hard-coded list that's
// known to work everywhere. For example, ":focus" doesn't work in IE 7.
// Reference: https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-classes
if s.Args == nil && !s.IsElement {
switch s.Name {
case "active", "first-child", "hover", "link", "visited":
continue
}
}
return false
}
}
}
}
return true
}

func (p *parser) parseURLOrString() (string, logger.Range, bool) {
t := p.current()
switch t.Kind {
Expand Down
18 changes: 18 additions & 0 deletions internal/css_parser/css_parser_test.go
View file
Expand Up @@ -1480,4 +1480,22 @@ func TestMangleDuplicateSelectorRules(t *testing.T) {
expectPrintedMangle(t, "a { color: red; color: red } b { color: red }", "a,\nb {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } b { color: red; color: red }", "a,\nb {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } b { color: blue }", "a {\n color: red;\n}\nb {\n color: #00f;\n}\n")

// Do not merge duplicates if they are "unsafe"
expectPrintedMangle(t, "a { color: red } unknown { color: red }", "a {\n color: red;\n}\nunknown {\n color: red;\n}\n")
expectPrintedMangle(t, "unknown { color: red } a { color: red }", "unknown {\n color: red;\n}\na {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } video { color: red }", "a {\n color: red;\n}\nvideo {\n color: red;\n}\n")
expectPrintedMangle(t, "video { color: red } a { color: red }", "video {\n color: red;\n}\na {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } a:last-child { color: red }", "a {\n color: red;\n}\na:last-child {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } a[b=c i] { color: red }", "a {\n color: red;\n}\na[b=c i] {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } & { color: red }", "a {\n color: red;\n}\n& {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } a + b { color: red }", "a {\n color: red;\n}\na + b {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } a|b { color: red }", "a {\n color: red;\n}\na|b {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } a::hover { color: red }", "a {\n color: red;\n}\na::hover {\n color: red;\n}\n")

// Still merge duplicates if they are "safe"
expectPrintedMangle(t, "a { color: red } a:hover { color: red }", "a,\na:hover {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } a[b=c] { color: red }", "a,\na[b=c] {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } a#id { color: red }", "a,\na#id {\n color: red;\n}\n")
expectPrintedMangle(t, "a { color: red } a.cls { color: red }", "a,\na.cls {\n color: red;\n}\n")
}

0 comments on commit 6e724a1

Please sign in to comment.