Use of <a> in source code highlighting is challenging

[hadley]

e.g.

pandoc
```r
a + b
```

yields

<div class="sourceCode" id="cb1"><pre class="sourceCode r"><code class="sourceCode r"><a class="sourceLine" id="cb1-1" data-line-number="1">a <span class="op">+</span><span class="st"> </span>b</a></code></pre></div>

This is challenging because many css templates set a:hover (instead of the technically correct a:link:hover, a:visited:hover), meaning that every syntax highlighted line gets an underline (or other special treatment)

[jgm]

This is challenging because many css templates set a:hover (instead of the technically correct a:link:hover, a:visited:hover), meaning that every syntax highlighted line gets an underline (or other special treatment)

Do you have suggestions for an alternative? By the way, this relates to the jgm/skylighting package. We've had a number of CSS changes, and there are a lot of constraints. Pinging @dbaynard

[hadley]

<span> seems like the obvious answer, so I suspect there's some constraint that prevents it. That said, I thought most browsers now supported id attributes on arbitrary tags.

[dbaynard]

It was for line numbers; <a> means the line numbers link to the code line. Originally there would be <a> with line numbers, and <span> without, though that had its own problems. Might be worth revisiting — how widespread is the problem?

I do like your solution (r-lib/pkgdown@34a83e8) and would be keen to integrate upstream. However, I don't think that would fix your problem, as the samples in the linked issue don't appear to be using the css generated by skylighting (as output by pandoc) — is that correct?

[hadley]

What do you mean by "line numbers link to the code line"?

It's my understanding that <span class="sourceLine" id="cb1-1" data-line-number="1"> would still work with the url fragment #cb1-1

[dbaynard]

Yes, span would act like a as the target of a link. I meant: an aspect of the previous code the changes intended to replicate — that clicking line numbers linked to that line — wouldn't work without the line numbers being wrapped with <a>. For code generated using vanilla pandoc, and all code generated using skylighting, only the line numbers themselves act as links (e.g. https://dbaynard.github.io/test-html/tests/skylighting.html). There are various other changes that are down to the css bundled with the generated code; without that css the whole line becomes a clickable link.

It might actually be possible to wrap the lines in a <span> but precede them with a (zero-width) <a>. This would be the most flexible, at a cost of far more verbose code. That said, the code is more representative of what is going on.

<a class="sourceLine" id="cb1-1" data-line-number="1"></a><span class="sourceLine">…</span>

The reason for the changes was that despite a 'line of code' being a meaningful concept in documents that might be produced by pandoc, there was no representation of a 'line of code' in the html that pandoc
(and skylighting) produced. Now that the generated html reflects this, I've intended to provide sensible defaults that users can then override. So I'm happy to implement whatever will provide a sensible default for users.

And please excuse me; I had to rush off and didn't hit send the other day (though it did give me the time to think up the potential solution).

[hadley]

Ah got it, thanks for the explanation and the fix idea!

[dbaynard]

Great - let me know what's most helpful, then.

[hadley]

Your proposed solution of "empty" <a> in containing <span> sounds good from my perspective.

[mb21]

So the reason for the empty <a> would be to make the :before pseudo-elements (that render the line numbers) clickable?

[dbaynard]

Yes. Though there's no point bothering with the :before pseudo elements if there's just going to be extra data written in to the file itself. We might as well generate the actual line numbers at compile time…

But… I was hoping it would be possible to generate the line numbers dynamically, in the browser. That way it's possible to update the line numbers in generated html without having to recompile the source. Indeed that's what the 'data-line-number' business was originally about.

I've actually just figured out a way to do it using css counters instead, generating style="counter-reset: source-line 0" in the code tags. I'm very much in favour of this change, but it means continuing to use the pseudo elements.

This could be done in a way to reduce line noise, by moving all but the href into the span's attributes.

<span class="sourceLine" id="cb1-1" data-line-number="1"><a href="#cb1-1"></a>…</span>

I'll raise the required issue with skylighting, then.

Edit: I'll just PR to skylighting when I've made the changes.

[dbaynard]

Nobody seems to have raised further issues with this, and it is needed somewhat urgently to avoid regressions on jgm/skylighting#32. I'll fix at the weekend.

[dbaynard]

I've been unable to make the time to do this — I'll aim to do it this week. It's really the regression testing that takes the time; the changes are already described in this issue.

[Guillawme]

@dbaynard may we have an update about this issue?

For now, I am trying to use a custom CSS to customize the styling of sourceLine, but it would be nice to eventually have a correct styling out of the box with default CSS files that ship with common websites/blog themes.

Thank you.

[dbaynard]

Yes, I had to prioritise other work over pandoc. I apologise for the delay. I'll put it in the to do list for next weekend; meanwhile anybody else is welcome to PR.

[dbaynard]

Just to say; I'm not sure when I'll get around to this. I can help if anybody else wishes to contribute.

[patrickhlauke]

i see some mention of the rationale for the <a> being that it's clickable...just to point out that semantically an <a> without an href attribute is an anchor, not a link. by default these don't receive keyboard focus. also, not quite sure i follow the rationale of clickability...it doesn't appear to actually focus it or change the URL to include the fragment identifier...

[jgm]

@patrickhlauke they only become clickable if in lineAnchors in FormatOptions is set to True. Setting the line-anchors does this in pandoc:

``` {.ruby .line-anchors}
x = 3
```
<div class="sourceCode" id="cb1"><pre class="sourceCode ruby line-anchors"><code class="sourceCode ruby"><a class="sourceLine" id="cb1-1" href="#cb1-1" title="1">x = <span class="dv">3</span></a></code></pre></div>

[jgm]

There was a proposal above to wrap the line in a span following an empty a element. This sounds good to me, and @dbaynard seems to agree, right? I don't think the code changes would be difficult. If @dbaynard doesn't have time to do it, someone else, or I, could.

[dbaynard]

I will, shortly (next week). Thanks for your patience!

[darrylabbate]

Is there any way to simply disable the <a> tags from being generated without disabling syntax highlighting altogether? For whatever reason the use of <a> tags causes Safari to ignore code blocks in Reader view.

[jgm]

@dbaynard - it seems you haven't found time to do this? I can make this change myself.

[dbaynard]

@dbaynard - it seems you haven't found time to do this? I can make this change myself.

Yes, please make the change. I have not found the time (phd thesis woes, very long story… though now thankfully coming to an end). Thanks for your patience.

[jgm]

Should the span with class sourceCode go around the empty div and all the other spans on a line?
Should we introduce a new class, say, sourceLine, to distinguish it from the class on the a element?

[jgm]

I can change the HTML, but I may need help figuring out how to change the CSS so everything looks as it did before...

[mb21]

I can also help out (also with CSS), but I haven't followed the discussion closely...

[dbaynard]

I can now actually schedule time to do this (this weekend)! As I understand it, the html should look like this.

<code style="counter-reset: source-line n">
<span class="sourceLine" id="cb1-1" data-line-number="1"><a href="#cb1-1"></a>…</span>
</code>

where the n is either 0, or the starting line number (subtract 1).
Putting the counter-reset in the tag directly means that the starting line number can be customised, straightforwardly.

The css needs to generate the pseudo element on the a. So something like the following (which lacks the correct positioning).

pre.numberSource > code a:first-child::before {
    counter-increment: source-line;
    content: counter(source-line);
}

(Edit: the above has been fixed; it used to say numberLines).

The tricky part is testing it; I've used something ad-hoc, before, but it might be worth going with a more robust option (e.g. https://www.browserstack.com/open-source). And it needs to be run through epubcheck, too.

[mb21]

<code style="counter-reset: source-line n"> is fine for n>0, i.e. when we have something like {.numberLines startFrom="100"}. But for the default case n=0, I wouldn't emit a style tag, but use CSS:

pre.numberLines > code {
  counter-reset: source-line 0;
}

[mb21]

@dbaynard Generally, this seems like a good approach.

But it seems we can now simplify the generated HTML a bit. Apologies if these points were already discussed. Potentially, they might also break some users' scripts/custom css?

• do we need the sourceLine class on the spans? why not target them with .numberLines > .sourceCode > span
• what is the numberSource class for on the pre tag? isn't numberLines enough?
• with css counters, we can also remove the data-line-number attribute?
• pre.numberLines > code a:first-child::before could also be .numberLines > .sourceCode > a::before

[dbaynard]

(on mobile, I'll edit formatting later)

do we need the sourceLine class on the spans? why not target them with .numberLines > .sourceCode > span

We can do this.

what is the numberSource class for on the pre tag? isn't numberLines enough?

It is. Forgive me, I think I used the wrong name in my example (pandoc/skylighting use different terms, I believe).

with css counters, we can also remove the data-line-number attribute?

Probably. That also pre-dated the id. I believe the CSS counters will work on all platforms where the data attribute would work. Having the attribute makes it easy to target single lines by number, though I don't think it's anything that could not be achieved with the id and :nth-child selectors.

pre.numberLines > code a:first-child::before could also be .numberLines > .sourceCode > a::before

Each a would be wrapped in the span, so your proposed solution wouldn't match. If adjusted to include a span >, it would match any link in the code. I thought the first-child solution avoided this trouble but it might need both the span > and the first-child.

[dbaynard]

But it seems we can now simplify the generated HTML a bit. Apologies if these points were already discussed.

The original, default, print CSS was quite broken — it used tables for line numbers, and it was impossible to supply a solution after pandoc processing as skylighting output didn't demarcate lines of code. Unfortunately, the solution I picked had a few issues. I don't think there was that much more to the discussion!

[mb21]

Sounds good, I suppose there could also be changes made to skylighting (which pandoc uses)... but maybe @jgm prefers to not change too many things due to backwards-compatibility...

[dbaynard]

I've made the PR (all the changes are in skylighting). I've made many small commits so it should be straightforward (for me) to remove any backwards-incompatible changes.

[jgm]

do we need the sourceLine class on the spans? why not target them with .numberLines > .sourceCode > span?

Is there a performance penalty to using > in CSS rather than a class? That could be a reason to keep things as they are.

[mb21]

Yeah, targeting span instead of a class has a performance penalty. Using > (direct child) instead of the space (any child) should however speed things up.

startFrom doesn't seem to work on current master? seems like the <code style="counter-reset: source-line n"> was forgotten even for n>0?

Also we have the following weird thing in the output:

div.sourceCode
  {   }
@media screen {
code.sourceCode > span > a:first-child::before { text-decoration: underline; }
}

Why would you want the line numbers be underlined?

[dbaynard]

startFrom doesn't seem to work on current master? seems like the <code style="counter-reset: source-line n"> was forgotten even for n>0?

Ach I knew I'd forgotten something… I'll PR later today. Thanks!

Why would you want the line numbers be underlined?

This matched previous behaviour. I suppose it suggests they are links (hence why it's for screen only)?

The div.sourceCode line corresponds to no overriding colours (that is where they would go). I can remove if desired?

[jgm]

I'm uncertain about the underlining on line numbers. It looks cluttered, but it is nice to have a visual indication that it's a link. It should be possible for users to disable this with their own CSS, right?

[mb21]

It should be possible for users to disable this with their own CSS, right?

yes...
maybe it should be underlined only on hover?

[jgm]

maybe it should be underlined only on hover?

Most browsers already give you a visual indication of a link on hover (cursor changes).
The point of the underline would to tell you that it's a link without the need to hover and check (since one might not think of doing that). But I don't feel too strongly about this either way.

[dbaynard]

@mb21 see jgm/skylighting#73 for the startFrom fix. I apologise for forgetting it, the first time.

[dbaynard]

Also, it would be good to test compatibility with a real CSS example; @mb21 is there one you could recommend?

[mb21]

Maybe @hadley and @aubertc can test/give feedback on the new source-code line-numbering HTML/CSS output of the nightly release?

[dbaynard]

I've just made jgm/skylighting#75 to indicate wrapped lines, in print css. @mb21 I'd be grateful if you'd take a look.

[aubertc]

@mb21 I just tried with the hash-8507d98a1 version, and it works just fine.

Same code as in this issue, Microsoft Edge 41.16299.1004.0.

Thanks a lot!

[silky]

fwiw i'll just observe that @hadley ' s comment that "a:link:hover" is the best behaviour doesn't quite work (because i went down a path of trying to work it out)

this will work if you set, say, a background color, but it won't work if you also set the color field; it doesn't apply to the text of the link, then.

it's quite frustrating that you can't remove the link tags being generated; i.e that they are only generated with link-anchors set, because it makes it very hard to style links in a nice way.

for example, hakyll - https://github.com/jaspervdj/hakyll/blob/master/web/css/syntax.css - sets pointer-events: none; on these non-links to make them not have the 'hover' behaviour

but is terrible from a UX point of view, because it means you can no longer nicely select and copy lines!

so one is left only with the option of over-riding all the styles for links in code (that one doesn't even want turned on in the first place!)

-- edit: and, the fact that it places a line number on the line as the title is odd; it would be great if that could be an option to disable.