Change search to only show results after submit #263
Conversation
lfdebrux
force-pushed
the
ldeb-search-page
branch
12 times, most recently
from
fa89880
to
74f6454
Compare
There was a problem hiding this comment.
Thanks for taking this on, @lfdebrux 👏 I haven't tried it locally yet (that's next on my list), but the screenshots and description of the new behaviour are helpful. I've just got a couple of notes below:
lfdebrux
changed the title
lfdebrux
force-pushed
the
ldeb-search-page
branch
from
5bd225f
to
baf0ba2
Compare
lfdebrux
force-pushed
the
ldeb-search-page
branch
2 times, most recently
from
1355757
to
0150770
Compare
There was a problem hiding this comment.
This looks good. @selfthinker suggested that we either, 1) try to make the modal more constrained to stop the user interacting with the rest of the page, or 2) push the results down into the page like the rest of the content. My feeling is that this option (2) is less prone to confusion and errors, because we're not trying to manipulate the native focus behaviours of the browser or screen readers.
lfdebrux
force-pushed
the
ldeb-search-page
branch
from
0150770
to
4a2627c
Compare
|
Thank you @selfthinker for reviewing! @lfdebrux Here are the 4 minor accessibility issues that came up.
Let me know if I got anything wrong! |
|
Just to clarify, Jaws missed the first heading because of the aria role, not because it wasn't an h1. |
|
Thanks for clarifying @selfthinker! |
lfdebrux
force-pushed
the
ldeb-search-page
branch
from
8aa0cce
to
303f30a
Compare
There was a problem hiding this comment.
I've now had a chance to try this out locally. Nice job - it works really well!
The new nav on the left hand side was a bit of a cause of confusion though. I get that the Search link takes you to the search help text, and the Results link takes you to the results (if there are any), but there's no visual feedback that this is what it's doing, as I guess it was primarily for assistive tech users.
I think having both links visible at the same time is a bit of a strange one. When there are no results, the Results link takes you nowhere, and when there are results, the Search link takes you nowhere. Or rather, it takes you to #how-to-search, but this is within a <div> that has aria-hidden on it, so presumably the anchor won't resolve for assistive tech users in these cases.
I think a better solution would be to either remove the links altogether, or, if it is an accessibility requirement, perhaps combine them into one general "Skip to main content" link. This would link to the ID of the container that would, depending on context, either have help text, or results. I would then make this general skip link visible only on focus, i.e. invisible by default to sighted users. Do you have any thoughts on this?
Would be good to get a frontend dev's eyes on this too 👀 . Otherwise, code-wise, I think this is good to go 👍
| } | ||
| } | ||
| .search-results__inner { |
There was a problem hiding this comment.
I think we can remove some of the padding in this class, which presumably was only in place to allow space for the Close button, which no longer exists. See screenshot, where the right-hand padding (in DevTools view, so smaller screen) means little space for the results text.
There was a problem hiding this comment.
Hmm, I see your point, but I'm reluctant to make a change without input for a designer... I'd want to make sure any change would be making things better rather than worse! I think it's important to keep a certain amount of right padding, but that amount might be different at different viewports.
I think the best thing for now would be to create a new issue about this, is that something you would be happy to do? Sorry to be difficult!
There was a problem hiding this comment.
Totally understood. I'll raise an issue after this PR is merged (as it isn't an issue until then!).
Fantastic, thanks very much!
Ah, I'm guessing you're looking at govuk-developer-docs, which does not using the multipage nav? Removing the single page table of contents sidebar navigation for the sidebar seems fine to me, I will add a commit to do this. |
Ah, this is going to be harder than I thought... the naive way to have an empty sidebar navigation is to override the sidebar content in the core erb template, however for mobile devices this either leaves a table of contents button that does nothing, or omits the search box entirely. So making this change the proper way would involve massively rewriting the core template, which will take time and be risky. I guess the hacky way to do it would be to do it with JavaScript; this will be slightly easier, but not so great maintenance wise. Edited to add: also, I'm not sure how with JavaScript we could detect whether the page is using a multipage nav or not. |
There was a problem hiding this comment.
I think broadly speaking these changes look OK. I'm still a little unsure about the general idea of trying to mimic a server-side search, but I know this has tested well at the accessibility clinic so happy to defer.
I do think the code is getting a little unwieldy and hard to follow, but we should look at refactoring this later (ideally having improved the test coverage).
There are a couple of minor comments that could do with looking at before I approve.
We want to remove the search results dialog and have a more traditional form experience, as this is better for assistive tech. This commit changes the search JavaScript so that: - the search input is now progressively enhanced to submit the query to the search page when the JavaScript loads. - typing into the search input no longer causes results to appear immediately. - the search results are now rendered only on the search page `/search`. - the search query can be loaded into the search input from the URL query string. - the text shown to the user when the search index is loading (which is now more visible) is simpler to understand. This commit also removes the search close button, as it is redundant. Note that with this commit users can now get search analytics from the URL and normal click events, so the custom Google Analytics code is less needed; however this commit keeps it in place for backwards compatibility.
We want the search results on the search page to behave like normal page content, instead of a dialog. Particularly, the skip link should take the user to the search results, and other page furniture like the footer should be visible and tab-accessible. This commit moves the search results HTML from below the search input into the search page template, in the normal content slot. The styles are changed so that instead of being absolutely positioned and obscuring the rest of the page the search results now push other contents down. Note that the search help is not visible when search results are visible, as the help is now hidden by the JavaScript function `showResults()`. This commit also has to copy some Sass from the `toc` stylesheet and generally shuffle things around to get things looking the same as before.
Normally the search input is hidden behind the table of contents button when viewing tech docs on a mobile device. However, for the search page, we want the search input to be immediately visible, both so the user can see their search query, and also so the help text makes sense. This commit fiddles around with the styles so that on the search page, the `html` element always has the class `has-search-results-open`, and the table of contents pane is displayed so that the search box can be displayed. Note that this uses the `:not` psuedo-selector, which is not supported in IE8; I may come back and refactor this later to try and remove that requirement. In browsers that do not support `:not()` the search input is always visible on mobile, which isn't a great hardship however.
This is needed for scenario when `multipage_nav` is false; otherwise the
site build will fail with the error
TypeError at /search
no implicit conversion of nil into String
...
tech-docs-gem/lib/govuk_tech_docs/table_of_contents/heading.rb: in href
@page_url + "#" + @attributes["id"]
Note that we cannot give the heading the expected id `search`, because
that is taken by the search input, and it is improper HTML to have more
than one element with the same id.
If a tech docs site is being served by nginx the path /search will be redirected to /search/. This commit updates the test of whether the browser is on the search page to handle this.
Having role="alert" was stopping screenreaders including JAWS from treating the search results heading as a heading. We could have added the role to a span inside the heading, however after checking guidance from MDN [1] I decided the alert role was not appropriate for this situation, as aria-live="assertive" is sufficient for screenreaders to be aware of changes to the text. [1]: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_alert_role
Pages should have unique titles; this commit adds a small bit of JavaScript to add the search query (if present) to document title of a search page.
A page should always have a h1 heading. The search page has a h1 'Search' when the search results are not visible, but when the search results are visible that is hidden, and the 'Results' heading is a h2. This commit changes the heading 'Results' to be a h1. This does mean that the page will have two h1s if the page loads without CSS, so we also change the search results container to be hidden using the 'hidden' HTML attribute, instead of CSS.
We want page headings to be unique and helpful, to fulfil WCAG success criterion 1.3.1 info and relationships [1]. This commit adds code to updateTitle to add more information the search page results heading. [1]: https://www.w3.org/TR/WCAG21/#info-and-relationships
Co-authored-by: Mark Green <mark.green@digital.cabinet-office.gov.uk>
lfdebrux
force-pushed
the
ldeb-search-page
branch
4 times, most recently
from
fb75afb
to
1ec66c7
Compare
If the search JSON has poor cache headers (which sadly is probably quite likely) then it'll be re-requested when the user hits the search page anyway. If in future we make it easier to have better caching we can always change it back. To keep the tests working we add module functions `isOnSearchPage` and `getQuery` which can be overridden.
Co-authored-by: Mark Green <mark.green@digital.cabinet-office.gov.uk>
lfdebrux
force-pushed
the
ldeb-search-page
branch
from
1ec66c7
to
ea5f829
Compare
|
Thank you to everyone who reviewed this, especially @zapthedingbat, @ChrisBAshton, @m-green, @selfthinker, and @36degrees. I'm aware that there are one or two unresolved comments, we've agreed on Slack though that we can release these changes as-is and fix the outstanding issues in later releases; we're feeling good that these changes are an improvement accessibility-wise and will help users, as well as us and service teams. |
Closes #262, hopefully.
The search user experience is now more accessible for screenreader users.
Users can no longer see search results as you type, they must hit enter or click the search button to see their results.
Adds a new page,
/search, which is used to show search results. The JavaScript previously used to search the index and render results is still used, it has just been limited in scope to only working on this page. The search input is now progressively enhanced to point to the search page.The intention is to try and create a more 'traditional' browser experience, which will make the user experience with assistive tech better.
This is not a breaking change for consumers of the gem, but it is a major change of the search user experience. Previously the search results would appear immediately as the user types, however now the user must hit enter or click the search button to see the search results.
Preview
I've deployed a version of the GOV.UK Frontend tech docs with this branch of govuk_tech_docs to make it easier to preview the changes in this pull request.
You can access the preview at https://deploy-preview-147--govuk-frontend-docs-preview.netlify.app/search/.
If you are a consumer of the
govuk_tech_docsgem you should also try this version locally by changing theGemfileto use this branch:Screenshots
Appearance of search results
Appearance of new search page
If the user visits the search page without a search query, a brief help page appears. If the user does not have JavaScript enabled the search page will not work, so there is also a help message that is visible only when JavaScript is not running.
Work remaining: