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

Generated docs on long pages render text on top of itself #88545

Closed
mqudsi opened this issue Aug 31, 2021 · 23 comments · Fixed by #88816
Closed

Generated docs on long pages render text on top of itself #88545

mqudsi opened this issue Aug 31, 2021 · 23 comments · Fixed by #88816
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@mqudsi
Copy link
Contributor

mqudsi commented Aug 31, 2021

I hope this is the right repo to report this, I wasn't sure if it should go in the main rust repo instead.

The generated docs for pages with a crazy number of items seem to give up on positioning and then just dump all subsequent entries on the same line past a certain point. It looks like it might be related to hardware acceleration, but I'm not sure.

For example, viewing https://doc.rust-lang.org/beta/core/arch/arm/index.html in Chrome 92 on Windows and hitting ctrl+pgdwn to go to the very bottom shows this:

image

@jyn514 jyn514 transferred this issue from rust-lang/docs.rs Aug 31, 2021
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 31, 2021
@jyn514
Copy link
Member

jyn514 commented Aug 31, 2021

@mqudsi no worries - rustdoc is used for both docs.rs and the main doc.rust-lang.org website, but docs.rs doesn't have control over the main site.

@jyn514 jyn514 added A-rustdoc-ui Area: Rustdoc UI (generated HTML) C-bug Category: This is a bug. labels Aug 31, 2021
@jyn514
Copy link
Member

jyn514 commented Aug 31, 2021

I can't reproduce this on mobile Chrome though - maybe it's related to the screen size?
Screenshot_20210831-161236

@Seadragon91
Copy link

Can confirm that problem on my PC in Edge.
Also on my smartphone in Edge and Chrome, I need to change the view of the website to "Desktop website" then I see it there too.

@camelid
Copy link
Member

camelid commented Sep 1, 2021

I can't reproduce this in Firefox on macOS, but I can in Chrome on macOS. So I'm guessing it's a bug specific to Blink because I think Chrome and Edge both use Blink, whereas Firefox does not.

@ChrisDenton
Copy link
Member

It seems to be a bug with Blink's grid implementation. If I turn off the display: grid style it works (but obviously changes the layout).

@mqudsi
Copy link
Contributor Author

mqudsi commented Sep 1, 2021

It seems to be a bug with Blink's grid implementation. If I turn off the display: grid style it works (but obviously changes the layout).

I’m not saying it’s not grid, but just pointing out that changing the display completely changes the layout and so may avoid the problem even if it’s something else. In theory, anyway.

@ChrisDenton
Copy link
Member

ChrisDenton commented Sep 1, 2021

Fair point. A minimal test case appears to show the same issue but I'll admit that does not necessarily determine the root cause:

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8">
    <title>Test display grid with lots of children</title>
</head>

<body style="display: grid;">
<script>
for (let i = 0; i < 600; i++) {
    document.write(`<div>Testing</div>`);
    document.write(`<div>Overtype</div>`);
}
</script>
</body></html>

@mqudsi
Copy link
Contributor Author

mqudsi commented Sep 1, 2021

I think your test case confirms the cause, at least from a black box perspective, sufficiently ;)

@dbalcomb
Copy link

dbalcomb commented Sep 6, 2021

It looks like this issue was previously reported here and there are some relevant links towards the end. Notably a confirmation that there is currently a 1000 row limit to grids in Blink. There is also a recent reddit thread on the issue.

@GuillaumeGomez
Copy link
Member

Do you have an idea @dns2utf8 ? The best would be to not use grid at all. If you have no idea, I'll send a fix in the next coming days.

@dns2utf8
Copy link
Contributor

dns2utf8 commented Sep 9, 2021

I can confirm this with Chromium 93 and IE Edge 91.
I tried adding a <br> after the grid-layout (works with floats sometimes) but will try some more.

It works with Firefox 91 and 92 so I assume it is a chrome render-engine bug.

@ChrisDenton
Copy link
Member

Yes it's a Blink bug: https://bugs.chromium.org/p/chromium/issues/detail?id=688640

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Sep 9, 2021

@dns2utf8 Which is why I think we should switch to another way of rendering which doesn't use the current grid at all (firefox also has a limit, it's just a higher one).

@dns2utf8
Copy link
Contributor

dns2utf8 commented Sep 9, 2021

Hm, ... I see your point.
Since it solves the viewport-scaling with mobile very nicely, what if we split the table into 1000 element chunks?
That would not make a difference on mobile and on desktop we would just loose the width consistency of the first column every 1000 element.

@GuillaumeGomez
Copy link
Member

I don't think this is a good idea. I'd really prefer to not rely on "limits" that could change independently outside of our control. I think we need to find another approach.

@dns2utf8
Copy link
Contributor

dns2utf8 commented Sep 9, 2021

One idea I have is that we switch back to the table element if the table is too long.
This I would probably implement in the HTML generator.

Another one, we use the mobile layout on desktop too if the table is too long.
Here I would add a check in JavaScript to see if the last two elements overlap and then add a class that switches the behaviour.

Or something else ☺️

@GuillaumeGomez
Copy link
Member

The annoying part is that the left column has to be the width of the largest element. Maybe we could rework that part so it doesn't need something so complicated and keep how it's displayed on mobile? If you have suggestions... :)

@mqudsi
Copy link
Contributor Author

mqudsi commented Sep 9, 2021

cough tables cough.

But seriously, if you use a css reset to remove all browser styling (are we already doing that?) you can use the table layout in the renderer to create code like our current grid (because we are barely using any of the actual grid features). A little bit of css to the cell classes makes it responsive.

@GuillaumeGomez
Copy link
Member

Or maybe we can do even simpler and just set width to both parts to 50% (or 30-70, well up to debate). Like this, the alignment for the right part will work and we don't need to worry about grid or table anymore.

@dns2utf8
Copy link
Contributor

dns2utf8 commented Sep 9, 2021

In the upcoming CSS Grid-2 spec (https://www.w3.org/TR/css-grid-2/#overlarge-grids) there is this sentence:

...  in the range [-10000, 10000]), dropping all lines outside that limit.

So we would run into trouble if a table gets longer than 10k entries.

However:

... if a UA only supported grids with at most 1000 tracks in each dimension, the following placement properties:
  grid-row:
  grid-column:

To me the spec reads as, the UA (User Agent aka Browser) is free to enforce lower limits if there is not enough memory available.

I would like to look at the display: table* because that would allow us to keep the automatic layout switching on mobile (and support for it looks good) https://caniuse.com/?search=table

Manishearth added a commit to Manishearth/rust that referenced this issue Sep 10, 2021
…ements_grid_bug, r=GuillaumeGomez

Workaround blink/chromium grid layout limitation of 1000 rows

I made this in case we don't come up with a better solution in time.

See rust-lang#88545 for more details.

A rendered version of the standard library is hosted here:
https://data.estada.ch/rustdoc-nightly_497ee321af_2021-09-09/core/arch/arm/index.html

r? `@GuillaumeGomez` `@jsha`
dns2utf8 added a commit to dns2utf8/rust that referenced this issue Sep 11, 2021
cuviper pushed a commit to cuviper/rust that referenced this issue Sep 14, 2021
@jyn514
Copy link
Member

jyn514 commented Sep 15, 2021

This was fixed by #88776, right?

@GuillaumeGomez
Copy link
Member

I'm waiting for #88816 before closing it.

@camelid
Copy link
Member

camelid commented Sep 16, 2021

@GuillaumeGomez Can you add Closes <this issue number> to the PR description then, to make sure the issue is closed on merge?

Mark-Simulacrum pushed a commit to dns2utf8/rust that referenced this issue Oct 3, 2021
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 5, 2021
…nts, r=GuillaumeGomez

Rustdoc migrate to table so the gui can handle >2k constants

Closes rust-lang#88545.

This PR adds a test for overlapping entries in the `item-table` rust-lang#88545
It currently includes the commit with the workaround from rust-lang#88776
@bors bors closed this as completed in 52d3afa Oct 5, 2021
dns2utf8 added a commit to dns2utf8/rust that referenced this issue Oct 9, 2021
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) C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants