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 a button instead of a bar for search #133279

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Nov 21, 2024

r? @GuillaumeGomez

CC @lolbinarycat @liigo

This is an attempt to address three complaints:

  1. The header area takes up a lot of vertical space, and is generally quite "heavy" feeling.

    Fixes [rustdoc] issues of the three-big-buttons #132386

  2. Back and forward are kind of wacky, because there's no "single source of truth" for whether the search mode is open or not. Can't find an issue, but here's a screen recording where I reproduce it.

    screen-record-history-bug.mp4

    https://imgur.com/ZVnHv3o

  3. You can't see the crate picker list, or anything else that we might want to show you, until after you've already typed your search term. Fixes [rustdoc search] allow setting crates to search in before showing results #129537 (also notice the example searches, which exist in Hoogle and this tweak adds)

Preview page: https://notriddle.com/rustdoc-html-demo-15/search-button/std/index.html

Video:

screen-record.mp4

https://imgur.com/c37VxLb

Screenshots:

Desktop
image
image

Tablet
image
image

Mobile
image
image

The last commit of this PR tweaks the Settings and Help icons to give them roughly equal lightness.

icon-tweaks

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 21, 2024
@rust-log-analyzer

This comment has been minimized.

@lolbinarycat
Copy link
Contributor

My immediate concern is how this will look on mobile, expecially with long crate names.

the status quo is already pretty bad.

Screenshot_20241121-005613

@GuillaumeGomez
Copy link
Member

I'm not too sure what to think about it. Turning the search input into a button (to make the search input appear) makes it more tricky to access the search feature.

Also, on mobile it's really not great as the space available is very little.

However I like a few things from this PR:

  1. When we focus on the search focus, we enter the "search page" which allows to give more information to the users.
    • However, on mobile we need a button to leave this "search page"
  2. I like the new layout of the search with the crates filter dropdown above the search input

@rust-log-analyzer

This comment has been minimized.

@notriddle
Copy link
Contributor Author

Turning the search input into a button (to make the search input appear) makes it more tricky to access the search feature.

A little bit, but it does have an explicit "Search" label, and the magnifying glass is very popular anyway.

Also, on mobile it's really not great as the space available is very little.

Here's one concept I came up with to try to claw back more space:
image

When we focus on the search focus, we enter the "search page" which allows to give more information to the users.

I don't want to make focus do that on its own, because it acts badly when tabbing through the page. A switch like that needs to be a button or link.

However, on mobile we need a button to leave this "search page"

The button I have now should act just like a normal link. To exit, use your browser's back button.

It's a very deliberate choice, to avoid bugs like I mentioned under bullet point 2: you should never be in the search page unless the ?search= URL parameter is on there, and the only way to get that URL parameter added is to click the button.

It happens to be implemented in JavaScript, but that should be invisible to the end-user. As far as the interaction model is concerned, that search button should be indistinguishable from an ordinary link. You can even right-click on it and open it in a new tab.

https://imgur.com/ZVnHv3o

@lolbinarycat
Copy link
Contributor

making things behave like links instead of magically doing stuff on hover is much better ux, that's the main reason I abandoned the old popover-on-hover PR.

@GuillaumeGomez
Copy link
Member

The whole mobile experience is big downgrade: the search button is small compared to the rest of the elements, and it's not necessarily obvious what we'll be search. The search input is doing a perfect job for this case.

For desktop, like I said I'm very shared. I like some improvements, but having to click on a button to have access to the search feature is (in my opinion) a big inconvenience. The search in the rust docs is a major feature and something we want to make as accessible as possible. Hiding it behind a button, even if it seems like a small inconvenience is actually (again, since in my opinion) a huge step back.

I don't want to make focus do that on its own, because it acts badly when tabbing through the page. A switch like that needs to be a button or link.

Fair enough. Maybe we could have some "down arrows" button appearing below the search input when focused which would switch to this view?

@lolbinarycat
Copy link
Contributor

Fair enough. Maybe we could have some "down arrows" button appearing below the search input when focused which would switch to this view?

I think anything conditional on focus is an anti-pattern (yes, I'm aware I was the one who suggested that idea in the first place, but after I implemented it, I realized just how annoying it was). Anyone trying to navigate via keyboard input will naturally want to focus that button before selecting it, which will be extremely confusing if it is only visible when a certain other element is focused.

How's this for an interesting compromise: a button that looks like a search bar

@GuillaumeGomez
Copy link
Member

Then what's the point into having a button and not just the input?

@lolbinarycat
Copy link
Contributor

the point is to have a single source of truth for "are we in search mode", and to allow other elements to only show up in search mode.

@GuillaumeGomez
Copy link
Member

Yes but when convenience is sacrificed in exchange of new context, it's not really an improvement.

@lolbinarycat
Copy link
Contributor

What convenience is sacrificed? clicking or tapping on the search button would immediately activate search mode, just like before.

@GuillaumeGomez
Copy link
Member

Except on mobile you need to go back to the previous history page to exit it. Also it adds an extra complexity in JS to handle this button we didn't need before.

@lolbinarycat
Copy link
Contributor

Except on mobile you need to go back to the previous history page to exit it.

trivially easy on andriod, i guess we could add an "exit search mode" button on mobile for the benefit of iOS users.

this is also only really a problem if you frequently start a search and then decide you actually don't want to do a search after all.

Also it adds an extra complexity in JS to handle this button we didn't need before.

the bulk of the extra "complexity" is actually just the logic for building the example queries (which is a feature that I was planning on implementing if noone else did) otherwise it's pretty close to neutral, mostly just stuff being moved around and lazily initialized.

@GuillaumeGomez
Copy link
Member

trivially easy on andriod, i guess we could add an "exit search mode" button on mobile for the benefit of iOS users.

This is not trivially easy. It completely breaks the browsing experience.

the bulk of the extra "complexity" is actually just the logic for building the example queries (which is a feature that I was planning on implementing if noone else did) otherwise it's pretty close to neutral, mostly just stuff being moved around and lazily initialized.

It remains new code added for benefits I'm still not sure to see.

@notriddle
Copy link
Contributor Author

notriddle commented Nov 22, 2024

the search button is small compared to the rest of the elements

Continuing the mobile concept of moving the settings and help buttons up into the top bar, the search and summary might have room for real labels?

image

image

image

The third screenshot is current nightly, meant to show how, even though we're line wrapping more, we're still coming out ahead for vertical screen real estate taken up in total.

and it's not necessarily obvious what we'll be search

How is that different from the current setup? The bar says "Press `s` or `/` to search, `?` for more options..." — what part of that phrase says anything about the scope of the search?

Maybe we could have some "down arrows" button appearing below the search input when focused which would switch to this view?

That would be a mystery meat icon, because its meaning is potentially ambiguous. Clicking on a down arrow next so a search field might do many different things:

  • It might list recently run searches.
  • It might list possible completions of the text that has already been entered.
  • It might lead to an "advanced search" page.

I've taken every opportunity I could find (without significant regressions anywhere else) to reduce and avoid icons without labels, for exactly that reason. They're never as clear as you think they are.

Except on mobile you need to go back to the previous history page to exit it. Also it adds an extra complexity in JS to handle this button we didn't need before.

I can add an explicit return to docs button for mobile users. It's already there, but I didn't think to label it.

image

@rust-log-analyzer

This comment has been minimized.

@notriddle
Copy link
Contributor Author

notriddle commented Nov 27, 2024

@GuillaumeGomez @lolbinarycat I've pushed a new version with most of the changes you seemed happy with:

  • The Search button now changes to an Exit button when you're in search mode. Clicking it puts you back in the document view.
  • The mobile view now pulls settings and help up into the top bar, so Settings and Summary can both have labels. This makes them easier to click, and fixes the Summary button's unfortunate lack of a label on mobile.

@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/search-button branch from d07ca1d to c1a88b9 Compare December 8, 2024 00:45
@notriddle notriddle marked this pull request as ready for review December 8, 2024 01:02
@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2024

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

Some changes occurred in GUI tests.

cc @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/search-button branch from c1a88b9 to 6812adb Compare December 8, 2024 02:13
@lolbinarycat
Copy link
Contributor

I think this is a good change because it makes the behavior where it starts searching automatically less surprising.

The area that can be used for hints is also very useful.

@GuillaumeGomez
Copy link
Member

It's been open for a while and I'd like for the team to discuss about it. I added it to next month's rustdoc meeting agenda.

@notriddle notriddle force-pushed the notriddle/search-button branch from 78ca20f to 39c5d7e Compare January 13, 2025 23:33
@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/search-button branch from 39c5d7e to 98a8c5b Compare January 14, 2025 02:24
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
.......... (130/135)
...FF      (135/135)


/checkout/tests/rustdoc-gui/search-result-display.goml search-result-display... FAILED
[WARNING] `tests/rustdoc-gui/search-result-display.goml` line 37: Delta is 0 for "x", maybe try to use `compare-elements-position` instead?

[ERROR] `tests/rustdoc-gui/search-result-display.goml` line 67: The following errors happened: [expected a width of `515`, found `179`]: for command `assert-size: ("#crate-search", {"width": 515})`
/checkout/tests/rustdoc-gui/search-result-keyword.goml search-result-keyword... FAILED
/checkout/tests/rustdoc-gui/search-result-keyword.goml search-result-keyword... FAILED
[ERROR] `tests/rustdoc-gui/search-result-keyword.goml` line 7: Error: The CSS selector "#search-tabs" was not found: for command `wait-for: "#search-tabs"`
Error: ()
Build completed unsuccessfully in 0:04:54
  local time: Tue Jan 14 03:17:23 UTC 2025
  network time: Tue, 14 Jan 2025 03:17:23 GMT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
5 participants