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

HTML Syntax Highlighting: CSS Issues With Line-Numbers #4128

Closed
tajmone opened this issue Dec 7, 2017 · 24 comments
Closed

HTML Syntax Highlighting: CSS Issues With Line-Numbers #4128

tajmone opened this issue Dec 7, 2017 · 24 comments

Comments

@tajmone
Copy link
Contributor

tajmone commented Dec 7, 2017

pandoc 2.0.3

There are a number of CSS issue with the current way pandoc syntax highlights code with line number in HTML (and similar CSS based formats possibly).

I'll explain here the various issue, and provide examples of how I've worked around them in my custom CSS/SCSS stylesheets.

Line Numbers Background Color

The main issue — at least IMO — has to do with using line numbers when a background color is set for the code block. Line numbers are generated via div.sourceLine::before with a negative absolute position, and adding a positive left margin to pre.numberSource.

Here is a rough resume of the mentioned CSS in pandoc's default generated CSS:

pre.numberSource div.sourceLine::before,
.numberSource a.sourceLine::before {
    position: absolute;
    left: -5em;
/* [... other CSS ...] */
}

pre.numberSource {
    margin-left: 3em;
    border-left: 1px solid #bdae9d;
/* [... other CSS ...] */
}

This positions the line numbers outside the background color of the source code; and this seems the reason why setting the line numbers background color in custom JSON styles has no effect on their BG color.

In a project of mine, that uses only custom CSS to style pandoc higlighted code, I've worked around this by removing the margin (and border) from pre.numberSource, and add it instead to pre.numberSource div.sourceLine, so that only the div with the source line is shifted right, leaving the background color where the line numbers will be produced by div.sourceLine::before. I also changed the CSS for the line numbers border.

Here are my changes that allow to preserve background color behind line numbers (it's taken from a Sass file, but the point is clear):

pre.numberSource {
// *** REMOVED: ***
    // margin-left: 3em;
    // border-left: 1px solid red;
// [... more SCSS, omitted ...]
}
// *** ADDED: *** Needed to preserve Line Numbers background-color
pre.numberSource div.sourceLine {
    margin-left: 4em;  // Space for Line Numbers with 4 digits maximum!
    padding-left: 4px; // Add some space to distance from line numbers
}
// *** LINE-NUMS BORDER: *** The border separating linenums from code
pre.numberSource div.sourceLine::before {
    border-right: 1px solid $LineNumsBorderColor;
}

Empty Lines Height Issue

Also, I've experienced vertical gaps in the line numbers' border when one or more empty lines of code are present. This is due to the lack of actual contents in div.sourceLine, which affects the height of the line.

My workaround consists in using ::after to always add a thin space, so that even empty line will have some content:

pre.numberSource div.sourceLine::after {
    content: "\2009"; // Thin space:  
}

NOTE: The presence of the mentioned gaps will depend largely on the basic CSS of the document. In my case, it showed up in documents that use normalize.css, so chances are that the gaps will show up in most cases. Furthermore, this thin space solution shouldn't cause problems in any context.

Inline Code Issue

The following pandoc default CSS ends up forcing pandoc styles on inline code too:

pre, code {
    color: #bdae9d;
    background-color: #2a211c;
}

IMO, pandoc styles should not affect code tags that weren't highlighted/generated by pandoc. This is safer and less intrusive:

// Changed: added the .sourceCode class to avoid interfering with base styles!
pre.sourceCode,
code.sourceCode {
    color: #bdae9d;
    background-color: #2a211c;
}

... but I haven't yet tested it agains using pandoc to highlight inline code (I suppose the class names are the same).

@jgm
Copy link
Owner

jgm commented Dec 7, 2017 via email

@tajmone
Copy link
Contributor Author

tajmone commented Dec 8, 2017

Ok, I've updated to v2.0.4.

I noticed some changes in the CSS, but the problems mentioned above don't seem affected by them, and they still persist.

I'll paste here some shell scripts to demonstrate the problem and possible solution.

Problem Example

cat << 'EOF' | pandoc -s --highlight-style=zenburn -o problem.html

# Problem Example

<style media="screen" type="text/css">
.sourceLine {
    /* VERTICAL BORDER TO SHOW GAPS */
    border-left: 2px solid red;
}
</style>

``` { .python .numberLines }
# empty line ahead:

print "Hello World!"
```
EOF
  • Linenumbers are still shifted left via negative position values, so they're outside of the background color of the actual code.
  • The added custom CSS provided in the example demonstrates that empty lines cause line-height differences (ie: gaps in the red border)

Solution Example

This variation on the above example fixes the first two problems mentioned:

cat << 'EOF' | pandoc -s --highlight-style=zenburn -o solution.html

# Solution Example

<style media="screen" type="text/css">
    /* VERTICAL BORDER TO SHOW GAPS */
        .sourceLine {
            border-left: 2px solid red;
        }
    /* ===  EMPTY LINES GAP FIX === */
        .sourceLine::after {
            content: "\2009"; /* Thin space */
        }
    /* ===  LINE-NUMBERS BG COLOR FIX === */
        pre.numberSource a.sourceLine::before {
            color: white; /* linenumbers color */
        }
        pre.numberSource a.sourceLine {
            margin-left: 4em;
            padding-left: 4px;
        }
</style>

``` { .python .numberLines }
# empty line ahead:

print "Hello World!"
```
EOF

@tajmone
Copy link
Contributor Author

tajmone commented Dec 8, 2017

I think it's also worth considering pandoc default CSS for overflow:

.sourceCode { overflow: visible; }

IMO, this is not the best setting since it allows code to overflow and spill outside its container. Source code lines are usually quite long, due to monospaced fonts; often they'll be well over 80 characters.

This poses a problem, especially on handheld devices.

Overflow Problem

This example demonstrated the overflow issue by adding some CSS to restrict body width to 400px:

cat << 'EOF' | pandoc -s --highlight-style=zenburn -o overflow-problem.html

# Overflow Problem

<style media="screen" type="text/css">
    /* BODY WITH TO DEMONSTRATE CODE OVERFLOW */
        body { width: 400px; }
</style>
``` python
# I'm a very long comment and will cause an overflow in my div container, causing this line to spill over the div... 

print "Hello World!"
```
EOF

Overflow Solution

And this example demonstrates how a default setting of .sourceCode { overflow: auto; } might be a better choice:

cat << 'EOF' | pandoc -s --highlight-style=zenburn -o overflow-solution.html

# Overflow Solution

<style media="screen" type="text/css">
    /* BODY WITH TO DEMONSTRATE CODE OVERFLOW */
        body { width: 400px; }
    /* === CODE OVERFLOW FIX === */
        .sourceCode { overflow: auto; }

</style>
``` python
# I'm a very long comment but I will not cause an overflow in my div container, because of the CSS fix... 

print "Hello World!"
```
EOF

The problem here is that pandoc built-in styles impose overflow: visible on the .sourceCode class; this is a very specific definition that will take precedence over most general stylesheets — which usually style the <pre> and <code> tags, which are less specific than pandoc's CSS targetting the classname.

What are the reasons behind the visible setting choice?

@jgm
Copy link
Owner

jgm commented Dec 8, 2017

These are great suggestions. I'd like to get some feedback from @dbaynard who designed the current HTML/CSS formatting (jgm/skylighting#14). Note that this is provided by the skylighting library, not pandoc itself, though we can keep this issue here for now.

@dbaynard
Copy link

dbaynard commented Dec 8, 2017

The general principle behind the changes was to provide sensible defaults and behaviour which can be overwritten where necessary by users. The existing implementation made some styling impossible; there were some bugs with the table implementation of line numbers.

Hopefully these points will highlight where the decisions were deliberate and where I made oversights.

Line numbers background colour

There was originally a div wrapping the whole block, that was removed due (I believe) to a spacing issue. That would be the better solution, as if may fix multiple issues. I see you have to un-style the line numbers, too, by default, which makes overriding css trickier, though that's a small issue.

Empty lines height

The current approach (setting min-height) is a bit hacky, but the extra gap is just weird. I'm wary of using both before and after, though. I've found a way to fix the line height by setting .sourceLine to position: absolute; this means users are free to use after, but there are issues with background-color (which an enclosing div would solve). Do you have any suggestions?

Overflow problem

I'm not sure why that line is there, actually - I think I may have been trying to override something - but in any case the goal was to preserve the existing formatting. Given this is now handled sensibly by @print I agree that the scrolling version should probably be the default.

Inline code issue

This seems like a pandoc/skylighting integration issue (that preceded my changes) in that skylighting produces self-contained html+css. If all the code generated by skylighting is wrapped in a class, I agree the more specific code should be generated.


I think it would be helpful to have the wrapping div back; if regressions can be avoided it would more elegantly solve the second issue, and also solve the first.

Also, I mentioned using clay in skylighting a while ago, but it wasn't clear whether there'd be much benefit. I now suspect it would be worthwhile as it will improve the documentation of the skylighting css generation code.

@jgm
Copy link
Owner

jgm commented Dec 8, 2017

@dbaynard I have no objections to using clay in skylighting if that makes things easier.
As for the wrapping div, it depends on whether we can get a good solution to the line spacing issue.
See jgm/skylighting#14 October 26 comments for an explanation of how they arose.

@jgm
Copy link
Owner

jgm commented Dec 9, 2017

I see why we have overflow: visible. If you switch it to auto, line numbers are not visible, since they're outside of the pre!

@jgm
Copy link
Owner

jgm commented Dec 9, 2017

I'm getting better results with the div container this time -- not sure why. I need to do some more experiments, but this might be the way to go.

jgm added a commit to jgm/skylighting that referenced this issue Dec 9, 2017
jgm added a commit that referenced this issue Dec 9, 2017
This fixes a bug in 2.0.4, whereby pandoc could not
read the theme files generated with `--print-highlight-style`.

It also fixes some CSS issues involving line numbers.
Highlighted code blocks are now enclosed in a div with class
sourceCode.

Highlighting CSS no longer sets a generic color for pre
and code; we only set these for class `sourceCode`.

This will close #4133 and #4128.
@jgm
Copy link
Owner

jgm commented Dec 9, 2017

I made some tweaks (added enclosing div, added sourceCode class on pre and code css).
Undoubtedly this can be cleaned up, but it already seems better.

To try it out, clone the jgm/skylighting repository in a directory skylighting parallel to your
pandoc source directory, then check out the skylighting-0.5 branch in pandoc and build with make.

@dbaynard
Copy link

dbaynard commented Dec 9, 2017

It should be possible to fix the collapsing element issue, and I'll be able to take a look this morning. I'm sure a css expert would have been able to solve the problem instantly... but in any case given there seems to be agreement on the goals, when I get something working I'll PR skylighting and we can continue there.

@tajmone if you have any further suggestions or comments, I'd love to hear them. In particular, sample css that you expected to work would be helpful to see.

@tajmone
Copy link
Contributor Author

tajmone commented Dec 9, 2017

Where can I preview the new CSS code? (right now, I'm not equipped to compile Haskell on this machine, and the other PC hasn't been updated for ages)

Line numbers background colour

If I've understood correctly, the idea of an outer div to solve the linenumbers background issue would imply that the background color (and borders) will then be defined in this outer div?

Without seeing the actual CSS is a bit difficoult to imagine how this would affect changes. Generally, I'd say that an extra outer div is a bit superflows (we already have a wrapping <pre> and <code>), and adding a further enclosing div might complicate CSS inheritance — especially if pandoc is integrated into a more complex context, like a CMS, which might have its own conventions for CSS themes.

What has to be kept in mind, though, is that often the source code container will end up having rounded corners and a border. With pandoc v1, because of tables usage in linenumbers, implementingborder-radius on code blocks required quite a few tweaks in order to avoid having rounded corners between the linenumbers columns and the adjacent one with the actual code.

Currently, I'm using pandoc 2 in a custom CMS project, and the simple hack of shifting to the right the code with a positive margin (to make linenums fall within the colored backround) produced good results without too many CSS headhakes — I'm using a different theme for each highlighted language, and another external highlighter too (for syntaxes not covered by pandoc), so I have to juggle the Sass/CSS to handle code blocks for both highlighters. So far, details like borders and border radius are handled for both highlighters, and just inherited; skylighting-specific CSS enters the scene mostly when line numbers are envolved.

Empty lines height

I think that if line numbers are handled by a ::before with negative margin to left, then the actual line numbers should simply compensate with a positve margine to the right, so that both fall withing the boundaries of their natural container(s).

The current approach (setting min-height) is a bit hacky, but the extra gap is just weird. I'm wary of using both before and after, though. I've found a way to fix the line height by setting .sourceLine to position: absolute; this means users are free to use after, but there are issues with background-color (which an enclosing div would solve). Do you have any suggestions?

Why not using both before and after? Chances are that users won't be needing ::after in highlighted code, if your concern is about using up a selector that the end user might want to use otherwise? I think that if pandoc is going to resort to ::before it might as well use ::after too. Of course, a thin space will show up if for some reasons the user has set different background color for code lines — but usually this is only done for actual syntax elements, and the thin space should be outside any span tags.

What I like of the ::after soluion is that it adds the missing content to the empty line, which is hack that solves the problem by retouching the HTML content, instead of the CSS. From my experience, fiddling with positions, heights, margins and paddings trying to achieve pixel perfection always ends up bumping into browsers behavioural differences. The thinspace hack grants that no line will be truly empty, and therefore it will by styles like non-empty lines (whatever the browser differences are).

tajmone added a commit to tajmone/pb-archives-test that referenced this issue Dec 9, 2017
This is a premature commit of pandoc's default highlighter theme, for the purpose of sharing code example in a pandoc issue discussion --- jgm/pandoc#4128

Define skylighting default theme (Base16 Monokai, with extra colors). All SCSS readapted from pandoc v1 to v2.
@tajmone
Copy link
Contributor Author

tajmone commented Dec 9, 2017

Some Real-Case Examples

Since I was currently working on skylighting's default style for my CMS projects, I thought I could make an early commit of the Sass files and HTML output, so I could share a live example for this topic.

Please, bare in mind that it's still a WIP draft, nothing definitive — but at least you can still find commented out lines from the original pandoc CSS, and my comments to custom changes and additions.

Sass Skylighting Theme

This the Sass file that deals with the default highlighting theme for pandoc:

Live HTML Preview

And this is a Live-Preview link to the converted HTML doc used for testing syntax highlighting theme progress:

it contains various code blocks highlighted via pandoc (with and without line numbers), and some other code highlighted by Highlight, an external syntax highlighting tool.

NOTE: From the "Highlight Default" section onward, the code examples are not highlighted by pandoc but by André Simon's Highlight tool.

... I hope this might help to illustrate a real-case scenario of the needs and problems when integrating pandoc into a wider HTML project.

@dbaynard
Copy link

dbaynard commented Dec 9, 2017

Line numbers background colour

If I've understood correctly, the idea of an outer div to solve the linenumbers background issue would imply that the background color (and borders) will then be defined in this outer div?

There are multiple layers to displaying highlighted code: three notions of the code as

  • a set of lines,
  • as a block, and
  • with all its accessories (such as line numbers).

This is not reflected in the current output of skylighting, due to some quirks of css. I proposed the div as a way to add the full outer layer, but it may actually be unnecessary.

Without seeing the actual CSS is a bit difficoult to imagine how this would affect changes. Generally, I'd say that an extra outer div is a bit superflows (we already have a wrapping <pre> and <code>), and adding a further enclosing div might complicate CSS inheritance — especially if pandoc is integrated into a more complex context, like a CMS, which might have its own conventions for CSS themes.

I'm more concerned that users can build with these abstractions than users with their own css will have to change their code; everybody benefits from not having to hack around. Indeed, this motivated my initial PR wrapping lines. Do your CSS conventions match the model I set out below, then?

What has to be kept in mind, though, is that often the source code container will end up having rounded corners and a border. With pandoc v1, because of tables usage in linenumbers, implementingborder-radius on code blocks required quite a few tweaks in order to avoid having rounded corners between the linenumbers columns and the adjacent one with the actual code.

Ideally, the outermost layer fits everything: line numbers, code, maybe drafting comments. Backgrounds could be applied and corners rounded at will.

The next layer is a block containing all the code but no line numbers or comments, say. I don't consider line numbers to be a part of the code, so it should be possible to style the code without the line numbers, including rounding corners. This can be implemented either with the div or by setting code.sourceLines to display: block.

Finally, it should be possible to style the lines.

If it's possible to have the three different layers correspond to the pre, the code and the line divs then that is probably the way to go as a default, and if anybody wants to do anything more complicated with pandoc they can write a filter. In that case your changes to padding and margins probably make sense. If that isn't possible the layers would correspond to the outer div, the pre and the inner divs respectively, and I'd probably agree with your padding and margins changes.

Empty lines height

I think that if line numbers are handled by a ::before with negative margin to left, then the actual line numbers should simply compensate with a positve margine to the right, so that both fall withing the boundaries of their natural container(s).

I'm not quite sure what you mean by this, but I've set out above what I consider the natural containers.

Why not using both before and after? Chances are that users won't be needing ::after in highlighted code, if your concern is about using up a selector that the end user might want to use otherwise? I think that if pandoc is going to resort to ::before it might as well use ::after too. Of course, a thin space will show up if for some reasons the user has set different background color for code lines — but usually this is only done for actual syntax elements, and the thin space should be outside any span tags.

What I like of the ::after soluion is that it adds the missing content to the empty line, which is hack that solves the problem by retouching the HTML content, instead of the CSS. From my experience, fiddling with positions, heights, margins and paddings trying to achieve pixel perfection always ends up bumping into browsers behavioural differences. The thinspace hack grants that no line will be truly empty, and therefore it will by styles like non-empty lines (whatever the browser differences are).

I myself need :after, but in general users will not be able to add their own pseudo-elements; that is my concern. I don't follow the logic about the :before; it makes sense that line numbers should be considered pseudo-elements of the line, but if there's a better method than using after to add extra invisible elements I would recommend that. I think I have that solution, using :empty and setting the height (and possibly line-heights) explicitly.

Some Real-Case Examples

I see some of the improvements here, but there are regressions with @media print and I find the shifting margin from unnumbered to numbered code jarring. In any case, skylighting should provide a sensible default and enable both hanging and flat line numbers.

I'll put up an example once I've implemented some of these changes and post the link.

@tajmone
Copy link
Contributor Author

tajmone commented Dec 9, 2017

@dbaynard , I agree with the layered approach you outlined.

I find the shifting margin from unnumbered to numbered code jarring.

... there isn't really a nice way to go about it, providing margin to unnumbered code would even be uglier. The problem here is that we can't control the width of the linenumbers margin.

I think that pandoc should implment an option to specify the number of digits in line numbering, something like:

~~~ { .python .numberLines .numberLinesWidth-3-Digits }
~~~

where you can specify how many digits wide should be the line numbering margin. Currently, pandoc is offering a quite wide margin, capable of 4 digits (if I'm correct) so it will cover code from very long source files (eg: displaying lines 1200-1250 from a very long source file).

The dafult CSS could offer a predefined margin to cover two-digits line numbers, and then offer some default classes for wider digits (in the above example, .numberLinesWidth-3-Digits would be a default CSS with an extra em margin, and .numberLinesWidth-4-Digits could be a class with two extra em margin). Each extra digit should be equivalent to an extra em spacing.

Having a few default CSS classes should allow coexistence of code blocks with different line numbers widths. My guess is that the normal expectation is 2 digits wide (1 to 99 lines being good to cover code examples), bigger projects quoting longer files might need 3 to 5 digits — it all depends if you are quoting piceces of code from a larg file or not. I doubt that someonw will place more than 1000 lines of code in a markdown document, but quoting 3 lines from a long file might require 4-digits wide linenumbers.

Whatever the baseline, some way to control the width should be provided.

With Highlight tool there is a specific CLI option for this:

-J, --line-length=<num>        line length before wrapping (see -V, -W)
-j, --line-number-length=<num> line number width incl. left padding (default: 5)

If the CSS classes approach above is not good, then some other way could be offered to control that left margin.

@jgm
Copy link
Owner

jgm commented Dec 9, 2017 via email

@dbaynard
Copy link

dbaynard commented Dec 9, 2017

I'll make one, then. How about my lunchtime tomorrow (I'm GMT) - if I can't get the solution without using after I'll go with the proposal here?

@jgm
Copy link
Owner

jgm commented Dec 9, 2017 via email

jgm pushed a commit to jgm/skylighting that referenced this issue Dec 10, 2017
* Fix empty line height, explicitly
* Ensure long lines scroll on screen
* Only apply colour to the outer div
* Don't reset line number colour
* Fix borders on empty code lines

See jgm/pandoc#4128
@jgm
Copy link
Owner

jgm commented Dec 10, 2017

Compiling against latest skylighting with @dbaynard's changes, I see that the addition of the outer div seems to have reintroduced the collapsing/spacing problem.

This can be seen from this gist: https://gist.github.com/jgm/426fc08026f4fe8b36743f1e970172f2

Here's what I'm seeing in the browser:

screen shot 2017-12-10 at 11 59 33 am

Note the extra space that is introduced when we have the div around the pre; this was the reason I originally removed the surrounding div. I'm not sure why I didn't see the extra space when I added the div in 8b8f028e2df17819cab9f934ef8f3d74ea10a948, but the margin collapsing behavior may be sensitive to the CSS properties that are set on the elements.

This problem needs to be fixed, somehow, either by more CSS tweaks or by going back to a system where the pre is the outer element.

@dbaynard
Copy link

I'll take a look, shortly. If I can't solve it tonight, by all means roll back.

I've been migrating to clay, meanwhile, so I'll probably make my changes to that version.

@dbaynard
Copy link

I believe I've fixed that problem by ensuring the outer div now collapses correctly. Just running into a minor issue with the clay variant.

I'm going to push and PR with the changes I've made without introducing clay; just need to cherry pick.

I'll also PR with clay; that's not ready to go, yet, and I'll explain the decision that awaits in that PR.

jgm added a commit that referenced this issue Dec 11, 2017
This fixes a bug in 2.0.4, whereby pandoc could not
read the theme files generated with `--print-highlight-style`.

It also fixes some CSS issues involving line numbers.
Highlighted code blocks are now enclosed in a div with class
sourceCode.

Highlighting CSS no longer sets a generic color for pre
and code; we only set these for class `sourceCode`.

This will close #4133 and #4128.
@tajmone
Copy link
Contributor Author

tajmone commented Dec 11, 2017

You often mention "clay", is it the CSS preprocessor?

Will this add benefits to end users or is going to be just for internal use by pandoc/skylighting?

@dbaynard
Copy link

Yes; mainly for easier maintenance. But it also ensures correct cross-browser variants are selected.

@jgm
Copy link
Owner

jgm commented Dec 13, 2017

Closed by 2.0.5 cchanges.

@jgm
Copy link
Owner

jgm commented Jan 12, 2018

@dbaynard - any thoughts about jgm/skylighting#32?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants