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

getByText()/queryByText() (or some new query) should search across nested elements #201

Closed
helixbass opened this issue Feb 4, 2019 · 18 comments
Labels
help wanted Extra attention is needed needs discussion We need to discuss this to come up with a good solution needs investigation

Comments

@helixbass
Copy link

Describe the feature you'd like:

In my app, I'm rendering some markup like:

<div>
  <span>a. </span>
  <span class="something">Here is the answer text.</span>
</div>

It feels like I should be able to test that the right "answer prefix" (here, "a.") is rendered with the corresponding answer text just by doing eg:

getByText(/a\. Here is the answer text\./)

(which would select the enclosing <div>)

So I guess recursively concatenating text node children from nested elements?

It didn't look like there's already a way to do this

Suggested implementation:

I haven't looked at the implementation, just checked that in __tests__/element-queries.js I saw a test for sibling text node concatenation but not for nested child element text nodes

Would possibly be up for implementing this but am not super familiar with low-level DOM APIs

Describe alternatives you've considered:

I guess adding a data-testid to the enclosing <div> and then examining its children individually? But in the spirit of "what matters is how the user experiences it", that seems fragile compared to just searching text content across whatever arbitrary nested markup you may be rendering

Teachability, Documentation, Adoption, Migration Strategy:

I think adding an example with simple nested content eg:

<div>Some <span>nested</span> text</div>

with

getByText(/Some nested text/)

would be pretty clear as far as the gist of the supported feature

@kentcdodds
Copy link
Member

I think that I'm in favor of this if we can make it work. I can't promise that I'll be able to work on it any time soon, but I would be happy to review and merge a PR that implements this kind of text searching for all queries. One potential issue I see is:

// Given: <div>Some <span>nested</span> text</div>
getByText(/.*nested.*/) // what should this return?

The outer div and the span both match. I think a naive implementation of this would result in returning the <div>, but perhaps the <span> is what you'd want. This is tricky. I've opened #202 with an idea to help deal with this problem.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Feb 4, 2019

Hmm, I thought .innerText would do something like that, while .textContent doesn't. Wasn't the reason for the recent change proposal - to handle nested options in a select tag more realistically?

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText

@kentcdodds kentcdodds added help wanted Extra attention is needed needs discussion We need to discuss this to come up with a good solution needs investigation labels Feb 4, 2019
@alexkrolick
Copy link
Collaborator

I think this would fix these queries, if someone wants to pick it up: #190

@alexkrolick
Copy link
Collaborator

alexkrolick commented Feb 20, 2019

Turns out #190 probably isn't the right solution since .innerText isn't actually implemented/implementable in JSDOM (missed that part, sorry).

But removing the text node filter (below) in get-node-text gets pretty close to the behavior at the cost of duplicating matched nodes when you have something like <outer><inner>text</inner></outer>:

 child.nodeType === window.Node.TEXT_NODE

#202 would help with the duplication but if you go outside-in a lot of queries would return the body element... 🤔

@kentcdodds
Copy link
Member

Ok, so I'm working on this a bit and I've hit a bit of a road block partially due to the way this works, but also partially due to not being sure of the best way to go about this.

For example:

<div id="one">
  <span>a. </span>
  <span class="something">Here is the answer text.</span>
</div>

With that you want to get the div#one element.

But what about this:

<div id="another">
  <div id="one">
    <span>a. </span>
    <span class="something">Here is the answer text.</span>
  </div>
</div>

Which element should you get in this case?

I'm not sure that we can actually come to a "right" answer here... Honestly, I don't know how to proceed.

@alexkrolick
Copy link
Collaborator

Yeah, that's similar to what I ran into - in a lot of cases you'll even end up with document.body if the tree is simply nested

@helixbass
Copy link
Author

Is returning the "smallest enclosing" element problematic? So eg in the second example above, you'd get div#one because it matches the query (and is the smallest such element)

@kentcdodds
Copy link
Member

I think that's probably the most sensible. This will be a nontrivial change I'm afraid. But probably possible. I think I'd like to work on it, but I'd like to hear what other people think about it.

@mpareja
Copy link

mpareja commented Apr 2, 2019

What if there was a way to define tags (selectors?) to ignore / consider as the enclosing node?

For the example above: getByText(/Some nested text/, { ignore: 'span' })
Alternatively: getByText(/Some nested text/, { consider: 'div' })

(I didn't give much though to the option names. )

It's a bit of a trade-off between "do what the user does" and knowing some of the HTML internals. (Or maybe it's not that far off since typically the user does see the different styling, like bold text, and then ends-up interacting with the whole parent element rather than just the bold text.)

@kentcdodds
Copy link
Member

What about getByText((element) => /* return true if it's what you're looking for */)

Because that's currently supported :)

@Miklet
Copy link

Miklet commented Apr 19, 2019

Thank you @kentcdodds for the tip. I had element which looked like this:

 <div                  
   class="description" 
 >                     
   <span               
     class="money"     
   >                   
     <span             
       class="integral"
     >                 
       0               
     </span>           
     <span             
       class="floating"
     >                 
       .               
     </span>           
     <span             
       class="floating"
     >                 
       00              
     </span>           
     <span             
       class="floating"
     >                 
                       
       PLN             
     </span>           
   </span>             
 </div>                

and I wanted to match "0.00 PLN". I found it using custom matcher:

getByText((content, element) => {
  return element.textContent === '0.00 PLN';
}),

@kentcdodds
Copy link
Member

Cool. I think we'll just stick with that as a workaround and we'll be good 👍

@dispix
Copy link

dispix commented Oct 16, 2019

Hi! Just wanted to add to the solution commonly accept (that is, use getByText with a function). In the example by @Miklet above, the getByText would throw because multiple element pass the test (two to be exact, <div class="description"> and <span class="money">).

Replacing it with getAllByText would mean that you make situations where the text is duplicated pass. Not ideal. This has been reported already, but the answer was using getByText with a function.

Here's a small CodeSandbox that reproduces that use case

What would be the recommended way of testing situations like this one then?

@moretti
Copy link

moretti commented Oct 18, 2019

I found this answer on SO, where the OP implemented his own helper getByTextWithMarkup, based on node.textContent.

I think it would be great to add it to dom-testing-library, either as an extra option for *ByText or as a separate function (eg. *byMarkup)...what do you think?

@eps1lon
Copy link
Member

eps1lon commented Oct 18, 2019

I think it would be great to add it to dom-testing-library, either as an extra option for *ByText or as a separate function (eg. *byMarkup)...what do you think?

I'm working on a more general approach that will allow querying elements with roles by their accessible name. This should solve your issue.

@kentcdodds
Copy link
Member

I think that's a great idea @eps1lon 👍 Looking forward to hearing more about it.

@Raynos
Copy link

Raynos commented May 13, 2020

After looking at eps1lon/dom-accessibility-api#233 and w3c/aria#1117

I think the getByRole function is super useful for button or tab and other navigation elements.

But getByRole seems to not be recommend for getting nested text in <li> or <span> or <p> or <div>

Adding getByTextWithMarkup to dom-testing-library would be super useful;

@avbentem
Copy link

@dispix,

In the example by @Miklet above, the getByText would throw because multiple element pass the test (two to be exact, <div class="description"> and <span class="money">).

Simply only returning elements that actually have content themselves, seems to filter out unwanted parents just fine:

expect(
  // - content: text content of current element, without text of its children
  // - element.textContent: content of current element plus its children
  screen.getByText((content, element) => {
    return content !== '' && element.textContent === '0.00 PLN';
  })
).toBeInTheDocument();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed needs discussion We need to discuss this to come up with a good solution needs investigation
Projects
None yet
Development

No branches or pull requests

10 participants