-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 ui: adjust tooltip z-index to be above sidebar #119477
Conversation
r? @notriddle (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @jsha Some changes occurred in GUI tests. |
@@ -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; |
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@bors r+ rollup |
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened. |
and calculate the z-indices of things that go over the sidebar
0c6d43d
to
b1853eb
Compare
I just realized that e306044 was incorrect and unintentionally changed the positioning of Sorry for the noise, but this will need to be re-approved. |
@bors r=notriddle |
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
Alternatively it would look like this if we leave the resizer at 200:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Nominating for beta-backport. |
…iddle rustdoc ui: adjust tooltip z-index to be above sidebar In rust-lang#115660 the sidebar's z-index was changed to 100. This PR changes the tooltip's z-index to 101 to be above the sidebar again. Fixes [after beta-backport] rust-lang#119472.
…iddle rustdoc ui: adjust tooltip z-index to be above sidebar In rust-lang#115660 the sidebar's z-index was changed to 100. This PR changes the tooltip's z-index to 101 to be above the sidebar again. Fixes [after beta-backport] rust-lang#119472.
Rollup of 21 pull requests Successful merges: - rust-lang#119086 (Query panic!() to useful diagnostic) - rust-lang#119239 (Remove unnecessary arm in `check_expr_yield`) - rust-lang#119298 (suppress change-tracker warnings in CI containers) - rust-lang#119319 (Document that File does not buffer reads/writes) - rust-lang#119434 (rc: Take *const T in is_dangling) - rust-lang#119444 (Rename `TyCtxt::is_closure` to `TyCtxt::is_closure_or_coroutine`) - rust-lang#119474 (Update tracking issue of naked_functions) - rust-lang#119476 (Pretty-print always-const trait predicates correctly) - rust-lang#119477 (rustdoc ui: adjust tooltip z-index to be above sidebar) - rust-lang#119479 (Remove two unused feature gates from rustc_query_impl) - rust-lang#119487 (Minor improvements in comment on `freshen.rs`) - rust-lang#119492 (Update books) - rust-lang#119494 (Deny defaults for higher-ranked generic parameters) - rust-lang#119498 (Update deadlinks of `strict_provenance` lints) - rust-lang#119505 (Don't synthesize host effect params for trait associated functions marked const) - rust-lang#119510 (Report I/O errors from rmeta encoding with emit_fatal) - rust-lang#119512 (Mark myself as back from leave) - rust-lang#119514 (coverage: Avoid a query stability hazard in `function_coverage_map`) - rust-lang#119523 (llvm: Allow `noundef` in codegen tests) - rust-lang#119534 (Update `thread_local` examples to use `local_key_cell_methods`) - rust-lang#119544 (Fix: Properly set vendor in i686-win7-windows-msvc target) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#119477 - lukas-code:tooltip-z-index, r=notriddle rustdoc ui: adjust tooltip z-index to be above sidebar In rust-lang#115660 the sidebar's z-index was changed to 100. This PR changes the tooltip's z-index to 101 to be above the sidebar again. Fixes [after beta-backport] rust-lang#119472.
@lukas-code This does not backport cleanly (I think some prior PRs are perhaps missing?) -- please prepare a PR targeting current beta branch and cc me on it (happy to rubber stamp that). |
[beta] rustdoc ui: adjust tooltip z-index to be above sidebar Backport of rust-lang#119477. Fixes rust-lang#119472. range-diff: ```range-diff 446: d796ad4 = 11: 53637cd rustdoc ui: adjust tooltip z-index to be above sidebar 459: b1853eb ! 12: c4c4ff6 use css variable for z-index of the sidebar `@@` src/librustdoc/html/static/css/rustdoc.css: so that we can apply CSS-filters to margin-top: 7px; border-radius: 3px; border: 1px solid var(--border-color); -`@@` src/librustdoc/html/static/css/rustdoc.css: a.tooltip:hover::after { - } - .src #sidebar-button { - left: 8px; -- z-index: 101; -+ z-index: calc(var(--desktop-sidebar-z-index) + 1); - } - .hide-sidebar .src #sidebar-button { - position: static; ``` The "show sidebar" button on the [source view page](https://doc.rust-lang.org/nightly/src/std/lib.rs.html) works differently on beta and nightly, but it is in fact above the sidebar in both versions. beta button: ![beta button](https://github.com/rust-lang/rust/assets/26522220/24d7e86f-d19e-452f-bef2-6b6004b42255) nightly button: ![nightly button](https://github.com/rust-lang/rust/assets/26522220/239dfdda-d9ca-4945-94dd-96254ed1c13f) cc `@Mark-Simulacrum`
…odals, r=fmease rustdoc: hide modals when resizing the sidebar Follow-up for rust-lang#119477 (comment) CC `@lukas-code`
Rollup merge of rust-lang#119746 - notriddle:notriddle/resize-close-modals, r=fmease rustdoc: hide modals when resizing the sidebar Follow-up for rust-lang#119477 (comment) CC `@lukas-code`
In #115660 the sidebar's z-index was changed to 100. This PR changes the tooltip's z-index to 101 to be above the sidebar again.
Fixes [after beta-backport] #119472.