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

Fix table in docblocks #88742

Merged
merged 2 commits into from
Sep 11, 2021
Merged

Conversation

GuillaumeGomez
Copy link
Member

"Overwrite" of #88702.

Instead of adding a z-index to the sidebar (which only hides the issue, doesn't fix it), I wrap <table> elements inside a <div> and limit all chidren of .docblock elements' width to prevent having the scrollbar on the whole doc block.

Screenshot from 2021-09-08 15-11-24

Thanks @nbdd0121 for overflow-x: auto;. ;)

r? @notriddle

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Sep 8, 2021
@GuillaumeGomez GuillaumeGomez added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2021
@@ -522,6 +522,11 @@ nav.sub {
position: relative;
}

.docblock > * {
max-width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is actually unnecessary. According to the MDN, overflow: auto should introduce new block formatting context by itself. So if the element this applies to is a block element, then max-width: 100% is not needed. If the element this applies to is an inline element, then max-width is not useful because it does not apply to the non-replaced inline elements.

So I think we just need

.docblock > * {
	overflow-x: auto;
}

Or maybe

.docblock > * {
	display: block;
	overflow-x: auto;
}

if we want to be absolutely sure, but this is likely not necessary since inline elements should be wrapped with <p></p> already.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it for images actually. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't images be wrapped within <p></p>?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not always, for example:

//! <img src="whatever.png">
//!
//! ![alt text](https://github.com/adam-p/markdown-here/raw/master/src/common/images/icon48.png "Logo Title Text 1")

First one isn't but the second one is. The beauty of markdown. :D

@GuillaumeGomez
Copy link
Member Author

Let's move on then. Thanks for the @nbdd0121 ! (if there is anything else, please tell me so I r-).

@bors: r=nbdd0121 rollup

@bors
Copy link
Contributor

bors commented Sep 9, 2021

📌 Commit 021b8ff has been approved by nbdd0121

@bors
Copy link
Contributor

bors commented Sep 9, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2021
…arth

Rollup of 15 pull requests

Successful merges:

 - rust-lang#85200 (Ignore derived Clone and Debug implementations during dead code analysis)
 - rust-lang#86165 (Add proc_macro::Span::{before, after}.)
 - rust-lang#87088 (Fix stray notes when the source code is not available)
 - rust-lang#87441 (Emit suggestion when passing byte literal to format macro)
 - rust-lang#88546 (Emit proper errors when on missing closure braces)
 - rust-lang#88578 (fix(rustc): suggest `items` be borrowed in `for i in items[x..]`)
 - rust-lang#88632 (Fix issues with Markdown summary options)
 - rust-lang#88639 (rustdoc: Fix ICE with `doc(hidden)` on tuple variant fields)
 - rust-lang#88667 (Tweak `write_fmt` doc.)
 - rust-lang#88720 (Rustdoc coverage fields count)
 - rust-lang#88732 (RustWrapper: avoid deleted unclear attribute methods)
 - rust-lang#88742 (Fix table in docblocks)
 - rust-lang#88776 (Workaround blink/chromium grid layout limitation of 1000 rows)
 - rust-lang#88807 (Fix typo in docs for iterators)
 - rust-lang#88812 (Fix typo `option` -> `options`.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 130e2e1 into rust-lang:master Sep 11, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 11, 2021
@GuillaumeGomez GuillaumeGomez deleted the fix-table-in-docblocks branch September 11, 2021 09:35
notriddle added a commit to notriddle/rust that referenced this pull request Dec 8, 2022
* The rule `display: block` had no noticeable effect. Technically, because
  markdown tables have a tbody and thead, they get wrapped in an [anonymous
  table box] in the CSS tree, nested within the `<table>` element's block
  layout box.

  This rule was added in rust-lang#87230 to make the table side-scrolling, but
  this same issue was doubly fixed in rust-lang#88742 by wrapping it in an explicit
  `<div>` tag. Since accessibility advocates recommend the wrapper div over
  marking the table as `display: block`, we'll stick with that.

  https://adrianroselli.com/2020/11/under-engineered-responsive-tables.html

* The rule `width: calc(100% - 2px)` had no visible effect, because the
  anonymous table box was not affected.

* The style is tweaked to basically be the same style GitHub uses.
  In particular, it adds zebra stripes, and removes dotted borders.

[anonymous table box]: https://www.w3.org/TR/CSS2/tables.html#anonymous-boxes
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 9, 2022
…-css, r=GuillaumeGomez

rustdoc: clean up docblock table CSS

# Preview

http://notriddle.com/notriddle-rustdoc-demos/table-2/test_dingus/fn.test.html

# Before

![image](https://user-images.githubusercontent.com/1593513/206364287-1b80eaaf-2e0e-4138-8b56-4aa8ff39abac.png)

# After

![image](https://user-images.githubusercontent.com/1593513/206364209-d287d165-31be-4de1-9b43-05b35ce2a86b.png)

# Details

* The rule `display: block` had no noticeable effect. Technically, because markdown tables have a tbody and thead, they get wrapped in an [anonymous table box] in the CSS tree, nested within the `<table>` element's block layout box.

  This rule was added in rust-lang#87230 to make the table side-scrolling, but this same issue was doubly fixed in rust-lang#88742 by wrapping it in an explicit `<div>` tag. Since accessibility advocates recommend the wrapper div over marking the table as `display: block`, we'll stick with that.

  https://adrianroselli.com/2020/11/under-engineered-responsive-tables.html

* The rule `width: calc(100% - 2px)` had no visible effect, because the anonymous table box was not affected.

* The style is tweaked to basically be the same style GitHub uses. In particular, it adds zebra stripes, and removes dotted borders.

  https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Markdown.20table.20styling

[anonymous table box]: https://www.w3.org/TR/CSS2/tables.html#anonymous-boxes
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…-css, r=GuillaumeGomez

rustdoc: clean up docblock table CSS

# Preview

http://notriddle.com/notriddle-rustdoc-demos/table-2/test_dingus/fn.test.html

# Before

![image](https://user-images.githubusercontent.com/1593513/206364287-1b80eaaf-2e0e-4138-8b56-4aa8ff39abac.png)

# After

![image](https://user-images.githubusercontent.com/1593513/206364209-d287d165-31be-4de1-9b43-05b35ce2a86b.png)

# Details

* The rule `display: block` had no noticeable effect. Technically, because markdown tables have a tbody and thead, they get wrapped in an [anonymous table box] in the CSS tree, nested within the `<table>` element's block layout box.

  This rule was added in rust-lang#87230 to make the table side-scrolling, but this same issue was doubly fixed in rust-lang#88742 by wrapping it in an explicit `<div>` tag. Since accessibility advocates recommend the wrapper div over marking the table as `display: block`, we'll stick with that.

  https://adrianroselli.com/2020/11/under-engineered-responsive-tables.html

* The rule `width: calc(100% - 2px)` had no visible effect, because the anonymous table box was not affected.

* The style is tweaked to basically be the same style GitHub uses. In particular, it adds zebra stripes, and removes dotted borders.

  https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Markdown.20table.20styling

[anonymous table box]: https://www.w3.org/TR/CSS2/tables.html#anonymous-boxes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants