Wrap source lines and re-implement line numbers #14

dbaynard · 2017-08-27T11:55:32Z

Fixes #13. Would be good to get into pandoc 2.0. From the changelog:

Wrap lines of source code in a div with display set to inline-block.
The divs make per-line processing easier. They cannot be set as
display: block as that introduces extra new lines when copying and pasting.
Render line numbers in html using css pseudo elements rather than a table.
The line numbers are always produced, as data-line-number attributes, and
css to display them as ::before elements are always produced. The option to
switch on line numbering only toggles a class; this means it is possible to toggle
line numbering without re-running skylighting.
If the linkAnchors option is set, wrap with an a element rather than a div,
set so that clicking the line number (and only the line number) will jump to that
line.
Code wraps by default when html is printed, with wrapped lines indented.

The new output with no line numbers

and with line numbers

The code matches all features of the previous, table base code, with one exception: previously, copying code from a printed pdf didn't pick up the line markers, but now it does. However, I have judged the changes are worth it, as copying from a pdf also gets line breaks wrong so is inherently imperfect.

- Note, line links and custom start number no longer work

jgm · 2017-08-27T15:23:35Z

Can you comment on browser support? Can we rely on these features being available in browsers?

jgm · 2017-08-27T15:25:41Z

The basic approach looks pretty clean; I guess my worries are (a) breaking existing templates that hard-code CSS (maybe acceptable, since it's better not to hard-code highlighting CSS, and going forward we have a way to define custom styles), and (b) browser support.

jgm · 2017-08-27T15:30:54Z

I notice that with line numbers, there is also much more space between lines. Can this be fixed so that toggling line numbers doesn't change the line spacing?

dbaynard · 2017-08-27T15:43:20Z

I'll check the browser support shortly, and have a think about existing css. As for the line spacing, it has to be set to avoid the numbers collapsing. I can set them both to the same, though do note it's set using min-height so long lines will break correctly. What do you think?

Actually, we could add an option to wrap the lines or not so both behaviours are available, when not using line numbers. How does that sound?

dbaynard · 2017-08-28T00:52:05Z

According to caniuse everything is supported with good coverage. I suspect screen readers don't read the numbers (I don't know how to test this) but the previous way of adding line numbers using a table likely suffered a similar problem.

I'd also like to use clay for the (limited) css generation here as it's currently quite ad-hoc (and easy to introduce bugs) — I'll raise a separate issue. That would ensure the correct browser prefixes are supplied.

Adding an option with both behaviours means touching the definition of FormatOptions. I can do it, but it might not be worthwhile — your call.

jgm · 2017-08-28T01:01:59Z

+++ dbaynard [Aug 28 17 00:52 ]:

Adding an option with both behaviours means touching the definition of FormatOptions. I can do it, but it might not be worthwhile — your call.

What do you mean, both behaviors? The old one-row table + your new way? I'd rather just drop support for the old method, if we make this change. But I do plan to bump the version to 0.4 this, since this is a change that might bite people if they don't update their code, templates, etc. Thus, a change to FormatOptions wouldn't be out of the question.

jgm · 2017-08-28T01:05:50Z

+++ dbaynard [Aug 27 17 08:43 ]:

collapsing. I can set them both to the same, though do note it's set using min-height so long lines will break correctly. What do you think?

I think using min-height in both cases is fine. But the current min-height with line numbers seems a little too high.

Actually, we could add an option to wrap the lines or not so booth behaviors are available, when not using line numbers. How does that sound?

Isn't wrapping controlled by a global CSS property? Sorry, I'm not following this, maybe you could explain in a bit more detail what you have in mind.

jgm · 2017-08-28T01:07:29Z

+++ dbaynard [Aug 28 17 00:52 ]:

I'd also like to use [2]clay for the (limited) css generation here as it's currently quite ad-hoc (and easy to introduce bugs) — I'll raise a separate issue. That would ensure the correct browser prefixes are supplied.

I guess I'm okay with using clay, if it avoids bugs; it's another dependency, but at least it doesn't bring in new transitive dependencies.

dbaynard · 2017-08-28T01:35:04Z

By 'both behaviours' I mean wrapping each line of output in a div, or not: the act of wrapping breaks any css which relies on code > span (direct children).

When each line of displayed code is wrapped in an element it is possible to style it using css, but the element must be added during html generation by skylighting.

The change would be adding a new command line option restoring the non-wrapping behaviour (I'd prefer the default to be wrapped). It appears this could be implemented quite succinctly, but I'd be happy to bump the version without adding what would be a legacy option to the API.

It is the presence of line numbers that is controlled by css, although in order to handle setting the starting line numbers, all the line numbers must be produced by skylighting. Eventually (when attr is supported by browsers inside calc) the whole thing could be done in css, though each line would still have to be wrapped in a suitable element.

dbaynard · 2017-08-28T01:49:49Z

I made the suggested changes, including a min-height of 1.25 em (if it isn't set there are display issues with blank lines). Again, sensible defaults. Please excuse the force push; I'm trying to keep the commits easy to follow, for my own sake as much as anything.

Also I have not yet tested these changes with pandoc. I'm building pandoc with this version of skylighting, and I'll check pandoc's tests.

jgm · 2017-08-28T04:08:37Z

What if we just used code span instead of code > div > span and code > a > span? (EDITED)
Is this a performance issue?

dbaynard · 2017-08-28T08:43:42Z

The problem isn't the css in this package; the problem is that users may have their own css which uses the code > span form, and now the generated html does not match that. It is not a performance issue, though I don't know whether it could be (I doubt it). Does this answer your question?

I suspect this is a minor problem, though, as few users are likely to override the syntax highlighting, it will be obvious from the output that there's been some change, and the fix is just once find/replace operation away.

Actually, using code span here wouldn't hurt, and may be a better solution as it means any custom css users write in future is likely to rely on the exact nesting of elements.

jgm · 2017-08-28T14:54:35Z

Right, I wasn't thinking it would help with existing uses of 'code > span', more just that it simplifies the CSS we need to include to make the highlighting work (and as you say, it may make things more future-proof). But there may be a significant performance difference? I really have no idea.

dbaynard · 2017-08-28T15:17:52Z

I'm changing it to code span, then. Browsers apparently match css from right to left (source: Stack Overflow) so it shouldn't make any difference to performance, never mind a significant difference.

I'm getting some test failures from pandoc's test suite, though I think the cause is my setup (I'm not certain I've bootstrapped the syntax files correctly in my local pandoc repository). I'd like to confirm whether the failures need fixing here because they are regressions, or fixing in pandoc's test suite because they are a change in the api. Once I've done that, I'd consider the code ready for consideration.

…d selector

dbaynard · 2017-08-29T19:22:04Z

I think the pandoc test failures are to do with me not bootstrapping skylighting correctly. They occur when I try to use local git commits to provide the skylighting dependency (stack doesn't like skylighting's build pattern, it seems).

3534.md #1

+   1 <p>I want to explain the interface of <code class="java">public class MyClass</code>.</p>
-   1 <p>I want to explain the interface of <code class="sourceCode java"><span class="kw">public</span> <span class="kw">class</span> MyClass</code>.</p>

Old latex writer lhs to normal (lhs-test.latex)

No highlighting defined, e.g.

-  41 \newcommand{\KeywordTok}[1]{\textcolor[rgb]{0.00,0.44,0.13}{\textbf{#1}}}

html writer lhs to normal (lhs-test.html)

ditto
lhs to lhs (lhs-test.html+lhs)

ditto

Writers LaTeX inline code struck out and highlighted

-------------------------------------------------------------- input ---
[Plain [Strikeout [Code ("",["haskell"],[]) "foo",Space,Str "bar"]]]
------------------------------------------------------------- result ---
- \sout{\mbox{\VERB|\NormalTok{foo}|} bar}
+ \sout{\mbox{\texttt{foo}} bar}
------------------------------------------------------------------------

HTML inline code haskell

-------------------------------------------------------------- input ---
[Plain [Code ("",["haskell"],[]) ">>="]]
------------------------------------------------------------- result ---
- <code class="sourceCode haskell"><span class="fu">&gt;&gt;=</span></code>
+ <code class="haskell">&gt;&gt;=</code>
------------------------------------------------------------------------

dbaynard · 2017-09-09T14:29:59Z

I've fixed the bootstrapping in my pandoc build, and I can no confirm the downstream pandoc issues correspond to the intended changes.

I've fixed these failing pandoc tests to correspond to the changes introduced by this PR, in jgm/pandoc#3905

As a result, I'm satisfied with this PR.

In pandoc, the copy-paste now works better than with tables. It is now possible to copy a whole document, including numbered lines of source code, and the line numbers will not be copied. If the numbers are wanted this can be changed by a user, in css.

dbaynard · 2017-09-09T14:31:28Z

I should add, I've investigated the failing travis build, but I have no idea why it failed. I can compile fine with GHC 8.0.2.

jgm · 2017-09-09T16:10:05Z

It's probably an out of time/memory issue. I've been seeing this recently. +++ dbaynard [Sep 09 17 07:31 ]:

…

I should add, I've investigated the failing travis build, but I have no idea why it failed. I can compile fine with GHC 8.0.2. — You are receiving this because you commented. Reply to this email directly, [1]view it on GitHub, or [2]mute the thread. References 1. #14 (comment) 2. https://github.com/notifications/unsubscribe-auth/AAAL5HaKiOZ8oAnmLabm2a-sqS4OC7v2ks5sgqFAgaJpZM4PDx74

jgm · 2017-09-09T18:58:58Z

Great, I've merged this!

jgm · 2017-10-26T18:39:54Z

I'm noticing on pandoc that, with this revised version of skylighting, we get extra blank space around highlighted code blocks. (that is, significantly more blank space than with non-highlighted blocks, and too much, in my opinion) Example:

Any ideas about why or how to fix?

jgm · 2017-10-26T20:59:11Z

Looks like taking out the enclosing div fixes this.
I don't think we need the enclosing div any more...it was just there to give us something uniform that applied both with line numbers (where previously we had a table) and without.

jgm · 2017-10-26T21:34:10Z

I think the explanation is that with the bare pre we have overlapping margins with the preceding p. (See https://www.w3.org/TR/CSS21/box.html#collapsing-margins). Putting the pre in a div defeats the margin-collapsing feature, so we get the margin for the p AND the margin for the pre.

Keeping the surrounding div but adding div.sourceCode pre { margin: 0 } works for code next to a paragraph, but gives bad results when e.g. you have two code blocks in a row.

dbaynard · 2017-10-26T23:42:02Z

I suspect it's an interaction between the spans wrapping the code identifiers and the new top level div. Should be a simple fix; I can take a look tomorrow. I presume the code producing that fragment is the obvious?

Does the issue appear with inline code, too?

jgm · 2017-10-27T00:42:09Z

I already put out a release removing the outermost div. That seems to work well. As I noted, I think it has to do with margin collapsing. (You can see this quite clearly in Safari's inspector.) +++ dbaynard [Oct 26 17 16:42 ]:

…

I suspect it's an interaction between the spans wrapping the code identifiers and the new top level div. Should be a simple fix; I can take a look tomorrow. I presume the code producing that fragment is the obvious?

larryhynes · 2017-11-07T11:39:04Z

Hi

I just kind of got bitten by this (no big deal! 😄), and I'm curious about it. In the intro above @dbaynard says

Wrap lines of source code in a div with display set to inline-block.

Where should this css (display: inline-block) appear? For instance, the following input:

# Foo

bar

~~~sh
for i in ls; do
	echo "$i"
done
~~~

when output with pandoc -s -t html, produces:

<!DOCTYPE html>
<html>
    <head>
        <meta charset="utf-8">
        <meta name="generator" content="pandoc">
        <meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=yes">
                        <title>t</title>
        <!--[if lt IE 9]>
        <script src="http://html5shim.googlecode.com/svn/trunk/html5.js"></script>
        <![endif]-->

                            </head>
    <body>
        <div class="container">
            <div class="row">
                <div class="col">
                                                                                <h1 id="foo">Foo</h1>
<p>bar</p>
<pre class="sourceCode sh" id="cb1"><code class="sourceCode bash"><div class="sourceLine" id="cb1-1" data-line-number="1"><span class="kw">for</span> <span class="ex">i</span> in ls<span class="kw">;</span> <span class="kw">do</span></div>
<div class="sourceLine" id="cb1-2" data-line-number="2">    <span class="bu">echo</span> <span class="st">&quot;</span><span class="va">$i</span><span class="st">&quot;</span></div>
<div class="sourceLine" id="cb1-3" data-line-number="3"><span class="kw">done</span></div></code></pre>
                                    </div>
            </div>
        </div>
    </body>
</html>

So the div in question (which I'm guessing is sourceLine) doesn't appear to get the inline-block style by default (unless I'm missing something)? This means that one gets a sort of double line-spacing in code blocks with highlighting applied. Like, if I converted the sample markdown above with pandoc -s --no-highlight -t html, I would get different line spacing on my output.

Maybe recent changes to skylighting haven't made it into pandoc yet (I'm using pandoc 2.0.1.1)?

In my own case, I just had to add a rule to my stylesheet:

.sourceLine {
    display: inline-block;
}

...and all was well with the world.

dbaynard · 2017-11-07T11:50:26Z

The css should appear inline in head; these changes haven't touched that behavior, as far as I am aware. But yes, you are correct that the comment about inline-block refers to .sourceLine. I'm not familiar with the very recent changes to pandoc.

Given this is the skylighting tracker, though, is the output correct when just the code is converted using skylighting?

larryhynes · 2017-11-07T12:14:31Z

Ah, OK, thanks. (I've just spotted that pandoc now seems to have an explicit html4 output option, which seems new to me, but I'll stop mentioning pandoc now as I'm in the skylight issue tracker.)

I'm afraid I don't have a standalone skylight to test with; I grew (very) tired of haskell's stack / cabal and ripped the whole lot out some time ago; sorry I can't be more help with that.

I'll make any further enquiries in the pandoc tracker.

dbaynard added 7 commits August 27, 2017 12:47

Wrap source in div.sourceLine

52d1a02

Add div to resulting css

11b8c1f

Add line numbering css

8a44b86

Replace table line-numbers with pseudo-element

1637f09

- Note, line links and custom start number no longer work

Customize the start number, using data-line-number

4a80a2a

Fix extra lines appearing during copy paste

5a74137

Enable clickable line numbers with linkAnchors

38f086a

dbaynard force-pushed the feat/line-numbers branch from c4ed9fa to 79f4262 Compare August 28, 2017 01:05

dbaynard force-pushed the feat/line-numbers branch from 79f4262 to 7a9a405 Compare August 28, 2017 01:09

dbaynard added 4 commits August 28, 2017 02:28

Match line spacing between numbered and non-numbered code

28533a5

Highlight tokens in code without wrapped lines

84032db

Fix now invalid/inaccurate documentation

7ff92b6

Update changelog

b9ee4d0

dbaynard force-pushed the feat/line-numbers branch from 7a9a405 to b9ee4d0 Compare August 28, 2017 01:29

Apply default highlighting to 'code span.*' using descendant not chil…

793b8b6

…d selector

dbaynard mentioned this pull request Sep 9, 2017

Handle skylighting’s new code output jgm/pandoc#3905

Closed

Restore default previous (lack of) wrapping of code

8496843

jgm merged commit 9ff873c into jgm:master Sep 9, 2017

jgm mentioned this pull request Oct 26, 2017

whitespace around code blocks jgm/pandoc#3996

Closed

jgm mentioned this pull request Nov 2, 2017

Non-unique HTML id attribute values in source-line references of code-blocks jgm/pandoc#4031

Closed

larryhynes mentioned this pull request Nov 7, 2017

Comment span class for lua is 'do', not 'co' as I would expect #23

Closed

jgm mentioned this pull request Dec 8, 2017

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

Closed

lawcho mentioned this pull request Jun 11, 2024

Fix jgm/pandoc#9292 #193

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap source lines and re-implement line numbers #14

Wrap source lines and re-implement line numbers #14

dbaynard commented Aug 27, 2017

jgm commented Aug 27, 2017

jgm commented Aug 27, 2017

jgm commented Aug 27, 2017

dbaynard commented Aug 27, 2017 •

edited

Loading

dbaynard commented Aug 28, 2017

jgm commented Aug 28, 2017 via email

jgm commented Aug 28, 2017 via email

jgm commented Aug 28, 2017 via email

dbaynard commented Aug 28, 2017

dbaynard commented Aug 28, 2017

jgm commented Aug 28, 2017 •

edited

Loading

dbaynard commented Aug 28, 2017 •

edited

Loading

jgm commented Aug 28, 2017 via email

dbaynard commented Aug 28, 2017

dbaynard commented Aug 29, 2017

dbaynard commented Sep 9, 2017

dbaynard commented Sep 9, 2017

jgm commented Sep 9, 2017 via email

jgm commented Sep 9, 2017

jgm commented Oct 26, 2017

jgm commented Oct 26, 2017

jgm commented Oct 26, 2017

dbaynard commented Oct 26, 2017

jgm commented Oct 27, 2017 via email

larryhynes commented Nov 7, 2017

dbaynard commented Nov 7, 2017

larryhynes commented Nov 7, 2017

Wrap source lines and re-implement line numbers #14

Wrap source lines and re-implement line numbers #14

Conversation

dbaynard commented Aug 27, 2017

jgm commented Aug 27, 2017

jgm commented Aug 27, 2017

jgm commented Aug 27, 2017

dbaynard commented Aug 27, 2017 • edited Loading

dbaynard commented Aug 28, 2017

jgm commented Aug 28, 2017 via email

jgm commented Aug 28, 2017 via email

jgm commented Aug 28, 2017 via email

dbaynard commented Aug 28, 2017

dbaynard commented Aug 28, 2017

jgm commented Aug 28, 2017 • edited Loading

dbaynard commented Aug 28, 2017 • edited Loading

jgm commented Aug 28, 2017 via email

dbaynard commented Aug 28, 2017

dbaynard commented Aug 29, 2017

dbaynard commented Sep 9, 2017

dbaynard commented Sep 9, 2017

jgm commented Sep 9, 2017 via email

jgm commented Sep 9, 2017

jgm commented Oct 26, 2017

jgm commented Oct 26, 2017

jgm commented Oct 26, 2017

dbaynard commented Oct 26, 2017

jgm commented Oct 27, 2017 via email

larryhynes commented Nov 7, 2017

dbaynard commented Nov 7, 2017

larryhynes commented Nov 7, 2017

dbaynard commented Aug 27, 2017 •

edited

Loading

jgm commented Aug 28, 2017 •

edited

Loading

dbaynard commented Aug 28, 2017 •

edited

Loading