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

Add getByDescriptionTerm #140

Closed
nicolaslohrer opened this issue Oct 26, 2018 · 16 comments
Closed

Add getByDescriptionTerm #140

nicolaslohrer opened this issue Oct 26, 2018 · 16 comments

Comments

@nicolaslohrer
Copy link

nicolaslohrer commented Oct 26, 2018

Hey @kentcdodds,

I'm totally enjoying using dom- and react-testing-library - thanks a lot!

Describe the feature you'd like:

We're using HTML description lists in our application:

<dl>
  <dt>term1</dt>
  <dd>value1</dd>
  <dt>term2</dt>
  <dd>value2</dd>
</dl>

I think it might be handy to select a dd element by its dt. In React for example, it could be used like this:

const { getByDescriptionTerm } = render(<MyDescriptionList />);
const dd = getByDescriptionTerm("term1");

Suggested implementation:

Tbh, I haven't yet looked into the internals of dom-testing-library. I suppose, it wouldn't be too different from other existing selectors. One thing that might be tricky though is that dt doesn't necessarily contain a string but might also wrap other DOM elements.

Describe alternatives you've considered:

So far, I'm just asserting that all dt and dd elements exist and have the expected content. I haven't figured out a good way to match dt and dd elements to each other in my tests though.

I'm curious to hear what you think about the idea and its feasibility!

Best,
Nicolas

@playvm7001
Copy link

Thanks i did try it

@kentcdodds
Copy link
Member

Hi @nicolasschabram,

I think that tags like dl, dt, and dd are implementation details. Unless I'm mistake, the same could be accomplished via divs and spans and the user doesn't care either way.

In addition I literally had never heard of these tags until just now and had to look up on MDN to see what they even are. I don't think that they're used enough to justify the extra code. You could however create your own queries for your application if you have need of this frequently.

I hope that's helpful. Good luck!

@qasimalyas
Copy link

qasimalyas commented Jun 15, 2020

I wanted to comment on this and possibly provide an alternative in case anyone else wants to tests definition lists - which may not be very common but are very useful.

You can use the getByRole to get the dt or dd but you might need to stick on an aria-label attribute and give it a value to use, i.e.

<dl>
  <dt aria-label="term1">term1</dt>
  <dd>value1</dd>
  <dt aria-label="term2">term2</dt>
  <dd>value2</dd>
</dl>

in RTL you can then use the below and assert as necessary

it("Display term", () => {
    expect(screen.getByRole("term", { name: "term1" }))
});

@zogar1993
Copy link

zogar1993 commented Aug 21, 2021

I wanted to comment on this and possibly provide an alternative in case anyone else wants to tests definition lists - which may not be very common but are very useful.

You can use the getByRole to get the dt or dd but you might need to stick on an aria-label attribute and give it a value to use, i.e.

<dl>
  <dt aria-label="term1">term1</dt>
  <dd>value1</dd>
  <dt aria-label="term2">term2</dt>
  <dd>value2</dd>
</dl>

in RTL you can then use the below and assert as necessary

it("Display term", () => {
    expect(screen.getByRole("term", { name: "term1" }))
});

According to the spec, aria-label should only be used on the following elements:

  • Interactive elements
  • Landmark elements
  • Elements that have an explicit widget role
  • iframe and img elements

Since description lists are none of those, it seems to me that adding aria-label to the term is not a good solution, since it could mess with accessibility.

@zogar1993
Copy link

zogar1993 commented Aug 21, 2021

Going back to the original question, I think that using the react-testing-library to get things by term is still relevant. Although Description Lists are not used much, they are still an important part of the accessibility toolkit, and not having an easy way to test them encourages workarounds or bad accessibility (or lack of tests altogether). I made a rather raw but thorough implementation for people looking for a way to solve this.

function getByTerm(term, component) {
	const terms = component.getAllByRole("term")
	const termComponent = terms.find(x => x.textContent === term)
	if(termComponent === undefined) throw Error(`term '${term}' not found`)
	const details = getDetailsOfTerm(termComponent)
	if(details.length === 0) throw Error(`No detail found for term '${term}'`)
	if(details.length > 1) throw Error(`Multiple details found for term '${term}'`)
	return details[0]
}

function getDetailsOfTerm(term) {
	let detail = term.nextSibling
	while (detail?.tagName === "DT") detail = detail.nextSibling
	const details = []
	while (detail?.tagName === "DD") {
		details.push(detail)
		detail = detail.nextSibling
	}
	return details
}

So, in order to use it, you do something like

<dl>
  <dt>term1</dt>
  <dd>value1</dd>
</dl>
const dd = getByTerm("term1", screen)
expect(dd.textContent).toBe("value1")

--- EDIT ---

There was an issue in the react-testing-library which prevented getting terms by role, and this workaround was needed for my solution to work. The issue is now closed, so the workaround is no longer necessary.

@phegman
Copy link

phegman commented Mar 9, 2022

I created a custom query that seems to work pretty well for me:

import { buildQueries, getAllByRole, getNodeText } from "@testing-library/dom";

const queryAllByDescriptionTerm = (container: HTMLElement, name: string) => {
  const terms = getAllByRole(container, "term").filter(
    (term) => getNodeText(term) === name
  );

  const definitions = terms.flatMap((term) =>
    term.nextElementSibling.tagName === "DD" ? [term.nextElementSibling] : []
  );

  return definitions as HTMLElement[];
};

const [
  queryByDescriptionTerm,
  getAllByDescriptionTerm,
  getByDescriptionTerm,
  findAllByDescriptionTerm,
  findByDescriptionTerm,
] = buildQueries(
  queryAllByDescriptionTerm,
  (c, name) => `Found multiple descriptions from term with name of ${name}`,
  (c, name) => `Unable to find a description from term with name of ${name}`
);

export {
  queryAllByDescriptionTerm,
  queryByDescriptionTerm,
  getAllByDescriptionTerm,
  getByDescriptionTerm,
  findAllByDescriptionTerm,
  findByDescriptionTerm,
};

Then add the custom queries as documented in https://testing-library.com/docs/react-testing-library/setup#add-custom-queries

2022-03-10: Updated to use flatMap instead of map combined with filter.

@nick-lvov-dev
Copy link

@phegman wow, 8 hours ago, now I don't have to write it myself, thanks! 😄

@nick-lvov-dev
Copy link

For some reason testing-library didn't recognize my dt elements as terms so I've modified the helper by @phegman a bit:

const queryAllByDescriptionTerm = (container: HTMLElement, name: string) =>
  getAllByRole(container, "definition").filter(
    (term) =>
      term.tagName === "DD" &&
      isHTMLElement(term.previousElementSibling) &&
      getNodeText(term.previousElementSibling) === name,
  );

@matheusmb
Copy link

I've improved @phegman code by using @nick-lvov-dev modified helper and adding support for RegExp. Here's the final version, including the missing isHTMLElement helper function.

import { buildQueries, getAllByRole, getNodeText } from '@testing-library/dom';

function isHTMLElement(element: any): element is HTMLElement {
  return element instanceof HTMLElement;
}

const queryAllByDescriptionTerm = (
  container: HTMLElement,
  textMatch: string | RegExp,
) => {
  const hasText = (node: HTMLElement) => {
    const nodeText = getNodeText(node);
    return nodeText === textMatch || nodeText.match(textMatch);
  };
  return getAllByRole(container, 'definition').filter(
    (term) =>
      term.tagName === 'DD' &&
      isHTMLElement(term.previousElementSibling) &&
      hasText(term?.previousElementSibling),
  );
};

const [
  queryByDescriptionTerm,
  getAllByDescriptionTerm,
  getByDescriptionTerm,
  findAllByDescriptionTerm,
  findByDescriptionTerm,
] = buildQueries(
  queryAllByDescriptionTerm,
  (c, name) => `Found multiple descriptions from term with name of ${name}`,
  (c, name) => `Unable to find a description from term with name of ${name}`,
);

export {
  queryAllByDescriptionTerm,
  queryByDescriptionTerm,
  getAllByDescriptionTerm,
  getByDescriptionTerm,
  findAllByDescriptionTerm,
  findByDescriptionTerm,
};

@phegman
Copy link

phegman commented Mar 9, 2022

@nick-lvov-dev maybe the dt elements weren't being recognized because of #703? That was fixed last October 2021 so if you are running an old version of testing library that could be the issue 🤷

@tw-eugeneoei
Copy link

Hello all. Adding on to the original question of getting a term by its description/name.

In this section of the MDN docs on "ARIA: term role", it says,

Allow the term itself to define the accessible name. Do not use aria-label or aria-labelledby.

Correct me if I'm wrong, but doesn't that mean the text content of <dt> will be the name to use ie screen.getByRole("term", { name: "insert-dt-text-content-here" })? But, it doesn't seem to be the case.

Assuming a react component of the following,

<Box
    m={2}
    sx={{
        border: "1px solid black",
        padding: "4px",
    }}
>
    <dl>
        <dt>Coffee</dt>
        <dd>Black hot drink</dd>
        <dt>Milk</dt>
        <dd>White cold drink</dd>
    </dl>
</Box>

I was expecting screen.getByRole("term", { name: "Coffee" }) be able to access the <dt>Coffee</dt> element.

Instead I got the following error:

TestingLibraryElementError: Unable to find an accessible element with the role "term" and name "Coffee"

    Here are the accessible roles:

      term:

      Name "":
      <dt />

      Name "":
      <dt />
.
.
.

any thoughts?

@MatanBobi
Copy link
Member

MatanBobi commented Apr 29, 2022

Hi @tw-eugeneoei, thanks for reaching out.
Unfortunately, the MDN docs aren't the actual standard when it comes to accessibility.
We're currently following the WAI-ARIA 1.2 which states that the accessible name for the role term is generated by "author" meaning it's not auto calculated by the content.
More info can be found in the spec.
In WAI-ARIA 1.3 which is currently in draft mode, the role term is under "Roles which cannot be named".

Thanks again :)

@tw-eugeneoei
Copy link

Hi @MatanBobi, thanks for pointing me in the right direction.

To make sure I got it right, according to the aria-label property definition found here, it says,

If the label text is available in the DOM (i.e. typically visible text content), authors SHOULD use aria-labelledby and SHOULD NOT use aria-label.

Am i right to say that in the example I provided above, I should be giving the <dt> element the attribute aria-labelledby for its name?

@MatanBobi
Copy link
Member

You should be using aria-labelledby only if a different element defines the name for this element. Since what you want is basically the name to be the content, I'd use aria-label. But, I'm not an expert, aria is a convoluted spec :)

@tw-eugeneoei
Copy link

@MatanBobi oh right. i think i got it. thanks for clarifying!

@Zach-Jaensch
Copy link

#140 (comment)

I've improved @phegman code by using @nick-lvov-dev modified helper and adding support for RegExp. Here's the final version, including the missing isHTMLElement helper function.

Thank you for helping in creating a custom query.
I had to take it a couple steps further as it didn't handle the case when a term has multiple definitions (ie <dt>term</dt><dd>definition</dd><dd>definition</dd>, and it wouldn't detect text inside a term if there is another element (ie<dt><span>Cannot be found</span></dt>)

Here is what I have done

import { buildQueries, getAllByRole } from "@testing-library/react";

const queryAllByDescriptionTerm = (
  container: HTMLElement,
  textMatch: string | RegExp
): HTMLElement[] => {
  // find all terms matching the textMatch
  const terms = getAllByRole(container, "term").filter((term) => {
    // ensure that the term is a DT element
    return term.tagName === "DT" && term.textContent === textMatch;
  });

  const definitions: HTMLElement[] = [];

  terms.forEach((term) => {
    let testEl: HTMLElement = term;
    // find all DD elements that are siblings of the term
    // eg <dt>term</dt><dd>definition</dd><dd>definition</dd>
    while (testEl.nextElementSibling) {
      if (!(testEl instanceof HTMLElement)) {
        break;
      }

      testEl = testEl.nextElementSibling as HTMLElement;

      if (testEl.tagName !== "DD") {
        break;
      }

      definitions.push(testEl);
    }
  });

  return definitions;
};

const [
  queryByDescriptionTerm,
  getAllByDescriptionTerm,
  getByDescriptionTerm,
  findAllByDescriptionTerm,
  findByDescriptionTerm,
] = buildQueries(
  queryAllByDescriptionTerm,
  (c, name) => `Found multiple descriptions from term with name of ${name}`,
  (c, name) => `Unable to find a description from term with name of ${name}`
);

export {
  queryAllByDescriptionTerm,
  queryByDescriptionTerm,
  getAllByDescriptionTerm,
  getByDescriptionTerm,
  findAllByDescriptionTerm,
  findByDescriptionTerm,
};

Not that this will also find all the description for terms that have the same text.

ie

<dl>
  <dt>term 1</dt>
  <dd>one</dd>
  <dt>term 1</dt>
  <dd>two</dd>
</dl>

would return [<dd>one</dd>, <dd>two</dd>] but imo you should be writing tests to what you expect, which would catch this.

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

No branches or pull requests