From 1ee4500574165ce2d6c7613c9c500e22b68477e0 Mon Sep 17 00:00:00 2001 From: Felix Lee Date: Wed, 22 Aug 2018 21:10:28 -0700 Subject: [PATCH] fix handling of some malformed css This fixes two incompatibilities between shady-css-parser and browsers. 1. If the css is missing a closing brace like div { color:red then Shady drops the last rule, but browsers keep it. 2. If a css ruleset has an empty selector like /*empty*/ { color: red } div { color: blue } then Shady discards the { } and produces this: color:red;div { color: blue; } which has an entirely different meaning. Browsers parse the empty-selector block, but then ignore it as invalid. For this case, we could either make Shady keep the block or drop the block. I decided to keep the block, because Shady generally passes through many other not-quite-valid css text that browsers would ignore. --- src/shady-css/parser.ts | 51 ++++++++++++++++-------------------- src/test/parser-test.ts | 36 +++++++++++++++++++++++++ src/test/stringifier-test.ts | 8 ++++++ 3 files changed, 66 insertions(+), 29 deletions(-) diff --git a/src/shady-css/parser.ts b/src/shady-css/parser.ts index 8246686..9310b80 100644 --- a/src/shady-css/parser.ts +++ b/src/shady-css/parser.ts @@ -88,7 +88,8 @@ class Parser { } else if (token.is(TokenType.comment)) { return this.parseComment(tokenizer); - } else if (token.is(TokenType.word)) { + } else if (token.is(TokenType.word) || + token.is(TokenType.openBrace)) { return this.parseDeclarationOrRuleset(tokenizer); } else if (token.is(TokenType.propertyBoundary)) { @@ -241,13 +242,13 @@ class Parser { * @param tokenizer A Tokenizer node. */ parseDeclarationOrRuleset(tokenizer: Tokenizer): Declaration|Ruleset|null { - let ruleStart = null; - let ruleEnd = null; - let colon = null; + if (!tokenizer.currentToken) { + return null; + } - // This code is not obviously correct. e.g. there's what looks to be a - // null-dereference if the declaration starts with an open brace or - // property boundary.. though that may be impossible. + let ruleStart = tokenizer.currentToken; + let ruleEnd = ruleStart.previous; + let colon = null; while (tokenizer.currentToken) { if (tokenizer.currentToken.is(TokenType.whitespace)) { @@ -266,25 +267,15 @@ class Parser { if (tokenizer.currentToken.is(TokenType.colon)) { colon = tokenizer.currentToken; } - - if (ruleStart === null) { - ruleStart = tokenizer.advance(); - ruleEnd = ruleStart; - } else { - ruleEnd = tokenizer.advance(); - } + ruleEnd = tokenizer.advance(); } } - if (tokenizer.currentToken === null) { - // terminated early - return null; - } - // A ruleset never contains or ends with a semi-colon. - if (tokenizer.currentToken.is(TokenType.propertyBoundary)) { + if (!tokenizer.currentToken || + tokenizer.currentToken.is(TokenType.propertyBoundary)) { const nameRange = - tokenizer.getRange(ruleStart!, colon ? colon.previous : ruleEnd); + tokenizer.getRange(ruleStart, colon ? colon.previous : ruleEnd); const declarationName = tokenizer.cssText.slice(nameRange.start, nameRange.end); @@ -298,12 +289,13 @@ class Parser { this.nodeFactory.expression(expressionValue, expressionRange); } - if (tokenizer.currentToken.is(TokenType.semicolon)) { + if (tokenizer.currentToken && + tokenizer.currentToken.is(TokenType.semicolon)) { tokenizer.advance(); } const range = tokenizer.trimRange(tokenizer.getRange( - ruleStart!, + ruleStart, tokenizer.currentToken && tokenizer.currentToken.previous || ruleEnd)); @@ -313,16 +305,17 @@ class Parser { } else if (colon && colon === ruleEnd) { const rulelist = this.parseRulelist(tokenizer); - if (tokenizer.currentToken.is(TokenType.semicolon)) { + if (tokenizer.currentToken && + tokenizer.currentToken.is(TokenType.semicolon)) { tokenizer.advance(); } - const nameRange = tokenizer.getRange(ruleStart!, ruleEnd.previous); + const nameRange = tokenizer.getRange(ruleStart, ruleEnd.previous); const declarationName = tokenizer.cssText.slice(nameRange.start, nameRange.end); const range = tokenizer.trimRange(tokenizer.getRange( - ruleStart!, + ruleStart, tokenizer.currentToken && tokenizer.currentToken.previous || ruleEnd)); @@ -330,16 +323,16 @@ class Parser { declarationName, rulelist, nameRange, range); // Otherwise, this is a ruleset: } else { - const selectorRange = tokenizer.getRange(ruleStart!, ruleEnd); + const selectorRange = tokenizer.getRange(ruleStart, ruleEnd); const selector = tokenizer.cssText.slice(selectorRange.start, selectorRange.end); const rulelist = this.parseRulelist(tokenizer); - const start = ruleStart!.start; + const start = ruleStart.start; let end; if (tokenizer.currentToken) { end = tokenizer.currentToken.previous ? tokenizer.currentToken.previous.end : - ruleStart!.end; + ruleStart.end; } else { // no current token? must have reached the end of input, so go up // until there diff --git a/src/test/parser-test.ts b/src/test/parser-test.ts index 0ed28d7..aded6c0 100644 --- a/src/test/parser-test.ts +++ b/src/test/parser-test.ts @@ -147,6 +147,42 @@ describe('Parser', () => { nodeFactory.discarded(';') ])); }); + + it('can parse empty selectors', () => { + expect(parser.parse('{ empty-a } { empty-b } empty-c { empty-d }')) + .to.containSubset(nodeFactory.stylesheet([ + nodeFactory.ruleset( + '', nodeFactory.rulelist([ + nodeFactory.declaration('empty-a', undefined)])), + nodeFactory.ruleset( + '', nodeFactory.rulelist([ + nodeFactory.declaration('empty-b', undefined)])), + nodeFactory.ruleset( + 'empty-c', nodeFactory.rulelist([ + nodeFactory.declaration('empty-d', undefined)])), + ])); + }); + + it('can parse unclosed blocks', () => { + expect(parser.parse('uncl-a { uncl-b: uncl-c uncl-d')) + .to.containSubset(nodeFactory.stylesheet([ + nodeFactory.ruleset( + 'uncl-a', nodeFactory.rulelist([ + nodeFactory.declaration( + 'uncl-b', nodeFactory.expression('uncl-c uncl-d')) + ])) + ])); + }); + + it('can parse unclosed blocks without a colon', () => { + expect(parser.parse('uncol-a { uncol-b')) + .to.containSubset(nodeFactory.stylesheet([ + nodeFactory.ruleset( + 'uncol-a', nodeFactory.rulelist([ + nodeFactory.declaration('uncol-b', undefined) + ])) + ])); + }); }); describe('when extracting ranges', () => { diff --git a/src/test/stringifier-test.ts b/src/test/stringifier-test.ts index e6d7149..9d7834d 100644 --- a/src/test/stringifier-test.ts +++ b/src/test/stringifier-test.ts @@ -107,6 +107,14 @@ describe('Stringifier', () => { expect(cssText).to.be.eql(':root{--qux:vim;--foo:{bar:baz;};}'); }); + it('can stringify empty selectors', () => { + const cssText = + stringifier.stringify(parser.parse( + '{ empty-a } { empty-b } empty-c { empty-d }')); + expect(cssText).to.be + .eql('{empty-a;}{empty-b;}empty-c{empty-d;}'); + }); + describe('with discarded nodes', () => { it('stringifies to a corrected stylesheet', () => { const cssText =