sfirke commented on code in PR #43722:
URL: https://github.com/apache/superset/pull/43722#discussion_r3905080924
##########
superset-frontend/plugins/plugin-chart-handlebars/src/Handlebars.tsx:
##########
@@ -37,7 +37,16 @@ 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. Appending it to the template
+ // instead let a template opening with an HTML tag absorb the tag, so the
+ // first blank line in the CSS closed that block and every rule after it was
+ // parsed as a paragraph and rendered as visible text. Putting the style
+ // block first, separated by a blank line, keeps the CSS out of any block the
+ // template opens and leaves the template's own rendering untouched.
+ const templateSource = styleTemplateSource
Review Comment:
Both of these are right. Fixed, and the branch is squashed to a single
commit (79fee60b83) — sorry, that will mark some of the review history as
outdated.
The style block is back at the end of the document, separated from the
template by a blank line rather than reordered. The blank line closes whatever
block the template opened, so the `<style>` tag starts a raw-text block of its
own, which is what keeps Markdown out of the CSS. The template keeps its
position, so `table:first-child` matches the table the author wrote, and the
control's rules still come after any `<style>` inside the template and still
win the cascade.
I should be straight about how this got here, since you reviewed the detour.
Moving the style block to the front was my idea and it rested on a claim I got
wrong: I said appending a blank line after the template would turn a tight
Markdown list loose. It doesn't — I'd reasoned it from the spec instead of
running it. That was the only argument for reordering, so with it gone the fix
collapses back to the original one-line change plus a guard that skips the
separator when no CSS is configured. Your two comments are what surfaced that,
so thank you.
Tests now cover both of the things you flagged, and I checked they fail
against the reordered version.
One thing this deliberately doesn't fix. The template and the CSS are still
a single Markdown document, so the template can still affect how the CSS is
parsed. If the template ends while something is open that a blank line won't
close — an unclosed code fence, `<textarea>`, `<script>`, an unterminated
`<!--` — the CSS gets absorbed into it and can show up as text. That's the
behaviour on master, so it isn't a regression, and it needs a template that's
malformed at the point it renders.
The categorical fix is to stop concatenating: hand the template and the CSS
to `HandlebarsViewer` separately and render each through its own
`SafeMarkdown`, style second. The DOM comes out the same, but neither source
can affect the other's parsing and there's no separator rule left to get wrong.
I have it working if you'd prefer it — I kept it out of this PR because the
blank line fixes everything actually reported, and the smaller diff seemed like
the better trade.
---
🤖 _Drafted by Claude Code, co-signed by @sfirke._
##########
superset-frontend/plugins/plugin-chart-handlebars/src/Handlebars.tsx:
##########
@@ -37,7 +37,16 @@ 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. Appending it to the template
+ // instead let a template opening with an HTML tag absorb the tag, so the
+ // first blank line in the CSS closed that block and every rule after it was
+ // parsed as a paragraph and rendered as visible text. Putting the style
+ // block first, separated by a blank line, keeps the CSS out of any block the
+ // template opens and leaves the template's own rendering untouched.
+ const templateSource = styleTemplateSource
Review Comment:
Same fix — the style block is back at the end of the document, so a
`<style>` written into the template is overridden by the CSS Styles control
again, as on master. There's a regression test for it now. More detail in the
thread above.
---
🤖 _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]
