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.

Sign up for GitHub

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 ui: adjust tooltip z-index to be above sidebar #119477

Merged
merged 2 commits into from
Jan 3, 2024
Merged

rustdoc ui: adjust tooltip z-index to be above sidebar #119477

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d796ad4
rustdoc ui: adjust tooltip z-index to be above sidebar
Dec 31, 2023
b1853eb
use css variable for z-index of the sidebar
Dec 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/librustdoc/html/static/css/rustdoc.css
View file
Open in desktop
Copy link
Member

@fmease fmease Jan 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Btw, have you verified that flattening the z-index stack from (0, 2, 100, 101, 200) to just (0, 100, 101) doesn't incur further overlap regressions? @lukas-code

Copy link
Member Author

Choose a reason for hiding this comment

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

For the .sidebar-resizer i checked that it still looks correct for changing 200 to 101. The .popover going from 2 to 101 is exactly the bug this PR is trying to fix and I can't think of anything that's supposed to go over the popover at the moment.

Additionally, changing the resizer to 101 makes it appear below the popover, which seems more correct to me:

image

Alternatively it would look like this if we leave the resizer at 200:

image

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for double-checking, your version does indeed look better.

Copy link
Member Author

@lukas-code lukas-code Jan 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Apparently this also affects the order of the settings/help popups and the sidebar while resizing the sidebar in a really narrow window. I don't know if this is relevant enough to add a special case.

before:

2024-01-01_16-40-09.mp4

after:

2024-01-01_16-40-24.mp4

Copy link
Contributor

@notriddle notriddle Jan 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Clicking the sidebar to resize it should just close the Settings popover. Make this whole discussion a moot point.

That's not important right now, though. I'll make a separate PR to fix this after it merges.

Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,7 @@ so that we can apply CSS-filters to change the arrow color in themes */
position: absolute;
top: 100%;
right: 0;
z-index: 2;
z-index: 101;
Copy link
Member

Choose a reason for hiding this comment

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

I know that we haven't done it like that for other z-indices but personally speaking I'd love to use CSS variables here to make this more robust. Something like

Suggested change
z-index: 101;
z-index: calc(var(--sidebar-z-index) + 1);

where we have defined --sidebar-z-index somewhere reasonable. Not sure what the other rustdoc reviewers think about that.

Copy link
Member

Choose a reason for hiding this comment

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

It'd even be self-documenting, too, that's a bonus.

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like a good idea.

margin-top: 7px;
border-radius: 3px;
border: 1px solid var(--border-color);
Expand Down
12 changes: 12 additions & 0 deletions tests/rustdoc-gui/tooltip-over-sidebar.goml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Check that the doctest info tooltips are above the sidebar.
go-to: "file://" + |DOC_PATH| + "/test_docs/fn.foo.html"
move-cursor-to: ".example-wrap.ignore .tooltip"
wait-for: ".tooltip.popover"

// Move cursor to top left corner of the tooltip and check that it doesn't fade.
move-cursor-to: ".tooltip.popover"
wait-for: 100
assert: ".tooltip.popover:not(.fade-out)"

move-cursor-to: (0, 0)
wait-for: ".tooltip.popover.fade-out"
Loading