Off-screen/hidden elements still queryable #196
Comments
adiun
changed the title
|
If only Cypress had its DOM query functions reusable... It has functions that check exactly what you're describing. CC @bahmutov |
|
Cypress is really impressive. (Looking at https://docs.cypress.io/guides/core-concepts/interacting-with-elements.html#Actionability). FWIW for handling off-screen I worked around this by using |
|
Hi @adiun! I think this would be really cool. I've definitely run into this problem and it's actually why this exists (because my Another idea if we can't work this out is to improve |
kentcdodds
added
enhancement
|
In general JSDOM out of the box doesn't support screen sizes very well (I think the default size for all elements is 0x0) nor does it parse CSS stylesheets that can affect visibility of an element (in fact this is probably the primary factor). I think you'd be better off with an in-browser test like Cypress. |
|
I agree 👍 Though I wonder if there are some utilities that we could expose if we're in an environment that does do those things (like the browser). 🤔 |
|
Or even better, if there were a way we could filter the elements that are returned by the queries (as originally suggested) if we are able to determine their visibility (because we happen to be in an environment that provides us with that information). I think this is definitely doable if someone wants to try. |
|
I wonder what's the definition for "not visible" in this context. For instance, given the first test example above: test("off-screen element not queryable", () => {
const { queryByText } = render(
<div style={{ height: "150px", overflow: "auto" }}>
<div style={{ height: "100px" }}>Paragraph-1</div>
<div style={{ height: "100px" }}>Paragraph-2</div>
<div style={{ height: "100px" }}>Paragraph-3</div>
<div style={{ height: "100px" }}>Paragraph-4</div>
</div>
);
const paragraph = queryByText("Paragraph-4");
expect(paragraph).toBeNull();
});If we change the Also, would a screen reader not read |
|
I think that defining "not visible" as something that a screen reader wouldn't read and/or something you cannot find via ⌘ + f. |
I would actually say no in this case, because from the POV of the user, they don't see it. You said "become visible" which implies that it's not visible yet to the user. For more context, I'm specifically testing some custom auto-scroll functionality I wrote so I want the user to click on something that auto-scrolls that div, bringing Paragraph-4 into view and I want to verify that through a test. Maybe that's out of scope for this library though and I'd have to resort to higher-level UI testing frameworks like cypress which is fair... |
|
Maybe I was not clear enough, because of the different uses we can give to the adjective "visible". Yes, I said "become visible" and in that case I said it in the sense of it being actually drawn in a screen. But the sense of visible that this library aims for is more than that. Under no circumstance we should consider an element that's out of view just because of a viewport size constraint to be "invisible" and not considered in the queries. Even if jsdom allowed for it. As Kent said (which aligns with what I was aiming for when I questioned the concept of visibility for the queries): if a screen reader reads it out loud, and/or you can find it via Cmd/Ctrl+F, then it's visible. I just tested the markup of your first example, and both things happen. I'd say that that content is visible. |
|
Thanks for the explanation... that is an interesting requirement. Why is that though? Cypress regards a scrolled-out element as "not visible". When I think of "The more your tests resemble the way your software is used..." I think of a user seeing a panel that doesn't have "Paragraph-4". If they want to click on Paragraph-4, they have to scroll that panel. But I can see how the screen reader/Cmd+F definition can make the implementation more straightforward re. accessing accessible elements. Is that the main reason? |
|
Actually now that I think about it, actually visible on the screen makes more sense to me. |
|
The big issue for me is that Cypress can use computed styles, which includes CSS, but JSDOM can only use element styles and properties. So even if a lot of work was done to make visibility checkers in JSDOM more aware of viewports, there still would be a significant gap as far as testing the actual experience of an app. The "interactability" check Cypress performs before clicking an element does include viewport checks, but cypress's .visible assertion comes from jQuery and doesn't take into account scroll: https://api.jquery.com/visible-selector/ So... I'd say jest-dom and dom-testing-library aren't exactly the right places to support this - unless there is some scenario where you have compiled CSS as well as a jest environment running in-browser. A custom Cypress/chai-jquery add-on seems like a better fit. |
|
I agree that it's not something that we should even try to do in the JSDOM world, but I do think that people are running dom-testing-library in other environments (Cypress runs it in the browser, Karma runs it in the browser, and puppeteer runs it in the browser). So if we can detect that we're in a browser environment then we could probably do this filtering reasonably well I think. If we detect that JSDOM is the source of our DOM though then we shouldn't even try. That's what I'm suggesting anyway. |
|
Hmm, now that I think about it, I don't think that the queries themselves should have this check. For example, in a long form, a user would scroll the page while looking (querying) for some text. They wouldn't bail out. On the other hand, restricting events from being fired on off-screen elements makes sense (and in some limited case it may make sense to optionally query only the visible screen, perhaps as a variant of It would also make sense to have an assertion like |
|
Ah, that's a great point Alex. What does Cypress do when you try to query the page for something that's not in the viewport? Does it require you to scroll around to find things? 🤔 |
|
Cypress does this before firing events on elements: https://docs.cypress.io/guides/core-concepts/interacting-with-elements.html#Actionability
A lot of the checks are in here: |
|
Thanks for finding that Alex. I think I'd be more comfortable trying something like this out in another package and seeing how that goes before bringing it into the core. Anyone up for that? |
Maybe as part of To avoid confusion with simpler visibility checks like the jQuery .visible method I think avoiding |
|
Hi. I'm not sure if this is related, but I'm trying this:
What I get back is the first input, I'd expect to get the second one because the context of the query is the form and not the container that includes both forms. |
|
@dicardopegb, the problem there is you're selecting thin by the label text but there is no label. Please make a reproduction in codesandbox and post it to https://spectrum.chat/react-testing-library Thanks! |
|
I think we're going to opt to put this in another package for now. Anyone can feel free to work on that. |
|
@kentcdodds I'm not sure why you say there's no label there, I can see that there's no opening label tag for one of the labels, but it was just a typo. Anyway, let me write down that example when I'm back in my computer. |
|
@kentcdodds did you end up putting this in another package? If so which one? I'm running into the same issue in testcafe. Need to have DTL only return visible items |
|
I never did |
|
As dom-testing-library uses jsdom under the hood, which is still not a full browser DOM in that it doesn't implement proper CSS and browser layout, it's impossible to tell at this level if an element is really invisible if it is mounted. |
|
I still think it would be possible to dig into what Cypress (and other full browser test platforms) do and extract that into a function. Then you could wrap all the return values of the queries with that function in something like testcafe-testing-library. Also of note is that as far as I can tell Cypress doesn't check visibility so much as interactibility, and does so when you try to perform an action, not at query time. |
|
Unfortunately the challenge is that it requires determining layout information and that's a long-standing issue on jsdom. It's not impossible, but very difficult and I don't it'll ever be implemented. However, dom-testing-library can run in a real browser, so there's nothing stopping it out another library from being able to do this if it's running in an environment that supports layout. I'm just not going to work on this myself because I don't need it myself. |
"It's not impossible, but very difficult and I don't it'll ever be implemented." Yes, if it won't ever be implemented to me this equals to impossible.
Agree, thanks for pointing out that it's not the fault of dom-testing-library, and that you can provide it a real browser DOM instead of the mock JSDOM. Unfortunately, the fewer people work on certain high-value projects, the more opinionated the project roadmaps become. I'd love to see a solution to that, hopefully someone will be able to spend enough time to make it real, not just "possible" but non-existent. |
|
To focus this back on the latest request:
The testcafe docs say:
Is this somehow not working with your custom selectors? It looks like it should work even with the "custom function that returns a DOM element" selectors. |
alexkrolick
removed
enhancement
|
well the issue was that DTL was throwing an error before that check
happened because I was using getByText inside the browser instead of
getAllByText. Seems like that error should only be thrown if the elements are visible no?
I will try to put together an example today.
|
|
Could you use the findBy* and findAllBy* variants instead? |
|
Hmm. Good question. Let me chew on/play with that. I kinda like giving
users the ability to choose but then again the selector might handle it
anyway....
On Sun, Apr 28, 2019 at 7:36 AM Kent C. Dodds ***@***.***> wrote:
Could you use the findBy* and findAllBy* variants instead?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#196 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADBPBBOS7AE42AT62XFFMDPSWZADANCNFSM4GSU3ALA>
.
--
Ben Monro
Software Developer
|
|
ok so did a little more digging (I'm a bit new to Testcafe - got fed up w/ cypress)... So in Testcafe there's a couple perfectly reasonable options for checking visibility on a
Both of these make it perfectly workable for Having said that, it feels like |
|
Are you primarily talking about If someone does want to implement this, I'd suggest doing it as a function that takes a node and returns true or false for visibility. Then if you want to use this as the default for some lib, wrap and re-export the queries. Again, because of the limitations of the primary environment (JSDOM) and the availability of alternatives in other environments (Cypress, TestCafe, Puppeteer), this isn't something we'd likely work on. |
|
I would be in favor of having such a function built-in and in a JSDOM environment it always returns true, but in an environment that handles layout it can do the calculation. I won't work on this myself (because Cypress handles it for us, otherwise I'm in JSDOM), but I'd be fine if someone wanted to do it. It could probably exist in a separate package (maybe it already does!). |
|
One other note - how would you scroll to an element if it was offscreen? The way Cypress does this to attempt to scroll the element into view when you try to fire events on it after successfully querying it. For that reason I think it's a good idea to split the checks into off-screen vs other types of hidden. |
If the tests really resembled how a user would use the app, you wouldn't scroll to an element, you would just scroll in some direction and continue scrolling until you see the element you're looking for, or until you're out of luck finding it. |
Obviously there are a lot of complexities when trying to define visibility, and implement the checks for that. With that said, rather than an "all or nothing" approach, could we at a minimum use the It doesn't cover anything about sizing, scrolling, overflow, etc, but it looks like it covers using |
|
I like it in theory. The problem will inevitably be performance issues 😬 We already have issues with something similar with the I'll reopen this and I'd be willing to look at a PR for this. |
|
I created a matcher for this and tests if wanted I can make a PR, it should be pretty simple to integrate the hidden feature /**
* *ByText matcher for filtering out inaccessible elements
*
* @example
* For:
* <div>
* <div hidden>hello</div>
* <div>hello</div>
* </div>
*
* screen.getByText('hello') // ❌ finding 2 elements
* screen.getByText(accessibleTextContentMatcher('hello')) // ✅ pass - find only 1
*
* https://github.com/testing-library/dom-testing-library/issues/196#issue-403653186
*/
function accessibleTextContentMatcher(matcher: Matcher, options: SelectorMatcherOptions = {}): MatcherFunction {
return (content, element) => {
if (isInaccessible(element)) {
return false;
}
// This implements the missing implementation of the *ByText() that is needed here as it's not exported
// https://github.com/testing-library/dom-testing-library/blob/d1a57dd9266c41c42b9b384e3583f4b5d9131c64/src/queries/text.ts#L18-L46
if (matcher instanceof Function) {
return matcher(content, element);
}
if (options.exact === false) {
if (typeof matcher === 'string' || typeof matcher === 'number') {
return content.toLowerCase().includes(matcher.toString().toLowerCase());
}
return matcher.test(content);
}
if (matcher instanceof RegExp) {
return matcher.test(content);
}
return content === String(matcher);
};
}TestsTests were copied from https://github.com/testing-library/dom-testing-library/blob/edffb7c5ec2e4afd7f6bedf842c669ddbfb73297/src/__tests__/role.js and updated to this matcher describe('accessibleTextContentMatcher', () => {
it('should filter out hidden divs', () => {
const { getByText, getByTestId } = render(
<div>
<div hidden>hello</div>
<div data-test-id="displayed">hello</div>
</div>
);
const expectedElement = getByTestId('displayed');
const element = getByText(accessibleTextContentMatcher('hello'));
expect(element).toEqual(expectedElement);
});
test('by default excludes elements that have the html hidden attribute', () => {
const { getByText } = render(<div hidden>something</div>);
expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow();
});
test('should still find element that are not hidden', () => {
const { getByText } = render(<div>something</div>);
getByText(accessibleTextContentMatcher('something'));
});
test('by default excludes elements that have the html hidden attribute on any of their parents', () => {
const { getByText } = render(
<div hidden>
<div>something</div>
</div>
);
expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow();
});
test('by default excludes elements which have display: none', () => {
const { getByText } = render(<div style={{ display: 'none' }}>something</div>);
expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow();
});
test('by default excludes elements which have display: none on any of their parents', () => {
const { getByText } = render(
<div style={{ display: 'none' }}>
<div style={{ display: 'block' }}>something</div>
</div>
);
expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow();
});
test('by default excludes elements which have visibility hidden', () => {
// works in jsdom < 15.2 only when the actual element in question has this
// css property. only jsdom@^15.2 implements inheritance for `visibility`
const { getByText } = render(<div style={{ visibility: 'hidden' }}>something</div>);
expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow();
});
test('by default excludes elements which have aria-hidden="true"', () => {
const { getByText } = render(<div aria-hidden="true">something</div>);
expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow();
});
test('by default excludes elements which have aria-hidden="true" on any of their parents', () => {
// > if it, or any of its ancestors [...] have their aria-hidden attribute value set to true.
// -- https://www.w3.org/TR/wai-aria/#aria-hidden
// > In other words, aria-hidden="true" on a parent overrides aria-hidden="false" on descendants.
// -- https://www.w3.org/TR/core-aam-1.1/#exclude_elements2
const { getByText } = render(
<div aria-hidden="true">
<div aria-hidden="false">something</div>
</div>
);
expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow();
});
test('considers the computed visibility style not the parent', () => {
// this behavior deviates from the spec which includes "any descendant"
// if visibility is hidden. However, chrome a11y tree and nvda will include
// the following markup. This behavior might change depending on how
// https://github.com/w3c/aria/issues/1055 is resolved.
const { getByText } = render(
<div style={{ visibility: 'hidden' }}>
<div style={{ visibility: 'visible' }}>something</div>
</div>
);
getByText(accessibleTextContentMatcher('something'));
});
}); |
|
for a simpler workaround you can do: // we use *ByRole which filters out hidden elements
screen.getByRole(
(_, element: HTMLElement) => '<your-text-or-regex>'.match(getNodeText(element))
); |
Workaround doesn't seem to work, or at least the typing says that a function is not assignable to type @rluvaton Any updates on the PR? |
dom-testing-libraryversion: 3.16.4reactversion: 16.8.0-alpha.1nodeversion: 8.11.3npm(oryarn) version: 5.6.0Relevant code or config:
What you did:
Tried to write the tests above, expected them to pass.
What happened:
Tests failed:
Reproduction:
https://codesandbox.io/s/2zwjwyp13n
Problem description:
First off, I'm new to
react-testing-library. I read @kentcdodds philosophy - this quote I totally agree with: "The more your tests resemble the way your software is used, the more confidence they can give you.".That said, I was expecting in the tests above that the given elements would not be able to queried because they are hidden/non-visible/off-screen. I dug around in the
dom-testing-librarycode and noticed thatquerySelectoris still being used but I don't understand how this implementation accounts for elements that the user cannot see, like things that are off-screen, hidden, etc.I would absolutely love to be able to write tests that exercise a product the way a user would interact with it. But I'm unsure if I can trust
dom-testing-libraryin this sense.Outside of hidden/off-screen elements cases (and not related to this issue), what about clicking on elements that are unclickable? This is a (surprisingly) frequent z-index bug I've seen.
fireEventwouldn't really cut it. There are lots of edge cases here...Suggested solution:
I realize that it would be quite difficult to account for all of the cases where an element can't be queried (off the top of my head - opacity = 0, transforms off screen, scaled down, filters, clip-path, etc.) But I would expect at least display = none, visibility = hidden, and off-screen elements to be accounted for. I think this can be handled through
getBoundingClientRectbut that would sacrifice perf. Maybe this is something that a user could toggle throughrender?The text was updated successfully, but these errors were encountered: