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

Replace topbar dropdown menus working with JS by <details> #1939

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

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 29, 2022

First, there should be no UI changes. I replaced the dropdown menus in the topbar with <details> instead which work nicely with keyboard events and don't require JS, hence why the JS reduced drastically. Another big advantage is that it'll be possible to collapse/expand menus even without JS (the position of the dropdown will be a bit less good but that's the only downside).

There is one notable change though: in the current version of my changes, the ESCAPE and arrow keys don't work anymore. To expand/collapse a menu entry, you'll need to click on it or press ENTER/SPACE when it's focused.

With these changes, the JS is only here to prevent having two menus expanded at the same time and to still have the dropdown menu position working smoothly.

cc @Nemo157 @jsha

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Nov 29, 2022
@GuillaumeGomez
Copy link
Member Author

Updated the selectors in the tests too. Ready for review now! :)

@notriddle
Copy link
Contributor

notriddle commented Dec 6, 2022

Pressing escape works for closing all rustdoc modals. Having it mysteriously not work here would be confusing.

@GuillaumeGomez
Copy link
Member Author

Good point. I'll put this behaviour back.

@GuillaumeGomez
Copy link
Member Author

The handling of the escape key to close the menu has been put back.

@syphar
Copy link
Member

syphar commented Dec 16, 2022

@Nemo157 @jsha one of you has time for a review?

I could only check everything except the JS, so mostly testing it if you don't have any time

@syphar
Copy link
Member

syphar commented Jan 19, 2023

Some first manual test results

the line between these dropdowns is missing:
grafik

  • old behavior is that after opening the dropdown I can click anywhere else and the menu closes. right now it doesn't close any more.
  • odd thing: after page refresh when I click on the menu the first time, it is shown on the far left for a very short time, and then moves to the correct location.
  • which would be the keyboard events that work in the menu? on a page with release-lists, arrow up & down don't work when the menu is open, and only lead to records being selected. I assume something around the key handling priority?
  • tab-handling is also different (though I'm not sure if people even use this). Old behaviour on the homepage: with open menu I could use tab to switch between the search-field & the menu. Now not any more.

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jan 19, 2023
@jsha
Copy link
Contributor

jsha commented Mar 8, 2023

I think it used to be the case that on mobile, clicking the crate dropdown would take you to the crate page, which has all the same functionality. I tested now and it doesn't - the whole menu opens, even on mobile. Does anyone remember changing that, intentionally or unintentionally? The current behavior is not great, because the menu won't fit well on all mobile screens, and doesn't make efficient use of space.

At any rate, my vote is that we figure out why that's not happening on mobile and fix it, and also make that the behavior on no-JS browsers. Maintaining a whole interactive menu functionality that is supposed to work well both with and without JavaScript sounds like a high maintenance burden, particularly when we have a whole other page (the crate page) that we maintain for the express purpose of using it when the menu system is not available.

@notriddle
Copy link
Contributor

I think #1081 did it, since it got rid of the old href attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants