elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
6.09k stars 829 forks source link

[A11Y] Using nested `EuiPopover` in `EuiModal`: Pressing ESC in child component closes parent #7879

Closed alexwizp closed 1 month ago

alexwizp commented 2 months ago

Describe the problem

Pressing the ESC key within a child component nested inside an EuiPopover unexpectedly closes the parent EuiModal.

Steps to Reproduce:

  1. Open an EuiModal component.
  2. Open an EuiPopover component within the EuiModal.
  3. Focus on the EuiPopover component.
  4. Press the ESC key.

Expected Result:

EuiModal should remain open.

Actual Result:

Pressing the ESC key closes the parent EuiModal.

Additional Information:

Some components we have manual Escape window keydown listeners and some components we rely on our underlying focus trap library :grimacing: the resolution might be as easy as stop-propagation (hopefully), or it might be as difficult as creating a global Escape listener queue somewhere

Test code:

import React, { useState } from 'react';
import {
  EuiPopover,
  EuiPopoverTitle,
  EuiFilterGroup,
  EuiFilterButton,
  EuiSelectable,
  EuiButton,
  EuiModal,
  EuiModalBody,
  EuiModalHeader,
  EuiModalHeaderTitle,
  useGeneratedHtmlId,
} from '@elastic/eui';

export const Demo = () => {
  const [isPopoverOpen, setIsPopoverOpen] = useState(false);
  const [isModalVisible, setIsModalVisible] = useState(false);

  const onButtonClick = () => {
    setIsPopoverOpen(!isPopoverOpen);
  };

  const filterGroupPopoverId = useGeneratedHtmlId({
    prefix: 'filterGroupPopover',
  });

  const [items, setItems] = useState([
    { label: 'Johann Sebastian Bach', checked: 'on' },
    { label: 'Wolfgang Amadeus Mozart', checked: 'on' },
    { label: 'Antonín Dvořák', checked: 'off' },
    { label: 'Dmitri Shostakovich' },
    { label: 'Felix Mendelssohn-Bartholdy' },
    { label: 'Franz Liszt' },
    { label: 'Franz Schubert' },
    { label: 'Frédéric Chopin' },
    { label: 'Georg Friedrich Händel' },
    { label: 'Giuseppe Verdi' },
  ]);

  const button = (
    <EuiFilterButton
      iconType="arrowDown"
      badgeColor="success"
      onClick={onButtonClick}
      isSelected={isPopoverOpen}
      numFilters={items.filter((item) => item.checked !== 'off').length}
      hasActiveFilters={!!items.find((item) => item.checked === 'on')}
      numActiveFilters={items.filter((item) => item.checked === 'on').length}
    >
      Composers
    </EuiFilterButton>
  );

  return (
    <>
      <EuiButton
        onClick={() => {
          setIsPopoverOpen(false);
          setIsModalVisible(true);
        }}
      >
        Show modal
      </EuiButton>

      {isModalVisible && (
        <EuiModal onClose={() => setIsModalVisible(false)}>
          <EuiModalHeader>
            <EuiModalHeaderTitle>Modal title</EuiModalHeaderTitle>
          </EuiModalHeader>

          <EuiModalBody>
            <EuiFilterGroup>
              <EuiPopover
                id={filterGroupPopoverId}
                button={button}
                isOpen={isPopoverOpen}
                closePopover={() => {
                  setIsPopoverOpen(false);
                }}
                panelPaddingSize="none"
              >
                <EuiSelectable
                  allowExclusions
                  searchable
                  searchProps={{
                    placeholder: 'Filter list',
                    compressed: true,
                  }}
                  aria-label="Composers"
                  options={items}
                  onChange={(newOptions) => setItems(newOptions)}
                  loadingMessage="Loading filters"
                  emptyMessage="No filters available"
                  noMatchesMessage="No filters found"
                >
                  {(list, search) => (
                    <div style={{ width: 300 }}>
                      <EuiPopoverTitle paddingSize="s">
                        {search}
                      </EuiPopoverTitle>
                      {list}
                    </div>
                  )}
                </EuiSelectable>
              </EuiPopover>
            </EuiFilterGroup>
          </EuiModalBody>
        </EuiModal>
      )}
    </>
  );
};

Related to: https://github.com/elastic/observability-accessibility/issues/31.

mgadewoll commented 2 months ago

💭 I had a first look at this and I think we could improve EuiModal generically by checking the event target. We're using portals for EuiPopover, meaning that the target for a popover would not be inside the Modal. Both EuiModal and EuiPopover already have stopPropagation applied and my hunch is that the issue lies around the portal and focus trap contexts. For this first look, I checked it against some similar components (EuiSelectable, EuiSelect, EuiSuperSelect, EuiCombobox) so far, but I think this could be a rather light weight alternative to check deeper.

https://github.com/elastic/eui/assets/44670957/c519fbbb-7048-4871-967c-0f33db640eaa