Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popover does not handle keyboard focus of Grid #8098

Closed
knoobie opened this issue Nov 6, 2024 · 2 comments · Fixed by #8102
Closed

Popover does not handle keyboard focus of Grid #8098

knoobie opened this issue Nov 6, 2024 · 2 comments · Fixed by #8102

Comments

@knoobie
Copy link
Contributor

knoobie commented Nov 6, 2024

Description

Adding a Grid inside a Popover results in accessibility problems because I can't focus the Grid with tab - once the Popover is opened.. pressing Tab three times results in a "blue line" (Firefox) of the Grid's top border or nothing (Safari / Chrome).. instead of focusing the Grid's content like normally.

grafik

Once I interact with the mouse - like clicking the first row - it's possible afterwards to use Tab to navigate to the Grid.

grafik

Expected outcome

Grid works all the time :)

Minimal reproducible example

    var popover = new Popover();
    popover.setModal(true);
    popover.setBackdropVisible(true);
    popover.setPosition(PopoverPosition.BOTTOM_END);
    var button = new Button();
    popover.setTarget(button);

    var grid = new Grid<String>();
    grid.addColumn(s -> s).setHeader("Lorem");
    grid.setItems(List.of("1", "2", "3"));
    popover.add(new TextField("1"), new TextField("2"), grid);
    add(button);

Steps to reproduce

  1. Open Popover
  2. Press Tab

Environment

Vaadin version(s): 24.5
OS: MacOS + Windows

Browsers

Chrome, Firefox, Safari

@web-padawan
Copy link
Member

web-padawan commented Nov 7, 2024

Investigated this a bit and the problem is related to the fact that _resetKeyboardNavigation() is not called after the popover is opened - as a result, _headerFocusable is undefined and pressing Tab moves focus to the "focusexit" element. This only happens with Flow counterpart, I wasn't able to reproduce it with the web component.

const firstVisibleRow = [...this.$[section].children].find((row) => row.offsetHeight);
const firstVisibleCell = firstVisibleRow ? [...firstVisibleRow.children].find((cell) => !cell.hidden) : null;
if (firstVisibleRow && firstVisibleCell) {
this[`_${section}Focusable`] = this.__getFocusable(firstVisibleRow, firstVisibleCell);
}

This doesn't happen in case of Dialog because there the grid is only rendered after calling dialog.open().
UPD: if you add(dialog) to the UI instead of auto-attaching it, the same problem can be reproduced there.

@web-padawan
Copy link
Member

web-padawan commented Nov 7, 2024

UPD: looks like the _resetKeyboardNavigation() in the web component can be triggered by _flatSizeChanged observer as a result of clearCache() call when using items property (array data provider) based on isAttached.

So, when using dataProvider instead, this problem can be also reproduced in the web component, see this code:

<vaadin-button id="button">Discount</vaadin-button>

<vaadin-popover for="button" position="bottom-start" modal with-backdrop></vaadin-popover>

<script type="module">
  import '@vaadin/button';
  import '@vaadin/grid';
  import '@vaadin/popover';
  import '@vaadin/text-field';

  const popover = document.querySelector('vaadin-popover');

  popover.renderer = (root) => {
    if (root.firstChild) {
      return;
    }

    root.innerHTML = `
      <vaadin-text-field label="1"></vaadin-text-field>
      <vaadin-text-field label="2"></vaadin-text-field>
      <vaadin-grid size="3">
        <vaadin-grid-column path="name" header="First name"></vaadin-grid-column>
      </vaadin-grid>
    `;

    const grid = root.querySelector('vaadin-grid');
    grid.dataProvider = (_, cb) => {
      const users = [{ name: 'Foo' }, { name: 'Bar' }, { name: 'Baz' }];
      cb(users);
    };
  };

  requestAnimationFrame(() => {
    // Render an invisible grid
    popover.requestContentUpdate();
  });
</script>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants