algolia / instantsearch

⚡️ Libraries for building performant and instant search and recommend experiences with Algolia. Compatible with JavaScript, TypeScript, React and Vue.
https://www.algolia.com/doc/guides/building-search-ui/what-is-instantsearch/js/
MIT License
3.72k stars 521 forks source link

Render of refinementList creates accessibility challenges #4499

Closed bobprokop closed 1 year ago

bobprokop commented 4 years ago

🐛 When refinementList widgets are rendered, the innerHTML of some elements are updated. These DOM changes cause loss of focus from the currently active element. Natural tabbing order is lost, creating accessibility issues for keyboard users.

🔍 Bug reproduction

Steps to reproduce the behavior:

  1. Go to https://community.algolia.com/instantsearch.js/v2/widgets/refinementList.html#example
  2. Click on Any checkbox option -- try "Apple" -- which has 442 results associated with it in the example, making it the 5th item in the list.
  3. The "Apple" checkbox is now checked. It should have received (and retained) the focus event.
  4. Using your keyboard, try tabbing to the next option. Since the refinementList widget re-sorts the list (moving your chosen option to the top), tab should move you to the next checkbox -- in this case "Insignia".
  5. When pressing tab, focus event now moves to the option that originally followed your chosen option in the list. In this case, that is "GE", with 394 results.
  6. Whenever you act on an option (checkbox) in the refinementList widget, the focus event moves to the <body> tag due to how the DOM is re-shuffled upon each render. Try opening your console and then checking an option in the refinementList widget. Now check document.activeElement in your console -- you will see that the <body> tag now has received focus due to innerHTML changes.

Live reproduction:

https://codesandbox.io/s/github/algolia/create-instantsearch-app/tree/templates/instantsearch.js

💭 Expected behavior

Natural tabbing order should be respected and preserved whenever refinementList widget is rendered.

🖥 Screenshots

Environment

Additional context

There is a discussion of the issue here: https://discourse.algolia.com/t/prevent-sort-of-options-in-refinementlist/10267. Retaining native tab order by diffing DOM elements using a method such as Preact will work (see example here: https://codesandbox.io/s/reverent-shaw-7tyve?file=/src/app.js) -- but since natural tab order is such an important core accessibility issue (see: https://www.w3.org/TR/2008/REC-WCAG20-20081211/#navigation-mechanisms-focus-order) this behavior should be available OOB without the need to implement a DOM diffing solution such as Preact, React, Vue, etc.

francoischalifour commented 4 years ago

Thank you for opening this issue — this is a problem that I'd really like to solve but that is not trivial because of our internal templating system.

I explained why is happens in a Discourse thread but let me expand and give more context here for all contributors.

InstantSearch rendering system

InstantSearch provides multiple layers to allow users to build search UIs in a basic way, or in a advanced way.

Things go wrong at this last layer: templates. Let's take the refinementList widget as an example.

Why does this happen?

When we render the RefinementList component, we retrieve the item template that the user provided. This template is a string that is injected in the page using 'Preact's dangerouslySetInnerHTML.

dangerouslySetInnerHTML acts as a hard DOM reset and browsers are not able to keep track of which DOM elements has become which one, which results in focus lost, and therefore accessibility issues. This is why frameworks like React, Vue, etc. do some DOM diff computation to reduce DOM changes and have the concept of keys. They don't reset and recompute the whole DOM, they tell browser what are the minimal changes required to re-render the UI, which allow browsers to keep track of DOM elements changes.

User-land fix

Right now, the only way to prevent this accessibility issue user-land is to use connectRefinementList and a UI library that handles UI diffing, like this Preact example.

Internal fix

Fixing this behavior internally is tricky because we need to rework our entire templating system.

We still have the possibility to use Preact components as default templates, and to switch to string user-defined templates when provided. This means that the default implementation of refinementList will be accessible, but as soon as users modify the rendering, they will get back to the current behavior, unable to keep focus between renders.

dhayab commented 1 year ago

This should be working as expected starting from instantsearch@4.46.0 which deprecated string-based templates and introduced tagged templates with htm.