Skip to content
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

admonitions (note and warn) are not correctly rendered on github #68

Closed
stefanfoulis opened this issue Jul 16, 2011 · 28 comments
Closed

Comments

@stefanfoulis
Copy link

With rst it is easy to make admonitions for notes and warnings:

.. warning:: don't do this

Using github-markup to render this sort of thing correctly creates this output:

<div class="warning">
    <p class="first admonition-title">Warning</p>
    <p class="last">don't do this</p>
</div>

On github it seems this is just rendered as two normal <p> tags (In gists, the source browser and README.rst).

<p>Warning</p>
<p>don't do this</p>

And that turns out to look like this:


github current admonition rendering


This makes admonitions useless. Is github doing some extra post-processing of the output? If so, can it be modified to allow this construct to get through.

Just copying the correct code into the browser on a github page in firebug revealed, that stylesheets for this already exist (although they are not that pretty):


github current admonition rendering if the markup were correct


Something similar to the styling on readthedocs.org would be cool:


readthedocs admonition rendering


@chickenkiller
Copy link

The same kind of issues appear for some asciidoc specific renderings which use classes. The root cause is that a HTML sanitization step is done, and among other things, removes class names.

Maybe we should find a way to detect some common classe names per format and give them to Markup so that they are considered harmless, and could also be unified (e.g. Warning, Note are common cases that happen to exist in several formats) so that a CSS style defined by Markup (or higher level layers such as Gollum?) can be applied to them.

@rvanlaar
Copy link

.. note :: is still not supported. I'ld this issue to be reopend. See also #72

@beniwohli
Copy link

Common @github, this shouldn't be to hard and would be a huge improvement for rendering README's and the like.

@mgaitan
Copy link

mgaitan commented May 10, 2013

any progress on this?

@mojavelinux
Copy link
Contributor

AsciiDoc has the same problem. It would be helpful if:

  1. there are a set of styles classes in the GitHub stylesheet to use (perhaps there are, but they need to be pointed out)
  2. the style classes from the rendered HTML aren't stripped out so that the admonition blocks actually work

@mojavelinux
Copy link
Contributor

Actually, I found these styles in the second stylesheet loaded:

#readme.rst div.admonition,#readme.rst div.attention,#readme.rst div.caution,#readme.rst div.danger,#readme.rst div.error,#readme.rst div.hint,#readme.rst div.important,#readme.rst div.note,#readme.rst div.tip,#readme.rst div.warning{
margin:2em;border:medium outset;padding:1em
}
#readme.rst div.admonition p.admonition-title,#readme.rst div.hint p.admonition-title,#readme.rst div.important p.admonition-title,#readme.rst div.note p.admonition-title,#readme.rst div.tip p.admonition-title{
font-weight:bold;font-family:sans-serif
}
#readme.rst div.attention p.admonition-title,#readme.rst div.caution p.admonition-title,#readme.rst div.danger p.admonition-title,#readme.rst div.error p.admonition-title,#readme.rst div.warning p.admonition-title{
color:red;font-weight:bold;font-family:sans-serif
}

I'm glad they are there, but why make them specific to rst? Why not a standard way of doing admonitions so that any language that has them can use them?

@mojavelinux
Copy link
Contributor

Ah, I see now that @stefanfoulis already figured out that these styles are there using firebug hacking. As it turns out, they are ugly as sin.

@dbrgn
Copy link

dbrgn commented Oct 8, 2013

Please fix this. Warnings are commonly used in README files...

@matthijskooijman
Copy link

Hm, just bumped into this issue as well. Without proper support for admonitions, I can't properly write my protocol documentation and I'll have to look for some other way to render them :-S

As for the suggested fix: a whitelist of classnames could be made, but why are CSS class stripped in the first place? To prevent messing up the github layout?

@matthijskooijman
Copy link

@github @gjtorikian Would it help if I tried to distill a list of classes that docutils can possibly use in the generated HTML? If I can do anything else to get his issue fixed sooner rather than later, let me know.

@gjtorikian
Copy link
Contributor

They're intentionally being stripped out. It happens in comments here, too:

Warning

don't do this

We have a pretty strict whitelist of what can come through, as defined here: https://github.com/jch/html-pipeline/blob/master/lib/html/pipeline/sanitization_filter.rb

Part of this is done for security reasons. Another part is dealing with supporting class names for multiple markups.

I'm on the fence about closing this as a "won't fix." It's interesting that there are styles in place (probably from years ago). There are actually several RST styles there (for tables and images). And, in general, I'm kind of in favor of showing some light admonition rendering. I'll have to dig around for the rationale and reasoning for stripping out class names from markup. It's not super high on my "list of shit to do" but I can at least guarantee I'll look into it. Hope this helps.

@matthijskooijman
Copy link

I was going to argue that writing raw HTML in a comment is different from the rendered HTML, but in the end both of them are user controlled. The actual HTML parts generated by the markup processor could possibly be safe, but since most markup languages have features to include raw HTML as well, there is no way to tell the difference, and sanitizing the generated HTML is the only sensible route.

Stripping class names as part of the sanitizing suprises me a bit, but I can imagine that there sane arguments for that as well.

I wonder if docutils and/or other markup processors would have some configuration options to make them emit a single standardized set of classes, instead of each their own. No time to look into that right now, though.

Thanks for looking into this at any rate, it helps :-)

@mojavelinux
Copy link
Contributor

I was going to argue that writing raw HTML in a comment is different from the rendered HTML, but in the end both of them are user controlled. The actual HTML parts generated by the markup processor could possibly be safe, but since most markup languages have features to include raw HTML as well, there is no way to tell the difference, and sanitizing the generated HTML is the only sensible route.

AsciiDoc is aware of when it is outputting built-in HTML vs user-generated "pass through" HTML. I would expect that the built-in HTML would be trusted, at least enough to keep the CSS classes around. By stripping the CSS classes, it becomes impossible to distinguish between the different block types, admonitions in particular.

What would be nice is if the GitHub stylesheet would offer a list of high-level CSS classes for basic styling of established structures and then require the markup processors to produce HTML that conforms to those HTML structures. I can certainly make that happen in AsciiDoc using the custom Asciidoctor templates.

The problem so far has been that we haven't received any indication from GitHub that customizing the output of the processor would be an investment that would lead to any action. GitHub, if you give us requirements, then we can work to meet them. But without any information, we're all just standing around hoping for something to happen.

@mitar
Copy link

mitar commented Jan 10, 2014

+1

@andreas-h
Copy link

+1

1 similar comment
@gene1wood
Copy link

+1

@brifordwylie
Copy link

+1 supporting a least some subset of admonitions would be super.

@qrilka
Copy link

qrilka commented Jul 17, 2014

+1

2 similar comments
@itsjohncs
Copy link

+1

@tautrimas
Copy link

+1

@kaycebasques
Copy link

+1

5 similar comments
@lukemarsden
Copy link

+1

@sunbit
Copy link

sunbit commented Oct 23, 2014

+1

@geoffroy-aubry
Copy link

+1

@yvaucher
Copy link

+1

@ErwinRieger
Copy link

+1

@Orcomp
Copy link

Orcomp commented Dec 10, 2014

+1

Can we overcome this by creating fictional languages? (i.e. Note and Warning)

'''Warning
Put some warning in here
'''

'''Note
Put some notes here
'''

Purposely did not use back quotes to show the idea.

@bkeepers
Copy link
Contributor

Thanks everyone for letting us know this is important to you. I would love this feature as well. However this cannot be implemented in this library, so I'm going to close this issue. If you'd like to voice support for this feature, [email protected] keeps a tally of feature requests.

@github github locked and limited conversation to collaborators Dec 18, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests