fix: fix set focus on keyboard navigation (#1151)

qlik-oss · Mar 16, 2023 · 17cf57b · 17cf57b
1 parent 7881c72
commit 17cf57b
Show file tree

Hide file tree

Showing 6 changed files with 93 additions and 12 deletions.
diff --git a/apis/nucleus/src/components/ActionsToolbar.jsx b/apis/nucleus/src/components/ActionsToolbar.jsx
@@ -15,6 +15,7 @@ import More from './ActionsToolbarMore';
 const PREFIX = 'ActionsToolbar';
 
 const classes = {
+  item: `${PREFIX}-item`,
   itemSpacing: `${PREFIX}-itemSpacing`,
   firstItemSpacing: `${PREFIX}-firstItemSpacing`,
   lastItemSpacing: `${PREFIX}-lastItemSpacing`,
@@ -53,13 +54,13 @@ const ActionsGroup = React.forwardRef(
           const isFirstItem = first && ix === 0;
           const isLastItem = last && actions.length - 1 === ix;
           if (isFirstItem && !isLastItem) {
-            cls = [classes.firstItemSpacing];
+            cls = [classes.firstItemSpacing, classes.item];
           }
           if (isLastItem && !isFirstItem) {
-            cls = [...cls, classes.lastItemSpacing];
+            cls = [...cls, classes.lastItemSpacing, classes.item];
           }
           if (!isFirstItem && !isLastItem && cls.length === 0) {
-            cls = [classes.itemSpacing];
+            cls = [classes.itemSpacing, classes.item];
           }
           return (
             <Grid item key={e.key} className={cls.join(' ').trim()}>

diff --git a/apis/nucleus/src/components/listbox/ListBoxInline.jsx b/apis/nucleus/src/components/listbox/ListBoxInline.jsx
@@ -45,6 +45,12 @@ const StyledGrid = styled(Grid, { shouldForwardProp: (p) => !['containerPadding'
     [`& .${classes.listboxWrapper}`]: {
       padding: containerPadding,
     },
+    '&:focus': {
+      boxShadow: `inset 0 0 0 2px ${theme.palette.custom.focusBorder} !important`,
+    },
+    '&:focus-visible': {
+      outline: 'none',
+    },
   })
 );
 

diff --git a/apis/nucleus/src/components/listbox/components/ListBoxRowColumn/components/ListBoxRoot.jsx b/apis/nucleus/src/components/listbox/components/ListBoxRowColumn/components/ListBoxRoot.jsx
@@ -36,6 +36,15 @@ const RowColRoot = styled('div', {
     outline: 'none',
   },
 
+  '& .value': {
+    '&:focus': {
+      boxShadow: `inset 0 0 0 2px ${theme.palette.custom.focusBorder} !important`,
+    },
+    '&:focus-visible': {
+      outline: 'none',
+    },
+  },
+
   [`& .${classes.row}`]: {
     flexWrap: 'nowrap',
     color: theme.listBox?.content?.color ?? theme.palette.text.primary,

diff --git a/...ucleus/src/components/listbox/interactions/__tests__/list-box-keyboard-navigation.test.js b/...ucleus/src/components/listbox/interactions/__tests__/list-box-keyboard-navigation.test.js
@@ -297,20 +297,70 @@ describe('keyboard navigation', () => {
       expect(setKeyboardActive).toHaveBeenCalledWith(true);
     });
 
-    test('should focus container with Escape', () => {
-      const element = { focus: jest.fn() };
+    test('should change focus with Escape on a row', () => {
+      const target = {
+        classList: { contains: jest.fn().mockReturnValue(false) },
+        setAttribute: jest.fn(),
+        blur: jest.fn(),
+        focus: jest.fn(),
+      };
+      const currentTarget = { setAttribute: jest.fn(), blur: jest.fn(), focus: jest.fn() };
       const event = {
-        currentTarget: element,
+        target,
+        currentTarget,
         nativeEvent: { keyCode: 27 },
         preventDefault: jest.fn(),
         stopPropagation: jest.fn(),
       };
       handleKeyDownForListbox(event);
-      expect(element.focus).toHaveBeenCalledTimes(1);
+      expect(target.classList.contains).toHaveBeenCalledTimes(1);
+      expect(target.classList.contains).toHaveBeenCalledWith('listbox-container');
+      expect(target.setAttribute).toHaveBeenCalledTimes(1);
+      expect(target.setAttribute).toHaveBeenCalledWith('tabIndex', '-1');
+      expect(target.blur).toHaveBeenCalledTimes(1);
+      expect(target.focus).not.toHaveBeenCalled();
+
+      expect(currentTarget.setAttribute).toHaveBeenCalledTimes(1);
+      expect(currentTarget.setAttribute).toHaveBeenCalledWith('tabIndex', '0');
+      expect(currentTarget.blur).not.toHaveBeenCalled();
+      expect(currentTarget.focus).toHaveBeenCalledTimes(1);
+
+      expect(setKeyboardActive).not.toHaveBeenCalled();
+
       expect(event.preventDefault).toHaveBeenCalledTimes(1);
       expect(event.stopPropagation).not.toHaveBeenCalled();
+    });
+    test('should change focus with Escape on a listbox', () => {
+      const target = {
+        classList: { contains: jest.fn().mockReturnValue(true) },
+        setAttribute: jest.fn(),
+        blur: jest.fn(),
+        focus: jest.fn(),
+      };
+      const currentTarget = { setAttribute: jest.fn(), blur: jest.fn(), focus: jest.fn() };
+      const event = {
+        target,
+        currentTarget,
+        nativeEvent: { keyCode: 27 },
+        preventDefault: jest.fn(),
+        stopPropagation: jest.fn(),
+      };
+      handleKeyDownForListbox(event);
+      expect(target.classList.contains).toHaveBeenCalledTimes(1);
+      expect(target.classList.contains).toHaveBeenCalledWith('listbox-container');
+      expect(target.setAttribute).not.toHaveBeenCalled();
+      expect(target.blur).not.toHaveBeenCalled();
+      expect(target.focus).not.toHaveBeenCalled();
+
+      expect(currentTarget.setAttribute).not.toHaveBeenCalled();
+      expect(currentTarget.blur).not.toHaveBeenCalled();
+      expect(currentTarget.focus).not.toHaveBeenCalled();
+
       expect(setKeyboardActive).toHaveBeenCalledTimes(1);
       expect(setKeyboardActive).toHaveBeenCalledWith(false);
+
+      expect(event.preventDefault).toHaveBeenCalledTimes(1);
+      expect(event.stopPropagation).not.toHaveBeenCalled();
     });
     test('not matched key should not call event methods', () => {
       const element = { focus: jest.fn() };

diff --git a/apis/nucleus/src/components/listbox/interactions/listbox-keyboard-navigation.js b/apis/nucleus/src/components/listbox/interactions/listbox-keyboard-navigation.js
@@ -90,16 +90,28 @@ export function getListboxInlineKeyboardNavigation({
   constraints,
 }) {
   const focusInsideListbox = (element) => {
-    const fieldElement = element.querySelector('.search input, .value.selector, .value, .ActionsToolbar-* button');
+    const fieldElement = element.querySelector('.search input, .value.selector, .value, .ActionsToolbar-item button');
     setKeyboardActive(true);
     if (fieldElement) {
       fieldElement.focus();
     }
   };
 
-  const focusContainer = (element) => {
-    setKeyboardActive(false);
-    element.focus();
+  const changeFocus = (event) => {
+    if (event.target?.classList.contains('listbox-container')) {
+      // Focus currently on a listbox
+      // Esc on a list box should move the focus to its parent, i.e. a filter pane if any
+      setKeyboardActive(false);
+    } else {
+      // Focus currently on a row
+      // Esc on a row should move the focus to its parent, i.e. a listbox
+      // First unfocus the row
+      event.target?.setAttribute('tabIndex', '-1');
+      event.target?.blur();
+      // Then focus the listbox
+      event.currentTarget?.setAttribute('tabIndex', '0');
+      event.currentTarget?.focus();
+    }
   };
 
   const updateKeyScrollOnHover = (newState) => {
@@ -121,14 +133,17 @@ export function getListboxInlineKeyboardNavigation({
         focusInsideListbox(event.currentTarget);
         break;
       case KEYS.ESCAPE:
-        focusContainer(event.currentTarget);
+        changeFocus(event);
         break;
       case KEYS.ARROW_UP:
         updateKeyScrollOnHover({ up: 1 });
         break;
       case KEYS.ARROW_DOWN:
         updateKeyScrollOnHover({ down: 1 });
         break;
+      case KEYS.ARROW_LEFT:
+      case KEYS.ARROW_RIGHT:
+        return; // let it propagate to top-level
       case KEYS.PAGE_UP:
         updateKeyScrollOnHover({ up: currentScrollIndex.stop - currentScrollIndex.start });
         break;

diff --git a/test/rendering/listbox/listbox.spec.js-snapshots/listbox-key-scroll-linux.png b/test/rendering/listbox/listbox.spec.js-snapshots/listbox-key-scroll-linux.png