-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[WebProfilerBundle][WIP] Real-time twig debug, aka xray view #28056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I warmly appreciate your work to prepare this proposal. However, before getting into technical details, could we please discuss about this proposal in general? For example, my biggest concern is that I've never needed this feature myself and I can't see me using this in any of my Symfony apps. In other words: it solves a problem I've never had. But this is just my own experience ... so I'd like to read other people comments. Thanks |
Yes please :) that's why opened the PR as early as possible. I know you prefer RFCs/issue first, but i wanted to try this and show a real POC. Also see twigphp/Twig-extensions#194 where ppl are upvoting this idea :) Personally i'd be very happy with "Visual element > Click > Source file" as a workflow. |
I like that. Not sure about "invisible" yet: this asks authors write directives for a debugging tool. |
Correct, and it's not ideal. Then again if such a modifier lives in twig core, it would be a no-op in case debugging is disabled / not available. That allows to "fix" e.g. upstream templates independently. To me that's important to make it viable long-term.
The thing is.. we need the comment boundaries server-side, to actually get us the offsets client-side. But i like the way you're thinking :) Maybe we can inject a hidden unique UTF-8 char sequence to use as a boundary. Then it should be possible to dump a mapping of boundary indexes to source data. Conceptually injecting a hidden UTF-8 can be just as wrong as injecting a HTML comment, but would be less likely to break i guess 🤔 |
Could it work with random unique string used as a marker, that we would remove before returning the output and that would help build the source map? |
Not really into twig internals, that's why i asked @yceruto :)) but i can imagine it would work like a shadow root. So one big comment at the bottom of the page containing the actual rendered templates which we can match with what's in the actual DOM. That gives us the regions client side, but increases the amount of rendering and im not sure how reliable the match would be. |
I think this is very useful, I find myself very often going to the profiler twig section to see which twig files are involved in some pages, specially if it's not my code (for example, I have been trying to adapt eZPlatform to my project, and it's a nightmare to guess which twig files is using) But, as it is, I couldn't use it in that case, because it force me to change the external twigs from the third party to enable this feature (if I'm not wrong) |
First, let me say I like this a lot, having this info available would be invaluable to UI people who are doing HTML/CSS + Twig, but not PHP. I'd include a screenshot from the fiddle in the PR description to explain what this is about at a glance, wasn't clear to me. The color picker is not needed IMO, you can easily find a highly contrasting color with JS and use that, it can even be different as you move across the page. |
@dkarlovi if we can automate the color approach that be great i guess. Like to see a fiddle for that :) as im not sure how it would work technically (im drawing a boundary box positioned above all elements between start/end comments). Either way, dealing with the color/contrast is a must.
@magarzon you're right :) but only if those templates are breaking. Then they need a upstream fix. Could be done with the suggested Ideally it works out-of-the-box, and we're trying to find a way how that could be done :) |
@ro0NL there's Update: simple math. |
@dkarlovi but that's the issue, we don't hover an individual element in source. We hover a computed region element, which visually spans multiple elements in source. Currently the background color (default red) is controllable, and the text color (region tooltip) is inverted using CSS. Also not sure which computed color we'd use (border, background, text?) |
I've needed this feature before, especially with includes/render-controller places or multiple includes it would make it a lot easier to find out which part of the template belongs to what. |
This makes me think and I want to believe that's possible as long as we keep the advantage of inspecting dynamic content (i.e. one template that is loaded through AJAX) |
As @fabpot already rejected adding such feature in Twig core, I doubt he will agree adding it in Symfony core. Could it be implemented as a thrid-party bundle instead ? |
Sure, but conceptually it needs to solve the same problem. I think from a bundle pov it would come with it's own tag to let the user define debug-able regions. As such it wouldnt be compatible with any vendor template out there today. I was striving for the opposite, at least a noble goal on itself :))
The ref i found dates back to 2016 and is still open. I agree if we can't nail it, we shouldnt do it. And that's a big if at the moment. The feature on itself is priceless. On the technical side; another idea was to parse the result HTML; check for all semantically valid debug comments; compute the invalid ones and remove those. |
@ro0NL the existing PR on Twig-Extensions relies on a configurable blacklist of templates instead of forcing templates to add a tag when they don't want to be wrapped (which would also create an extra dependency as long ad the tag is not part of Twig itself). Here, you added a hardcoded blacklist, and then you talk about adding a tag (which might not even work to solve this need) |
<input type="color" id="__twig-debug-color" value="#ff0000"> | ||
</div> | ||
<div class="sf-toolbar-info-piece"> | ||
<span><a href="javascript:;" id="__twig-debug-enable">Enable debug</a></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
href="javascript:;"
breaks the CSP compatibility. Use button tags instead (which are semantically correct) or href="#"
with event.preventDefault()
in the listener (more complex due to browser compat as this is part of the toolbar, not of the profiler UI for which we can afford supporting only modern browsers).
</div> | ||
<div class="sf-toolbar-info-piece"> | ||
<span><a href="javascript:;" id="__twig-debug-enable">Enable debug</a></span> | ||
<span><a href="javascript:;" id="__twig-debug-discover" style="display: none;">Discover</a></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline styles should be avoided for CSP compat. Use a class with styles instead.
/** | ||
* @author Yonel Ceruto <yonelceruto@gmail.com> | ||
*/ | ||
class HtmlDebugEnterComment extends Node implements \Twig_NodeOutputInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symfony uses the namespaced version of all Twig classes and interfaces, to make sure we are ready for the future version of Twig dropping support for the non-namespaced version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, should Twig\Node\NodeOutputInterface
be used instead.
bar.textContent = label; | ||
} | ||
bar.style.top = parseInt(rect.top + (document.documentElement.scrollTop || document.body.scrollTop) + rect.height) + 'px'; | ||
bar.style.left = parseInt(rect.left + (document.documentElement.scrollLeft || document.body.scrollLeft)) + 'px'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseInt
is wrong here, as you don't give it a string as input, but a number.
return !!document.getElementById('__twig-debug-bar'); | ||
}; | ||
|
||
var twigDebugEnable = function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the logic in this method is not compatible with old browsers, which is not good for something running in in the toolbar (as the toolbar runs in the userland page, which might need to support them).
so there is 2 solutions here:
- ensure the code is compatible with old IE versions
- feature-detect the support for features being used, and forbid enabling the Twig debug mode if not supported.
@stof the issue is e.g. https://github.com/symfony/recipes/blob/master/symfony/twig-bundle/3.3/templates/base.html.twig we dont want to exclude the entire template, assuming it has more valueable blocks/regions in real life projects. However, within this template we do need to exclude
That would be solved with a tag / at the syntax-level. But i prefer to autodetect this as suggested previously by parsing the actual HTML and remove invalid debug comments. |
If the issue is to understand which templates are used in any given place, an alternative approach would be to render separate ("source") view where we can highlight ANY part of ANY format and tell user where it came from. This would render actual source (text) and thus doesn't care about format of the content (could do this for JSON too). Downside is that it's not "inline" and doesn't work like "Inspect Element" in browsers. |
I propose to close this PR as it is stale. I'm still not convinced that it should be in core. Would be happy to see a bundle doing that though. |
This PR aims to enable real-time twig debugging, aka xray view, aka layout inspection, etc.
Inspired by https://github.com/beyondcode/laravel-view-xray me and @yceruto tried to port this into Symfony/Twig.
See https://github.com/yceruto/twigxray for the initial work done. This PR is mostly a direct port to move the discussion to Symfony core.
What this PR is about
Live demo
https://jsfiddle.net/r30g4boL/5/
However, there's a big technical limitation. We cant really tell if the output is truly intended HTML, and as such the hidden HTML comments we're adding could break things. And it does :) hence WIP.
This must be handled server-side, and to make it really robust i want to propose the following steps to finalize this;
Twig bundle configuration
The
included
filter reduces most of the unwanted templates, and can be further downsized using theexcluded
filter. E.g. webprofiler would prepend it's own templates here, to effectively disable HTML debugging globally.Twig syntax
At least the following block will break forms:
symfony/src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_horizontal_layout.html.twig
Lines 58 to 60 in 0eea077
But also something like
<title>{% block title %}Welcome!{% endblock %}</title>
leads to unwanted HTML comments.To avoid this per case we need some modifier at the syntax level, so the developer can clarify the intend, think:
Any block etc. within this boundary is free from HTML debug comments wrapped arround it.
Thoughts?