-
Notifications
You must be signed in to change notification settings - Fork 329
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
Add search-as-you-type (inline search results) feature #2093
Changes from all commits
45b6447
1a1112b
aee0d92
ead8b4e
37143ed
bdf3385
e531e70
c9b5daa
1f522ed
1db1290
710b383
66c93ca
00bb0af
5b06319
c203489
8f3de69
f40c6ff
5132816
19bab1c
4388a76
07d4b62
49025fa
78abe78
23d2db2
be4ea8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -261,6 +261,7 @@ var addEventListenerForSearchKeyboard = () => { | |
// also allow Escape key to hide (but not show) the dynamic search field | ||
else if (document.activeElement === input && /Escape/i.test(event.key)) { | ||
toggleSearchField(); | ||
resetSearchAsYouTypeResults(); | ||
} | ||
}, | ||
true, | ||
|
@@ -332,6 +333,170 @@ var setupSearchButtons = () => { | |
searchDialog.addEventListener("click", closeDialogOnBackdropClick); | ||
}; | ||
|
||
/******************************************************************************* | ||
* Inline search results (search-as-you-type) | ||
* | ||
* Immediately displays search results under the search query textbox. | ||
* | ||
* The search is conducted by Sphinx's built-in search tools (searchtools.js). | ||
* Usually searchtools.js is only available on /search.html but | ||
* pydata-sphinx-theme (PST) has been modified to load searchtools.js on every | ||
* page. After the user types something into PST's search query textbox, | ||
* searchtools.js executes the search and populates the results into | ||
* the #search-results container. searchtools.js expects the results container | ||
* to have that exact ID. | ||
*/ | ||
var setupSearchAsYouType = () => { | ||
if (!DOCUMENTATION_OPTIONS.search_as_you_type) { | ||
return; | ||
} | ||
|
||
// Don't interfere with the default search UX on /search.html. | ||
if (window.location.pathname.endsWith("/search.html")) { | ||
return; | ||
} | ||
Comment on lines
+354
to
+357
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should also account for dirhtml builds, which I think (?) will have a url like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I introduced a temporary change to |
||
|
||
// Bail if the Search class is not available. Search-as-you-type is | ||
// impossible without that class. layout.html should ensure that | ||
// searchtools.js loads. | ||
// | ||
// Search class is defined in upstream Sphinx: | ||
// https://github.com/sphinx-doc/sphinx/blob/6678e357048ea1767daaad68e7e0569786f3b458/sphinx/themes/basic/static/searchtools.js#L181 | ||
if (!Search) { | ||
return; | ||
} | ||
|
||
// Destroy the previous search container and create a new one. | ||
resetSearchAsYouTypeResults(); | ||
let timeoutId = null; | ||
let lastQuery = ""; | ||
const searchInput = document.querySelector( | ||
"#pst-search-dialog input[name=q]", | ||
); | ||
|
||
// Initiate searches whenever the user types stuff in the search modal textbox. | ||
searchInput.addEventListener("keyup", () => { | ||
const query = searchInput.value; | ||
|
||
// Don't search when there's nothing in the query textbox. | ||
if (query === "") { | ||
resetSearchAsYouTypeResults(); // Remove previous results. | ||
return; | ||
} | ||
|
||
// Don't search if there is no detectable change between | ||
// the last query and the current query. E.g. the user presses | ||
// Tab to start navigating the search results. | ||
if (query === lastQuery) { | ||
return; | ||
} | ||
|
||
// The user has changed the search query. Delete the old results | ||
// and start setting up the new container. | ||
resetSearchAsYouTypeResults(); | ||
|
||
// Debounce so that the search only starts when the user stops typing. | ||
const delay_ms = 300; | ||
lastQuery = query; | ||
if (timeoutId) { | ||
window.clearTimeout(timeoutId); | ||
} | ||
timeoutId = window.setTimeout(() => { | ||
Search.performSearch(query); | ||
document.querySelector("#search-results").classList.remove("empty"); | ||
timeoutId = null; | ||
}, delay_ms); | ||
}); | ||
}; | ||
|
||
// Delete the old search results container (if it exists) and set up a new one. | ||
// | ||
// There is some complexity around ensuring that the search results links are | ||
// correct because we're extending searchtools.js past its assumed usage. | ||
// Sphinx assumes that searches are only executed from /search.html and | ||
// therefore it assumes that all search results links should be relative to | ||
// the root directory of the website. In our case the search can now execute | ||
// from any page of the website so we must fix the relative URLs that | ||
// searchtools.js generates. | ||
var resetSearchAsYouTypeResults = () => { | ||
if (!DOCUMENTATION_OPTIONS.search_as_you_type) { | ||
return; | ||
} | ||
// If a search-as-you-type results container was previously added, | ||
// remove it now. | ||
let results = document.querySelector("#search-results"); | ||
if (results) { | ||
results.remove(); | ||
} | ||
|
||
// Create a new search-as-you-type results container. | ||
results = document.createElement("section"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gabalafou would appreciate your perspective on what is the best node type for an appearing/disappearing list of search results, and how this can/should/will interact with things like tab focus. |
||
results.classList.add("empty"); | ||
// Remove the container element from the tab order. Individual search | ||
// results are still focusable. | ||
results.tabIndex = -1; | ||
drammock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// When focus is on a search result, make sure that pressing Escape closes | ||
// the search modal. | ||
results.addEventListener("keydown", (event) => { | ||
if (event.key === "Escape") { | ||
drammock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
event.preventDefault(); | ||
event.stopPropagation(); | ||
toggleSearchField(); | ||
resetSearchAsYouTypeResults(); | ||
} | ||
}); | ||
// IMPORTANT: The search results container MUST have this exact ID. | ||
// searchtools.js is hardcoded to populate into the node with this ID. | ||
results.id = "search-results"; | ||
let modal = document.querySelector("#pst-search-dialog"); | ||
modal.appendChild(results); | ||
|
||
// Get the relative path back to the root of the website. | ||
const root = | ||
"URL_ROOT" in DOCUMENTATION_OPTIONS | ||
? DOCUMENTATION_OPTIONS.URL_ROOT // Sphinx v6 and earlier | ||
: document.documentElement.dataset.content_root; // Sphinx v7 and later | ||
|
||
// As Sphinx populates the search results, this observer makes sure that | ||
// each URL is correct (i.e. doesn't 404). | ||
const linkObserver = new MutationObserver(() => { | ||
const links = Array.from( | ||
document.querySelectorAll("#search-results .search a"), | ||
); | ||
// Check every link every time because the timing of when new results are | ||
// added is unpredictable and it's not an expensive operation. | ||
links.forEach((link) => { | ||
link.tabIndex = 0; // Use natural tab order for search results. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Attempting to make sure that inline search results links are focusable here |
||
// Don't use the link.href getter because the browser computes the href | ||
// as a full URL. We need the relative URL that Sphinx generates. | ||
const href = link.getAttribute("href"); | ||
if (href.startsWith(root)) { | ||
// No work needed. The root has already been prepended to the href. | ||
return; | ||
} | ||
link.href = `${root}${href}`; | ||
}); | ||
}); | ||
|
||
// The node that linkObserver watches doesn't exist until the user types | ||
// something into the search textbox. This second observer (resultsObserver) | ||
// just waits for #search-results to exist and then registers | ||
// linkObserver on it. | ||
let isObserved = false; | ||
const resultsObserver = new MutationObserver(() => { | ||
if (isObserved) { | ||
return; | ||
} | ||
const container = document.querySelector("#search-results .search"); | ||
if (!container) { | ||
return; | ||
} | ||
linkObserver.observe(container, { childList: true }); | ||
isObserved = true; | ||
}); | ||
resultsObserver.observe(results, { childList: true }); | ||
}; | ||
|
||
/******************************************************************************* | ||
* Version Switcher | ||
* Note that this depends on two variables existing that are defined in | ||
|
@@ -857,6 +1022,7 @@ documentReady(addModeListener); | |
documentReady(scrollToActive); | ||
documentReady(addTOCInteractivity); | ||
documentReady(setupSearchButtons); | ||
documentReady(setupSearchAsYouType); | ||
documentReady(setupMobileSidebarKeyboardHandlers); | ||
|
||
// Determining whether an element has scrollable content depends on stylesheets, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -291,3 +291,34 @@ def test_breadcrumb_expansion(page: Page, url_base: str) -> None: | |
expect(page.get_by_label("Breadcrumb").get_by_role("list")).to_contain_text( | ||
"Update Sphinx configuration during the build" | ||
) | ||
|
||
|
||
@pytest.mark.a11y | ||
def test_search_as_you_type(page: Page, url_base: str) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @drammock re: the search results container itself receiving focus, it seems like this test should have failed? Does CI not run the a11y tests on different OS / browser permutations? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, seems it should have 🤷🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added #2095 to track the issue. I assume we'll want to address that in a separate PR |
||
"""Search-as-you-type feature should support keyboard navigation. | ||
|
||
When the search-as-you-type (inline search results) feature is enabled, | ||
pressing Tab after entering a search query should focus the first inline | ||
search result. | ||
""" | ||
page.set_viewport_size({"width": 1440, "height": 720}) | ||
page.goto(urljoin(url_base, "/examples/kitchen-sink/blocks.html")) | ||
# Click the search textbox. | ||
searchbox = page.locator("css=.navbar-header-items .search-button__default-text") | ||
searchbox.click() | ||
# Type a search query. | ||
query_input = page.locator("css=#pst-search-dialog input[type=search]") | ||
expect(query_input).to_be_visible() | ||
query_input.type("test") | ||
page.wait_for_timeout(301) # Search execution is debounced for 300 ms. | ||
search_results = page.locator("css=#search-results") | ||
expect(search_results).to_be_visible() | ||
# Navigate with the keyboard. | ||
query_input.press("Tab") | ||
# Make sure that the first inline search result is focused. | ||
actual_focused_content = page.evaluate("document.activeElement.textContent") | ||
first_result_selector = "#search-results .search li:first-child a" | ||
expected_focused_content = page.evaluate( | ||
f"document.querySelector('{first_result_selector}').textContent" | ||
) | ||
assert actual_focused_content == expected_focused_content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user types a query into the search box and then presses
Esc
to close the modal, then the last inline search results should be removed.