Fixes #34802 - Pin tabbable version

[sjha4]

We are getting some test failures in katello related to FocusTrap on modals and wizards..setting disableFocusTrap on Modal component to false fixes the test failures for us..We are still getting the "[Error: Your focus-trap must have at least one container with at least one tabbable node in it at all times]" on wizards..and modals without passing disableFocusTrap as false..

From my debugging and googling, the latest version of Tabbable that is 5.3.0 (and onwards) needs some changes to the Modal/Wizard components in Patternfly4 to be able to support tests.

Specifically this: https://github.com/focus-trap/tabbable#testing-in-jsdom

Modal currently has the prop disableFocusTrap that can be set to false and that lets us work around the issue without ever activating the trap. I spoke to folks on PF4 slack and they lock the version of FocusTrap but Focustrap brings in the latest 5.x.x tabbable causing the test failures.

There's an open PF4 issue around this: patternfly/patternfly-react#7288

[theforeman-bot]

Issues: #34802

[theforeman-bot]

Issues: #34802

[evgeni]

packaging wise this makes sense

[evgeni]

@sjha4 this is devel only dependency, right, as I don't see tabbable packaged at all today? Could you add it to the exclude list then please? https://github.com/theforeman/foreman/blob/develop/package-exclude.json

[sjha4]

@sjha4 this is devel only dependency, right, as I don't see tabbable packaged at all today? Could you add it to the exclude list then please? https://github.com/theforeman/foreman/blob/develop/package-exclude.json

@evgeni : https://www.npmjs.com/package/focus-trap is a dependency of patternfly/react-core. And https://www.npmjs.com/package/tabbable is a dependency of focus-trap. I'd think we do package it. It's not technically a dev dependency.

[sjha4]

We might be doing it inside the foreman-js packages..This change probably makes more sense in that repo?

[evgeni]

I was about to say that, yeah.

We have no patternfly in https://github.com/theforeman/foreman-packaging/tree/rpm/develop/packages/foreman, it all comes via foreman-js.

[sjha4]

Strangely tabbable is locked at 5.2.1 which is correct but it doesn't reflect with a npm i in foreman without foreman-js. https://github.com/theforeman/foreman-js/blob/8a7806cd7da51b66388e5dbd3c433df30b944073/packages/vendor-core/package-lock.json#L9887

The package-lock only applies when running directly through foreman-js build locally. We'll need to lock it in foreman for the dev boxes and github CI only..So it should go into dev dependencies as it doesn't need packaging and foreman-js is on the correct version already.

[sjha4]

Updated..Thanks @evgeni for the pointers..

[Ron-Lavi]

Thanks @sjha4 !
I wonder if for now we should just mock tabbable in tests instead of dealing with it in package.json, e.g:

// __mocks__/tabbable.js
const lib = jest.requireActual('tabbable');

const pf = {
  ...lib,
  tabbable: (node, options) => lib.tabbable(node, { ...options, displayCheck: 'none' }),
  focusable: (node, options) => lib.focusable(node, { ...options, displayCheck: 'none' }),
  isFocusable: (node, options) => lib.isFocusable(node, { ...options, displayCheck: 'none' }),
  isTabbable: (node, options) => lib.isTabbable(node, { ...options, displayCheck: 'none' }),
};

module.exports = pf;

[Ron-Lavi]

OK let's get it in, and if needed we could change and add it to the __mock__ dir

[Ron-Lavi]

though need an ack from @theforeman/packaging anyway before merging :P