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

rustdoc: Scrollbar shown instead of warning (or error) icon #89185

Closed
camelid opened this issue Sep 22, 2021 · 25 comments · Fixed by #90233
Closed

rustdoc: Scrollbar shown instead of warning (or error) icon #89185

camelid opened this issue Sep 22, 2021 · 25 comments · Fixed by #90233
Labels
A-rustdoc-ui Area: rustdoc UI (generated HTML) C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Milestone

Comments

@camelid
Copy link
Member

camelid commented Sep 22, 2021

It looks like it's inheriting overflow-x: auto for some reason.

nightly (broken)

image

beta (good)

image

@camelid camelid added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. C-bug Category: This is a bug. A-rustdoc-ui Area: rustdoc UI (generated HTML) labels Sep 22, 2021
@camelid camelid added this to the 1.57.0 milestone Sep 22, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 22, 2021
@camelid
Copy link
Member Author

camelid commented Sep 22, 2021

cc @rust-lang/rustdoc

@apiraino
Copy link
Contributor

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 22, 2021
@nbdd0121
Copy link
Contributor

nbdd0121 commented Sep 22, 2021

I couldn't replicate locally on Windows (both Edge 93 and Firefox 92 seems to render it correctly).

@camelid
Copy link
Member Author

camelid commented Sep 22, 2021

I couldn't replicate locally (both Edge and Firefox seems to render it correctly).

I'm using Firefox, so at least my version does not render it correctly :)

I'm using the latest stable version of Firefox on macOS.

@camelid
Copy link
Member Author

camelid commented Sep 22, 2021

I checked in Chrome on macOS and it renders correctly, so it seems like the situation is this:

Browser OS Status
Firefox macOS
Chrome macOS
Edge ?
Firefox ?

@apiraino apiraino added P-medium Medium priority and removed P-high High priority labels Sep 23, 2021
@camelid
Copy link
Member Author

camelid commented Oct 14, 2021

I haven't bisected, but I'm pretty sure this was caused by the following code added in #88742:

.docblock > * {
max-width: 100%;
overflow-x: auto;
}

@GuillaumeGomez could you look into this bug? It'd be good to get it fixed since it makes the UI confusing. I don't have enough knowledge of rustdoc's CSS to try to fix it myself right now.

@nbdd0121
Copy link
Contributor

#88742 adds a horizontal scrollbar (overflow-x) though, the scrollbar that appears on the screenshot is a vertical one.

The information box has no explicit height set so there is no reason that it should overflow. The scrollbar makes no sense to me, I wonder if this is a browser bug...

@jsha
Copy link
Contributor

jsha commented Oct 14, 2021

For me, Chrome Version 94.0.4606.81 (Official Build) (64-bit) and Firefox 93.0 (64-bit) on Linux both render correctly. @camelid do you have any extensions installed? Does it reproduce in a fresh Firefox profile?

@camelid
Copy link
Member Author

camelid commented Oct 15, 2021

I do not have extensions installed, and yes, it reproduces with a fresh profile. See my table above (#89185 (comment)); it seems like it may be specific to Firefox on macOS.

@GuillaumeGomez
Copy link
Member

Just tested on my mac. The problem seems to be firefox because there is no overflow rule on the item and yet firefox says there is. Also we can scroll the first paragraph of the docblock. Well in short, firefox seems to be completely broken...

@camelid
Copy link
Member Author

camelid commented Oct 22, 2021

The problem seems to be firefox because there is no overflow rule on the item and yet firefox says there is.

What do you mean? Chrome shows the same overflow rule that Firefox does, but the scrollbar bug is not present in Chrome. The following is the rule I'm talking about; are you describing a different one?

.docblock > * {
    max-width: 100%;
    overflow-x: auto;
}

Also we can scroll the first paragraph of the docblock.

I don't see any scrollbars other than for the icons and the whole page. What are you referring to?

@GuillaumeGomez
Copy link
Member

Taking screenshots on mac when you maintain something is a nightmare so I didn't but you can scroll the first paragraph of a docblock (even though there is no scrollbar).

As for the tooltip icon, it's not a direct child of .docblock so this rule isn't applied to it.

@camelid
Copy link
Member Author

camelid commented Oct 22, 2021

you can scroll the first paragraph of a docblock (even though there is no scrollbar).

Ah, my computer mouse does not have a scroll wheel, so I don't have a way to test the scrolling you're describing.

@camelid
Copy link
Member Author

camelid commented Oct 22, 2021

As for the tooltip icon, it's not a direct child of .docblock so this rule isn't applied to it.

It is a direct child of .docblock, which is why the rule is applied (.information is the container for the icon):

image

@GuillaumeGomez
Copy link
Member

Ah, my computer mouse does not have a scroll wheel, so I don't have a way to test the scrolling you're describing.

Touchpad powa!

As for the tooltip icon, it's not a direct child of .docblock so this rule isn't applied to it.

It is a direct child of .docblock, which is why the rule is applied (.information is the container for the icon):

The tooltip is inside information, so the scroll isn't applied to it directly (which is what I meant), but its parent has it, indeed. I guess we were saying the same thing. :3

This is a very interesting bug though.

@camelid
Copy link
Member Author

camelid commented Oct 22, 2021

This is a very interesting bug though.

Yeah, I think this might be both a Firefox and a rustdoc bug 😆

@GuillaumeGomez
Copy link
Member

Let's cut the apple in two: it's 100% Firefox's bug. We can now happily close this issue and move on with our lives knowing we were not wrong. 😺

@camelid
Copy link
Member Author

camelid commented Oct 22, 2021

Hehe, this bug really drives me nuts though. And I imagine it bothers everyone else who uses Firefox on macOS to read rustdoc docs :)

I'd imagine there's a way for us to workaround this bug, though, right? I could file a Firefox bug, but I don't know how to minimize this.

@GuillaumeGomez
Copy link
Member

It works mostly fine on my (old) mac, so it's hard for me to try anything... Can you try to add a :not(.information) on the scroll/overflow rules?

@camelid
Copy link
Member Author

camelid commented Oct 22, 2021

Can you try to add a :not(.information) on the scroll/overflow rules?

Yeah, that fixes it. It's kind of a hack, but I guess it's better than the current situation. Do you want to open a PR or should I?

@GuillaumeGomez
Copy link
Member

As you prefer!

@GuillaumeGomez
Copy link
Member

Oh before I forget: if you send a PR, don't forget to add a GUI test to check that there is no overflow/scroll rule on it please. :)

If you haven't sent a PR tomorrow, I'll send one. ;)

@camelid
Copy link
Member Author

camelid commented Oct 22, 2021

Oh before I forget: if you send a PR, don't forget to add a GUI test to check that there is no overflow/scroll rule on it please. :)

But GUI tests are run in Chromium, right? Is this just to make sure we don't have a bug for all browsers?

@camelid
Copy link
Member Author

camelid commented Oct 22, 2021

If you haven't sent a PR tomorrow, I'll send one. ;)

I think I might not have time to send a PR, but I'm happy to review it if you open a PR :)

@GuillaumeGomez
Copy link
Member

But GUI tests are run in Chromium, right? Is this just to make sure we don't have a bug for all browsers?

The goal here is just to ensure that the CSS rules which cause the bug in firefox aren't applied on .information. So the browser doesn't matter.

I think I might not have time to send a PR, but I'm happy to review it if you open a PR :)

Then I'll send one tomorrow. Thanks in advance for the review!

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Oct 25, 2021
@bors bors closed this as completed in 6c0dcb4 Oct 25, 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. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. 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.

6 participants