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

The source code typography is broken #378

Open
Songbird0 opened this issue Jul 17, 2017 · 19 comments
Open

The source code typography is broken #378

Songbird0 opened this issue Jul 17, 2017 · 19 comments
Labels
A-Style Area: Style (CSS, etc.) C-papercut Category: A small usability bug

Comments

@Songbird0
Copy link

Songbird0 commented Jul 17, 2017

Hi,

I'm translating RBE from English to French so I've copied this source code to translate its comments:

// La création et destruction des lifetimes sont illustrées ci-dessous par des lignes.
// `i` possède la plus grande lifetime car son contexte englobe 
// `borrow1` et `borrow2`. La durée de vie de `borrow1` ne peut pas être 
// comparée à celle de `borrow2` car elle ne se trouve pas dans le 
// même contexte.
fn main() {
    let i = 3; // Lifetime for `i` starts. ────────────────┐
    //                                                     │
    { //                                                   │
        let borrow1 = &i; // `borrow1` lifetime starts. ──┐│
        //                                                ││
        println!("borrow1: {}", borrow1); //              ││
    } // `borrow1 ends. ──────────────────────────────────┘│
    //                                                     │
    //                                                     │
    { //                                                   │
        let borrow2 = &i; // `borrow2` lifetime starts. ──┐│
        //                                                ││
        println!("borrow2: {}", borrow2); //              ││
    } // `borrow2` ends. ─────────────────────────────────┘│
    //                                                     │
}   // Lifetime ends. ─────────────────────────────────────┘

I preserved the originals comments not to break typography. When I put this code in {{#playpen source/foo.rs}}, the server displays:

image

How to fix it ?

Regards.

@budziq
Copy link
Contributor

budziq commented Jul 17, 2017

Hi @Songbird0 as far as I know RBE is not yet ported to mdbook (it uses a customized gitbook). And if the snippet you have pasted your comment is any indicator it looks like the typography is already broken and the text gets misaligned on monospaced font (possibly the RBE gitbook uses non monospaced font and the snippet was hand edited to make it align.) I was able to realign it by hand reasonably easily in github editor (mdbook renderer will give similar results as the github's pre block)

Unfortunately I'll not be able to help during the next two weeks as I'm on holidays mobile only.

cc @mdinger @steveklabnik

@steveklabnik
Copy link
Member

Yes, RBE is not yet using mdBook, see rust-lang/rust-by-example#248

@Songbird0
Copy link
Author

Hi,

Thanks all for answering.

Hi @Songbird0 as far as I know RBE is not yet ported to mdbook

Yes, RBE is not yet using mdBook

Yes, I know, but it will be ported and you'll find the same problem certainly, so I'd like to solve it on my own.

And if the snippet you have pasted your comment is any indicator it looms like the typography is broken already and the text gets misaligned on monospaced font (possibly the RBE gitbook uses non monospaced font and the snippet was hand edited to make it align.)

I'm not sure to understand. I cannot hand edit a snippet rendered by mdbook. :/

@budziq
Copy link
Contributor

budziq commented Jul 17, 2017

I'm not sure to understand. I cannot hand edit a snippet rendered by mdbook. :/

What I meant is that most likely the foo.rs that you are including has already misaligned comments ( the code that you included in your comment certainly does) and as a result it does not align on the final rendering by mdbook. If I'm correct then the problem should be corrected in the foo.rs instead of mdbook. You can check if the code is properly aligned by viewing it in an editor and choosing a monospaced font like consolas/menlo (google for the one typical for your OS and editor)

I'd like to have a look on the original file that you are trying to render (foo.rs) to be sure. Could you attach it to this bug report? But as stated before I'll might not be able to help right now.

@mdinger
Copy link
Contributor

mdinger commented Jul 17, 2017

The page alignment doesn't appear to render correctly on mobile. I don't know the reason for that behavior but it should be well behaved in most normal desktop text editors. It looks like mdbook use pulldown-commonmark as the renderer. You should use a small testcase of the problem, such as something like this and run it though the markdown renderer and see if the correct HTML is generated. If it is not, submit it as an issue in the cmark renderer. If it is not the renderer, then the problem is elsewhere:

────┐
//  │

@Songbird0
Copy link
Author

Songbird0 commented Jul 18, 2017

@budziq:

If I'm correct then the problem should be corrected in the foo.rs instead of mdbook. You can check if the code is properly aligned by viewing it in an editor and choosing a monospaced font like consolas/menlo (google for the one typical for your OS and editor)

Ok, I tried with Sublime Text 3.

font_face: Consolas

Result (in Sublime Text 3):

image

Refresh... Result (in my Firefox browser):

image
Same thing.

Note(!): When JavaScript and fonts are disabled on my browser, the typography isn't broken, see:

image

If I allow maxcdn.bootstrapcdn.com fonts, the problem reappears:

image

I'd like to have a look on the original file that you are trying to render (foo.rs) to be sure. Could you attach it to this bug report?

Done ! -> brokentypo.zip

But as stated before I'll might not be able to help right now.

No problem, I'm not in hurry.

@mdinger:

It looks like mdbook use pulldown-commonmark as the renderer. You should use a small testcase of the problem, such as something like this and run it though the markdown renderer and see if the correct HTML is generated.

I track their repository. Thanks again for your help.

@mdinger
Copy link
Contributor

mdinger commented Jul 18, 2017

It looks correct on my computer in firefox. Did you try a fresh profile? It might not be markdown rendering in that case. It might be a font related issue or something else.

@budziq
Copy link
Contributor

budziq commented Jul 18, 2017

@Songbird0 I've looked on the attached file with mobile text editor and the lines match perfectly on "DejaVu Sans Mono" font and are completely broken on any other (monospaced or not) so this has nothing to do with renderer and most likely the default pre font on your browser (and the one used by RBE gitbook variant) happens to be DejaVu or compatible while the downloaded google fonts from cdn are not. Unfortunately I cannot say more atm.

I guess I would prefer not to modify mdbook fontset just to cater for this particular comment formatting (lifetime lint lines from rustc work ok on all monospaced fonts that I've tested sone time ago)

@Songbird0
Copy link
Author

Songbird0 commented Jul 18, 2017

@budziq & @mdinger:

I guess I would prefer not to modify mdbook fontset just to cater for this particular comment formatting (lifetime lint lines from rustc work ok on all monospaced fonts that I've tested sone time ago)

It won't be necessary.

When "Allow pages to choose their own fonts, instead of my selections above" is unchecked, all is fine. Here's my configuration (works with other fonts too):

image

->
image

Firefox fonts are used instead of maxcdn.bootstrapcdn.com. However, with these options, the Ayu theme typography isn't perfect:
image

Not a problem for me. :)

@budziq
Copy link
Contributor

budziq commented Jul 18, 2017

@Songbird0 unfortunately requiring readers to change their browser settings for the webpage to render properly is a nonsolution 😞 . I'll look into the problem further when I'm back behind proper keyboard.

@Songbird0
Copy link
Author

unfortunately requiring readers to change their browser settings for the webpage to render properly is a nonsolution 😞 . I'll look into the problem further when I'm back behind proper keyboard.

All right. See you soon! :)

@budziq
Copy link
Contributor

budziq commented Jul 31, 2017

Hi I'had a cursory look over the problem and Im not able to reproduce this issue on my Arch Linux Firefox (renders ideally) and chromium (also looks very good but not as pixel perfect).

On the other hand it looks that RBE's typography on android chromium is similarly broken.

I have to admit that I'm not a webdev and someone with cross browser webdev experience would have to look at this issue. Some google webfonts magic might help here.

@azerupi
Copy link
Contributor

azerupi commented Aug 1, 2017

With the source code pro font, that is set as default, I am experiencing a very slight misalignment of the vertical bars. But nowhere near as much as in the screenshots provided above. If I remove the source code pro font alternative from the CSS it renders perfectly.

@Songbird0 when you have some time, could you try to fiddle with the fonts on the rendered page to see if other fonts work better? You can change them on the fly using the developers tools in your browser.

@Songbird0
Copy link
Author

Hello there!

With the source code pro font:

image

Without the source code pro font:

image

My page is properly rendered.

Ayu theme:

image

Correctly rendered too.

@azerupi
Copy link
Contributor

azerupi commented Aug 1, 2017

Thanks!
I am not sure why there is a problem, but clearly Source code pro seems to be the culprit.

This font was introduced in PR #250, maybe we should remove it from the list?

@budziq
Copy link
Contributor

budziq commented Aug 1, 2017

Hmm the Source code pro seams to do exactly the opposite of what was advertised in #250. And what is the behaviour in mobile browser if we remove Source code pro from the list?

@azerupi
Copy link
Contributor

azerupi commented Aug 1, 2017

And what is the behaviour in mobile browser if we remove Source code pro from the list?

Unsure, but the fallback stack should be sufficiently exhaustive to have at least one monospace font available on mobile?

@budziq budziq added A-Style Area: Style (CSS, etc.) C-papercut Category: A small usability bug labels Aug 1, 2017
@budziq
Copy link
Contributor

budziq commented Aug 1, 2017

I've looked into android mobile chromium. It looks like like only Source Code Pro and monospace are the only ones recognized in the installed list (both being equally broken).
Also I've checked all monospace fonts on https://fonts.google.com and unfortunately all looked as bad on android chromium with the RBE snippet.

@AndreKR
Copy link

AndreKR commented Mar 22, 2020

The problem currently still exists on https://doc.rust-lang.org/stable/.

See for example https://doc.rust-lang.org/stable/rust-by-example/scope/lifetime.html.
image

The problem is that while the font itself (Source Code Pro Medium) does include the box drawing characters (U+2500-259F), the hosted version via https://fonts.googleapis.com/css?family=Source+Code+Pro:500 does not. The browser then falls back to a different font with different tracking, Consolas in my case.

I see three possible ways to fix this:

  1. Self-host the whole font (this would also resolve Remove Google surveillance #847)
  2. Self-host a subset with just the line-drawing characters
  3. Fall back to a locally installed font. This would solve the problem at least for those people that have Source Code Pro Medium installed on their system.

I think 1 is probably the best solution, but I see that #848 wasn't going forward, so maybe 2 or 3 might be worth pursuing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Style Area: Style (CSS, etc.) C-papercut Category: A small usability bug
Projects
None yet
Development

No branches or pull requests

6 participants