Debug mode: Never wrap head-emitting helper output in debug spans - #1953
Debug mode: Never wrap head-emitting helper output in debug spans#1953kcdragon wants to merge 8 commits into
Conversation
A partial rendered into a layout's <head> has no <head> node in its own AST, so the element-stack exclusion in `in_excluded_context?` never fires and helpers like `csrf_meta_tags` or `javascript_include_tag` got wrapped in a `<span style="display: contents;">`. Since <span> is invalid head content, browsers implicitly close </head> there and push the remaining scripts/stylesheets into <body>, breaking Turbo apps. Add a name-based `head_content_helper?` skip (alongside the existing `complex_rails_helper?` guard) covering the standard head helpers and `tag.meta`/`tag.link`/`tag.title`/`tag.base`, and add `meta`, `link`, and `base` to the excluded tags for the inline-in-layout case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E16jrPfFSya7LHfBXMd5yg
| false | ||
| end | ||
|
|
||
| # TODO: Rewrite using Prism Nodes once available |
There was a problem hiding this comment.
I added this TODO because related methods have this same TODO
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E16jrPfFSya7LHfBXMd5yg
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E16jrPfFSya7LHfBXMd5yg
f3f19cd to
5ce00a6
Compare
Flag helpers whose output belongs in <head> (meta/link/script includes from actionview, actioncable, importmap-rails, and turbo-rails) with head_content: true in their registry YAMLs, and expose them through HelperRegistry.head_content_helpers / head_content?(name) so consumers can query the list instead of hardcoding helper names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E16jrPfFSya7LHfBXMd5yg
Replace the hardcoded helper name list in head_content_helper? with a pattern built from HelperRegistry.head_content_helpers. This picks up helpers the list missed (action_cable_meta_tag, the turbo-rails meta tag helpers, turbo_include_tags, and the remaining importmap-rails helpers) and drops viewport_meta_tag / stylesheet_import_tag, which don't exist in any registered gem. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E16jrPfFSya7LHfBXMd5yg
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E16jrPfFSya7LHfBXMd5yg
| gem: "actioncable" | ||
| output: "html" | ||
| visibility: "public" | ||
| head_content: true |
There was a problem hiding this comment.
@kcdragon I think this is the best thing to do long-term!
Maybe we could reverse it to something like:
context: "head"So other helpers could define other contexts, like:
context: "body"
context: "html"
context: "table"There is also this other pull request which adds more schema-like metadata that could be also useful to have: #1030
There was a problem hiding this comment.
@marcoroth Cool. I'll change this PR to use context: "head". The other PR is interesting. Is the idea that if we knew which element these helpers outputted, we could automatically determine if it could only appear in <head> without needing something like context: "head"?
There was a problem hiding this comment.
Yeah, at least as long as the helper actually maps to an HTML tag.
Replace the head_content: true boolean with context: "head" in the helper YAMLs so other helpers can declare other contexts (e.g. "body", "html", "table") without adding a new flag per placement. The registry now exposes BY_CONTEXT and HelperRegistry.by_context(context) in place of HEAD_CONTENT and head_content_helpers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E16jrPfFSya7LHfBXMd5yg
| source: "Engine::DebugModeTest#test_0074_standalone meta link and base content erb expressions do NOT get debug spans" | ||
| input: "{source: \"<meta name=\\\"description\\\" content=\\\"<%= @description %>\\\">\\n<link rel=\\\"canonical\\\" href=\\\"<%= canonical_url %>\\\">\\n<base href=\\\"<%= base_href %>\\\">\\n\", options: {debug: true}}" | ||
| --- | ||
| _buf = ::String.new; _buf << '<meta name="description" content="'.freeze; _buf << ::Herb::Engine.attr((@description)); _buf << '" data-herb-debug-outline-type="view" data-herb-debug-file-name="unknown" data-herb-debug-file-relative-path="unknown" data-herb-debug-file-full-path="unknown" data-herb-debug-attach-to-parent="true"> |
There was a problem hiding this comment.
@marcoroth does this snapshot seem right to you? I'm surprised it's got the herb data attributes. Though the attributes appear to be on meta instead of a span that's wrapping it.
There was a problem hiding this comment.
@kcdragon I think the problem is its not directly enclosed in a <head> context, so it doesn't apply the skip.
Problem
While using Herb in development, I'm seeing an error from Turbo saying that I'm loading Turbo from a
<script>inside<body>despite it being inside<head>in my source code.app/views/shared/_head.html.erb
Solution
A partial rendered into a layout's has no node in its own AST, so the element-stack exclusion in
in_excluded_context?never fires and helpers likecsrf_meta_tagsorjavascript_include_taggot wrapped in a<span style="display: contents;">. Since is invalid head content, browsers implicitly close there and push the remaining scripts/stylesheets into , breaking Turbo apps.Add a name-based
head_content_helper?skip (alongside the existingcomplex_rails_helper?guard) covering the standard head helpers andtag.meta/tag.link/tag.title/tag.base, and addmeta,link, andbaseto the excluded tags for the inline-in-layout case.Is there a better fix than this?
It seems like it would be impossible to catch all the potential helper methods that belong in
<head>which is the direction this PR is going in. Maybe its good enough for now? It seemed like a fine approach since its similar to the existing check done bycomplex_rails_helper?(code). I'm curious if you know of a better approach.Another option is a herb linter that disscourages
<head>partials but I think those a fairly common especially in apps with multiple layouts.