fix(parser): support Liquid tag blocks in HTML tag names#1190
Open
SinhSinhAn wants to merge 1 commit intoShopify:mainfrom
Open
fix(parser): support Liquid tag blocks in HTML tag names#1190SinhSinhAn wants to merge 1 commit intoShopify:mainfrom
SinhSinhAn wants to merge 1 commit intoShopify:mainfrom
Conversation
Previously the parser threw LiquidHTMLSyntaxError for tag names that
contained {% ... %} blocks:
<{% if true %}div{% endif %}></{% if true %}div{% endif %}>
Only {{ ... }} (LiquidVariableOutput) was allowed in tag name
positions. This pattern existed in older Dawn themes and is still
usable in the theme editor, so the parser should accept it.
Grammar change: add liquidTagClose, liquidTagOpen, and liquidTag as
alternatives in leadingTagNamePart and trailingTagNamePart. The CST
and AST builders already iterate over the name array and convert each
part individually, so the existing pipeline handles the new node types
once the grammar allows them.
Name matching for open/close tag pairing in getName() now uses the raw
source slice for any non-text non-drop part, so {% if true %}div{% endif %}
compares byte-for-byte across open and close tags.
Closes Shopify#1155
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR?
Fixes a
LiquidHTMLSyntaxErrorfalse positive where the parser rejected HTML tags whose names contain{% ... %}blocks:<{% if true %}div{% endif %}></{% if true %}div{% endif %}>What was the issue?
#1155 reported that
@shopify/liquid-html-parserthrows on this syntax, but the theme editor accepts it and older versions of the Dawn theme used this pattern (example).The parser grammar only allowed
{{ ... }}(liquidDrop/LiquidVariableOutput) in tag name positions.{% ... %}blocks were not listed as alternatives inleadingTagNamePart/trailingTagNamePart, so parsing failed at the{character with the familiarexpected a letter, "{{", "wbr" (case-insensitive)...error.How this PR fixes it
Grammar (
liquid-html.ohm)Added
liquidTagClose,liquidTagOpen, andliquidTagas alternatives in bothleadingTagNamePartandtrailingTagNamePart. Order matters: the close/open variants are listed before the genericliquidTagto prevent the broader base-case rule from greedily absorbing specific block openers.Types (
stage-1-cst.ts,stage-2-ast.ts)Widened the
namefield onConcreteHtmlTagOpen,ConcreteHtmlTagClose,ConcreteHtmlSelfClosingElementto acceptConcreteLiquidNodealongsideConcreteTextNode. Mirrored the change on the AST side forHtmlElement,HtmlDanglingMarkerClose, andHtmlSelfClosingElement(now(TextNode | LiquidNode)[]).Name matching (
getName()instage-2-ast.ts)The existing implementation only handled
TextNodeandLiquidVariableOutput. Added a branch for any other liquid node (liquid tags, raw tags, etc.) that returns the raw source slice between the part's start and end positions. This means the comparison between<{% if %}div{% endif %}>and its closing tag happens byte-for-byte, and mismatches still produce the existing"Attempting to close HtmlElement ... before ... was closed"error.AST builder behavior
No change needed. The AST builder's
cstToAstalready recursively converts each name-array part individually. When the grammar emits aLiquidTagOpen + TextNode + LiquidTagClosesequence, the builder assembles them into a singleLiquidTagnode with the text wrapped in aLiquidBranchchild, which is the cleanest possible representation.Tests
Added 3 test cases across the CST and AST spec files:
stage-1-cst.spec.ts: verifies the CST correctly emitsLiquidTagOpen + TextNode + LiquidTagClosefor both open and close HTML tagsstage-2-ast.spec.ts: verifies the AST correctly assembles into a singleLiquidTagwith branch childrenstage-2-ast.spec.ts: verifies mixed drop + liquid-tag tag names (<{{ prefix }}-{% if cond %}suffix{% endif %}>)Regression coverage
All existing tests still pass:
1,162 total tests pass, zero regressions.
Manual spot check:
Closes #1155