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

logRoles does not print hidden elements #1201

Open
cupojoe opened this issue Apr 15, 2023 · 7 comments
Open

logRoles does not print hidden elements #1201

cupojoe opened this issue Apr 15, 2023 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@cupojoe
Copy link

cupojoe commented Apr 15, 2023

  • @testing-library/react version: 14.0.0
  • Testing Framework and version: Jest 29.4.3
  • DOM Environment: jsdom 29.4.3

Relevant code or config:

const buttons = await waitFor(
    screen.getAllByRole("button", { hidden: true })
);

logRoles(buttons)

What you did:

I was trying to logRoles for a DOM tree with hidden elements. Failed to see the elements show in the roles list.

What happened:

Logging roles for hidden elements produces an empty log.

Reproduction:

  1. Get all elements of a DOM tree including hidden ones. Make sure you have hidden ones.
  2. Log using logRoles
  3. See that some log entries are empty compared to what you see when you screen.debug

Problem description:

If you log an element you found using { hidden: true }, logRoles will not print it. Similarly won't print hidden elements in a DOM tree you pass in. This is the opposite of what I would expect from this tool. I want to know why something may be failing to query, so I want to see everything in the tree.
The logRole function does take a { hidden } parameter and by default is set to false, however the function signature in Typescript does not accept a second parameter.

Suggested solution:

A useful thing would be that hidden was on by default, the function signature would be updated to take the second parameter, and that elements logged that are hidden, report so in the pretty role output.

@eps1lon eps1lon added the bug Something isn't working label May 19, 2023
@naorpeled
Copy link

Hey,
I'd like to investigate this.
@eps1lon feel free to assign me 🙏

@naorpeled
Copy link

naorpeled commented Aug 5, 2023

Hey @cupojoe,
sorry for the big delay,
were the elements that you tried to print divs without explicit role attributes?
In general, elements with a generic role are not included in the output.

@MatanBobi should we perhaps change that behavior or add an option to include these elements in the output?
This is a confusing behavior imo.

@MatanBobi
Copy link
Member

I think that printing out generic roles is quite confusing and misses the purpose a bit. generic role means nothing to screen readers or to users, it's just the implicit role that is given to elements so it won't help to have it printed out because it's not a role we'd like our users to use. So if that's the case, IMO I think it's a non issue. If the issue is with hidden, then that's a bug we should solve :)

@cupojoe
Copy link
Author

cupojoe commented Aug 19, 2023

@MatanBobi Well, I think it's both. On one hand, the function takes a parameter that I had to dig through the source code to understand. It should pop in the autocomplete in the IDE and be in the docs. Unless I am missing something (which is possible) and in that case I apologize.

On the other hand, the usefulness of a debug tool is to dump information that the internal mechanics are using in its behavior. I think it is relevant to show the role (even if it is generic) if that role is being used to match a result from a query. I want to know why something was matched (or not matched). Worst case scenario the debug tool should let me enable more information if I want it, not make those decisions for me, right? If I need more data to debug I should be able to get it.

@MatanBobi
Copy link
Member

@cupojoe if you mean the hidden flag, then @naorpeled had a fix for the types, I just merged it :)
About the debug tool, logRoles aims to be a helper function to decide which roles to use (from the docs):

This can be helpful for finding ways to query the DOM under test with getByRole.

That's why, adding generic is not something we'd want to do because it's not something we want users to use, it has no meaning for our users or screen readers.

@cupojoe
Copy link
Author

cupojoe commented Sep 13, 2023

That is great news that it was merged! Thank you @naorpeled
I think I am not explaining myself clearly. Yeah, generic has no meaning, but some elements have a generic role. If you think the element you are searching for has a different role, it would be good for the print out to tell you that it has generic, so you can either change the element type or apply a custom aria role to it (both for the test and accessibility sake). I think it would have the opposite effect of encouraging usage. I would be ok if you had a warning when someone tried to get by role using generic. That would be ok. But obscuring the situation of what is happening already seems counterproductive.

@naorpeled
Copy link

That is great news that it was merged! Thank you @naorpeled
I think I am not explaining myself clearly. Yeah, generic has no meaning, but some elements have a generic role. If you think the element you are searching for has a different role, it would be good for the print out to tell you that it has generic, so you can either change the element type or apply a custom aria role to it (both for the test and accessibility sake). I think it would have the opposite effect of encouraging usage. I would be ok if you had a warning when someone tried to get by role using generic. That would be ok. But obscuring the situation of what is happening already seems counterproductive.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants