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

Change layout to fix missing scrollbar highlighting (chromium) Attempt 2 #637

Merged
merged 1 commit into from
Mar 14, 2020

Conversation

cynecx
Copy link
Contributor

@cynecx cynecx commented Mar 14, 2020

See the previous PR (#635 ) for more insights. However this PR includes a fix for the broken anchors, which is now fixed. The solution was borrowed from the bootstrap css framework. thanks.

@cynecx cynecx force-pushed the fix-chrome-search-highlight branch 3 times, most recently from 7558cf1 to d15f8fb Compare March 14, 2020 21:07
@cynecx cynecx force-pushed the fix-chrome-search-highlight branch from d15f8fb to b4dba04 Compare March 14, 2020 21:07
@jyn514
Copy link
Member

jyn514 commented Mar 14, 2020

r? @GuillaumeGomez

@jyn514
Copy link
Member

jyn514 commented Mar 14, 2020

@cynecx can you post some screenshots of what this looks like?

@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

@jyn514

Peek 2020-03-14 22-36

@GuillaumeGomez
Copy link
Member

This is super weird. O.o But today I learned something new so I'm quite happy! :)

So I confirmed two things: it doesn't break anchors and the theme picker button is still where it's supposed to be. Is there anything else to be checked?

@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

@GuillaumeGomez Welcome to the world of CSS and whatever the fuck is going on.

Oh, I've just realized that the line numbers anchors (source code view) is still broken, but it can be fixed with the same workaround:

// Fix for anchors, so that they are well positioned right below the sticky header
.line-numbers > *::before {
    display: block;
    content: "";
    height: $top-navbar-height;
    margin-top: (-$top-navbar-height);
}

However, I am having a bad feeling that this might bloat the DOM tree somehow, when we are displaying a large file for example? Other than that, this seems to work well too. Also perhaps this is more less of a problem, so we can just leave it as it is, but that's just my opinion.

@jyn514
Copy link
Member

jyn514 commented Mar 14, 2020

Maybe #595 ? That's hard to test without an iPhone though and is kind of a separate issue so I'm ok with not fixing it in the same PR.

@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

@jyn514 I've just tested docs.rs (without this PR) on my iPhone (7 + iOS 13.3) and it works just fine and scrolling feels like how it should be (smooth? I guess).

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Mar 14, 2020

This is a real nightmare... But at least it seems to be working.

@jyn514 Should we give it another try?

@jyn514
Copy link
Member

jyn514 commented Mar 14, 2020

I think #595 might only show up on iOS 12. Anyway it shouldn't block this PR, I'm happy with it if @GuillaumeGomez is.

@jyn514
Copy link
Member

jyn514 commented Mar 14, 2020

Sure, let's go for it.

@jyn514 jyn514 merged commit 3ababaf into rust-lang:master Mar 14, 2020
@jyn514
Copy link
Member

jyn514 commented Mar 14, 2020

@cynecx can you add the same CSS fix for small-section-header? As a test case you can use https://docs.rs/rcc/0.8.0/rcc/intern/struct.InternedStr.html#implementations.

Side note: I'm worried that we'll keep whacking moles for a while. Is there maybe a better solution that would work for all headers at once?

@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

@jyn514 Yeah I will look into it, but I have to agree, this is a nightmare... xD

@GuillaumeGomez
Copy link
Member

Side note: I'm worried that we'll keep whacking moles for a while. Is there maybe a better solution that would work for all headers at once?

The problem here is that rustdoc generates HTML that doesn't care about being embedded (because of the left menu mainly). And when I wrote most of the HTML, I have to admit that I didn't think about it at all. Even less when the first mobile issues appeared. :)

@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

Well, I've tried several things, the most "promising" was this one:

// Fix for anchors, so that they are well positioned right below the sticky header
// FIXME: This is a hack and should be replaced with a proper solution.
h1[id]::before, h2[id]::before, h3[id]::before, h4[id]::before, h5[id]::before, h6[id]::before {
  display: block;
  content: "";
  height: 3rem;
  margin-top: -3rem;
}

// Due to the anchor fix and sub-optimal rustdoc css, the method collapser is mispositioned.
// FIXME: Workaround by applying a top margin.
.method > a[class="collapse-toggle"] {
    margin-top: 3rem;
}

However this causes issues like:

Screenshot from 2020-03-15 00-20-04

Notice the large left margin. This is due to justify-content: space-between; and browsers considering ::before content as first child element, therefore flexbox doing its thing and produces the margin.

(A workaround for this would be to wrap the inner flexbox elements within a new wrapper so the ::before element can't affect the flexbox calculation, however this would require changes to rustdoc or dynamic DOM tree manipulation.)

Overall I can say that this is not worth the complexity, this requires a more thorough rework of what rustdoc produces.

In the meantime, I would propose simply adding more ::before-anchor workarounds for the "remaining" items.

EDIT: #640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants