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: use focus for search navigation #84462

Merged
merged 1 commit into from
May 18, 2021

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Apr 23, 2021

Rather than keeping track of highlighted element inside the JS, take advantage of .focus() and the :focus CSS pseudo-class.

This required wrapping each row of results in one big <a> tag (because anchors can be focused, but table rows cannot). That in turn required moving from a table layout to a div layout with float.

This makes it so Ctrl+Enter opens links in new tabs, and using the arrow keys to navigate off the bottom of the page scrolls the rest of the page into view. It also simplifies the keyboard event handling. It eliminates the need for click handlers on the search results, and for tracking mouse movements.

This changes the UI treatment of mouse hovering. A hovered element now gets a light grey background, but does not change the focus. It's possible to have two highlighted search results: one that is focused (via keyboard) and one that is hovered (via mouse). Pressing enter will activate the focused link; clicking will activate the hovered link. This matches up with how Firefox and Chrome handle suggestions in their URL bar, and avoids stray mouse movements changing the focus.

Selecting tabs is now done with left/right arrows while any search result is focused. The visibility of results on each search tab is
controlled with the "active" class, rather than by setting display: none directly. Note that the old code kept track of highlighted search element when tabbing back and forth. The new code doesn't.

Demo at https://hoffman-andrews.com/rust/focus-search-results2/std/?search=fn

Fixes #84384
Fixes #79962
Fixes #79872

@jsha jsha requested a review from GuillaumeGomez April 23, 2021 00:28
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2021
@jsha
Copy link
Contributor Author

jsha commented Apr 23, 2021

@bors r? @GuillaumeGomez

BTW I see in #65212 (comment) that a desired functionality is that you can press down arrow a few times, and then type a letter key to add to your search. That doesn't work in the current revision but should be easy to restore: We can make it so that typing any letter key while the search results are highlight focuses the search input and inserts that letter.

@jsha
Copy link
Contributor Author

jsha commented Apr 23, 2021

Also cc @cratelyn and @Manishearth who had great feedback about keyboard navigation in #65212.

@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 added A-rustdoc-ui Area: Rustdoc UI (generated HTML) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 23, 2021
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

That's what I was a bit afraid of: scroll with arrows isn't working anymore, we have to use the mouse now. I'm a bit shared on this. Also: can you "restore" the previous coloring for the focused search result please?

Another important point: when we go back to the search input, can you scroll to the top too and not simply focus on the input? We add some issues about this behavior because some browsers only care if the element is in the viewport and not if it's reachable/visible (because hidden under other elements). That could be an issue on docs.rs for example.

BTW I see in #65212 (comment) that a desired functionality is that you can press down arrow a few times, and then type a letter key to add to your search. That doesn't work in the current revision but should be easy to restore: We can make it so that typing any letter key while the search results are highlight focuses the search input and inserts that letter.

That'd be nice to keep this behaviour, indeed. Question though: what happens if we press the letter 's'? Should it only focus the search input (like it does when it's not focused) or should it also enter the letter 's'?

Selecting tabs is now done with left/right arrows while any search result is focused. The visibility of results on each search tab is controlled with the "active" class, rather than by setting display: none directly. Note that the old code kept track of highlighted search element when tabbing back and forth. The new code doesn't.

So three things here:

  1. Being able to switch tabs when the search input is focused (using the current shortcut CTRL+up/down) would be nice, because for now it'd force to have a search result focused, which isn't super intuitive.
  2. If so, having two different shortcuts for a same behaviour seems really bad, no idea how to make it work otherwise though...
  3. Please keep track of the focused element in the other tabs.

One last point (small bug apparently): this search triggers a js error: https://hoffman-andrews.com/rust/focus-search-results2/std/?search=-%3E%20str . I was trying to check how it handled things in case the current tab was empty. The current behaviour is that it selects the first non-empty.

Also, I couldn't check but: does it keep "memory" of the last selected tab when you do another search (without selecting a result I mean, just doing another search when on search results).

@Manishearth
Copy link
Member

Manishearth commented Apr 23, 2021

This looks great!

That's what I was a bit afraid of: scroll with arrows isn't working anymore, we have to use the mouse now. I'm a bit shared on this. Also: can you "restore" the previous coloring for the focused search result please?

Scroll with arrows works fine for me in this prototype? It's actually better now since it's per-row instead of per-cell.

Oh I see what you mean. That's a fair point, I guess users can use page up/page down, maybe.

Being able to switch tabs when the search input is focused (using the current shortcut CTRL+up/down) would be nice, because for now it'd force to have a search result focused, which isn't super intuitive.

Idk, for me it's quite intuitive, you hit the down arrow once to enter the search results, and then you can use all four arrow keys to naturally navigate. This is how most tab-based keyboard navigation works.

Please keep track of the focused element in the other tabs.

I'm against this: this is overriding browser default focus behavior with JS and I have never seen that go well. Fine to override focus to the start of the list, but custom stuff tends to break down, and is usually bad for accessibility.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Apr 23, 2021

Oh I see what you mean. That's a fair point, I guess users can use page up/page down, maybe.

I'm sad to say this but: those keys aren't available on all computers... XD (not even joking here)

Idk, for me it's quite intuitive, you hit the down arrow once to enter the search results, and then you can use all four arrow keys to naturally navigate. This is how most tab-based keyboard navigation works.

I'd argue that this is not the default behavior considering that we're creating it. And also that this changes the current behaviour. Not a blocker but something we should be careful about.

I'm against this: this is overriding browser default focus behavior with JS and I have never seen that go well. Fine to override focus to the start of the list, but custom stuff tends to break down, and is usually bad for accessibility.

It's very common for me to switch between two tabs and it'd be annoying to have to go through all the items again. Not sure what the accessibility issue here would be though. More explanations please? :)

@Manishearth
Copy link
Member

I'm sad to say this but: those keys aren't available on all computers... XD (not even joking here)

I'm aware, usually it exists behind a Fn key (Fn-arrow gives home/end/pgup/pgdn), but yeah, this isn't ideal. Not sure what the best option is here.

I'd argue that this is not the default behavior considering that we're creating it. And also that this changes the current behaviour. Not a blocker but something we should be careful about.

Browser focus navigation continues to work, which is the default behavior people will be used to more than site-specific keyboard shortcuts.

It's very common for me to switch between two tabs and it'd be annoying to have to go through all the items again. Not sure what the accessibility issue here would be though. More explanations please? :)

In general trying to replace existing browser features with custom JS never goes well in my experience, and ends up with things breaking. A lot of modern websites do this kind of thing a lot and it breaks down pretty quickly and is hard to keep cross-browser compatible.

@GuillaumeGomez
Copy link
Member

In general trying to replace existing browser features with custom JS never goes well in my experience, and ends up with things breaking. A lot of modern websites do this kind of thing a lot and it breaks down pretty quickly and is hard to keep cross-browser compatible.

The only thing the JS would do would be to re-focus the element that was previously focused in the tab when we switch between them. We are not overriding more than what we are already. We already use JS to switch tabs, so what would that change to re-focus the previously focused element in that tab?

@Manishearth
Copy link
Member

The only thing the JS would do would be to re-focus the element that was previously focused in the tab when we switch between them. We are not overriding more than what we are already.

Because we're trying to keep track of focus and remember it as well, as opposed to just setting it. I guess it's fine but in my experience this kind of thing doesn't go well.

I'd rather defer to @jsha on this though.

@jsha
Copy link
Contributor Author

jsha commented Apr 23, 2021

Also: can you "restore" the previous coloring for the focused search result please?

Right now the focused search result gets colorful backgrounds, and the mouse hover gets gray. I'd like focus vs hover to be visually distinct. I could switch those, so hover is colorful and focus is gray. Or I could make both colorful, but hover desaturated (but I think this will produce visual confusion).

scroll with arrows isn't working anymore, we have to use the mouse now

I'm confused in the same way @Manishearth was. For me, both search result selection and page scrolling with the arrow keys works the same as on master (using FF and Chrome on Linux). Can you tell me the keystrokes you're doing that cause a page scroll on master, but don't cause a page scroll in this branch? What I see is this: search input or search results focused -> down arrow selects a result. No focus -> down arrow scrolls the page.

when we go back to the search input, can you scroll to the top too and not simply focus on the input?

👍🏻

Question though: what happens if we press the letter 's'? Should it only focus the search input (like it does when it's not focused) or should it also enter the letter 's'?

It should also enter the letter 's'.

Being able to switch tabs when the search input is focused (using the current shortcut CTRL+up/down) would be nice, because for now it'd force to have a search result focused, which isn't super intuitive.
It's very common for me to switch between two tabs and it'd be annoying to have to go through all the items again.

I don't fully understand your use case here. Can you tell me more? What's an example search where you would be likely to focus an element in one tab, go to another tab, come back to the original tab and want to continue where you left off? Are these very long lists of search results?

@Manishearth
Copy link
Member

What I see is this: search input or search results focused -> down arrow selects a result. No focus -> down arrow scrolls the page.

This is what I see too. I think what @GuillaumeGomez is referring to is that previously you could scroll the entire search results page with the arrow key. I'm ... okay with losing that functionality when there's search focus.

@jsha
Copy link
Contributor Author

jsha commented Apr 23, 2021

previously you could scroll the entire search results page with the arrow key.

What I see on https://doc.rust-lang.org/std/?search=fn is:

  • When search is focused, you can't scroll the page with the arrow keys because arrow keys highlight search results.
  • When nothing is focused (i.e. click in a blank part of the page), you can scroll the page with the arrow keys.

But I can reproduce both of those behaviors on my demo.

@GuillaumeGomez
Copy link
Member

Also: can you "restore" the previous coloring for the focused search result please?

Right now the focused search result gets colorful backgrounds, and the mouse hover gets gray. I'd like focus vs hover to be visually distinct. I could switch those, so hover is colorful and focus is gray. Or I could make both colorful, but hover desaturated (but I think this will produce visual confusion).

For me there is only the normal focus (border with pointed line). I'd prefer something a bit more colorful, at least enough to see it and not just the pointed border.

scroll with arrows isn't working anymore, we have to use the mouse now

I'm confused in the same way @Manishearth was. For me, both search result selection and page scrolling with the arrow keys works the same as on master (using FF and Chrome on Linux). Can you tell me the keystrokes you're doing that cause a page scroll on master, but don't cause a page scroll in this branch? What I see is this: search input or search results focused -> down arrow selects a result. No focus -> down arrow scrolls the page.

No sorry, you're both right. Just double checked and when the focus isn't on the search input or on one of the result, it's working as expected.

Being able to switch tabs when the search input is focused (using the current shortcut CTRL+up/down) would be nice, because for now it'd force to have a search result focused, which isn't super intuitive.
It's very common for me to switch between two tabs and it'd be annoying to have to go through all the items again.

I don't fully understand your use case here. Can you tell me more? What's an example search where you would be likely to focus an element in one tab, go to another tab, come back to the original tab and want to continue where you left off? Are these very long lists of search results?

It happens when I'm looking for a type to be both in parameters and in the returned value. It's mostly on crates composed of a lot of similar functions with hard to read signatures (someone said simd?). So not sure how common my case is.

@bors
Copy link
Contributor

bors commented Apr 24, 2021

☔ The latest upstream changes (presumably #84490) made this pull request unmergeable. Please resolve the merge conflicts.

@jsha
Copy link
Contributor Author

jsha commented Apr 25, 2021

For me there is only the normal focus (border with pointed line). I'd prefer something a bit more colorful, at least enough to see it and not just the pointed border.

I can't reproduce this, on FF, Chrome, or Edge. Here's what I see:

image

In what browser are you seeing the focus without color? And what mouse clicks / keystrokes do you use to get there?

@jsha
Copy link
Contributor Author

jsha commented Apr 25, 2021

Another important point: when we go back to the search input, can you scroll to the top too and not simply focus on the input? We add some issues about this behavior because some browsers only care if the element is in the viewport and not if it's reachable/visible (because hidden under other elements). That could be an issue on docs.rs for example.

The natural way to do this is with element.scrollIntoView(). I've tried this locally, and it has a downside: if you're already at the top of the page, it actually scrolls the page down by about a half-line so the search box is at the very tippy-top of the page.

Focusing the search box will generally do the right thing. I can see how there's an issue where a topbar obscures part of the page (like on docs.rs), but I'd rather address that in docs.rs by making the topbar doesn't overlap, but instead shrinks the scrollable part of the page. Overlapping topbars already cause problems: they make page-up / page-down / spacebar scroll by an incorrect amount.

@Nemo157
Copy link
Member

Nemo157 commented Apr 25, 2021

In ayu theme the focus was a simple dotted blue outline, now it appears that both dark and ayu don't highlight the focused element at all.

EDIT: Ah, I see this is called out in the OP, I didn't notice this when skimming it and I already had your domain switched to ayu from a previous example.

@jsha
Copy link
Contributor Author

jsha commented Apr 25, 2021

I've pushed some changes and updated the demo. I went ahead and implemented "remember the focused item on the previous tab." It wasn't too complicated and I don't think it will have many unanticipated side effects.

I fixed a nasty bug where I accidentally overrode alt-left and alt-right (forward and back in browser history 😳). And another interesting bug where searching multiple times, or changing crates, would cause jumping by more than one item at a time.

All three themes should now work and show colorful highlighting.

We can make it so that typing any letter key while the search results are highlight focuses the search input and inserts that letter.

I haven't implemented this yet. Thinking about it some more, I think it will be quite hard to get right. For JS keyboard events we just get a keycode, not an indication of whether something is a letter or punctuation. And the set of keycodes that correspond to letters and punctuation will probably change by locale. My preference right now would be that, when you are navigating through search results, you have to hit 's' to re-focus the search input. Is that okay by you @GuillaumeGomez ?

@rust-log-analyzer

This comment has been minimized.

@jsha jsha force-pushed the focus-search-results2 branch from 65b580d to 2ffc815 Compare May 12, 2021 06:22
@jsha
Copy link
Contributor Author

jsha commented May 12, 2021

when an item is selected, its text becomes white

Fixed!

@GuillaumeGomez
Copy link
Member

Just realized something funny: the further down we go, the closer to the bottom of the window the focus is. Can be fixed later as it's not really a big issue, this PR has been open for long enough and I'd really like to see it merged. :D

@dns2utf8 One last check from you too please? :)

@bors
Copy link
Contributor

bors commented May 13, 2021

☔ The latest upstream changes (presumably #85258) made this pull request unmergeable. Please resolve the merge conflicts.

Rather than keeping track of highlighted element inside the JS, take
advantage of `.focus()` and the :focus CSS pseudo-class.

This required wrapping each row of results in one big <a> tag (because
anchors can be focused, but table rows cannot). That in turn required
moving from a table layout to a div layout with float.

This makes it so Ctrl+Enter opens links in new tabs, and using the arrow
keys to navigate off the bottom of the page scrolls the rest of the page
into view. It also simplifies the keyboard event handling. It eliminates
the need for click handlers on the search results, and for tracking
mouse movements.

This changes the UI treatment of mouse hovering. A hovered element now
gets a light grey background, but does not change the focused element.
It's possible to have two highlighted search results: one that is
focused (via keyboard) and one that is hovered (via mouse). Pressing
enter will activate the focused link; clicking will activate the hovered
link. This matches up with how Firefox and Chrome handle suggestions in
their URL bar, and avoids stray mouse movements changing the focus.

Selecting tabs is now done with left/right arrows while any search
result is focused. The visibility of results on each search tab is
controlled with the "active" class, rather than by setting display: none
directly. Note that the old code kept track of highlighted search
element when tabbing back and forth. The new code doesn't.
@jsha jsha force-pushed the focus-search-results2 branch from 2ffc815 to b615c0c Compare May 13, 2021 20:05
@GuillaumeGomez
Copy link
Member

Let's go forward with this then. Really can't wait for this improvement to get merged! :)

@bors: r+

@bors
Copy link
Contributor

bors commented May 18, 2021

📌 Commit b615c0c has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request May 18, 2021
Rollup of 7 pull requests

Successful merges:

 - rust-lang#84462 (rustdoc: use focus for search navigation)
 - rust-lang#85251 (Make `const_generics_defaults` not an incomplete feature)
 - rust-lang#85404 (Backport 1.52.1 release notes)
 - rust-lang#85407 (Improve display for "copy-path" button, making it more discreet)
 - rust-lang#85423 (Don't require cmake on Windows when LLVM isn't being built)
 - rust-lang#85428 (Add x.py pre-setup instructions)
 - rust-lang#85442 (fix typo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c9b6bb9 into rust-lang:master May 18, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 18, 2021
@jsha jsha deleted the focus-search-results2 branch May 18, 2021 21:10
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 19, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 19, 2021
RalfJung added a commit to RalfJung/rust that referenced this pull request May 19, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 20, 2021
…eck, r=jsha

Extend escape key check

Since rust-lang#84462 has been merged, we can now extend the `escape-key` test.

r? `@jsha`
jsha added a commit to jsha/rust that referenced this pull request May 24, 2021
This was broke in rust-lang#84462 by modifying a style that applied both to
searches and to module items (and other tables).
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 24, 2021
Restore sans-serif font for module items.

This was broke in rust-lang#84462 by modifying a style that applied both to
searches and to module items (and other tables).

Fixes rust-lang#85616.
Fixes rust-lang#85545.

r? `@camelid`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 30, 2022
…-padding-bottom, r=Dylan-DPC

rustdoc: remove unnecessary CSS `.search-results { padding-bottom }`

There's nothing underneath it anyway. The conversation on rust-lang#84462 never really spelled out why it was added.
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…-padding-bottom, r=Dylan-DPC

rustdoc: remove unnecessary CSS `.search-results { padding-bottom }`

There's nothing underneath it anyway. The conversation on rust-lang#84462 never really spelled out why it was added.
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