New issue
Advanced search Search tips

Issue 736199 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

md_browser does not render nested lists correctly

Project Member Reported by satorux@chromium.org, Jun 23 2017

Issue description



What steps will reproduce the problem?
(1) tools/md_browser/md_browser.py -d docs
(2) wget http://localhost:8080/android_studio.md -O out.html
(3) % grep 'The wizard' out.html 

What is the expected result?

Nested lists are rendered correctly.

What happens instead?

Nested lists are not rendered correctly.

Please use labels and text to provide additional information.


android_studio.md has nested lists like this one:

* Avoid running the setup wizard.
    * The wizard will force you to download unwanted SDK components to
      `//third_party/android_tools`.
    * To skip it, select "Cancel" when it comes up.

In Gitiles, nested lists are rendered correctly: https://chromium.googlesource.com/chromium/src/+/master/docs/android_studio.md

In md_browser, second level items are enclosed in <pre><code>...</code></pre>:

<ul>
<li>Avoid running the setup wizard.<pre><code>* The wizard will force you to download unwanted SDK components to
  `//third_party/android_tools`.
* To skip it, select "Cancel" when it comes up.
</code></pre>
</li>
</ul>

 
dpranke@, could you take a look? If it's not easy to fix, would be good to document as a known issue at
https://chromium.googlesource.com/chromium/src/+/master/tools/md_browser/README.md
I was hit by this too. I'm not using gitiles to preview markdown, which is pretty painful.

Comment 3 by derat@chromium.org, Jul 22 2017

Cc: vapier@chromium.org
Mike, your https://codereview.chromium.org/2772173002 changed the markdown.Markdown() call in src/tools/md_browser/md_browser.py to pass tab_length=2. Per discussion on https://github.com/waylan/Python-Markdown/issues/3, this argument determines the amount of indenting needed for nested lists. When I pass 4 (the default) instead, nested lists using 4-space indent are interpreted correctly.

Google's Markdown style guide says to use 4-space indent for nested lists: https://github.com/google/styleguide/blob/gh-pages/docguide/style.md#nested-list-spacing. This is what we use throughout Chrome and Chrome OS repositories.

The Gitiles doc referenced by your change (https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md#Lists) also uses 4 (and then 5!)-space indent for nested lists, but then goes on to use 2-space indent for paragraphs embedded in a single-level list:

---

1. Fruit

  In botany, a fruit is the seed-bearing structure in flowering
  plants.

  In common language usage, "fruit" normally means the fleshy
  seed-associated structures of a plant that are sweet or sour, and
  edible in the raw state, such as apples, bananas, grapes, lemons,
  oranges, and strawberries.

2. Cake

  The cake is a lie.

---

This diverges from the "spec" at https://daringfireball.net/projects/markdown/syntax#list: "List items may consist of multiple paragraphs. Each subsequent paragraph in a list item must be indented by either 4 spaces or one tab".

As I understand it, md_browser is intended to be used to preview files that will later be rendered by Gitiles, so it should match Gitiles's behavior as closely as possible.

I don't think that md_browser can match it exactly, because I don't think there's any way to get Python-Markdown to use Gitiles's weird "at least 2 leading spaces" logic for embedded paragraphs. But!! Gitiles's docs appear to be wrong: when I use the exact text from their example, the paragraph isn't embedded (gitiles_docs_incorrect.png).

So my thinking is that we should switch md_browser to use tab_length=4 and then continue telling people to write their docs like this:

---

*   List item

    Here's an embedded paragraph.
    It can be more than one line.

    *    Nested list item

         More nested text.

---

This lines up intuitively, and follows the Google style guide, and is rendered the same in md_browser (with tab_length=4) and Gitiles, as far as I can tell.

I'm attaching some more screenshots that may help illustrate the differences.
gitiles_docs_incorrect.png
27.4 KB View Download
md_browser_tab_length_2.png
35.3 KB View Download
md_browser_tab_length_4.png
34.7 KB View Download
gitiles.png
36.3 KB View Download

Comment 4 by derat@chromium.org, Jul 22 2017

Grepping through *.md in the chromium/src repo, there are examples of people using basically any style, because hey, Markdown. :-/ I see:

* Here's a multi-line
  bullet point.

and:

*   Here's a multi-line
    bullet point.

and even:

* Here's a multi-line
bullet point.

https://chromium.googlesource.com/chromium/src/+/master/docs/README.md says that docs must follow the Google style guide. My reading of https://github.com/google/styleguide/blob/gh-pages/docguide/style.md#lists is that 4-space ident should be used everywhere, with an exception carved off for trivial single-line, non-nested lists:

"When nesting lists, use a 4 space indent for both numbered and bulleted lists ... Even when there's no nesting, using the 4 space indent makes layout consistent for wrapped text ... However, when lists are small, not nested, and a single line, one space can suffice for both kinds of lists."

My preference is still to make md_browser use 4-space indent, since it'll steer people toward doing what they should probably be doing in any case.
Cc: yutak@chromium.org
Owner: ----
Status: Available (was: Untriaged)
I'm not sure when I'll get to this; marking it available in case someone else wants to look into what to do here ...

Comment 6 by derat@chromium.org, Sep 2 2017

Per #4, does anyone object to me making md_browser go back to using 4-space indents?

Comment 7 by derat@chromium.org, Sep 5 2017

Owner: derat@chromium.org
Status: Started (was: Available)
The approval for this is deafening, so going ahead.
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db0868eb17e4f5a0ca928a14024f6ee8c061c0e8

commit db0868eb17e4f5a0ca928a14024f6ee8c061c0e8
Author: Daniel Erat <derat@chromium.org>
Date: Fri Oct 20 05:30:39 2017

md_browser: Switch to 4-space indent.

The Google Markdown style guide advocates 4-space indent[1].
When configured to use 2-space indent (what we were using
before), md_browser doesn't render 4-space-indented nested
lists or wrapped text correctly. Gitiles (used by Gerrit and
chromium.googlesource.com to render Markdown) also wants
4-space indent rather than 2-space indent.

1. https://github.com/google/styleguide/blob/gh-pages/docguide/style.md#nested-list-spacing

Bug:  736199 
Change-Id: Icf4aa298f893672b6a7718aa1dc3e300f6559eff
Reviewed-on: https://chromium-review.googlesource.com/729749
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510349}
[modify] https://crrev.com/db0868eb17e4f5a0ca928a14024f6ee8c061c0e8/tools/md_browser/md_browser.py

Comment 9 by derat@chromium.org, Oct 20 2017

Status: Fixed (was: Started)

Sign in to add a comment