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: remove flicker during page load #90333

Merged
merged 1 commit into from
Nov 1, 2021

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Oct 27, 2021

The search bar has a :disabled style that makes it grey, which creates a distracting flicker from grey to white when the page finishes loading. The search bar should stay the same color throughout page load.

A blank white search bar might create an incorrect impression for users with JS turned off. Since they can't use the search functionality, I've hidden the search bar in noscript.css.

Fixes #90246
r? @GuillaumeGomez

Demo: https://rustdoc.crud.net/jsha/flashy-searchbar/std/string/struct.String.html

@jsha jsha added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: rustdoc UI (generated HTML) labels Oct 27, 2021
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 27, 2021
@jsha jsha changed the title rustdoc: remove flicker during page log rustdoc: remove flicker during page load Oct 27, 2021
@jyn514
Copy link
Member

jyn514 commented Oct 27, 2021

The search bar has a :disabled style that makes it grey, which creates a distracting flicker from grey to white when the page finishes loading. The search bar should stay the same color throughout page load.

Hmm, I kind of find the grey bar helpful on mobile, where it can take multiple seconds to load the page - seeing it change lets me know that search is now working. If it were white to start, I think it would seem like the page was broken.

@jsha
Copy link
Contributor Author

jsha commented Oct 27, 2021

seeing it change lets me know that search is now working. If it were white to start, I think it would seem like the page was broken.

We should remove the disabled attribute on the search box entirely, and just ensure that (a) when you start typing, we show a "searching..." indicator so you know the page has responded, and (b) when the search JS does finally load, it takes into account anything in the search bar.

Also, to clarify the request: Are you saying that when you load a page on mobile, you are often hoping to use the search box right away? Or are you using the color search box as a proxy for "whole page is loaded, now I can start scrolling?"

@jyn514
Copy link
Member

jyn514 commented Oct 27, 2021

Are you saying that when you load a page on mobile, you are often hoping to use the search box right away? Or are you using the color search box as a proxy for "whole page is loaded, now I can start scrolling?"

Neither really? I mean, above I was saying that "tapping on the search box does nothing" seems like a bug. I think ideally it would let me start to type in the box even if it didn't yet start searching.

@jsha
Copy link
Contributor Author

jsha commented Oct 28, 2021

I've updated this so the search box is not disabled; if you enter text before the JS is loaded, the JS will trigger a search once it finishes loading.


.search-input {
/* The search bar and related controls don't work without JS */
background-color: #e6e6e6;
Copy link
Member

Choose a reason for hiding this comment

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

Normally, there isn't supposed to be color outside of the theme files. However, I don't know how we can do it in here...

@GuillaumeGomez
Copy link
Member

I've updated this so the search box is not disabled; if you enter text before the JS is loaded, the JS will trigger a search once it finishes loading.

For example on a crate for embedded stuff, the search index is often quite huge but each page kinda small. So that'll mean that we enter something and nothing will happen for a few more seconds. It's not great from a user point of view...

@jyn514
Copy link
Member

jyn514 commented Oct 28, 2021

@GuillaumeGomez I don't see any way around that though; some pages just take a really long time to load. I guess we could show a loading indicator or something but that makes the page feel even more cluttered IMO.

@GuillaumeGomez
Copy link
Member

Well I guess it's fine then... Just need to fix the color situation on noscript CSS then.

@jsha
Copy link
Contributor Author

jsha commented Oct 30, 2021

Updated to remove the color in noscript.

@GuillaumeGomez
Copy link
Member

So now, we can enter things in the input, it's just that nothing will happen... Is there a CSS way to handle such case?

@jyn514
Copy link
Member

jyn514 commented Oct 30, 2021

@GuillaumeGomez I don't understand why you asked to remove the grey coloring. It looks fine in all three different themes, and AFAICT you didn't have an alternative in mind.

@GuillaumeGomez
Copy link
Member

I didn't asked the background colour to be removed. Also, I'm currently complaining about the behaviour when JS is disabled.

@jsha
Copy link
Contributor Author

jsha commented Oct 30, 2021

So now, we can enter things in the input, it's just that nothing will happen... Is there a CSS way to handle such case?

Unfortunately, no. The way to prevent entering things in the input is with the HTML disabled attributes, and you can't set HTML attributes from CSS. I guess we could say search-input:focused { background-color: red } or something similar in <noscript>, but that seems awkward..

@GuillaumeGomez
Copy link
Member

Can't we keep the disabled part and simply remove it at the beginning of the JS script?

Or maybe we should go with what you suggested: simply not displaying the buttons and the search input at all. That seems like the best and simplest solution. Sorry for making you go back to it...

If you go with this approach, please add a GUI test for it (you can disable js).

@jsha
Copy link
Contributor Author

jsha commented Oct 31, 2021

Can't we keep the disabled part and simply remove it at the beginning of the JS script?

@jyn514 made a good point that someone facing a slow loading page may want to start typing in the search before the JS is loaded. It's fairly easy to support that, so I added that in this PR. Going back to using the disabled attribute would prevent that use case.

Or maybe we should go with what you suggested: simply not displaying the buttons and the search input at all. That seems like the best and simplest solution.

Great, I also think this is the nicest solution. :-)

Also, remove the highlighting of the search bar in disabled state. This
reduces flicker when loading a page.
@GuillaumeGomez
Copy link
Member

r=me once CI pass. I'll open an issue for the double call on the search.

@jsha
Copy link
Contributor Author

jsha commented Oct 31, 2021

Just opened an issue :-D #90454

@jsha jsha removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2021
@jsha
Copy link
Contributor Author

jsha commented Oct 31, 2021

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Oct 31, 2021

📌 Commit a4fe76f has been approved by GuillaumeGomez

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 31, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 31, 2021
…omez

rustdoc: remove flicker during page load

The search bar has a `:disabled` style that makes it grey, which creates a distracting flicker from grey to white when the page finishes loading. The search bar should stay the same color throughout page load.

A blank white search bar might create an incorrect impression for users with JS turned off. Since they can't use the search functionality, I've hidden the search bar in noscript.css.

Fixes rust-lang#90246
r? `@GuillaumeGomez`

Demo: https://rustdoc.crud.net/jsha/flashy-searchbar/std/string/struct.String.html
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2021
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#89826 (Feature gate + make must_not_suspend allow-by-default)
 - rust-lang#89929 (Handling submodule update failures more gracefully from x.py)
 - rust-lang#90333 (rustdoc: remove flicker during page load)
 - rust-lang#90349 (Fix rare ICE during typeck in rustdoc scrape_examples)
 - rust-lang#90398 (Document `doc(keyword)` unstable attribute)
 - rust-lang#90441 (Test that promotion follows references when looking for drop)
 - rust-lang#90450 (Remove `rustc_hir::hir_id::HirIdVec`)
 - rust-lang#90452 (Remove unnecessary `Option` from `promote_candidate` return type)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bb6901d into rust-lang:master Nov 1, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 1, 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) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: search box flickers on page load
6 participants