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

should getByText return a title? #593

Open
benmonro opened this issue May 29, 2020 · 12 comments
Open

should getByText return a title? #593

benmonro opened this issue May 29, 2020 · 12 comments

Comments

@benmonro
Copy link
Member

benmonro commented May 29, 2020

  • @testing-library/dom version: latest master
  • Testing Framework and version: node, jest 12.x, 26.x
  • DOM Environment:

Relevant code or config:

test('foo', () => {
  renderIntoDocument(` <svg>
  <title data-testid="svg">Close</title>
  <g><path /></g>
</svg>`)

  screen.debug(screen.getByText('Close'))
})

What you did:

called screen.getByText

What happened:

I got back the title... but shouldn't getByText be accessible to everyone? If it's hidden, it won't be accessible to sighted users.

Reproduction:

Problem description:

Suggested solution:

have getByText ignore <title> elements.

@kentcdodds
Copy link
Member

There are a lot of situations where you could get an element back that's hidden:

// <div style="display:none;">Hi</div>
screen.getByText(/hi/i) // <div....

I'm not sure whether we can deliver on the promise that we'll never return you something that's not hidden (though *ByRole does a pretty good job of that).

@benmonro
Copy link
Member Author

benmonro commented May 29, 2020

true, but in this case it's not just a styling thing. afaik the browser will never show the text of a <title> element. title is semantically different than text no? anyway only reason this came up was because in the suggestions feature I tried to add a test for it (because it's actually listed in the examples when i went thru 'which query should i use" and i realized that getByTitle will ALWAYS be recommending getByText when you use <title>... which seemed kinda strange to me

anyway food for thought. :)

@kentcdodds
Copy link
Member

What about <meta> tags and stuff? 🤔 We could probably just update this:

ignore = 'script, style',

Should be pretty easy actually...

@kentcdodds
Copy link
Member

This change of mind brought to you by: testing-library/cypress-testing-library#134

In fact, the original reason for ignore was because getByText was getting me translation text that was served up in the script tag for my app when using cypress 😅 Seems it would make sense to add some other tags now that I think about it.

Anyone wanna do this?

@benmonro
Copy link
Member Author

benmonro commented May 30, 2020

Ah I see what you mean @kentcdodds this is easy to fix. What other tags do you think should be ignored by default? I can fix it as suggestions would need the same change.

@kentcdodds
Copy link
Member

I don't know for sure. Would take a little bit of research.

@benmonro
Copy link
Member Author

alright. i can try and see what i find and report back. somewhat related, what do you think of getByTitle returning the parent svg item rather than the title itself? seems a bit odd to me that if i did getByTitle("foo") on this:

<svg onclick="..."><title>foo</title><path /></svg>

that it returns the <title> instead of the <svg>

especially when i want to do fireEvent.click(getByTitle("foo"))

@kentcdodds
Copy link
Member

Honestly, I can't remember why we added that one and I don't use it myself so my understanding of the implications of removing it is limited. If you could do some archeology on that too figure out what use cases it's intended for them we can make a more informed decision.

@benmonro
Copy link
Member Author

benmonro commented May 31, 2020

I actually am using it to check for the presence of svg so I do think we should keep it. I'm just saying it should return the element it's a title for rather than the title element itself. Just like how get by label text will return the input element rather than the label.

@kentcdodds
Copy link
Member

That makes sense to me. Let's just make sure we understand the reason it was created so we don't miss something.

Also, this would be a breaking change. Perhaps we could make a new query *ByTitleText and deprecated *ByTitle. I prefer that option personally.

@andreasandpiper
Copy link

andreasandpiper commented Aug 27, 2020

I am chiming in here as someone who has a similar problem. I am testing for the display of an svg using jest-dom matcher .toBeVisible. This matcher checks for the presence of attributes such as visibility https://github.com/testing-library/jest-dom/blob/master/src/to-be-visible.js, and we use the 'visibility' attribute for svgs. Since getByTitle returns the title, not the SVG element, I can not use the matcher to test for visibility.

@rodoabad
Copy link

rodoabad commented Jun 7, 2021

How are you guys able to use getByTitle for an SVG? I have it in the debug but it's not able to find it still.

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

4 participants