From dfbbfae196f75fd27d50c560c183f92e2f33368c Mon Sep 17 00:00:00 2001 From: Andrew Jakubowicz Date: Fri, 5 Jan 2024 16:14:10 -0800 Subject: [PATCH 1/3] Make isSelfClosed check far more specific, checking void tags and self closing syntax. --- .../parse-html-node/parse-html-node.ts | 23 +++++++++++--- .../types/html-node/html-node-types.ts | 4 +++ .../src/lib/rules/no-unclosed-tag.ts | 6 +++- .../src/test/rules/no-unclosed-tag.ts | 30 +++++++++++++++++++ 4 files changed, 58 insertions(+), 5 deletions(-) diff --git a/packages/lit-analyzer/src/lib/analyze/parse/document/text-document/html-document/parse-html-node/parse-html-node.ts b/packages/lit-analyzer/src/lib/analyze/parse/document/text-document/html-document/parse-html-node/parse-html-node.ts index eab8ca82..affe476d 100644 --- a/packages/lit-analyzer/src/lib/analyze/parse/document/text-document/html-document/parse-html-node/parse-html-node.ts +++ b/packages/lit-analyzer/src/lib/analyze/parse/document/text-document/html-document/parse-html-node/parse-html-node.ts @@ -5,6 +5,9 @@ import { getSourceLocation, IP5TagNode, P5Node } from "../parse-html-p5/parse-ht import { parseHtmlNodeAttrs } from "./parse-html-attribute.js"; import { ParseHtmlContext } from "./parse-html-context.js"; +// List taken from https://html.spec.whatwg.org/multipage/syntax.html#void-elements +const VOID_ELEMENTS = new Set(["area", "base", "br", "col", "embed", "hr", "img", "input", "link", "meta", "source", "track", "wbr"]); + /** * Parses multiple p5Nodes into multiple html nodes. * @param p5Nodes @@ -70,14 +73,26 @@ export function parseHtmlNode(p5Node: IP5TagNode, parent: HtmlNode | undefined, } /** - * Returns if this node is self-closed. + * Returns if this node was explicitly self-closed or void. + * + * Note: Self-closing tags do not actually exist in HTML + * https://developer.mozilla.org/en-US/docs/Glossary/Void_element#self-closing_tags + * + * Therefore this function returns `true` if `p5Node` is + * a void element, or was explicitly self-closed using XML/JSX + * syntax. If the node is implicitly closed, for example a + * `p` element followed by a `div`, then `false` is returned. + * * @param p5Node * @param context */ function isSelfClosed(p5Node: IP5TagNode, context: ParseHtmlContext) { - const isEmpty = p5Node.childNodes == null || p5Node.childNodes.length === 0; - const isSelfClosed = getSourceLocation(p5Node)!.startTag.endOffset === getSourceLocation(p5Node)!.endOffset; - return isEmpty && isSelfClosed; + if (VOID_ELEMENTS.has(p5Node.tagName.toLowerCase())) { + return true; + } + const loc = getSourceLocation(p5Node)!; + const isSelfClosed = context.html[loc.startTag.endOffset - 2] === "/"; + return isSelfClosed; } /** diff --git a/packages/lit-analyzer/src/lib/analyze/types/html-node/html-node-types.ts b/packages/lit-analyzer/src/lib/analyze/types/html-node/html-node-types.ts index 1046bc6c..636c046c 100644 --- a/packages/lit-analyzer/src/lib/analyze/types/html-node/html-node-types.ts +++ b/packages/lit-analyzer/src/lib/analyze/types/html-node/html-node-types.ts @@ -20,6 +20,10 @@ export interface IHtmlNodeBase { attributes: HtmlNodeAttr[]; parent?: HtmlNode; children: HtmlNode[]; + /** + * Is true when an HTML node is either a void element, or was + * explicitly closed with self-closing XML syntax. + */ selfClosed: boolean; document: HtmlDocument; } diff --git a/packages/lit-analyzer/src/lib/rules/no-unclosed-tag.ts b/packages/lit-analyzer/src/lib/rules/no-unclosed-tag.ts index 04ded8a2..7ddd6b9a 100644 --- a/packages/lit-analyzer/src/lib/rules/no-unclosed-tag.ts +++ b/packages/lit-analyzer/src/lib/rules/no-unclosed-tag.ts @@ -11,7 +11,11 @@ const rule: RuleModule = { priority: "low" }, visitHtmlNode(htmlNode, context) { - if (!htmlNode.selfClosed && htmlNode.location.endTag == null) { + const { + selfClosed, + location: { endTag } + } = htmlNode; + if (!selfClosed && endTag == null) { // Report specifically that a custom element cannot be self closing // if the user is trying to close a custom element. const isCustomElement = isCustomElementTagName(htmlNode.tagName); diff --git a/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts b/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts index b6fa0ed8..cea6496d 100644 --- a/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts +++ b/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts @@ -11,3 +11,33 @@ tsTest("Don't report self closed tags", t => { const { diagnostics } = getDiagnostics("html``", { rules: { "no-unclosed-tag": true } }); hasNoDiagnostics(t, diagnostics); }); + +tsTest("Don't report void tags", t => { + const { diagnostics } = getDiagnostics("html``", { rules: { "no-unclosed-tag": true } }); + hasNoDiagnostics(t, diagnostics); +}); + +// The `

` tag will be closed automatically if immediately followed by a lot of other elements, +// including `

`. +// Ref: https://html.spec.whatwg.org/multipage/grouping-content.html#the-p-element +tsTest("Report unclosed 'p' tag that is implicitly closed via tag omission", t => { + const { diagnostics } = getDiagnostics("html`

`", { rules: { "no-unclosed-tag": true } }); + hasDiagnostic(t, diagnostics, "no-unclosed-tag"); +}); + +tsTest("Report unclosed 'p' tag that is implicitly closed via tag omission containing text content", t => { + const { diagnostics } = getDiagnostics("html`

Unclosed Content

`", { rules: { "no-unclosed-tag": true } }); + hasDiagnostic(t, diagnostics, "no-unclosed-tag"); +}); + +// Self-closing tags do not exist in HTML, but we can use them to check +// that the user intended that the tag be closed. +tsTest("Don't report self closing 'p' tag", t => { + const { diagnostics } = getDiagnostics("html`

`", { rules: { "no-unclosed-tag": true } }); + hasNoDiagnostics(t, diagnostics); +}); + +tsTest("Don't report self closing 'p' tag containing text content", t => { + const { diagnostics } = getDiagnostics("html`

Unclosed Content

`", { rules: { "no-unclosed-tag": true } }); + hasNoDiagnostics(t, diagnostics); +}); From e0bda3c075e7d3450a9eb23c893ef047e95871a7 Mon Sep 17 00:00:00 2001 From: Andrew Jakubowicz Date: Mon, 8 Jan 2024 10:25:43 -0800 Subject: [PATCH 2/3] non void elements must have an explicit closing tag --- .../parse-html-node/parse-html-node.ts | 27 ----------- .../types/html-node/html-node-types.ts | 5 -- .../src/lib/rules/no-unclosed-tag.ts | 47 +++++++++++++------ .../src/test/rules/no-unclosed-tag.ts | 24 ++++++---- 4 files changed, 47 insertions(+), 56 deletions(-) diff --git a/packages/lit-analyzer/src/lib/analyze/parse/document/text-document/html-document/parse-html-node/parse-html-node.ts b/packages/lit-analyzer/src/lib/analyze/parse/document/text-document/html-document/parse-html-node/parse-html-node.ts index affe476d..f005f42f 100644 --- a/packages/lit-analyzer/src/lib/analyze/parse/document/text-document/html-document/parse-html-node/parse-html-node.ts +++ b/packages/lit-analyzer/src/lib/analyze/parse/document/text-document/html-document/parse-html-node/parse-html-node.ts @@ -5,9 +5,6 @@ import { getSourceLocation, IP5TagNode, P5Node } from "../parse-html-p5/parse-ht import { parseHtmlNodeAttrs } from "./parse-html-attribute.js"; import { ParseHtmlContext } from "./parse-html-context.js"; -// List taken from https://html.spec.whatwg.org/multipage/syntax.html#void-elements -const VOID_ELEMENTS = new Set(["area", "base", "br", "col", "embed", "hr", "img", "input", "link", "meta", "source", "track", "wbr"]); - /** * Parses multiple p5Nodes into multiple html nodes. * @param p5Nodes @@ -52,7 +49,6 @@ export function parseHtmlNode(p5Node: IP5TagNode, parent: HtmlNode | undefined, const htmlNodeBase: IHtmlNodeBase = { tagName: p5Node.tagName.toLowerCase(), - selfClosed: isSelfClosed(p5Node, context), attributes: [], location: makeHtmlNodeLocation(p5Node, context), children: [], @@ -72,29 +68,6 @@ export function parseHtmlNode(p5Node: IP5TagNode, parent: HtmlNode | undefined, return htmlNode; } -/** - * Returns if this node was explicitly self-closed or void. - * - * Note: Self-closing tags do not actually exist in HTML - * https://developer.mozilla.org/en-US/docs/Glossary/Void_element#self-closing_tags - * - * Therefore this function returns `true` if `p5Node` is - * a void element, or was explicitly self-closed using XML/JSX - * syntax. If the node is implicitly closed, for example a - * `p` element followed by a `div`, then `false` is returned. - * - * @param p5Node - * @param context - */ -function isSelfClosed(p5Node: IP5TagNode, context: ParseHtmlContext) { - if (VOID_ELEMENTS.has(p5Node.tagName.toLowerCase())) { - return true; - } - const loc = getSourceLocation(p5Node)!; - const isSelfClosed = context.html[loc.startTag.endOffset - 2] === "/"; - return isSelfClosed; -} - /** * Creates source code location from a p5Node. * @param p5Node diff --git a/packages/lit-analyzer/src/lib/analyze/types/html-node/html-node-types.ts b/packages/lit-analyzer/src/lib/analyze/types/html-node/html-node-types.ts index 636c046c..daca7910 100644 --- a/packages/lit-analyzer/src/lib/analyze/types/html-node/html-node-types.ts +++ b/packages/lit-analyzer/src/lib/analyze/types/html-node/html-node-types.ts @@ -20,11 +20,6 @@ export interface IHtmlNodeBase { attributes: HtmlNodeAttr[]; parent?: HtmlNode; children: HtmlNode[]; - /** - * Is true when an HTML node is either a void element, or was - * explicitly closed with self-closing XML syntax. - */ - selfClosed: boolean; document: HtmlDocument; } diff --git a/packages/lit-analyzer/src/lib/rules/no-unclosed-tag.ts b/packages/lit-analyzer/src/lib/rules/no-unclosed-tag.ts index 7ddd6b9a..bcfddd64 100644 --- a/packages/lit-analyzer/src/lib/rules/no-unclosed-tag.ts +++ b/packages/lit-analyzer/src/lib/rules/no-unclosed-tag.ts @@ -2,6 +2,29 @@ import { RuleModule } from "../analyze/types/rule/rule-module.js"; import { isCustomElementTagName } from "../analyze/util/is-valid-name.js"; import { rangeFromHtmlNode } from "../analyze/util/range-util.js"; +// List taken from https://html.spec.whatwg.org/multipage/syntax.html#void-elements +// and parse5 list of void elements: https://github.com/inikulin/parse5/blob/86f09edd5a6840ab2269680b0eef2945e78c38fd/packages/parse5/lib/serializer/index.ts#L7-L26 +const VOID_ELEMENTS = new Set([ + "area", + "base", + "basefont", + "bgsound", + "br", + "col", + "embed", + "frame", + "hr", + "img", + "input", + "keygen", + "link", + "meta", + "param", + "source", + "track", + "wbr" +]); + /** * This rule validates that all tags are closed properly. */ @@ -11,22 +34,18 @@ const rule: RuleModule = { priority: "low" }, visitHtmlNode(htmlNode, context) { - const { - selfClosed, - location: { endTag } - } = htmlNode; - if (!selfClosed && endTag == null) { - // Report specifically that a custom element cannot be self closing - // if the user is trying to close a custom element. - const isCustomElement = isCustomElementTagName(htmlNode.tagName); - - context.report({ - location: rangeFromHtmlNode(htmlNode), - message: `This tag isn't closed.${isCustomElement ? " Custom elements cannot be self closing." : ""}` - }); + if (VOID_ELEMENTS.has(htmlNode.tagName.toLowerCase()) || htmlNode.location.endTag != null) { + return; } - return; + // Report specifically that a custom element cannot be self closing + // if the user is trying to close a custom element. + const isCustomElement = isCustomElementTagName(htmlNode.tagName); + + context.report({ + location: rangeFromHtmlNode(htmlNode), + message: `This tag isn't closed.${isCustomElement ? " Custom elements cannot be self closing." : ""}` + }); } }; diff --git a/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts b/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts index cea6496d..3ce43a9e 100644 --- a/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts +++ b/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts @@ -7,20 +7,20 @@ tsTest("Report unclosed tags", t => { hasDiagnostic(t, diagnostics, "no-unclosed-tag"); }); -tsTest("Don't report self closed tags", t => { - const { diagnostics } = getDiagnostics("html``", { rules: { "no-unclosed-tag": true } }); +tsTest("Don't report void elements", t => { + const { diagnostics } = getDiagnostics("html``", { rules: { "no-unclosed-tag": true } }); hasNoDiagnostics(t, diagnostics); }); -tsTest("Don't report void tags", t => { - const { diagnostics } = getDiagnostics("html``", { rules: { "no-unclosed-tag": true } }); +tsTest("Don't report void elements with self closing syntax", t => { + const { diagnostics } = getDiagnostics("html``", { rules: { "no-unclosed-tag": true } }); hasNoDiagnostics(t, diagnostics); }); // The `

` tag will be closed automatically if immediately followed by a lot of other elements, // including `

`. // Ref: https://html.spec.whatwg.org/multipage/grouping-content.html#the-p-element -tsTest("Report unclosed 'p' tag that is implicitly closed via tag omission", t => { +tsTest("Report unclosed 'p' tag that was implicitly closed via tag omission", t => { const { diagnostics } = getDiagnostics("html`

`", { rules: { "no-unclosed-tag": true } }); hasDiagnostic(t, diagnostics, "no-unclosed-tag"); }); @@ -30,14 +30,18 @@ tsTest("Report unclosed 'p' tag that is implicitly closed via tag omission conta hasDiagnostic(t, diagnostics, "no-unclosed-tag"); }); -// Self-closing tags do not exist in HTML, but we can use them to check -// that the user intended that the tag be closed. -tsTest("Don't report self closing 'p' tag", t => { +// Self-closing tags do not exist in HTML. They are only valid in SVG and MathML. +tsTest("Report non-void element using self closing syntax", t => { const { diagnostics } = getDiagnostics("html`

`", { rules: { "no-unclosed-tag": true } }); - hasNoDiagnostics(t, diagnostics); + hasDiagnostic(t, diagnostics, "no-unclosed-tag"); }); -tsTest("Don't report self closing 'p' tag containing text content", t => { +tsTest("Report self closing 'p' tag containing text content", t => { const { diagnostics } = getDiagnostics("html`

Unclosed Content

`", { rules: { "no-unclosed-tag": true } }); + hasDiagnostic(t, diagnostics, "no-unclosed-tag"); +}); + +tsTest("Don't report explicit closing 'p' tag containing text content", t => { + const { diagnostics } = getDiagnostics("html`

Unclosed Content

`", { rules: { "no-unclosed-tag": true } }); hasNoDiagnostics(t, diagnostics); }); From 53f7b563d01058ea19b4eef8ca3efbf6d671b5a5 Mon Sep 17 00:00:00 2001 From: Andrew Jakubowicz Date: Mon, 8 Jan 2024 10:51:40 -0800 Subject: [PATCH 3/3] add regression test and fix self closing custom-element in another test --- .../src/test/rules/no-nullable-attribute-binding.ts | 2 +- packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/lit-analyzer/src/test/rules/no-nullable-attribute-binding.ts b/packages/lit-analyzer/src/test/rules/no-nullable-attribute-binding.ts index 3e788653..b0fed9a4 100644 --- a/packages/lit-analyzer/src/test/rules/no-nullable-attribute-binding.ts +++ b/packages/lit-analyzer/src/test/rules/no-nullable-attribute-binding.ts @@ -11,7 +11,7 @@ tsTest("Cannot assign 'undefined' in attribute binding", t => { tsTest("Can assign 'undefined' in property binding", t => { const { diagnostics } = getDiagnostics([ makeElement({ slots: ["foo: number | undefined"] }), - 'html``' + 'html``' ]); hasNoDiagnostics(t, diagnostics); }); diff --git a/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts b/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts index 3ce43a9e..fee9ecbb 100644 --- a/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts +++ b/packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts @@ -30,6 +30,14 @@ tsTest("Report unclosed 'p' tag that is implicitly closed via tag omission conta hasDiagnostic(t, diagnostics, "no-unclosed-tag"); }); +// Regeression test for https://github.com/runem/lit-analyzer/issues/283 +tsTest("Report 'p' tag that is implicitly closed via tag omission containing a space", t => { + // Note, the browser will parse this case into: `

` which can be + // unexpected, but technically means the first `

` tag is not explicitly closed. + const { diagnostics } = getDiagnostics("html`

`", { rules: { "no-unclosed-tag": true } }); + hasDiagnostic(t, diagnostics, "no-unclosed-tag"); +}); + // Self-closing tags do not exist in HTML. They are only valid in SVG and MathML. tsTest("Report non-void element using self closing syntax", t => { const { diagnostics } = getDiagnostics("html`

`", { rules: { "no-unclosed-tag": true } });