While working on the related issues, I noticed that there are supposed to be <pre> tags around all the commit messages but they are not, however, actually showing up anywhere.
I'm 99% sure this is because it is posting comments using the Filtered HTML filter. In order for the system messages to be styled according to #936304: [META] Style issue comments, the System Message user must be allowed at the very least the Full HTML permission.
My concern is that I'm not entirely sure if giving this permission is enough, let alone be set as a "default" for this user account. We may have to go in with code and manually force it (hence creating this issue here).
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | Screen Shot 2014-05-30 at 11.41.39.png | 171.9 KB | wim leers |
Comments
Comment #1
sunWouldn't one of the following two options be simpler/better?
A. Change the back-link markup to use CODE instead of PRE.
B. Add PRE to the allowed tags of the Filtered HTML format.
Comment #2
markhalliwellYes. I thought about both of those approaches. Either one of those would work for just the message because it's a unique element that we can target. However, it still wouldn't allow us to wrap the "labels" (refs: 7.x-1.x, etc) in
<span>with a class to target it for styling.So unless we want to allow
<div>and<span>inFiltered HTML, I think it would be better if we simply added/forced theFull HTMLfilter for this specific "user". We set aside this user account for d.o messages, I think we can trust it ;)Comment #3
silverwing commentedUnless there's a really good reason to give everyonethe
divI'd rather give System Message the Full HTML role.I've also noticed that if any field in an issue is set to Full HTML (including comments, which is a node revision) non-Full HTML users can edit the node/comment on it. Although used rarely, it can be problematic.
Comment #4
drummIf possible, I'd like to keep system message comments as filtered HTML. It is an extra layer of security. If someone finds a way to get unwanted markup in, Drupal core's filters will stop it.
Classes on
<em>and<strong>are allowed, can that be used instead of<span>?Are there any drawbacks to allowing
<pre>?Comment #5
drummhttp://html5sec.org/ says
<pre>is safe, so we can allow it.Comment #6
markhalliwellYeah, we can use
<strong>for the labels (they're supposed to be bold anyway). We'll definitely need to add a class to it to target though so the font can be manipulated (and not just any strong tag).I think adding
<pre>should also be fine.Comment #7
drummDone. The markup for the labels can be fixed with a separate module in versioncontrol_project.
Comment #8
markhalliwellYes, I'm waiting for #2275317: Cleanup commit message theme hook to get in.
Then we can easily override the simpler theme hook in bluecheese since it's specific to that theme's styles.
Comment #9
wim leersThis caused a regression: HTML entities are now rendered escaped.
Comment #10
markhalliwell@Wim Leers, that isn't caused by this issue at all. Those pages are rendered using views (and have always had
<pre>tags). There may already be an issue (haven't checked), if not one should be created in https://drupal.org/project/issues/versioncontrol_project.Comment #11
wim leersHm okay. I'd swear this wasn't there last time I looked.
Sorry, and thanks! :)
Comment #12
drummYep, I've seen that before this change. The View is exported to http://cgit.drupalcode.org/versioncontrol_project/tree/versioncontrol_pr..., and probably affects vc_git_project_global_commits too.