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

List items have extra vertical space since Sphinx 3.1.x #7838

Closed
cjolowicz opened this issue Jun 15, 2020 · 25 comments
Closed

List items have extra vertical space since Sphinx 3.1.x #7838

cjolowicz opened this issue Jun 15, 2020 · 25 comments

Comments

@cjolowicz
Copy link

cjolowicz commented Jun 15, 2020

Describe the bug
List items in HTML output are displayed with extra vertical space, as if separated by blank lines. Compare the rendering of the feature list in this project, or the screenshots further below:

Tested on a recent Chrome for Mac, see below for environment info.

It appears this is due to changes in basic.css between Sphinx 3.0.4 and 3.1.0. See the diff of basic.css at the bottom of this issue description. The stylesheet rules reset the bottom margin of <p> when nested in <li>. Apparently this logic was broken in Sphinx 3.1.x for at least one browser.

To Reproduce
Steps to reproduce the behavior:

$ git clone https://github.com/cjolowicz/cookiecutter-hypermodern-python
$ git checkout 2020.6.15
$ cd cookiecutter-hypermodern-python
$ pip install -r docs/requirements.txt
$ cd docs
$ sphinx-build . _build
$ open _build/index.html

Expected behavior
List items are displayed in a compact way, without vertical spacing amounting to blank line.

Your project
https://github.com/cjolowicz/cookiecutter-hypermodern-python

Screenshots

With Sphinx 3.0.3:

list-sphinx-3 0 3

With Sphinx 3.1.0:

list-sphinx-3 1 0

Environment info

  • OS: macOS Mojave 10.14.6 (18G4032)
  • Python version: 3.8.2
  • Sphinx version: 3.1.0, 3.1.1
  • Sphinx extensions: -
  • Extra tools: Chrome 83.0.4103.97 (Official Build) (64-bit)

Additional context

Here is the diff of basic.css from Sphinx 3.0.4 to 3.1.0 which causes the breakage for me. IOW if I revert these changes, the list is displayed correctly.

diff --git a/_static/basic.css b/_static/basic.css
index 3a37f63..56f5efc 100644
--- a/_static/basic.css
+++ b/_static/basic.css
@@ -513,14 +513,30 @@ ol.upperroman {
     list-style: upper-roman;
 }

-li > p:first-child {
+ol > li:first-child > :first-child,
+ul > li:first-child > :first-child {
     margin-top: 0px;
 }

-li > p:last-child {
+ol ol > li:first-child > :first-child,
+ol ul > li:first-child > :first-child,
+ul ol > li:first-child > :first-child,
+ul ul > li:first-child > :first-child {
+    margin-top: revert;
+}
+
+ol > li:last-child > :last-child,
+ul > li:last-child > :last-child {
     margin-bottom: 0px;
 }

+ol ol > li:last-child > :last-child,
+ol ul > li:last-child > :last-child,
+ul ol > li:last-child > :last-child,
+ul ul > li:last-child > :last-child {
+    margin-bottom: revert;
+}
+
 dl.footnote > dt,
 dl.citation > dt {
     float: left;
@cjolowicz cjolowicz changed the title List items separated by blank line in HTML output List items have extra vertical space since Sphinx 3.1.x Jun 15, 2020
@tk0miya tk0miya added this to the 3.1.2 milestone Jun 15, 2020
@tk0miya
Copy link
Member

tk0miya commented Jun 15, 2020

@eric-wieser I guess this cames from your changes in 3.1. Could you check this please?

@eric-wieser
Copy link
Contributor

@tk0miya: I don't remember editing any stylesheets.

@eric-wieser
Copy link
Contributor

eric-wieser commented Jun 15, 2020

This came from @mgeier in #7657 I think

@mgeier
Copy link
Contributor

mgeier commented Jun 15, 2020

Yes, this came from me, and yes, this is a change in behavior, but I'm not sure whether it's a "breakage" or an "un-breakage".
I would argue that the new behavior is "more correct".

This is also the behavior you see when you open the HTML page without any CSS.

This is the same as Github Markdown uses for "loose" lists:

"Tight" list without <p>:

  • no <p>
  • no <p>

"Loose" list with <p>:

  • yes <p>

    yes <p>

  • yes <p>

I think the real question is: can we get Sphinx to generate list items without <p> tags (i.o.w. "tight" lists)?

@mgeier
Copy link
Contributor

mgeier commented Jun 15, 2020

I would like to explain my motivation (or rather one of the motivations) for the change in #7657:

Using the docutils demo document https://github.com/docutils/docutils/blob/master/docutils/docs/user/rst/demo.txt:

Bullet Lists
------------

- A bullet list

  + Nested bullet list.
  + Nested item 2.

- Item 2.

  Paragraph 2 of item 2.

  * Nested bullet list.
  * Nested item 2.

    - Third level.
    - Item 2.

  * Nested item 3.

Before Sphinx version 3.1, this looked like this (using the sphinxdoc theme):

image

Starting with Sphinx 3.1, it looks like this:

image

I would like to argue that the new spacing is more consistent, but there is for sure room for improvements!

@cjolowicz
Copy link
Author

Thanks @mgeier for the response and detailed explanation!

The style may be "more correct" for <li> with a nested <p>. But I would consider this particular HTML structure an implementation detail from the point of view of Sphinx authors. They have no way of controlling this, to my knowledge.

So it seems to me, if you want to change this, the way forward would be to first emit list items without <p> where appropriate, and then apply your style changes. Reversing this order does indeed break authors by switching them from something that visually amounts to "tight lists" in the non-nested case, to just plain "loose lists" everywhere without opt-out.

As for the more consistent layout: Your example from the docutils demo document uses a nested list. I'm not sure I follow you even for the nested case (I find the first rendering easier to the eye, because nested items are grouped together). But for the case of flat lists, list items are effectively interleaved with blank lines, which means they look less cohesive on a page, and are not separated well from other page elements. Also, this wastes a good amount of screen real estate. I think the links at the top of the issue description demonstrate this well.

Disclaimer: I'm not a Sphinx expert, and certainly not an expert in web design. I don't mind at all to fix this in my own CSS instead (but I do think it's in need of fixing).

@tk0miya
Copy link
Member

tk0miya commented Jun 16, 2020

Oh, sorry eric-wieser. it's my bad. Indeed, this was changed by mgeier.

I think the real question is: can we get Sphinx to generate list items without

tags (i.o.w. "tight" lists)?

No proper way (in reST). The reST parser always generates a paragraph node for each list item. Of course, you can do it with your own extension. But users can't control it via mark up.

@return42
Copy link

Sorry to say, but PR #7657 breaks nearly all layouts .. IMO these patches should be rewind complete.

@mgeier please stop changing basic CSS !!!

i'm pretty annoyed because i have been trying to find out how to fix this broken layout in customizing for hours ... and i can do that in all my projects .. thanks for all the work .. how do you get the idea to change the basic CSS so massively without having to test it broadly.

@eric-wieser
Copy link
Contributor

So it seems to me, if you want to change this, the way forward would be to first emit list items without

where appropriate, and then apply your style changes.

Is the solution here to add a docutils Transformer that walks over list items, and unwraps the paragraph nodes if all children contain exactly one?

@jakobandersen
Copy link
Contributor

I remember trying to figure out the deal with lists at some point, and it may be worth looking into the docutils writers first. If I'm not mistaken the paragraph-less lists are called "compact lists" there, and a quick grep (for "compact" in the writers folder) indicates that there may be a difference between the HTML 4 and HTML 5 writer. Maybe they are simply not implemented for HTML 5? If so it may be best to fix docutils (or at least patch the HTML 5 writer in Sphinx).

@eric-wieser
Copy link
Contributor

eric-wieser commented Jun 18, 2020

That's exactly what it is, the HTML4 writer has a custom visit_enumerated_list to handle compact lists, while the HTML5 one does not, despite still supporting the --compact-lists option. This is a bug in docutils.

I'm wrong, the HTML4 writer actually overloads a method in the base class. So if anything the HTML5 writer has better compact list support.

@eric-wieser
Copy link
Contributor

eric-wieser commented Jun 18, 2020

Ah, here's the relevant snippet from the HTML4 writer

    # Omit <p> tags to produce visually compact lists (less vertical
    # whitespace) as CSS styling requires CSS2.

It looks like this feature is quite deliberately not provided in the HTML5 writer, as it seems to have been dismissed as "a CSS problem". I don't agree with this choice of docutils, semantically a list of paragraphs is distinct from a list of sentences.

@jakobandersen
Copy link
Contributor

It looks like this feature is quite deliberately not provided in the HTML5 writer, as it seems to have been dismissed as "a CSS problem". I don't agree with this choice of docutils, semantically a list of paragraphs is distinct from a list of sentences.

I understand their reasoning (it's kind of a hax), and indeed there is a difference in semantics.
Where is the paragraph in the list items actually generated? Is is a docutils node that is simply written, or is it the HTML 5 writer that invents it?
We probably could provide a "compact lists" option in Sphinx and implement the old HTML 4 logic?
Could we provide directives to force a specific layout, per list, e.g.,

.. list-style:: compact

* item1
* item2

  .. list-style:: paragraph

  * subitem1
  * subitem2

I'm not sure how that could be implemented, but in the end there will probably be cases where one would like to override the default behaviour.

@return42
Copy link

@mgeier wrotes ..

I would like to argue that the new spacing is more consistent,

Not with the toc directive which is rendered much more compact.

@return42
Copy link

FWIW: here is my hot-fix (haven't tested on all my projects):

li > p:first-child {
    margin-top: 0;
}

li > p:last-child {
    margin-bottom: 0;
}

div.admonition dl {
    margin-bottom: 0;
}

@eric-wieser
Copy link
Contributor

eric-wieser commented Jun 18, 2020

Is is a docutils node that is simply written, or is it the HTML 5 writer that invents it?

It's a docutils node that's there from the beginning:

In [57]: import docutils.nodes
    ...: import docutils.parsers.rst
    ...: import docutils.utils
    ...: import docutils.frontend
    ...:
    ...: def parse_rst(text: str) -> docutils.nodes.document:
    ...:     parser = docutils.parsers.rst.Parser()
    ...:     components = (docutils.parsers.rst.Parser,)
    ...:     settings = docutils.frontend.OptionParser(components=components).get_default_value
    ...: s()
    ...:     document = docutils.utils.new_document('<rst-doc>', settings=settings)
    ...:     parser.parse(text, document)
    ...:     return document
    ...:

In [58]: d = parse_rst("""
    ...: testing
    ...:
    ...: * This is a short list
    ...: * With one line
    ...:
    ...: And
    ...:
    ...: * This is a much longer list
    ...:
    ...:   With multiple lines
    ...:
    ...: * And another item""")

In [62]: print(d.pformat())
<document source="<rst-doc>">
    <paragraph>
        testing
    <bullet_list bullet="*">
        <list_item>
            <paragraph>
                This is a short list
        <list_item>
            <paragraph>
                With one line
    <paragraph>
        And
    <bullet_list bullet="*">
        <list_item>
            <paragraph>
                This is a much longer list
            <paragraph>
                With multiple lines
        <list_item>
            <paragraph>
                And another item

hence my suggestion that maybe we handle this with a transform pass.

@return42
Copy link

return42 commented Jun 18, 2020

It's a docutils node that's there from the beginning:

I remember that I tried to fixed it years ago, but when I saw the Surceforge process I stopped thinking about.

I guess it is time to create a git-fork of docutils / Goodger will never use git and the development goes down to zero and when they made patches in the past they break projects using sphinx-doc.

Me and myriade of other developer like to contribute are asking Goodger for a change from svn to git, but Goodger is completly ignorand (did you ever read all the "migrate to git" threads .. the discussion is really annoying).

BTW: there is a git mirror https://repo.or.cz/w/docutils.git of the SVN repo.

@jakobandersen
Copy link
Contributor

I'm not sure changing docutils necessarily is the best approach. The parser can not know when a list should be compact or not. Post-transforms may change the tree, e.g., expand a seemingly small item into a multi-paragraph text. So having a late post-transform, or some logic in the HTML writer seems most appropriate to me at the moment, leaning towards the transform.
I bit of digging also let me to https://www.sphinx-doc.org/en/master/usage/configuration.html?highlight=compact#confval-html_compact_lists, which defaults to True, and seems to be used only at https://github.com/sphinx-doc/sphinx/blob/3.x/sphinx/transforms/compact_bullet_list.py#L65, for prettifying specific cases, and then at https://github.com/sphinx-doc/sphinx/blob/3.x/sphinx/builders/html/__init__.py#L418, which I guess is the link to docutils which goes unused when using the HTML 5 writer.

@jakobandersen
Copy link
Contributor

Eh, maybe I missed something, but it looks like docutils puts the CSS class simple on lists that it judges candidates for compaction. So would the following fix the problem?

@mgeier
Copy link
Contributor

mgeier commented Jun 18, 2020

Since the CSS classes simple and compact have been mentioned, I've taken the liberty of showing how it would look like when respecting those: #7852

As I mentioned there, I think some of the CSS classes are assigned to the wrong lists. [UPDATE: I was wrong!]

Maybe it would be better to keep ignoring the simple and compact classes and remove <p> wrappers with a Transformer, as suggested in #7838 (comment)?

@mgeier
Copy link
Contributor

mgeier commented Jun 18, 2020

@jakobandersen

Set the open and compact classes on the lists that we always want to be something or the other (e.g., TOCs) (https://repo.or.cz/docutils.git/blob/HEAD:/docutils/docutils/writers/_html_base.py#l502).

I don't know if that uses the latest docutils version, but the demo has the simple class on the TOC generated by the contents directive:

https://docutils.sourceforge.io/docs/user/rst/demo.html#contents

However, the simple class seems to be only present in the top-level list and it seems to be missing in nested lists:

https://docutils.sourceforge.io/docs/user/rst/demo.html#table-of-contents

@mgeier
Copy link
Contributor

mgeier commented Jun 20, 2020

I found a pure CSS solution: #7852 (comment).

I mentioned above (#7838 (comment)):

I think some of the CSS classes are assigned to the wrong lists.

It looks like I was wrong in my assumptions about what exactly "compact" lists are.
It turns out the currently generated CSS classes work fine.

With my new changes to #7852 the TOC created with the contents directive is also shown correctly, the top-level simple class is sufficient.

AFAICT, everything works now!

@cjolowicz
Copy link
Author

Thanks for your work @mgeier! I can confirm that my project is rendered correctly with your changes. 🚀

@mitya57
Copy link
Contributor

mitya57 commented Jun 21, 2020

Set the open and compact classes on the lists that we always want to be something or the other (e.g., TOCs) (https://repo.or.cz/docutils.git/blob/HEAD:/docutils/docutils/writers/_html_base.py#l502).

FWIW, you can use .. rst-class:: open or .. rst-class:: compact before the lists to force one of these two docutils classes. (With pure docutils it is just .. class::, but in Sphinx that directive is renamed.)

Some time ago I filed readthedocs/sphinx_rtd_theme#354 to support these in Sphinx RTD theme, and support for that was added.

@mgeier
Copy link
Contributor

mgeier commented Jun 23, 2020

@mitya57 Thanks for the information, I didn't know that!

This currently doesn't work (with the HTML5 writer), neither did it before Sphinx 3.1.

With #7852, this will work again.

@tk0miya tk0miya closed this as completed Jul 4, 2020
mikeri pushed a commit to mikeri/searx that referenced this issue Mar 7, 2021
This reverts commit 0616684.

Since PR sphinx-doc/sphinx#7878 has been merged into
Spinx-doc (v3.1.2), this patch is no longer needed:

  See sphinx-doc project, PR 7838 & 7484 with elementary patch to the basic CSS:

  - sphinx-doc/sphinx#7838 (comment)
  - sphinx-doc/sphinx#7484 (comment)

Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants