sfirke commented on code in PR #43722:
URL: https://github.com/apache/superset/pull/43722#discussion_r3916403313
##########
superset-frontend/plugins/plugin-chart-handlebars/src/Handlebars.tsx:
##########
@@ -37,7 +37,22 @@ export default function Handlebars(props: HandlebarsProps) {
const handlebarTemplateSource = formData.handlebarsTemplate
? formData.handlebarsTemplate
: '{{data}}';
- const templateSource = `${handlebarTemplateSource}\n${styleTemplateSource} `;
+ // Markdown only treats `<style>` as a raw-text block that may contain blank
+ // lines when the tag starts a block of its own. Joined to the template by a
+ // single newline, a template opening with an HTML tag absorbed the tag into
+ // its own block instead, so the first blank line in the CSS closed that
+ // block and every rule after it was parsed as Markdown and rendered as
+ // visible chart content. A blank line ends the template's block first, so
+ // the style block starts one of its own.
+ //
+ // The separator is added only when there is CSS to append: a blank line at
+ // the end of the template is not always inert, and appending one when the
+ // chart has no CSS at all would be a change to every existing template for
+ // no reason. Keep the style block last so the template stays first in the
+ // DOM, where positional selectors and cascade order expect it.
+ const templateSource = styleTemplateSource
+ ? `${handlebarTemplateSource}\n\n${styleTemplateSource} `
Review Comment:
Confirmed — thanks, that's real. I reproduced it against the pinned
handlebars version: the separator was appended before compilation, so `~}}` at
the end of the template stripped it and the `<style>` tag was glued back onto
the template's HTML block.
Appending after expansion does fix this case (whitespace control cannot act
on text that never goes through `Handlebars.compile`), but it still leaves the
coupling I flagged earlier — a template ending inside an unclosed code fence,
`<textarea>`, or `<!--` still swallows the CSS — and it needs the same plumbing
change either way, since the CSS has to reach the viewer separately from the
template. This is the third way the shared document has bitten, so e5fe8d3ad3
goes to the separate-render approach mentioned above: `HandlebarsViewer` now
takes the template and the style block as separate sources, compiles each
through Handlebars on its own (so expressions in the CSS field still expand,
but whitespace control in one source cannot reach the other), and renders each
through its own `SafeMarkdown`, style second.
The DOM comes out the same — react-markdown emits no wrapper element, so the
chart container's children are still the template's nodes followed by the style
element. Both of your earlier points hold and their tests pass unchanged: the
template stays first for positional selectors, and the CSS control still wins
the cascade. Added a regression test for the `--~}}` case and confirmed it
fails against the concatenated version.
---
🤖 _Drafted by Claude Code, co-signed by @sfirke._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
