Fix toggle position on mobile #85289
Conversation
GuillaumeGomez
added
T-rustdoc
|
Some changes occurred in HTML/CSS/JS. |
rust-highfive
added
the
S-waiting-on-review
GuillaumeGomez
changed the title
There was a problem hiding this comment.
Thanks for fixing this! Note that for the 700px media, there's already this rule:
.item-list > details.rustdoc-toggle > summary:not(.hideme)::before {
left: -10px;
}
I think that should be updated to use the same selector as the one you're updating here, and the same number of pixels. Or maybe the selector you're using here should be nested under .item-list?
By the way, what I found in #84602 (which is now out of date) was that when the exact same selector is both inside a media block and outside, they have exactly equal specificity. That is, the media block doesn't add any specificity. So whichever is last wins.
So I think the most correct and thorough fix is to make sure all "default" rules are above all media-based rules, so the media-based rules can override them.
| @@ -1763,7 +1770,7 @@ details.rustdoc-toggle[open] > summary.hideme > span { | |||
| } | |||
|
|
|||
| details.rustdoc-toggle[open] > summary::before { | |||
| content: "[−]"; | |||
| content: "[-]"; | |||
There was a problem hiding this comment.
Why the change here? I think I modeled the longer dash over what was being done with the JS-style toggles previously. I don't particularly object but it seems like an unrelated diff.
There was a problem hiding this comment.
Oh it's because the rendering was broken in puppeteer. Do you want me to revert it?
There was a problem hiding this comment.
Honestly I don't have an opinion - I just made sure to use the existing long dash because I assumed you cared about it. :-) Fine to leave as-is, but should have an explanation in a commit message and/or the PR description.
There was a problem hiding this comment.
I don't really care about it to be honest. I realized it was "broken" and simply changed it when adding the test and didn't pay attention when I committed.
There was a problem hiding this comment.
I'm pretty sure the idea is that + and − have exactly the same width. We don't want the toggle to shift around when you click it.
There was a problem hiding this comment.
Good point, gonna revert this change then.
There was a problem hiding this comment.
Compare these three ways of writing...
3+3| plus (U+002D)
3–3| minus (U+2212)
3-3| hyphen-minus (U+002B)
On my machine, at least, the first two are perfectly aligned, and the last is a bit narrower. This is probably the same in most fonts, since the plus and minus symbols are intended to be used together.
| @@ -1697,6 +1701,9 @@ details.rustdoc-toggle > summary.hideme { | |||
| cursor: pointer; | |||
| } | |||
|
|
|||
| details.rustdoc-toggle > summary, details.undocumented > summary { | |||
| list-style: none; | |||
There was a problem hiding this comment.
What necessitated this? Also, assuming we go forward with this I would rather add the rustdoc-toggle class when we emit undocumented toggles, instead of having two separate CSS selectors for this.
There was a problem hiding this comment.
It's because of chromium. The <details> arrow is still buggy on it so it needs extra rules.
There was a problem hiding this comment.
chromium as opposed to chrome? or both chrome and chromium?
And what's the display bug that happens without this?
There was a problem hiding this comment.
No, chromium specifically, not chrome and I assume not opera/safari/edge.
As for the bug, it shows the default <details> "arrow" alongside our summary text. It doesn't break the display, just looks weird.
| goto: file://|DOC_PATH|/test_docs/struct.Foo.html | ||
| size: (434, 600) | ||
| assert: (".top-doc", "open", "") | ||
| click: (8, 280) // This the position of the top doc comment toggle | ||
| assert-false: (".top-doc", "open", "") | ||
| click: (8, 280) | ||
| assert: (".top-doc", "open", "") |
There was a problem hiding this comment.
This test seems both too fragile (depends on exact pixel offsets) and not specific enough (if the position is only off by a few pixels, the click might still succeed). Do we have access to the DOM in these tests? Can we check something like "the toggle doesn't overlap with the left side of the screen" or "the toggle's calculated left position is at least N pixels away from the left side of the screen?"
There was a problem hiding this comment.
The framework is not that smart, it doesn't allow computation. I can eventually add a click event just 1 pixel on the right of the toggle to ensure it's not there?
There was a problem hiding this comment.
I can eventually add a click event just 1 pixel on the right of the toggle to ensure it's not there?
You mean 1 pixel to the right of the left-hand side of the screen? That seems fine, though still brittle.
In general there are a lot of things that differ in layout between desktop and mobile layouts. I don't think it's practical to write unittests for each of them. Is there a reason to write a unittest for this one? Well, maybe, since it's regressed once so far. But in general what would a test framework look like that verified certain layout properties held on mobile vs desktop? It seems like we want an entirely different paradigm for testing that. (idle thoughts; don't need to be fixed as part of this PR :-))
There was a problem hiding this comment.
The layout of rustdoc is supposed to not change much. By checking such small details, we can ensure that no unexpected changes ocurred (which happened way too many times ><). Let me add a click on the right and left of the toggle to ensure its position then.
GuillaumeGomez
force-pushed
the
fix-toggle-position-mobile
branch
from
0fd6672
to
01fdb74
Compare
GuillaumeGomez
force-pushed
the
fix-toggle-position-mobile
branch
from
01fdb74
to
dfc8b60
Compare
|
@bors r+ |
|
📌 Commit dfc8b60 has been approved by |
bors
added
S-waiting-on-bors
…mobile, r=jsha Fix toggle position on mobile Before:   After:   r? `@jsha`
…mobile, r=jsha Fix toggle position on mobile Before:   After:   r? ``@jsha``
…laumeGomez Rollup of 12 pull requests Successful merges: - rust-lang#84461 (rustdoc: Remove unnecessary `StripItem` wrapper) - rust-lang#85067 (Minimize amount of fake `DefId`s used in rustdoc) - rust-lang#85207 (Fix typo in comment) - rust-lang#85215 (coverage bug fixes and some refactoring) - rust-lang#85221 (dbg macro: Discuss use in tests, and slightly clarify) - rust-lang#85246 (Miner code formatting) - rust-lang#85253 (swap function order for better read flow) - rust-lang#85256 (Fix display for "implementors" section) - rust-lang#85268 (Use my real name) - rust-lang#85278 (Improve match statements) - rust-lang#85289 (Fix toggle position on mobile) - rust-lang#85323 (Fix eslint errors) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Before:
After:
r? @jsha