From 74de78a414043202ed70c239b2aca74dceb4c1a8 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Sat, 26 Nov 2022 09:52:58 -0700 Subject: [PATCH 1/2] rustdoc: improve popover focus handling JS This commit fixes a few inconsistencies and erratic behavior from the notable traits, settings, and sidebar popups: * It makes it so that pressing Escape closes the mobile sidebar. This is a bit difficult to do on iPhone, but on other setups like desktop tiling window managers, it's easy and makes sense. * It makes sure that pressing escape while a notable trait popover is open focuses the popover's toggle button, instead of leaving nothing focused, since that makes more sense with keyboard navigation. Clicking the settings, help, or sidebar buttons, however, will not focus the notable trait popover toggle button. * It ensures that notable trait and settings popovers are exclusive with the mobile sidebar. Nothing should ever overlap a popover, and there should never be more than one popover open at once. --- src/librustdoc/html/static/js/main.js | 24 ++++++++++++++++++---- src/librustdoc/html/static/js/settings.js | 2 +- src/test/rustdoc-gui/notable-trait.goml | 25 +++++++++++++++++++++++ src/test/rustdoc-gui/pocket-menu.goml | 21 +++++++++++++++++++ src/test/rustdoc-gui/sidebar-mobile.goml | 6 ++++++ 5 files changed, 73 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/html/static/js/main.js b/src/librustdoc/html/static/js/main.js index 2a109ffbc608d..ef1ae68c59eb1 100644 --- a/src/librustdoc/html/static/js/main.js +++ b/src/librustdoc/html/static/js/main.js @@ -202,6 +202,7 @@ function loadCss(cssUrl) { if (event.ctrlKey || event.altKey || event.metaKey) { return; } + window.hideAllModals(); addClass(getSettingsButton(), "rotate"); event.preventDefault(); // Sending request for the CSS and the JS files at the same time so it will @@ -377,7 +378,10 @@ function loadCss(cssUrl) { } ev.preventDefault(); searchState.defocus(); - window.hidePopoverMenus(); + // If the notable traits popover is open, and the user presses Escape, + // reset focus back to the link. + hideNotable(true); + window.hideAllModals(); } function handleShortcut(ev) { @@ -767,6 +771,7 @@ function loadCss(cssUrl) { }; function showSidebar() { + window.hideAllModals(); window.rustdocMobileScrollLock(); const sidebar = document.getElementsByClassName("sidebar")[0]; addClass(sidebar, "shown"); @@ -843,7 +848,7 @@ function loadCss(cssUrl) { // Make this function idempotent. return; } - hideNotable(false); + window.hideAllModals(); const ty = e.getAttribute("data-ty"); const wrapper = document.createElement("div"); wrapper.innerHTML = "
" + window.NOTABLE_TRAITS[ty] + "
"; @@ -1049,6 +1054,18 @@ function loadCss(cssUrl) { return container; } + /** + * Hide popover menus, notable trait tooltips, and the sidebar (if applicable). + * + * This version does not do anything to tweak the focused element. The caller + * should make sure it behaves reasonably. + */ + window.hideAllModals = function() { + hideSidebar(); + window.hidePopoverMenus(); + hideNotable(false); + }; + /** * Hide all the popover menus. */ @@ -1056,7 +1073,6 @@ function loadCss(cssUrl) { onEachLazy(document.querySelectorAll(".search-form .popover"), elem => { elem.style.display = "none"; }); - hideNotable(false); }; /** @@ -1081,7 +1097,7 @@ function loadCss(cssUrl) { function showHelp() { const menu = getHelpMenu(true); if (menu.style.display === "none") { - window.hidePopoverMenus(); + window.hideAllModals(); menu.style.display = ""; } } diff --git a/src/librustdoc/html/static/js/settings.js b/src/librustdoc/html/static/js/settings.js index 5256ae916a771..589bfc79360ce 100644 --- a/src/librustdoc/html/static/js/settings.js +++ b/src/librustdoc/html/static/js/settings.js @@ -268,7 +268,7 @@ event.preventDefault(); const shouldDisplaySettings = settingsMenu.style.display === "none"; - window.hidePopoverMenus(); + window.hideAllModals(); if (shouldDisplaySettings) { displaySettings(); } diff --git a/src/test/rustdoc-gui/notable-trait.goml b/src/test/rustdoc-gui/notable-trait.goml index aab3b11433e93..7e24af47ee817 100644 --- a/src/test/rustdoc-gui/notable-trait.goml +++ b/src/test/rustdoc-gui/notable-trait.goml @@ -200,12 +200,14 @@ move-cursor-to: "//*[@class='notable popover']" assert-count: ("//*[@class='notable popover']", 1) press-key: "Escape" assert-count: ("//*[@class='notable popover']", 0) +assert: "#method\.create_an_iterator_from_read .notable-traits:focus" // Check that clicking outside works. click: "//*[@id='method.create_an_iterator_from_read']//*[@class='notable-traits']" assert-count: ("//*[@class='notable popover']", 1) click: ".search-input" assert-count: ("//*[@class='notable popover']", 0) +assert-false: "#method\.create_an_iterator_from_read .notable-traits:focus" // Check that pressing tab over and over works. click: "//*[@id='method.create_an_iterator_from_read']//*[@class='notable-traits']" @@ -249,3 +251,26 @@ click: "#settings-menu a" press-key: "Escape" // We ensure we didn't come back to the previous focused item. assert-window-property-false: {"scrollY": |scroll|} + +// Opening the mobile sidebar should close the popover. +size: (650, 600) +click: "//*[@id='method.create_an_iterator_from_read']//*[@class='notable-traits']" +assert-count: ("//*[@class='notable popover']", 1) +click: ".sidebar-menu-toggle" +assert: "//*[@class='sidebar shown']" +assert-count: ("//*[@class='notable popover']", 0) +assert-false: "#method\.create_an_iterator_from_read .notable-traits:focus" +// Clicking a notable popover should close the sidebar. +click: "//*[@id='method.create_an_iterator_from_read']//*[@class='notable-traits']" +assert-count: ("//*[@class='notable popover']", 1) +assert-false: "//*[@class='sidebar shown']" + +// Also check the focus handling for the help button. +size: (1100, 600) +reload: +assert-count: ("//*[@class='notable popover']", 0) +click: "//*[@id='method.create_an_iterator_from_read']//*[@class='notable-traits']" +assert-count: ("//*[@class='notable popover']", 1) +click: "#help-button a" +assert-count: ("//*[@class='notable popover']", 0) +assert-false: "#method\.create_an_iterator_from_read .notable-traits:focus" diff --git a/src/test/rustdoc-gui/pocket-menu.goml b/src/test/rustdoc-gui/pocket-menu.goml index fb63ea62a48af..c3649dc7bda20 100644 --- a/src/test/rustdoc-gui/pocket-menu.goml +++ b/src/test/rustdoc-gui/pocket-menu.goml @@ -75,3 +75,24 @@ assert-css: ( ) compare-elements-css: ("#help-button .popover", "#help-button .top", ["border-color"]) compare-elements-css: ("#help-button .popover", "#help-button .bottom", ["border-color"]) + +// Opening the mobile sidebar should close the settings popover. +size: (650, 600) +click: "#settings-menu a" +assert-css: ("#settings-menu .popover", {"display": "block"}) +click: ".sidebar-menu-toggle" +assert: "//*[@class='sidebar shown']" +assert-css: ("#settings-menu .popover", {"display": "none"}) +// Opening the settings popover should close the sidebar. +click: "#settings-menu a" +assert-css: ("#settings-menu .popover", {"display": "block"}) +assert-false: "//*[@class='sidebar shown']" + +// Opening the settings popover at start (which async loads stuff) should also close. +reload: +click: ".sidebar-menu-toggle" +assert: "//*[@class='sidebar shown']" +assert-false: "#settings-menu .popover" +click: "#settings-menu a" +assert-false: "//*[@class='sidebar shown']" +wait-for: "#settings-menu .popover" diff --git a/src/test/rustdoc-gui/sidebar-mobile.goml b/src/test/rustdoc-gui/sidebar-mobile.goml index 453873f1b81b6..840091799a778 100644 --- a/src/test/rustdoc-gui/sidebar-mobile.goml +++ b/src/test/rustdoc-gui/sidebar-mobile.goml @@ -32,6 +32,12 @@ assert-css: ("//nav[contains(@class, 'sidebar')]//h2/a[text()='In test_docs']/pa click: "body" assert-css: (".sidebar", {"display": "block", "left": "-1000px"}) +// Open the sidebar menu, and make sure pressing Escape closes it. +click: ".sidebar-menu-toggle" +assert-css: (".sidebar", {"left": "0px"}) +press-key: "Escape" +assert-css: (".sidebar", {"display": "block", "left": "-1000px"}) + // Check that the topbar is visible assert-property: (".mobile-topbar", {"clientHeight": "45"}) From c26074afde3dbe5873874647709ccd4ce4b64813 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Sat, 26 Nov 2022 14:32:57 -0700 Subject: [PATCH 2/2] rustdoc: pass "true" to reset focus for notable traits --- src/librustdoc/html/static/js/main.js | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/librustdoc/html/static/js/main.js b/src/librustdoc/html/static/js/main.js index ef1ae68c59eb1..7230df36c075b 100644 --- a/src/librustdoc/html/static/js/main.js +++ b/src/librustdoc/html/static/js/main.js @@ -202,7 +202,7 @@ function loadCss(cssUrl) { if (event.ctrlKey || event.altKey || event.metaKey) { return; } - window.hideAllModals(); + window.hideAllModals(false); addClass(getSettingsButton(), "rotate"); event.preventDefault(); // Sending request for the CSS and the JS files at the same time so it will @@ -378,10 +378,7 @@ function loadCss(cssUrl) { } ev.preventDefault(); searchState.defocus(); - // If the notable traits popover is open, and the user presses Escape, - // reset focus back to the link. - hideNotable(true); - window.hideAllModals(); + window.hideAllModals(true); // true = reset focus for notable traits } function handleShortcut(ev) { @@ -771,7 +768,7 @@ function loadCss(cssUrl) { }; function showSidebar() { - window.hideAllModals(); + window.hideAllModals(false); window.rustdocMobileScrollLock(); const sidebar = document.getElementsByClassName("sidebar")[0]; addClass(sidebar, "shown"); @@ -848,7 +845,7 @@ function loadCss(cssUrl) { // Make this function idempotent. return; } - window.hideAllModals(); + window.hideAllModals(false); const ty = e.getAttribute("data-ty"); const wrapper = document.createElement("div"); wrapper.innerHTML = "
" + window.NOTABLE_TRAITS[ty] + "
"; @@ -1057,13 +1054,12 @@ function loadCss(cssUrl) { /** * Hide popover menus, notable trait tooltips, and the sidebar (if applicable). * - * This version does not do anything to tweak the focused element. The caller - * should make sure it behaves reasonably. + * Pass "true" to reset focus for notable traits. */ - window.hideAllModals = function() { + window.hideAllModals = function(switchFocus) { hideSidebar(); window.hidePopoverMenus(); - hideNotable(false); + hideNotable(switchFocus); }; /**