Eui: [a11y] Update react-beautiful-dnd to fix screen reader announcements

Created on 14 Oct 2019  ·  30Comments  ·  Source: elastic/eui

https://github.com/atlassian/react-beautiful-dnd/pull/1385 improved support for screen readers with react-beautiful-dnd. They moved away from aria-roledescription which only seems to be supported in Mac VoiceOver and not a lot of other clients.

We should update react-beautiful-dnd to improve our accessibility.

accessibility good first issue
Source
picture Zacqary

Most helpful comment

My first thought is to use Jest's per-test mocking (jest.mock) to replace useLayoutEffect with useEffect in the affected test files.

If that doesn't work, please push up your branch and we can help explore other options.

picture chandlerprall on 5 Mar 2020
👍2

All 30 comments

We should probably move react-beautiful-dnd to be a peerDependency, as well.

/cc @thompsongl for thoughts

chandlerprall picture chandlerprall on 15 Oct 2019

+1 for peerDependency.

That PR merged into a feature branch, which in turn merged into the 12.0 feature branch. So the improvements won't be available until 12.0 stable is released (beta-4 was released last week).

We're still on 10.x and they rewrote some internals in 11.x so we'll have some upgrade work to do for sure.

thompsongl picture thompsongl on 15 Oct 2019
❤1

Hey @elastic/eui-design folks, did this ever make it's way into EUI/Kibana that you know of?

jasonrhodes picture jasonrhodes on 2 Mar 2020

@jasonrhodes No, EUI is still on 10.x. Kibana has additional app-specific deps on 8.x and 12.x.
Just checked and they've now released v13 with _even more_ a11y improvements

thompsongl picture thompsongl on 2 Mar 2020
👍2

Thanks for the update, @thompsongl !

jasonrhodes picture jasonrhodes on 3 Mar 2020

Just curious, so is EUI looking to make react-beautiful-dnd (now on v13) a peer dependency and update all a11y improvements to match the current version?

If it is, I would be interested in picking this issue up.

LeonY1 picture LeonY1 on 4 Mar 2020

@LeonY1 yes, that is a good summary of what is needed to resolve this issue. I'll assign you, but feel free to ask any questions you may have about our implementation/wrapping of react-beautiful-dnd, specifics around keyboard/screen reader accessibility, etc.

Depending on where we are at with other projects, a detailed review of a PR may be delayed, but we'll definitely get to it if you put one together!

chandlerprall picture chandlerprall on 4 Mar 2020

Where can I view an example of the Draggable Components? I tried looking at the docs page for the data grid; however, I couldn't find such components regarding the toolbar.

Seeing as how there are major changes with v12 which cause major changes, I will probably start by updating it to v11 just to sequentially update it.

Things that will be changed for v12:

  • We are no longer using aria-roledescription for lift instructions. Please now use <DragDropContext /> | liftInstruction. This is what the PR was originally asking for.
  • We have renamed our data-* attributes. So if you were using them (perhaps in a test), then things will break for you. This means I will change the draggable.test.tsx and droppable.test.tsx
  • Patching onKeyDown, onMouseDown or onTouchStart in DragHandleProps. These event handlers have been removed to support our new sensor approach, and ultimately for good cloning and virtual list support. I don't think this ends up changing anything (correct me if I'm wrong) as they were not previously used in the code. However, dependencies on eui may break because of this. I'm not entirely how much others have extensively used these handlers.

I will provide more updates once I complete these.

LeonY1 picture LeonY1 on 4 Mar 2020

Our drag-and-drop documentation can be found at https://elastic.github.io/eui/#/display/drag-and-drop (or https://localhost:8030/eui/#/display/drag-and-drop when running yarn start for local development)

chandlerprall picture chandlerprall on 4 Mar 2020

I don't think this ends up changing anything (correct me if I'm wrong) as they were not previously used in the code. However, dependencies on eui may break because of this. I'm not entirely how much others have extensively used these handlers.

This sounds correct; we'd be marking the EUI release after this update as a breaking change anyway, so any necessary around these methods is fine.

chandlerprall picture chandlerprall on 4 Mar 2020

Another thing that will be changing is that DraggableProps themselves won't have children anymore in favor of a new type which will also be including a DraggableRubric. This is one of the major changes to the codebase.

It can be found in the changelog.

LeonY1 picture LeonY1 on 4 Mar 2020

Switching to v11 has caused the test draggable.test.tsx and droppable.test.tsx to break. Not entirely sure why. My guess is that it is related to Enzyme and React Hooks (from react-beautiful-dnd); however, I'm not entirely sure if this is the reason.

What would be the best way to proceed in ensuring that the test suites still run?

LeonY1 picture LeonY1 on 5 Mar 2020

What would be the best way to proceed in ensuring that the test suites still run?

It really depends on what the cause of the failures, and React hooks are certainly a potential source. What error messages do you receive from failing tests?

thompsongl picture thompsongl on 5 Mar 2020 
 FAIL  src/components/drag_and_drop/draggable.test.tsx
  ● EuiDraggable › is rendered

    Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial
, non-hydrated UI and the intended UI. To avoid this, useLayoutEffect should only be used in components that render exclusively on the client. See https://fb.me/react-uselayouteffect-s
sr for common fixes.%s

      2 | // failed prop types check.
      3 | console.error = message => {
    > 4 |   throw new Error(message);
        |         ^
      5 | };
      6 | 

      at BufferedConsole.Object.<anonymous>.console.error (scripts/jest/setup/throw_on_console_error.js:4:9)
      at warningWithoutStack (node_modules/react-dom/cjs/react-dom-server.node.development.js:76:32)
      at warning (node_modules/react-dom/cjs/react-dom-server.node.development.js:155:27)
      at Object.useLayoutEffect (node_modules/react-dom/cjs/react-dom-server.node.development.js:1416:3)
      at useLayoutEffect (node_modules/react/cjs/react.development.js:1635:21)
      at useStyleMarshal (node_modules/react-beautiful-dnd/dist/react-beautiful-dnd.cjs.js:5053:3)
      at App (node_modules/react-beautiful-dnd/dist/react-beautiful-dnd.cjs.js:6856:22)
      at processChild (node_modules/react-dom/cjs/react-dom-server.node.development.js:3204:14)
      at resolve (node_modules/react-dom/cjs/react-dom-server.node.development.js:3124:5)
      at ReactDOMServerRenderer.render (node_modules/react-dom/cjs/react-dom-server.node.development.js:3598:22)
      at ReactDOMServerRenderer.read (node_modules/react-dom/cjs/react-dom-server.node.development.js:3536:29)
      at Object.renderToStaticMarkup (node_modules/react-dom/cjs/react-dom-server.node.development.js:4261:27)
      at Object.renderToStaticMarkup [as render] (node_modules/enzyme-adapter-react-16/src/ReactSixteenAdapter.js:742:31)
      at render (node_modules/enzyme/src/render.js:21:25)
      at Object.<anonymous> (src/components/drag_and_drop/draggable.test.tsx:12:23)

  ● EuiDraggable › can be given ReactElement children

    Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial, non-hydrated UI and the intended UI. To avoid this, useLayoutEffect should only be used in components that render exclusively on the client. See https://fb.me/react-uselayouteffect-ssr for common fixes.%s

      2 | // failed prop types check.
      3 | console.error = message => {
    > 4 |   throw new Error(message);
        |         ^
      5 | };
      6 | 

      at BufferedConsole.Object.<anonymous>.console.error (scripts/jest/setup/throw_on_console_error.js:4:9)
      at warningWithoutStack (node_modules/react-dom/cjs/react-dom-server.node.development.js:76:32)
      at warning (node_modules/react-dom/cjs/react-dom-server.node.development.js:155:27)
      at Object.useLayoutEffect (node_modules/react-dom/cjs/react-dom-server.node.development.js:1416:3)
      at useLayoutEffect (node_modules/react/cjs/react.development.js:1635:21)
      at useStyleMarshal (node_modules/react-beautiful-dnd/dist/react-beautiful-dnd.cjs.js:5053:3)
      at App (node_modules/react-beautiful-dnd/dist/react-beautiful-dnd.cjs.js:6856:22)
      at processChild (node_modules/react-dom/cjs/react-dom-server.node.development.js:3204:14)
      at resolve (node_modules/react-dom/cjs/react-dom-server.node.development.js:3124:5)
      at ReactDOMServerRenderer.render (node_modules/react-dom/cjs/react-dom-server.node.development.js:3598:22)
      at ReactDOMServerRenderer.read (node_modules/react-dom/cjs/react-dom-server.node.development.js:3536:29)
      at Object.renderToStaticMarkup (node_modules/react-dom/cjs/react-dom-server.node.development.js:4261:27)
      at Object.renderToStaticMarkup [as render] (node_modules/enzyme-adapter-react-16/src/ReactSixteenAdapter.js:742:31)
      at render (node_modules/enzyme/src/render.js:21:25)
      at Object.<anonymous> (src/components/drag_and_drop/draggable.test.tsx:27:23)

Similar cases happen for droppable.test.tsx and drag_drop_context.test.tsx. Please let me know if this is fixable using Enzyme.

I think it sparks from the useStyleMarshal hook but the warning is in relation to the useLayoutEffect hook that they use.

LeonY1 picture LeonY1 on 5 Mar 2020

My first thought is to use Jest's per-test mocking (jest.mock) to replace useLayoutEffect with useEffect in the affected test files.

If that doesn't work, please push up your branch and we can help explore other options.

chandlerprall picture chandlerprall on 5 Mar 2020
👍2

I think that fixes the issue. I used React.useLayoutEffect = jest.fn(React.useEffect); and that test passed. However, I'm trying to see the best way to do this immutably.

LeonY1 picture LeonY1 on 5 Mar 2020

However, I'm trying to see the best way to do this immutably.

We've used this pattern in other code before:

jest.mock(
  'react',
  () => {
    const react = require.actual('react');
    return {
      ...react,
      useLayoutEffect: react.useEffect,
    };
  }
)

This tells Jest to use the object returned by the function when react is imported, but short-circuits that same mock when executing the function and loads the real react with require.actual, replacing the value of useLayoutEffect on the created module object.

Note: I didn't try running the above code, I may have a function name wrong or bad syntax somewhere.

chandlerprall picture chandlerprall on 5 Mar 2020

After further research, the issue still happens even with a quick check (still looking into jest.requireActual() to mock the hook). However, this only happens for the render test. The mount test passes successfully. These tests have been conducted on the most outer component drag_drop_context.test.tsx.

Update: I have tried this with droppable.test.tsx and same issue. Enzyme's render can't render the component. However, the components can be mounted.

test('is rendered', () => {
    React.useLayoutEffect = React.useEffect;
    const handler = jest.fn();
    const component = render(
      <EuiDragDropContext onDragEnd={handler} {...requiredProps}>
        <div />
      </EuiDragDropContext>
    );

    expect(component).toMatchSnapshot();
  });
LeonY1 picture LeonY1 on 5 Mar 2020

Because of this issue, I'll start preparing to push my branch to look into more options.

LeonY1 picture LeonY1 on 5 Mar 2020

We had a similarish issue with enzyme in code_block.test.tsx, and swapped it out for a custom ReactDOM.render call and wrote snapshotCodeBlock for snapshots. Might be needed here as well.

chandlerprall picture chandlerprall on 5 Mar 2020

It seems like I'll be using something similar to this for the drag_drop_context.

Sorry, I'm having some trouble trying to understand what the find is looking for for the component. Could you please explain?

function snapshotCodeBlock(component: ReactWrapper) {
  // Get the Portal's sibling and return its html
  const renderedHtml = component.find('Portal + *').html();
  const container = document.createElement('div');
  container.innerHTML = renderedHtml;
  return container.firstChild;
}
LeonY1 picture LeonY1 on 6 Mar 2020

No worries! I've intentionally been somewhat ambiguous in every response so it's not _here's a list of things to follow exactly_ - questions are more than welcome!

That find is pretty specific to how EuiCodeBlock works, given:

          {createPortal(children, this.codeTarget)}
          <span {...wrapperProps}>{codeSnippet}</span>

it has to first find the Portal component (returned by createPortal) and then find its sibling (the span), which uses the CSS selector Portal + * (could have been Portal + span to be a bit more explicit).

For your test, I _think_ it should be const renderedHtml = component.html();

chandlerprall picture chandlerprall on 6 Mar 2020

Thanks for the help! Upgrading to v11 has been successful. Now I'll be trying to upgrade to v12.

LeonY1 picture LeonY1 on 6 Mar 2020

Switching to v12 has caused some more issues to the draggable component. After doing some research on it, this only applies to the draggable component with Enzyme.

 ● EuiDraggable › is rendered

    Error: Uncaught [Error: %creact-beautiful-dnd

    %cA setup problem was encountered.

    > Invariant failed: Draggable[id: testDraggable]: Unable to find drag handle

    %c👷‍ This is a development only message. It will be removed in production builds.]

      2 | // failed prop types check.
      3 | console.error = message => {
    > 4 |   throw new Error(message);
        |         ^
      5 | };
      6 | 

      at reportException (node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:66:24)
      at invokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:209:9)
      at HTMLUnknownElementImpl._dispatch (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:119:9)
      at HTMLUnknownElementImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:82:17)
      at HTMLUnknownElementImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:30:27)
      at HTMLUnknownElement.dispatchEvent (node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:157:21)
      at Object.invokeGuardedCallbackDev (node_modules/react-dom/cjs/react-dom.development.js:385:16)
      at invokeGuardedCallback (node_modules/react-dom/cjs/react-dom.development.js:440:31)
      at flushPassiveEffectsImpl (node_modules/react-dom/cjs/react-dom.development.js:25392:7)
      at unstable_runWithPriority (node_modules/react-dom/node_modules/scheduler/cjs/scheduler.development.js:697:12)
      at CustomConsole.Object.<anonymous>.console.error (scripts/jest/setup/throw_on_console_error.js:4:9)
      at VirtualConsole.on.e (node_modules/jsdom/lib/jsdom/virtual-console.js:29:45)
      at reportException (node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:70:28)
      at invokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:209:9)
      at HTMLUnknownElementImpl._dispatch (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:119:9)
      at HTMLUnknownElementImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:82:17)
      at HTMLUnknownElementImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:30:27)
      at HTMLUnknownElement.dispatchEvent (node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:157:21)
      at Object.invokeGuardedCallbackDev (node_modules/react-dom/cjs/react-dom.development.js:385:16)
      at invokeGuardedCallback (node_modules/react-dom/cjs/react-dom.development.js:440:31)
      at flushPassiveEffectsImpl (node_modules/react-dom/cjs/react-dom.development.js:25392:7)
      at unstable_runWithPriority (node_modules/react-dom/node_modules/scheduler/cjs/scheduler.development.js:697:12)
      at runWithPriority$2 (node_modules/react-dom/cjs/react-dom.development.js:12149:10)
      at flushPassiveEffects (node_modules/react-dom/cjs/react-dom.development.js:25361:12)
      at Object.<anonymous>.flushWork (node_modules/react-dom/cjs/react-dom-test-utils.development.js:1041:10)
      at Object.act (node_modules/react-dom/cjs/react-dom-test-utils.development.js:1155:9)
      at act (node_modules/enzyme-adapter-react-16/src/ReactSixteenAdapter.js:346:13)
      at Object.wrapAct [as render] (node_modules/enzyme-adapter-react-16/src/ReactSixteenAdapter.js:415:16)
      at new render (node_modules/enzyme/src/ReactWrapper.js:115:16)
      at mount (node_modules/enzyme/src/mount.js:10:10)
      at Object.<anonymous> (src/components/drag_and_drop/draggable.test.tsx:28:23)

Solution could be found here: https://github.com/atlassian/react-beautiful-dnd/issues/1203#issuecomment-486987782

However, this introduces new peer dependencies for @storybook/addon-storyshots and enzyme-to-json. I wanted to understand what would be the best plan of action here.

Update: After further review the comment addresses innerRef which might not make sense here. However, I will try this first.

Update 2: I still can't seem to get it working even though I'm using them as dev dependencies.

LeonY1 picture LeonY1 on 9 Mar 2020

@chandlerprall I was wondering if you could help me out with this.

LeonY1 picture LeonY1 on 12 Mar 2020

Happy to take a look! Can you push up your branch with the changes so far?

chandlerprall picture chandlerprall on 12 Mar 2020

Here is the pull request: https://github.com/elastic/eui/pull/3064

LeonY1 picture LeonY1 on 13 Mar 2020

Sorry for being a little inactive recently, many changes have sparked up for my hackathon which will be in the next couple of weeks. I was wondering if I could still get help in trying to fix the draggable component issue for the test.

LeonY1 picture LeonY1 on 16 Mar 2020

I pulled your branch and ran

yarn lint
yarn test-unit

with no issues, were you able to solve the issue, or am I missing something?

chandlerprall picture chandlerprall on 16 Mar 2020

I didn't end up pushing with the errors because I wasn't sure if I was supposed to? However, I can push up that version to see if you can help out.

LeonY1 picture LeonY1 on 16 Mar 2020
Was this page helpful?
0 / 5 - 0 ratings

Related issues

chandlerprall picture chandlerprall  ·  3Comments
roberto910907 picture roberto910907  ·  3Comments
muhammadtalhas picture muhammadtalhas  ·  3Comments
miukimiu picture miukimiu  ·  3Comments
chetanmajithiya picture chetanmajithiya  ·  4Comments