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

Style: Reduce size of code font and improve responsiveness at narrow screen widths #2439

Merged
merged 13 commits into from
Mar 22, 2022

Conversation

CAM-Gerlach
Copy link
Member

@CAM-Gerlach CAM-Gerlach commented Mar 17, 2022

Right now, the code font is much larger than the prose font, which varies by platform (due to the former using a system font stack, while the latter uses a web font). As a stopgap solution, to ensure 79 characters fit on a line under all but the most constrained screen resolutions, and reduce the perceptible size difference between the prose and code fonts while making it more motivated, this PR follows @pradyunsg 's suggestion reduce the code font size to something similar to what Furo uses, 0.875rem/14px as was suggested and discussed there, until we decide on a final resolution to the font stack issue.

Furthermore, as also suggested by @methane on #2347 , it allows code blocks to break by letter than than by word, resolving formatting issues at narrow screen widths and matching the behavior of most editors/IDEs (while ensuring inline code breaks cleanly).

Resolves #2437
Resolves #2449

@CAM-Gerlach CAM-Gerlach added bug infra Core infrastructure for building and rendering PEPs labels Mar 17, 2022
@CAM-Gerlach CAM-Gerlach requested review from methane and hugovk March 17, 2022 01:31
@CAM-Gerlach CAM-Gerlach requested a review from AA-Turner as a code owner March 17, 2022 01:31
@CAM-Gerlach CAM-Gerlach self-assigned this Mar 17, 2022
@CAM-Gerlach CAM-Gerlach changed the title Style: Reduce size of code font to better match prose and fit 79 char Style: Reduce size of code font to match prose and break by letter in code blocks Mar 17, 2022
@CAM-Gerlach CAM-Gerlach changed the title Style: Reduce size of code font to match prose and break by letter in code blocks Style: Reduce size of code font to match prose and break by letter Mar 17, 2022
@CAM-Gerlach CAM-Gerlach changed the title Style: Reduce size of code font to match prose and break by letter Style: Reduce size of code block font to match prose and break by letter Mar 17, 2022
@CAM-Gerlach CAM-Gerlach force-pushed the style-fix-code-font-size branch from c8b95cc to e50d0d7 Compare March 17, 2022 01:42
Copy link
Member

@methane methane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK on macOS and Pixel 3.

@CAM-Gerlach CAM-Gerlach changed the title Style: Reduce size of code block font to match prose and break by letter Style: Reduce size of code font and improve responsiveness at narrow screen widths Mar 17, 2022
@CAM-Gerlach CAM-Gerlach marked this pull request as draft March 17, 2022 03:55
@CAM-Gerlach CAM-Gerlach marked this pull request as ready for review March 17, 2022 04:41
@CAM-Gerlach
Copy link
Member Author

I've made some further improvements here based on additional feedback on #2437 . See my comment there for more details.

@pradyunsg
Copy link
Member

pradyunsg commented Mar 17, 2022

We might want to reconsider making the table font smaller as the screen width reduces -- it makes the text a lot more difficult to read.

screenshot

@hugovk
Copy link
Member

hugovk commented Mar 17, 2022

I agree, changes here generally look good but let's not make the table text smaller, it makes is harder to read. If anything, we should be increasing the font size and whitespace on mobile.

Plus smaller text doesn't help for wider tables.

For example, the wide table in PEP 594 still affects the whole zoom on the page (Android/Samsung S10/Chrome):

There is a whole bunch of things that can be done to make tables responsive and accessible, but:

... the simplest way to make a table responsive is to put it in a container that will scroll. This way it does not break your design by pushing the layout and content past the viewport edges.

As a quick example, Boostrap does this too:

Responsive tables allow tables to be scrolled horizontally with ease.

Can we do the same?

@pradyunsg
Copy link
Member

pradyunsg commented Mar 17, 2022

That requires rewriting HTML that is generated by Sphinx (or doing a transform within Sphinx) to wrap the table tag in a div. There's code for that in various Sphinx themes that can be borrowed for this (for Furo's code which reparses the generated HTML, see https://github.com/pradyunsg/furo/blob/812935eee54aeec3ae964647c37f2f18d930c9ec/src/furo/__init__.py#L42).

@pradyunsg
Copy link
Member

Or, that's the only way that I know of. If you know of a better way, please say so here. I'd be more than happy to eliminate an HTML parse of potentially long documents from Furo's rendering step. :)

@hugovk
Copy link
Member

hugovk commented Mar 19, 2022

That requires rewriting HTML that is generated by Sphinx (or doing a transform within Sphinx) to wrap the table tag in a div. There's code for that in various Sphinx themes that can be borrowed for this (for Furo's code which reparses the generated HTML, see pradyunsg/furo@812935e/src/furo/init.py#L42).

Thanks. I agree something like that is probably the way to go (ideally using our existing HTML translation extensions rather than via post-build hacks). However, I'm not sure if that's really a good idea to implement in this PR focused just on straightforward CSS style tweaks to fix the immediate issues, since it is kinda hacky, entails significant code changes to the rendering system, and requires adding at least one new top-level dependency (BeautifulSoup).

Short term: if you want this PR to be CSS only, shall we remove the table shrinking CSS from this PR and do the Furo trick in another PR?

Medium term: is it worth asking Sphinx to wrap tables in a div? That would benefit us, Furo users and other Sphinx users.

Longer term: when Adam's back we can discuss further improvements in more depth.

@pradyunsg
Copy link
Member

pradyunsg commented Mar 19, 2022

Medium term: is it worth asking Sphinx to wrap tables in a div? That would benefit us, Furo users and other Sphinx users.

I think it is. I'm not quite sure how Sphinx handles markup changes under their somewhat-strict backwards compatibility model -- but yes, we should totally ask them! I adapted that from what sphinx-rtd-theme is doing so... this would be fairly widely useful. :)

@CAM-Gerlach
Copy link
Member Author

Short term: if you want this PR to be CSS only, shall we remove the table shrinking CSS from this PR and do the Furo trick in another PR?

Well, if we do that, it won't fix the primary issue reported in #2437 , which motivated this PR in the first place, and which the users who reported the issue and tested this seemed to be happy with. As I mentioned above,

I realize shrinking the table font is not ideal as it decreases readability while not scaling all the way to the narrowest widths, but ultimately potentially requiring some users to zoom in on a specific table is ultimately a lot less disruptive than requiring all users zoom out or scroll to see all of the rest of the page content, right?

It seems to be at least a modest net improvement for the majority of cases for the time being until a followup PR with a longer-term solution is implemented, particularly if we defer the Furo hack, since it will require an extra round of discussion, implementation in and testing to hack a kludge in, as well as additional dependencies, that will almost certainly get ripped back out again when the earlier of A. Adam gets back in a month and we implement it in the HTML translator, or B. Sphinx wraps the table in a div.

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Mar 19, 2022

Or instead, how about as a lighter-weight stopgap, we just wrap body tables in a <div> using a JS snippit? This should achieve the same result as the Furo hack while avoiding most of the costs, as it doesn't require changing the build system, pulling in extra deps and hacking the raw HTML. I'm pretty sure that's how I've handled similar situations to this in the past, IIRC, and It should be simple enough that I could just do it in this PR.

@hugovk
Copy link
Member

hugovk commented Mar 20, 2022

Okay, let's try some JavaScript. In general, I think it's better to fix the HTML itself, for those without JavaScript, but hopefully this a temporary thing!

@CAM-Gerlach
Copy link
Member Author

Yeah, better to fix the HTML on the Sphinx side, but indeed this JS will only be a temporary solution.

In any case, I've implemented it as described above (via JS using overflow-x) and ditched the previous font scaling approach.

for those without JavaScript,

Well, FWIW, considering 98% of websites use JS, only around 0.2% of users in 2016 had it disabled at some level, and this is only a non-essential usability enhancement, I'm not sure how compelling that reason is, particularly considering over 10 times that (1.5-3.6%) have browsers ancient enough that don't support the vw units currently used in the pure-CSS solution, and similar for other features of the existing CSS and JS.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thank you!

One comment here.

And I found something else needing a break, but I'll raise that separately so as not to delay this any more.

@@ -177,6 +177,11 @@ sup {top: -0.5em}
sub {bottom: -0.25em}

/* Table rules */
div.table-wrapper {
overflow-x: scroll;
scrollbar-width: thin; /* CSS Standards, not *yet* widely supported */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, only supported by Firefox: https://caniuse.com/?search=scrollbar-width

MDN also notes accessibility concerns, scrollbar-width: thin "can make content hard or impossible to scroll if the author does not provide an alternative scrolling mechanism": https://developer.mozilla.org/en-US/docs/Web/CSS/scrollbar-width#accessibility_concerns

As we're doing this partly for accessibility shall we skip this one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wanted to be consistent with the other scroll bars (e.g. for the sidebar), but that also uses the illegal, bespoke pseudo-elements to support WebKit/Blink-based browsers, so you're right—best to just eliminate this, fall back on the device-default scrollbars and use overflow-x: auto instead to automatically hide it when not needed instead of a transparent overlay. I also fixed several other additional existing responsiveness issues I spotted in further testing for this.

@CAM-Gerlach CAM-Gerlach force-pushed the style-fix-code-font-size branch from 8d2fcab to 9a0d8f8 Compare March 21, 2022 21:30
@CAM-Gerlach
Copy link
Member Author

I eliminated the poorly-supported, potentially inaccessible scroll bar tweaks as @hugovk suggested, and switched it to auto mode instead to hide and show the scroll bar on demand. I also fixed a couple more outstanding layout/responsiveness issues I spotted in further testing this, including:

  • Allowing breaking by letter in footnotes to avoid them also causing scrolling and sub-par layouts at very narrow widths, due to long URLs
  • Fixing a significant issue (currently present in production) where the sidebar was displayed below all of the content rather than hidden entirely at moderately narrow widths (when we already have it under the Contents dropdown at the top)
  • Adjusting the sidebar breakpoint to still show it for larger tablets, and modern phones in landscape mode, where there's room for it and it can ease navigation, while moving some of the other responsive tweaks up a little further to fall cleanly between these device classes
  • Eliminating the other instance of illegal scrollbar hacks that are browser dependent, poorly supported and harm accessibility, to be consistent with the requested changes to the table scrollbar above,

@CAM-Gerlach CAM-Gerlach requested a review from hugovk March 21, 2022 21:31
@hugovk
Copy link
Member

hugovk commented Mar 22, 2022

Looking good!

  • Allowing breaking by letter in footnotes to avoid them also causing scrolling and sub-par layouts at very narrow widths, due to long URLs

Nice, this was the other thing I was going to bring up :)

Breaking by word works nicely for URLs too (break-all left, break-word right):

image

https://pep-previews--2439.org.readthedocs.build/pep-0594/

And some PEPs have a lot of footnote text where break-all is inappropriate (again break-all left, break-word right):

image

https://pep-previews--2439.org.readthedocs.build/pep-0465/

The same PEP also has some long URLs in the body, causing a pesky horizontal scroll. Left is PR, right is with word-break: break-word on <body>:

image

@CAM-Gerlach
Copy link
Member Author

Hmm, I see. To note, I couldn't reproduce the issue in your screenshots (at least on FF on Windows), except at much narrower widths, because by default links were breaking on /s for me. Furthermore, applying said solution also caused the footnote numbers to render very poorly, one character to a line. What's more, work-break: break-word is deprecated, and only supported in certain browsers, and so is the word-wrap property, which was previously used for this.

To resolve all this, I used overflow-wrap: anywhere (with a fallback to overflow-wrap: break-word for the usual suspect, Webkit, that doesn't support modern standards), which achieves similar results to word-break: break-word in a modern, standards-conformant way. This ensures that breaks are only used when they are truly needed to prevent an overflow, and inline lines aren't broken arbitrarily, while still covering all links. Also, I fixed a padding issue that avoided the breaks in the footnote numbers. This should solve the problem once and for all, at least at all modern device widths.

(To note, neither of these cases (long links in the body, nor footnotes in general) would be a real problem for new or updated PEPs that followed the current guidance proscribing bare and footnote links in favor of inline links with proper display text, and this rule would be unnecessary.)

@hugovk
Copy link
Member

hugovk commented Mar 22, 2022

Thanks for all this, I think we're good now!


I should have mentioned my screenshots were Chrome/macOS.

Yep, good idea to use overwrap: anywhere instead of deprecated workd-break: break-word. MDN suggests that as a replacement.

Here's https://pep-previews--2439.org.readthedocs.build/pep-0465/ on Chrome with mobile layout.

Chrome

image

Firefox

image

Safari

image


To resolve all this, I used overflow-wrap: anywhere (with a fallback to overflow-wrap: break-word for the usual suspect, Webkit, that doesn't support modern standards), which achieves similar results to word-break: break-word in a modern, standards-conformant way. This ensures that breaks are only used when they are truly needed to prevent an overflow, and inline lines aren't broken arbitrarily, while still covering all links.

You'll see from the inspector that all three show:

overflow-wrap: break-word;
overflow-wrap: anywhere;

https://caniuse.com/wordwrap shows widespread support. Anyway, fine to keep both if you wish :)

@hugovk
Copy link
Member

hugovk commented Mar 22, 2022

I'll merge this now, thanks again!

@hugovk hugovk merged commit f30bc4c into python:main Mar 22, 2022
@CAM-Gerlach
Copy link
Member Author

Hmm, CanIUse, using data from on MDN, claims that Safari doesn't have support for overflow-wrap: anywhere, but it seems that may be out of date, at least in recent versions. Either way, since it isn't non-standard, I don't think it hurts to keep it as a fallback for now.

@encukou
Copy link
Member

encukou commented Mar 23, 2022

Wrapping makes the # These lines are... comments in PEP 678 unreadable on smaller screens.

@hugovk
Copy link
Member

hugovk commented Mar 23, 2022

Desktop:

image

Mobile:

image

(Chrome/macOS)

Instead of wrapping, perhaps we should do the horizontal scroll thing on code, like with tables?

GitHub does horizontal scroll for code (files, diffs, and in READMEs etc).

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Mar 23, 2022

Thanks; that makes more sense (at least for the great majority of literal blocks), and I'm not sure why I didn't think of it before. To note, readability is further impaired by that block not having the correct language tag applied, so syntax highlighting doesn't work; I'll go through and fix that separately in a PR for that purpose.

I've opened #2463 which implements the suggested solution, using overflow-x: auto instead of overflow-wrap: anywhere on pre blocks, and switching them from white-space: pre-wrap to white-space: pre.

@hugovk
Copy link
Member

hugovk commented Mar 25, 2022

#2463 is merged! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug CLA signed infra Core infrastructure for building and rendering PEPs
Projects
None yet
6 participants