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

HtmlFormatter: filename doesn't work well with linenos=table #1757

Closed
Cartroo opened this issue Mar 24, 2021 · 4 comments · Fixed by #1759
Closed

HtmlFormatter: filename doesn't work well with linenos=table #1757

Cartroo opened this issue Mar 24, 2021 · 4 comments · Fixed by #1759
Labels
changelog-update Items which need to get mentioned in the changelog
Milestone

Comments

@Cartroo
Copy link
Contributor

Cartroo commented Mar 24, 2021

When using HtmlFormatter with both the linenos=table and filename=testfile options, you get some output like that shown below. The problem is that the filename being output within the <td> for the code throws off the line number alignment.

<table class="highlighttable">
  <tr>
    <td class="linenos">
      <div class="linenodiv">
        <pre>...</pre>
      </div>
    </td>
    <td class="code">
      <div class="highlight">
        <span class="filename">testfile</span>
        <pre>...</pre>
      </div>
    </td>
  </tr>
</table>

I suggest that this is unlikely to be useful to anyone, since it will invariably throw off the line numbering alignment. Hence, in the special case of someone using linenos=table and specifying a filename I would suggest that another table row is output as a caption:

<table class="highlighttable">
  <tr>
    <td colspan="2" class="filename">testfile</td>
  </tr><tr>
    <td class="linenos">
      <div class="linenodiv">
        <pre>...</pre>
      </div>
    </td>
    <td class="code">
      <div class="highlight">
        <pre>...</pre>
      </div>
    </td>
  </tr>
</table>

I can look at entering a pull request to implement this, but I wanted to get some feedback on whether this would be acceptable first.

@Anteru
Copy link
Collaborator

Anteru commented Mar 24, 2021

That does make sense to me. I'm 100% sure this will break some theme somewhere, and you'll get some weird coloring there if you don't wrap it in highlight and maybe some CSS class will fail to match.

Don't get me wrong -- I'm all for this, but if you want to touch this kind of thing, make sure to document the before/after state and ideally provide some guidelines for theme writers. If you look at the 2.8 release notes, we did some non-trivial CSS change, but with the docs it doesn't seem to have caused nearly as much fallout as previous CSS changes.

I do agree that in practice nobody can be using this seriously as it will look plain broken. I'm just cautious :)

Thanks for your offer to help here!

@Cartroo
Copy link
Contributor Author

Cartroo commented Mar 25, 2021

Sure, you're quite right, of course. I'll aim for something more like the snippet below, although clearly may still be breaking for some themes. In principle I guess it could be controlled by another option, to maintain the backwards compatibility, but that's a bit clunky for something that only impacts this one edge case in my view.

<table class="highlighttable">
  <tr>
    <td colspan="2" class="filename"><div class="highlight">
      <span class="filename">testfile</span>
    </div></td>
  </tr><tr>
    <td class="linenos">
      <div class="linenodiv">
        <pre>...</pre>
      </div>
    </td>
    <td class="code">
      <div class="highlight">
        <pre>...</pre>
      </div>
    </td>
  </tr>
</table>

Note that I could make it even closer by using class="code" on the <td> for the filename. But personally I feel that there's a good chance people will want the option to style this caption differently to the code.

@Anteru
Copy link
Collaborator

Anteru commented Mar 25, 2021

Yes, I don't think <code> is required there, as it's not really code. As long as there is a unique class/CSS path attached to it everything should be fine.

Cartroo added a commit to Cartroo/pygments that referenced this issue Mar 27, 2021
* Emit `<th>` for `filename` if `linenos=table`.

* Added test cases for filename inclusion.
@Cartroo
Copy link
Contributor Author

Cartroo commented Mar 27, 2021

Here's a first attempt, I've used <th> partly because I felt it was appropriate (the filename is rather like a heading for the table) and partly because I felt it was less likely to fall foul of people's existing CSS rules (since they'd most likely have applied them on <td> elements).

I'd be interested in your feedback, in particular whether there's any additional documentation you feel should be updated.

@Anteru Anteru closed this as completed in 2095bd9 Mar 28, 2021
@Anteru Anteru linked a pull request Mar 28, 2021 that will close this issue
@Anteru Anteru added this to the 2.9 milestone Mar 28, 2021
@Anteru Anteru added the changelog-update Items which need to get mentioned in the changelog label Mar 28, 2021
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue May 6, 2021
Version 2.9.0
-------------

- Added lexers:
  * APDL, gcode
  * Kuin
  * NestedText
  * OMG IDL
  * TEAL
  * ThingsDB
  * WebAssembly

- Updated lexers:
  * AMDGPU
  * APL
  * C/C++: Improve namespace handling
  * Chapel
  * Coq
  * Cython
  * Groovy
  * JavaScript
  * Julia
  * Octave: Allow multiline and block-percent comments
  * PowerShell: Improve lexing of ``:``
  * PromQL
  * Python: Improve float parsing
  * Rust
  * Scala: Rewrite to support Scala3
  * Swift
  * Terraform: Support 0.14 syntax
  * Velocity: Detect multi-line patterns

- Add Pango formatter
- Autopygmentize uses ``file`` first instead of ``pygments -N``
- Fix links
- Fix issue with LaTeX formatter and ``minted``
- Improve alias order
- Improve line number colors
- Fix CTag related issue
- Recognize ``.leex`` as Elixir templates

- Updated `filename` handling in HTML formatter if `linenos='table'`

  * Previously the filename would be emitted within the `<td>` holding the
    code, but outside the `<pre>`. This would invariably break the alignment
    with line numbers.
  * Now if `filename` is specified, a separate `<tr>` is emitted before the
    table content which contains a single `<th>` with `colspan=2` so it
    spans both the line number and code columns. The filename is still
    within `<span class="filename">...</span>` so any existing styles
    should still apply, although the CSS path may need to change.
  * For an example of the new output format see
    `table_cls_step_1_start_1_special_0_noanchor_filename.html`
    in the `tests/html_linenos_expected_output/` directory.
  * For more details and discussion see the issue
    pygments/pygments#1757

- Added styles:

  * Gruvbox light+dark
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-update Items which need to get mentioned in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants