8000 [WebProfilerBundle][WIP] Real-time twig debug, aka xray view by ro0NL · Pull Request #28056 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Closed
wants to merge 2 commits into from
Closed

[WebProfilerBundle][WIP] Real-time twig debug, aka xray view #28056

wants to merge 2 commits into from

Conversation

ro0NL
Copy link
Contributor
@ro0NL ro0NL commented Jul 24, 2018
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

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

image

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

twig:
    html_debug:
        enabled: false # for BC
        included: ['*.html.twig']
        excluded: []

The included filter reduces most of the unwanted templates, and can be further downsized using the excluded 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:

{% block form_group_class -%}
col-sm-10
{%- endblock form_group_class %}

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:

{% invisible %} ... {% endinvisble %}

Any block etc. within this boundary is free from HTML debug comments wrapped arround it.

Thoughts?

@javiereguiluz
Copy link
Member

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

@ro0NL
Copy link
8000
Contributor Author
ro0NL commented Jul 25, 2018

could we please discuss about this proposal in general?

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.

@nicolas-grekas
Copy link
Member

I like that. Not sure about "invisible" yet: this asks authors write directives for a debugging tool.
About the implementation, would it be possible to get rid of the comments and replace them with a source map? I.e. keep track of the offsets in the output and map them to source in a separate artifact?

@ro0NL
Copy link
Contributor Author
ro0NL commented Jul 25, 2018

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.

About the implementation

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 🤔

@nicolas-grekas
Copy link
Member

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?

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 25, 2018
@ro0NL
Copy link
Contributor Author
ro0NL commented Jul 25, 2018

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.

@magarzon
Copy link

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)

@dkarlovi
Copy link
Contributor

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.

@ro0NL
Copy link
Contributor Author
ro0NL commented Jul 25, 2018

@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.

because it force me to change the external twigs from the third party to enable this feature (if I'm not wrong)

@magarzon you're right :) but only if those templates are breaking. Then they need a upstream fix. Could be done with the suggested {% invisible %} syntax, or until then exclude the template in your app, override it, or fully disable debugging.

Ideally it works out-of-the-box, and we're trying to find a way how that could be done :)

@dkarlovi
Copy link
Contributor
dkarlovi commented Jul 25, 2018

@ro0NL there's window.getComputedStyle() which would allow to fetch information about the element you're hovering and figure out the contrasting color by simple math. I don't have time for a fiddle right now, but in theory it sounds straightforward.

Update: simple math.

@ro0NL
Copy link
Contributor Author
ro0NL commented Jul 25, 2018

about the element you're hovering

@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?)

@linaori
Copy link
Contributor
linaori commented Jul 25, 2018

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.

@yceruto
Copy link
Member
yceruto commented Jul 25, 2018

... would it be possible to get rid of the comments and replace them with a source map? I.e. keep track of the offsets in the output and map them to source in a separate artifact?

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?

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)

@stof
Copy link
Member
stof commented Jul 25, 2018

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 ?

@ro0NL
Copy link
Contributor Author
ro0NL commented Jul 25, 2018

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 :))

As @fabpot already rejected adding such feature

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.

@stof
Copy link
Member
stof commented Jul 27, 2018

@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>&nbsp;
Copy link
Member

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>&nbsp;
<span><a href="javascript:;" id="__twig-debug-discover" style="display: none;">Discover</a></span>
Copy link
Member

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
Copy link
Member

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.

Copy link
Member

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';
Copy link
Member

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 () {
Copy link
Member

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.

@ro0NL
Copy link
Contributor Author
ro0NL commented Jul 27, 2018

@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

<title>{% block title %}Welcome!{% endblock %}</title>

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.

@artursvonda
Copy link
Contributor

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.

@fabpot
Copy link
Member
fabpot commented Feb 3, 2020

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.

@fabpot fabpot closed this Feb 3, 2020
@ro0NL ro0NL deleted the twig-debug branch February 3, 2020 11:16
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0