The Wayback Machine - https://web.archive.org/web/20201109012248/https://github.com/testing-library/jest-dom/issues/207
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

toBeActiveDescendant #207

Open
diegohaz opened this issue Feb 17, 2020 · 10 comments
Open

toBeActiveDescendant #207

diegohaz opened this issue Feb 17, 2020 · 10 comments
Labels

Comments

@diegohaz
Copy link
Contributor

@diegohaz diegohaz commented Feb 17, 2020

Describe the feature you'd like:

I'm testing composite components that use aria-activedescendant to move focus. I would like to test whether some item is "virtually" focused or not after pressing arrow keys:

const Test = () => (
  <Composite aria-label="composite">
    <CompositeItem>item1</CompositeItem>
    <CompositeItem>item2</CompositeItem>
    <CompositeItem>item3</CompositeItem>
  </Composite>
);

const { getByText, getByLabelText } = render(<Test />);
const composite = getByLabelText("composite");
const item1 = getByText("item1");
const item2 = getByText("item2");
const item3 = getByText("item3");

composite.focus();

pressArrowRight();
expect(composite).toHaveFocus();
expect(item1).toBeActiveDescendant();

pressArrowRight();
expect(composite).toHaveFocus();
expect(item2).toBeActiveDescendant();

...

Suggested implementation:

I'm not sure about the code specific to create jest matchers, but this the utility I'm currently using:

function expectActiveDescendant(element) {
  const { activeElement } = element.ownerDocument;
  const id = activeElement && activeElement.getAttribute("aria-activedescendant");
  expect(id).toBe(element.id);
}

Alternative implementations could also check for aria-selected="true" on the element, but from what I see, not all implementations include aria-selected, whereas aria-activedescendant is mandatory on the focused element.

Describe alternatives you've considered:

I'm currently using the utility function above. It's simple, but, since there are not so many people who know how aria-activedescendant works (for example, that it should be in the current activeElement), I guess the community would benefit from a matcher like that.

Teachability, Documentation, Adoption, Migration Strategy:

toBeActiveDescendant

toBeActiveDescendant(element: Element)

This allows you to check whether the given element is currently referenced in the aria-activedescendant attribute on the element which currently has DOM focus.

Examples

<ul role="listbox" aria-activedescendant="option1" data-testid="listbox">
  <li role="option" id="option1" aria-selected="true" data-testid="option1">option 1</li>
  <li role="option" id="option2">option 2</li>
  <li role="option" id="option3">option 3</li>
</ul>
const listbox = getByTestId('listbox')
const option1 = getByTestId('option1')

listbox.focus()
expect(option1).toBeActiveDescendant()
@eps1lon
Copy link
Member

@eps1lon eps1lon commented Feb 17, 2020

Hi, thanks for opening this issue and providing so much detail 👍

I follow the general gist of it but have some concern regarding the name. The name implies that this is "just" expect(listbox).toHaveAttribute('aria-activedescendant', option1.id). It's not clear where the composite widget comes from until you see the the implementation.

The listbox could still have the aria-activedescendant attribute even without having focus and you might want to assert that this is explicitly preserved after blurring the widget. At least I don't know of any restriction of aria-activedescendant.

You're essentially suggesting:

- expect(listbox).toHaveFocus();
- expect(listbox).toHaveAttribute('aria-activedescendant', option1.getAttribute('id'))
+ expect(option1).toBeActiveDescendant();

Personally I would prefer a composite assertion (following the composite nature of the widget):

- expect(listbox).toHaveFocus();
- expect(listbox).toHaveActiveDescendant(option1)

and then maybe add an additional helper with a bit more verbose name such as toHaveVirtualFocus() (or something else).

Regarding implementation:

  1. check that aria-activedescendant is on an element supporting this attribute
  2. We should assert that the active descendant is an actual descendant of or owned by the composite.
    or
  3. the element is a textbox with aria-controls pointing to an element supporting aria-activedescendant.
@eps1lon eps1lon added the enhancement label Feb 17, 2020
@gnapse
Copy link
Member

@gnapse gnapse commented Feb 17, 2020

I'm all for it.

There are interesting alternative ways to put it in @eps1lon's response. I still think I prefer what @diegohaz originally suggested. That is, I prefer expect(option).toBeActiveDescendant() over expect(listbox).toHaveActiveDescendant(option). With the former you only need the option, you do not need to grab what its listbox is, and the matcher will check that whatever element points to that option as the active descendant is also the currently active element as well. With the latter, I'm not sure if that assertion implies checking that listbox is the currently focused element.

As for the name, toHaveVirtualFocus() sounds good, but I like that toBeActiveDescendant() is more aligned with the naming of this feature in the code.

No strong opinions here, I'm open to be challenged in all this.

@eps1lon
Copy link
Member

@eps1lon eps1lon commented Feb 17, 2020

With the former you only need the option

I generally want to optimize for readability. It's the thing we do more often than writing.

@gnapse
Copy link
Member

@gnapse gnapse commented Feb 17, 2020

Oh ok. Fair enough.

But in my case I do not see it less readable. Just wanted to clear that up. Because I too favor readability. What is it less readable about the proposed api?

Anyway, let’s wait for @diegohaz’s thoughts on this. In any case my concerns are kind of nit picks because in any of these forms this app will be a welcome feature. No strong feelings either way.

@eps1lon
Copy link
Member

@eps1lon eps1lon commented Feb 17, 2020

What is it less readable about the proposed api?

So readable was probably the wrong word. "reasonable" or "being a test that I can follow" may be more appropriate. It may be unrelated but I've recently read some "older" test of mine using document.activeElement and every time I had to ask myself: "what element is actually active?" and "is it actually active?". It seems like the same would apply here. The implicit assumption is that it uses the active element i.e. document.activeElement but I would suspect that an explicit assertion about what the active element is would go a long way in describing the behavior that we're testing.

@gnapse
Copy link
Member

@gnapse gnapse commented Feb 17, 2020

Ok, I can relate to that. Thanks for clarifying it. Let's wait for @diegohaz's input then on what he things of all this. I hope we can get this soon. I'm even offering myself to do it, unless @diegohaz wants to work it.

@diegohaz
Copy link
Contributor Author

@diegohaz diegohaz commented Feb 18, 2020

Thank you for the quick and thoughtful responses! :)

I see how expect(option).toBeActiveDescendant() could be confusing. Even if you know what aria-activedescendant means (and unfortunately most developers don't), it's not obvious if this is only testing the aria attribute or the DOM focus as well.

The feature I'm proposing here is more an equivalent to toHaveFocus(), but for widgets where focus is managed via the aria-activedescendant attribute. So, toHaveVirtualFocus() seems to be more descriptive.

I was also curious about the term virtual. I usually use it to refer to this focus management strategy, but I hadn't found any mention to this term on the WAI-ARIA docs.

Found references on the MDN docs though: https://developer.mozilla.org/en-US/docs/Web/Accessibility/Keyboard-navigable_JavaScript_widgets#Managing_focus_inside_groups

aria-activedescendant: managing a 'virtual' focus

An audacious alternative is to enhance toHaveFocus() so it returns true for elements that have this virtual focus too. It makes sense if you consider that, both in the UI and for screen readers, those elements should behave as they were actually focused.

@eps1lon
Copy link
Member

@eps1lon eps1lon commented Feb 18, 2020

An audacious alternative is to enhance toHaveFocus() so it returns true for elements that have this virtual focus too

These match different CSS pseudo-classes so I wouldn't merge these. It's oftentimes intended to outline the widget container and their active descendant.

@diegohaz
Copy link
Contributor Author

@diegohaz diegohaz commented Feb 19, 2020

I'm even offering myself to do it, unless @diegohaz wants to work it.

Ah, feel free to take it! :)

@gnapse
Copy link
Member

@gnapse gnapse commented Mar 6, 2020

I may start to work on this soon, and I'm still torn on how to name it. Though it's not just the name conundrum, but also how the API would work.

Will outline the alternatives I distill from the discussion so far. Let me know if something here is being misrepresented. Also, would like to know if you see a path forward where we could offer ore than one of these alternatives at the same time (they do not seem to be strictly conflicting, but it could be confusing).

1. expect(option).toBeActiveDescendant()

The original proposal. Receives only the actual option element and asserts it is the active descendant of the element that's currently in focus.

Could be named: toHaveVirtualFocus and keep the same interface.

Cons

Assumes we always want to check the active descendant on the currently focused element. It could happen that I want to assert what's the active descendant of a listbox that's not necesarily focused.

2. expect(listbox).toHaveActiveDescendant(option)

Would not check that listbox has the focus, only that its active descendant is the given option element. Users would need to assert the listbox focus themselves if they want to, or use document.activeElement directly in place of listbox.

Solves the "Cons" part of the previous alternative. Based on this suggestion in a comment above: #207 (comment)

Cons

A bit more work if users want to assert that the owner element of the active descendant also has the actual focus (could be a pretty common scenario). Users could even omit that extra check by mistake or ignorance in tests that require would benefit from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.