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

Line numbers are misaligned on several Sphinx themes #1579

Closed
mgeier opened this issue Oct 21, 2020 · 11 comments
Closed

Line numbers are misaligned on several Sphinx themes #1579

mgeier opened this issue Oct 21, 2020 · 11 comments
Milestone

Comments

@mgeier
Copy link
Contributor

mgeier commented Oct 21, 2020

A change in Sphinx (sphinx-doc/sphinx#7482) combined with a change in Pygments (#1477) has led to a wrong alignment of line numbers.

This happens with Pygments 2.7+ and Sphinx 3.1+.

See sphinx-doc/sphinx#8254 for details including screenshots.

So far, I've found these Sphinx themes to be affected:

 

I'm not sure whether this should be fixed in Sphinx or in Pygments (or both?), but I found out that disabling this chunk of CSS (which comes from Pygments) seems to fix the problem for most themes (I didn't try all, though):

td.linenos pre {
    color: #000000;
    background-color: #f0f0f0;
    padding: 0 5px 0 5px;
}
@Anteru
Copy link
Collaborator

Anteru commented Oct 21, 2020

I'm not sure I see the bug. I've been changing it here: b6fb70f, to the output you see there, and I'd like to understand how that output is causing issues? Is Pygments on its own inconsistent with spacing, or is it just in conjunction with Sphinx and Sphinx expecting a particular class/tag? (Note that Sphinx can always just override some CSS if that's causing trouble.)

@mgeier
Copy link
Contributor Author

mgeier commented Oct 22, 2020

I'm not sure I see the bug.

But you see the misalignment?

You might not consider the colors a bug, but the misalignment definitely is, isn't it?

I'm not saying that it's necessarily a bug in Pygments, but it is a bug somewhere!

I've been changing it here: b6fb70f, to the output you see there, and I'd like to understand how that output is causing issues?

Does sphinx-doc/sphinx#8254 (comment) explain the issues sufficiently?

Is Pygments on its own inconsistent with spacing, or is it just in conjunction with Sphinx and Sphinx expecting a particular class/tag?

I don't know, I've only ever used Pygments in conjunction with Sphinx.

How would I test Pygments on its own?

(Note that Sphinx can always just override some CSS if that's causing trouble.)

I don't think so. At least not in a general way that un-breaks all affected themes.

Sphinx (in its basic.css) cannot overwrite the vertical padding, because it is defined by the theme, which basic.css or course has no knowledge of.

Only each individual theme knows its correct vertical padding, therefore the problem can only be fixed by the theme, and not by Sphinx.

I'm willing to fix my own theme (mgeier/insipid-sphinx-theme#21), but I think it would be better to simply not break all those themes in the first place.

Again, I'm not saying the bug is necessarily in Pygments. If you know a way to fix this in Sphinx in a generic way, please let me know!

@Anteru
Copy link
Collaborator

Anteru commented Oct 22, 2020

I agree it looks broken, I'm just saying I'm not sure if this qualifies as a bug if 2.6/2.7 kind of works with Sphinx 3.0 (as in, the output is not mismatched). It looks like there were (implicit) assumptions on both sides about how things are supposed to work, and something is broken now :)

Let me try raw Pygments output first. I'm fairly certain that did look correct. If it does indeed look correct, the next step will be to figure out how it violates Sphinx' assumptions.

To test Pygments on your own, you can simply run pygmentize, using the HTML formatter. That's how the test files were generated in the first place.

Anteru added a commit that referenced this issue Oct 22, 2020
This removes the top/bottom padding changes, and only keeps left/right
padding, in the hope that this does not break all Sphinx themes.
@Anteru
Copy link
Collaborator

Anteru commented Oct 22, 2020

It looks fine with Pygments "standalone". The colors for the linenos class are expected, as the bug was that line numbers were not properly colored (it was hardcoded.) The original style for linenos was to have 10 px padding on the right. I've changed it now to be 5px padding on the left and right, but not specify a top/bottom padding. I hope that this does not break Sphinx themes in the same way. Can you give 0d28e2b a try, please?

@mgeier
Copy link
Contributor Author

mgeier commented Oct 23, 2020

Thanks @Anteru, this indeed fixes the alignment problem!

As you mentioned, this changes the colors though, which I think most theme authors would consider a bug.

The colors for the linenos class are expected, as the bug was that line numbers were not properly colored (it was hardcoded.)

What was hardcoded?
Before Pygments 2.7, Pygments had no influence on the colors of the line numbers in Sphinx AFAICT.

The original style for linenos was to have 10 px padding on the right. I've changed it now to be 5px padding on the left and right,

OK, I don't really have an opinion on this.

Sphinx currently has additional padding on the parent element:

https://github.com/sphinx-doc/sphinx/blob/0476e1cea93b587e3c0ab295aa20b2b9f0f81a34/sphinx/themes/basic/static/basic.css_t#L734-L736

I'm not sure whether that's too much if Pygments also adds padding.

but not specify a top/bottom padding.

That's great!

@mgeier
Copy link
Contributor Author

mgeier commented Oct 23, 2020

It looks fine with Pygments "standalone".

Could you please provide an example pygmentize invocation including line numbers, to make sure we are talking about the same thing?

@Anteru
Copy link
Collaborator

Anteru commented Oct 23, 2020

The line number colors were hardcoded previously and didn't match the theme. The original CSS for them was background-color: #f0f0f0; padding-right: 10px; no matter what color theme you used (and those rules were in the CSS only, not inlined -- that's probably the main reason you're affected now, because Pygments properly inlines them now.) If the fix I wrote works for Sphinx, I'll get a 2.7.1 out with it (forcing the padding to 0 was not intentional.)

For pygmentize, use pygmentize -f html -O linenos=True,full=True path/to/a/file

@mgeier
Copy link
Contributor Author

mgeier commented Oct 24, 2020

that's probably the main reason you're affected now, because Pygments properly inlines them now

OK, thanks, that explains a lot!

I was wondering why this suddenly appeared out of the blue ...

Thanks for the pygmentize command, that's very helpful!
I've added the style to the options (-O linenos=True,full=True,style=friendly) to compare a few styles.

It looks like the "solarized" style (solarized-light and solarized-dark) is the only one that bothers to customize the line numbers:

line_number_color = DARK_COLORS['base01']
line_number_background_color = DARK_COLORS['base02']

line_number_color = LIGHT_COLORS['base01']
line_number_background_color = LIGHT_COLORS['base02']

All other styles seem to just take the default:

pygments/pygments/style.py

Lines 179 to 183 in 3e1b79c

#: line number font color
line_number_color = '#000000'
#: line number background color
line_number_background_color = '#f0f0f0'

Are there any plans to choose proper colors for the other styles?

I think the line number colors should definitely be overridden by Sphinx when using the table feature, which is currently the default, because the Pygments colors simply make no sense at all in that case.
Sphinx 4 will switch by default to inline (see sphinx-doc/sphinx#7942), where the Pygments colors might make sense, but only if the styles actually select meaningful colors!

@Anteru
Copy link
Collaborator

Anteru commented Oct 24, 2020

Yes, but there's history again here. All styles used to use the same colors, and it was plain broken for solarized. A PR was made to implement a way to fix it, and for solarized, it was fixed properly, but the other styles continue to use the default (which seems ok as nobody was complaining about styles other than solarized being broken -- it seems likely nobody was using line numbers with a dark style.)

I'm happy to accept a PR to fix all other styles, but it doesn't seem high priority. Sphinx is of course free to override things, and frankly that's what I'd expect from a downstream user of Pygments. On the Pygments side, we have to be "self consistent", i.e. Pygments output needs to work, and if someone else is applying styling on top, they need to be able to override anything Pygments is doing as otherwise we'll be constrained by all downstream users -- which we may not even know.

My plan for this weekend is to release 2.7.2 with the padding fix (as I mentioned, fixing the padding to 0 was not intentional), but when it comes to colors you need to find a solution in Sphinx. My suggestion would be to use !important where it matters. If you have ideas for a better long-term solution I'm all ears -- I could imagine that Sphinx would supply it's own theme to Pygments for instance.

@Anteru Anteru mentioned this issue Oct 24, 2020
@Anteru Anteru added this to the 2.7.2 milestone Oct 24, 2020
@Anteru Anteru added the changelog-update Items which need to get mentioned in the changelog label Oct 24, 2020
Anteru added a commit that referenced this issue Oct 24, 2020
This removes the top/bottom padding changes, and only keeps left/right
padding, in the hope that this does not break all Sphinx themes.
@mgeier
Copy link
Contributor Author

mgeier commented Oct 24, 2020

Sphinx is of course free to override things, and frankly that's what I'd expect from a downstream user of Pygments.

I've just suggested exactly that in sphinx-doc/sphinx#8333.

However, this affects only the table layout.

As mentioned above, the Pygments colors would probably make sense in the inline layout, so maybe I'll try to add some colors to the other styles, but I'm not promising anything.

If somebody else wants to update the colors, please go ahead!

I could imagine that Sphinx would supply it's own theme to Pygments for instance.

Well there is already a sphinx style (SphinxStyle) and apparently a PyramidStyle: https://github.com/sphinx-doc/sphinx/blob/3.x/sphinx/pygments_styles.py.

But I think it is a very good feature of Sphinx to not be limited to a single style and to allow arbitrary Pygments styles.

If you have ideas for a better long-term solution I'm all ears

The long-term plan for Sphinx is to drop table layout anyway, so this will become irrelevant within the next year.

But for a bright future with the inline layout it would be nice to get updated colors in all styles that still use the defaults. This way, Sphinx wouldn't need to override them.

@Anteru
Copy link
Collaborator

Anteru commented Oct 24, 2020

Sure, but that's further out. You may want to file an issue for themes where the default colors just don't work at all, maybe someone picks it up in the meantime.

I'm closing this bug as resolved, 2.7.2 will remove the vertical padding rule, so please update Sphinx to use <2.7 || >= 2.7.2.

@Anteru Anteru closed this as completed Oct 24, 2020
@Anteru Anteru removed the changelog-update Items which need to get mentioned in the changelog label Oct 24, 2020
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

2 participants