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

Preserve whitespace inside one-backtick codeblocks #65613

Merged
merged 1 commit into from Nov 25, 2019

Conversation

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Oct 19, 2019

Previously this was only done inside short docblocks (e.g., summary
lines), but we should also do so in general.

Fixes #65555

Previously this was only done inside short docblocks (e.g., summary
lines), but we should also do so in general.
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Oct 19, 2019

Previously:

image

New behavior:
image

Note the lack of a leading space in the first code element.

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Oct 19, 2019

Looking at the diff, I'm not sure this is actually sufficient. The rendered HTML omitted the space in my original example, so CSS wouldn't change that.

@sinkuu

This comment has been minimized.

Copy link
Contributor

sinkuu commented Oct 20, 2019

The rendered HTML omitted the space in my original example

One you provided in the issue (https://github.com/time-rs/time) is rendered as:

<td><code>%_d</code> =&gt; <code> 5</code> instead of <code>05</code></td>

(Are you using Firefox's inspector? It seems to hide spaces from text nodes for some reason, until you double-click them.)

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Oct 20, 2019

Yes, I am; that would certainly explain it.

@JohnTitor

This comment has been minimized.

Copy link
Member

JohnTitor commented Oct 27, 2019

Ping from triage: @GuillaumeGomez could you review this?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 27, 2019

For inline code blocks, I think it'd be better to just keep the whitespace using &nbsp; instead of changing CSS. Did you try this and did you have issues?

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Nov 2, 2019

Ping from triage
@Mark-Simulacrum can you please address the comments from @GuillaumeGomez ?
thanks.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Nov 2, 2019

For inline code blocks, I think it'd be better to just keep the whitespace using   instead of changing CSS. Did you try this and did you have issues?

I expect this is rather hard to do and would be pretty bad for the size of the HTML we ship as we'd likely need to convert every space inside inline code blocks to &nbsp; which is a 6x increase in bytes :)

Why is the modification to CSS bad? Is there some downside to the existing PR I'm not seeing?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 2, 2019

By changing the CSS, you're changing the behavior for everyone. I personally don't like the result which is why I'd prefer to let users decide what they want.

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 2, 2019

@GuillaumeGomez Surely if a space is placed inside backticks, that's intentional? I can't think of a situation were someone would put a space there but expect it to not be shown.

By using a nonbreaking space, a user wouldn't be able to copy-paste it in more complex examples than mine.

@ollie27

This comment has been minimized.

Copy link
Contributor

ollie27 commented Nov 3, 2019

Is there some downside to the existing PR I'm not seeing?

Well one downside is that rustdoc would behave differently to other Markdown renders. For example GitHub renders:

foo ` 5` bar

as:

foo 5 bar

mdBook and dingus show the same.

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 3, 2019

Fwiw I just checked the GFM specification. A single leading and trailing space is to be stripped iff both are present. One-sided whitespace should not be stripped.

[ref]

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Nov 3, 2019

By changing the CSS, you're changing the behavior for everyone. I personally don't like the result which is why I'd prefer to let users decide what they want.

To be clear, you're advocating for not landing any change here, right? i.e., we would just accept that if you want leading spaces you'd literally write &nbsp; in your inline code blocks?

That seems fine to me, I guess, but I tend to agree with some others on this thread that I'd expect that we preserve spaces in code blocks, leading or not.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 4, 2019

Yes, I think this is the best solution.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Nov 4, 2019

I am my sure what "this" you refer to. If you think that we should not preserve white space ourselves, via CSS, then please close this PR and the associated issue, since we then don't want to make any change here.

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 4, 2019

@GuillaumeGomez Why not follow the spec that I linked? I get that the HTML is being rendered properly, but it's not being shown as I'd expect. Perhaps this issue/PR could be posted elsewhere for community feedback?

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 4, 2019

To be clear on my previous comment, the spec recommends the following CSS is used.

code {
  white-space: pre-wrap;
}
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 5, 2019

@Mark-Simulacrum Sorry, I meant keeping the current situation. So I'll close this PR and the linked issue with your approval. Thanks for taking a look into this anyway!

@jhpratt Because we follow the commonmark spec.

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 5, 2019

@GuillaumeGomez CommonMark says the exact same thing.

I think this should at least be brought up to the community rather than singlehandedly closing the issue and PR.

Unrelated, but CommonMark definitely isn't what's followed, as it doesn't even support tables.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 6, 2019

It allows extensions, which is what we do. Also, commonmark does it like we do: https://spec.commonmark.org/dingus/?text=%60%60%20foo%20%60%20bar%20%60%60%0A

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 6, 2019

@GuillaumeGomez That's a completely different example than the situation I originally mentioned. That one is explicitly provided for in the specification.

Trimming a leading space is unexpected, as there's not a matching trailing space. I understand that the HTML is being rendered correctly, and it's the CSS that needs to change. I just don't understand why rustdoc can't use the CSS snippet that the spec recommends? That's what this PR does.

I will also note that I've asked for community input multiple times now and received no response on your end.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 7, 2019

I will also note that I've asked for community input multiple times now and received no response on your end.

?

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 7, 2019

@GuillaumeGomez The issue and PR were singlehandedly closed by you, as your opinion is that I should use a nonbreaking space. You're the only one in either the issue or PR that has that opinion, at least having explicitly said so. I have said that this should be brought up with the community to see what most people think.

You originally dismissed my comment regarding the specification because I referenced the GitHub spec, not CommonMark. Since they both say the same thing (recommending the use of the above CSS snippet), why not use it? It's obviously recommended for a reason.

Saying that "commonmark does it like we do" with the linked example is extremely misleading, given that 1) you're only talking about the HTML rendering, not the actual display and 2) you're pulling an example straight from the spec that has an clear use case; I typed ` 5` expecting that the space would be shown.

Can you think of a situation where a leading space is present where the user would not expect the leading space to be shown (other than also having trailing space, which is explicitly provided for in the specification)? If not, there should be no harm in merging this, correct?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 8, 2019

And I'm telling you that we're following a spec. The spec discards the first whitespace for some reason but since we're following it, wether we agree with it or not, we have to follow it.

Anyway, if you feel like this should be brought up to the community, please do so. My opinion isn't absolute, I'm just the maintainer in here. But please also keep in mind that this change is a breaking one and that exceptions are what make a code base difficult to maintain.

@sinkuu

This comment has been minimized.

Copy link
Contributor

sinkuu commented Nov 8, 2019

Example 332 of CommonMark explicitly says not to remove space in this case, and rustdoc is following it correctly.

The stripping only happens if the space is on both sides of the string
` a` <p><code> a</code></p>

The discrepancy here is that rustdoc is not using the CSS recommended in CommonMark Spec (code{white-space: pre-wrap;}). The official commonmark.js demo is also not using this recommended CSS though.

@GuillaumeGomez GuillaumeGomez reopened this Nov 8, 2019
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 8, 2019

You convinced me. I'll need to check if we don't have side-effects but otherwise let's get it in.

@ollie27

This comment has been minimized.

Copy link
Contributor

ollie27 commented Nov 8, 2019

My main concern here is that rustdoc would be deviating from other Markdown renders which might cause confusion. Do any other Markdown renders use this CSS by default?

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Nov 16, 2019

Ping from triage - this has sat idle for 8 days
Can anyone answer the question from @ollie27 ?
Also - this pr looks like it's ready for review + merging, @GuillaumeGomez
cc: @Mark-Simulacrum

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 17, 2019

Waiting for an answer to @ollie27's question as well before going further.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Nov 17, 2019

I do not have the time to do a survey of other Markdown renderers. It also seems like it's not really about the Markdown rendering, but rather the associated stylesheets. OTOH, it looks like GitHub doesn't preserve the leading space at all in f, even just in the HTML.

I still feel like we should make this change as it's what I expect from code blocks (I consider GH behavior a bug). However, if we believe that a survey of other rendering pipelines would be helpful, then someone else will have to do that (maybe @jhpratt would be interested).

If this hits PR triage again we should close it.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 19, 2019

I'm more inclined into merging so if another triage shows up, just r=me.

I also confirmed that this change worked.

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 19, 2019

Will do. It's not a high priority for me, but I'll get around to possibly doing a rough survey eventually. It'll be renderers that go straight to webpages, though, as it involves CSS.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 20, 2019

Thanks @jhpratt ! :)

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Nov 24, 2019

Ping from triage, this has sat idle for a few days.
@GuillaumeGomez can you review+merge this PR?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 24, 2019

Still waiting for @jhpratt.

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 24, 2019

Given that this isn't a priority for me, triage can close this until I get around to it if it makes things easier for them.

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Nov 24, 2019

@jhpratt Ok, closing this, thanks.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 25, 2019

Ok so like I said, if no consensus is reached, then we merge. Thanks @Mark-Simulacrum !

@bors: r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 25, 2019

📌 Commit bc3ed32 has been approved by GuillaumeGomez

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 25, 2019
…, r=GuillaumeGomez

Preserve whitespace inside one-backtick codeblocks

Previously this was only done inside short docblocks (e.g., summary
lines), but we should also do so in general.

Fixes rust-lang#65555
bors added a commit that referenced this pull request Nov 25, 2019
Rollup of 7 pull requests

Successful merges:

 - #65613 (Preserve whitespace inside one-backtick codeblocks)
 - #66512 (Add unix::process::CommandExt::arg0)
 - #66569 (GitHub Actions: preparations, part 1)
 - #66678 (Remove useless line for error index generation)
 - #66684 (Drive-by cleanup in region naming)
 - #66694 (Add some comments to panic runtime)
 - #66698 (tidy: Remove unused import)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Nov 25, 2019
Rollup of 7 pull requests

Successful merges:

 - #65613 (Preserve whitespace inside one-backtick codeblocks)
 - #66512 (Add unix::process::CommandExt::arg0)
 - #66569 (GitHub Actions: preparations, part 1)
 - #66678 (Remove useless line for error index generation)
 - #66684 (Drive-by cleanup in region naming)
 - #66694 (Add some comments to panic runtime)
 - #66698 (tidy: Remove unused import)

Failed merges:

r? @ghost
@bors bors merged commit bc3ed32 into rust-lang:master Nov 25, 2019
6 checks passed
6 checks passed
pr Build #20191108.58 succeeded
Details
pr #20191125.18 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.