kuceb / cypress-plugin-tab

A cypress plugin to add a tab command
MIT License
95 stars 11 forks source link

cy.tab() doesn't accept tabindex="-1" elements as focusable #18

Open geebru opened 4 years ago

geebru commented 4 years ago

Sadly I don't have a super clean example of this in a Plunkr or similar but I'm having an issue tabbing out of elements that are focusable through an included [tabindex="-1"].

On navigation to some of our pages, we move the default focus to the titles as a way to skip our logo by default via a -1 tabindex and a focus shift on initialize. Cypress correctly passes these elements as having focus (which is verified via manual testing as well) but trying to tab() out of those elements results in an error from the plugin.

sample error

cy.get('#passwordResetTitle')
   .should('have.focus')
   .tab();

Let me know what other info would be helpful here! Or if I'm using the plugin incorrectly to test these interactions, also let me know that.

Edit: Using cy.focused().tab(); also results in the same behavior. Seems like Cypress sees the elements as focusable, but the plugin differs from what it considers focusable.

kuceb commented 4 years ago

@geebru this is a bug, right now the command parses all keyboard-tabbable elements and expects the subject to be one of them. It should be updated to only check whether the subject is focusable, then find the next tabbable from there.

geebru commented 4 years ago

@Bkucera Is the issue around includeOnlyTabbable: true, this option from a11y.js? I saw their default is false and their demo seems to work as I would expect (tabbing passes -1 items, but if you click on a -1, then tab, it correctly moves to the next (or previous) item).

More than happy to whip up a quick PR but if it's more complex than that I don't want to open a can of worms :)

kuceb commented 4 years ago

the problem is currently the subject is required to be tabbable, but it only has to be focusable, so includeOnlyTabbable won't help. I don't see a clear way to get the next tabbable element from a specific element using the ally.js lib. I think I'll rip out ally.js, and use a different lib like jquery-ui's :tabbable or https://github.com/marklagendijk/jQuery.tabbable

kuceb commented 4 years ago

If you could write up a simple failing test that would help get this fixed

geebru commented 4 years ago

@Bkucera Makes sense - here's a quick test project that tests tabbing before, after, and over a -1 element with the plugin - https://github.com/geebru/cypress-tab-test

Or here's the Cypress spec file directly if that's easier:

context('Keyboard user', () => {
  beforeEach(() => {
    cy.visit('localhost);
  });

  describe('Move focus away from tabindex -1 element', () => {
    beforeEach(() => {
      cy.get('#title')
        .focus()
        .should('have.focus');
    });

    it('Should focus content link after tab', () => {
      cy.tab();

      cy.get('#contentLink')
        .should('have.focus');
    });

    it('Should focus skip link before tab', () => {
      cy.tab({ shift: true });

      cy.get('#skipLink')
        .should('have.focus');
    });
  });

  describe('Tab should skip -1 element', () => {
    it('Follows expected tab order', () => {
      cy.get('body')
        .tab();

      cy.get('#skipLink')
        .should('have.focus');

      cy.tab();

      cy.get('#contentLink')
        .should('have.focus');
    });
  });
});
seanforyou23 commented 4 years ago

Seeing this also. Here's a test that when I use tabindex="0" for the main containers (.non-interactive-elements, [data-disabled-button-tooltip-example], etc.) it works, but when I try to use -1 as a value it breaks. Hoping to use this to verify sending focus to a specific container in response to dynamic content - the overall test is actually working beautifully, just that having to use 0 as a value means all keyboard users tab across this which isn't what I want.

geebru commented 4 years ago

@Bkucera Since Cypress has removed (at least temporarily) the native events item from the active roadmap, is there a chance this issue could be addressed since it sounds like it will remain the only solution for a while? I was hoping native events would kick in and we wouldn't require this plugin anymore but sounds like that's become a bit more hazy of a goal.

Happy to help test things out or help where I can as this is a primary test-type in our tests (primarily accessibility testing in Cypress).

kuceb commented 4 years ago

@geebru thanks for the reminder - I need to replace a11y with jquery-ui's :tabbable and I believe that will fix the issues

Also accepting PRs, happy to help guide on that front

geebru commented 4 years ago

@Bkucera As a note I've started just playing with ally.js instead of trying to replace it with tabbable.js. It does include focusable in addition to tabSequence which does correctly see -1 elements as focusable, but not tabbable.

I feel like the solution here might just be an extra check when calling .tab() to see if the current activeElement is focusable and if true, tab, and if not, then fail. Right now it fails if the activeElement is not tabbable but it doesn't check if it's valid from a focusable perspective.

Let me know if I'm missing a piece here (JS definitely isn't my strong suit) as to why ally couldn't work here.

EDIT: Small note that my local changes have fixed the issue with tabbing from a -1 element. It will still error if you try tabbing out of an invalid focusable element but not from an item with -1. However, now trying to tab forwards from a tabindex -1 element goes backwards so I think I'm close - but missing a key piece in getting the "next" element from the current.