Off-screen/hidden elements still queryable

adiun commented 5 years ago

dom-testing-library version: 3.16.4
react version: 16.8.0-alpha.1
node version: 8.11.3
npm (or yarn) version: 5.6.0

Relevant code or config:

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();
});

test("non-displayed element not queryable", () => {
  const { queryByText } = render(
    <div style={{ height: "200px", overflow: "auto" }}>
      <div style={{ height: "100px" }}>Paragraph-1</div>
      <div style={{ height: "100px", display: "none" }}>
        NotDisplayed-Paragraph
      </div>
    </div>
  );
  const paragraph = queryByText("NotDisplayed-Paragraph");
  expect(paragraph).toBeNull();
});

test("hidden element not queryable", () => {
  const { queryByText } = render(
    <div style={{ height: "200px", overflow: "auto" }}>
      <div style={{ height: "100px" }}>Paragraph-1</div>
      <div style={{ height: "100px", visibility: "hidden" }}>
        Hidden-Paragraph
      </div>
    </div>
  );
  const paragraph = queryByText("Hidden-Paragraph");
  expect(paragraph).toBeNull();
});

What you did:

Tried to write the tests above, expected them to pass.

What happened:

Tests failed:

off-screen element not queryable
2ms
expect(received).toBeNull()

Expected value to be null, instead received
  <div style="height: 100px;">Paragraph-4</div>

  14 |     </div>
  15 |   );
  16 |   const paragraph = queryByText("Paragraph-4");
> 17 |   expect(paragraph).toBeNull();
  18 | });
  20 | test("non-displayed element not queryable", () => {
non-displayed element not queryable
1ms
expect(received).toBeNull()

Expected value to be null, instead received
  <div style="height: 100px; display: none;">NotDisplayed-Paragraph</div>

  27 |     </div>
  28 |   );
  29 |   const paragraph = queryByText("NotDisplayed-Paragraph");
> 30 |   expect(paragraph).toBeNull();
  31 | });
  33 | test("hidden element not queryable", () => {
hidden element not queryable
1ms
expect(received).toBeNull()

Expected value to be null, instead received
  <div style="height: 100px; visibility: hidden;">Hidden-Paragraph</div>

  40 |     </div>
  41 |   );
  42 |   const paragraph = queryByText("Hidden-Paragraph");
> 43 |   expect(paragraph).toBeNull();
  44 | });

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-library code and noticed that querySelector is 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-library in 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. fireEvent wouldn'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 getBoundingClientRect but that would sacrifice perf. Maybe this is something that a user could toggle through render?

sompylasar commented 5 years ago

If only Cypress had its DOM query functions reusable... It has functions that check exactly what you're describing. CC @bahmutov

adiun commented 5 years ago

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 wait with an expectation callback to check if an element is within a container. Would be awesome if I could avoid wait though.

kentcdodds commented 5 years ago

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 getByText query in cypress-testing-library was returning stuff in a script tag 😱). I don't have the time to work on or investigate it myself, but if someone wants to do that and open a PR that would be very welcome.

Another idea if we can't work this out is to improve toBeVisible from jest-dom to support handling more cases.

alexkrolick commented 5 years ago

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.

kentcdodds commented 5 years ago

I agree :+1:

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). 🤔

kentcdodds commented 5 years ago

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.

gnapse commented 5 years ago

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 overflow style to scroll, then would we consider Paragraph-4 to be visible again? Because in that case it can easily become visible.

Also, would a screen reader not read Paragraph-4 above? Does a screen reader has the concept of screen an element sizes into account?

kentcdodds commented 5 years ago

I think that defining "not visible" as something that a screen reader wouldn't read and/or something you cannot find via ⌘ + f.

adiun commented 5 years ago

If we change the overflow style to scroll, then would we consider Paragraph-4 to be visible again? Because in that case it can easily become visible.

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...

gnapse commented 5 years ago

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.

adiun commented 5 years ago

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?

kentcdodds commented 5 years ago

Actually now that I think about it, actually visible on the screen makes more sense to me.

alexkrolick commented 5 years ago

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.

kentcdodds commented 5 years ago

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.

alexkrolick commented 5 years ago

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 within).

It would also make sense to have an assertion like toBeInViewport or something, but again, it would be part of jest-dom which doesn't run in Puppeteer or Karma anyways.

kentcdodds commented 5 years ago

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? 🤔

alexkrolick commented 5 years ago

Cypress does this before firing events on elements: https://docs.cypress.io/guides/core-concepts/interacting-with-elements.html#Actionability

Checks and Actions Performed

Scroll the element into view.

Ensure the element is not hidden.

Ensure the element is not disabled.

Ensure the element is not animating.

Ensure the element is not covered.

Scroll the page if still covered by an element with fixed position.

Fire the event at the desired coordinates.

A lot of the checks are in here:

https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/dom/visibility.js

kentcdodds commented 5 years ago

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?

alexkrolick commented 5 years ago

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 user-event, or an add-on that could be called as a pre-filter in another library: isInteractable(element[, window]).

To avoid confusion with simpler visibility checks like the jQuery .visible method I think avoiding isVisible as the function name is a good idea.

dicardopegb commented 5 years ago

Hi. I'm not sure if this is related, but I'm trying this:

I've two forms in the same HTML page, both have inputs with the same labels, like this:

(...)
<form>
<label for="firstName">
First name
</label>
<input
class="form-control"
id="firstName"
maxlength="32"
name="firstName"
placeholder="First name"
required=""
value=""
/>
</form>
(...)
<form data-testid="someid">
First name
</label>
<input
class="form-control"
id="firstName"
maxlength="32"
name="firstName"
placeholder="First name"
required=""
value=""
/>
</form>
(...)

The I try to select the second input by label text by doing this:

const form = getByTestId("someid");
const firstNameInput = getByLabelText(form, "First name");

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.

kentcdodds commented 5 years ago

@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!

kentcdodds commented 5 years ago

I think we're going to opt to put this in another package for now. Anyone can feel free to work on that.

Soulforged commented 5 years ago

@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.

benmonro commented 5 years ago

@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

kentcdodds commented 5 years ago

I never did

sompylasar commented 5 years ago

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.

alexkrolick commented 5 years ago

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.

kentcdodds commented 5 years ago

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.

sompylasar commented 5 years ago

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.

"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.

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.

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.

alexkrolick commented 5 years ago

To focus this back on the latest request:

I'm running into the same issue in testcafe.

The testcafe docs say:

Before executing an action, TestCafe waits for the target element to appear in the DOM and become visible. If this does not happen within the selector timeout, the test fails.

https://devexpress.github.io/testcafe/documentation/test-api/actions/

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.

benmonro commented 5 years ago

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.

kentcdodds commented 5 years ago

Could you use the findBy and findAllBy variants instead?

benmonro commented 5 years ago

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 notifications@github.com 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 https://github.com/testing-library/dom-testing-library/issues/196#issuecomment-487385057, or mute the thread https://github.com/notifications/unsubscribe-auth/AADBPBBOS7AE42AT62XFFMDPSWZADANCNFSM4GSU3ALA .

-- Ben Monro Software Developer

benmonro commented 5 years ago

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 Selector.

1) use .with({visibilityCheck: true}) (it's false by default) which will cause Testcafe to wait for any elements returned by the Selector, or fail if they never become visible.

2) use .filterVisible() or .filterHidden() which will filter out elements as you might expect.

Both of these make it perfectly workable for testcafe-testing-library.

Having said that, it feels like dom-testing-library should have the ability to use getBy when only a single item is visible. Going by the guiding principals (which I 😍 btw), if the tests should be written the way they're intended to be used, then a user wouldn't see 2 things (with the same text) if one was visible and one was hidden, so neither should getBy. Just my $0.02;

alexkrolick commented 5 years ago

Are you primarily talking about display: none (in which case you need a full understanding of all the CSS loaded into the page), or off-screen (in which case you need all the CSS plus a layout engine plus knowledge of the screen dimensions)? None of those things are really feasible in a component-oriented JSDOM test, so even if dom-testing-library had support it would have to be opt-in. Since most testing frameworks that could possibly use this code already have builtin checks for actionability (except Karma?), I'd suggest using those implementations.

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.

kentcdodds commented 5 years ago

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!).

alexkrolick commented 5 years ago

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.

sompylasar commented 5 years ago

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.

jessethomson commented 4 years ago

Another idea if we can't work this out is to improve toBeVisible from jest-dom to support handling more cases.

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 toBeVisible functionality in the queries? That seems to at least be closer to how a user experiences the user interface.

It doesn't cover anything about sizing, scrolling, overflow, etc, but it looks like it covers using display, visibility, opacity, and the hidden attribute, which imo is probably the most common thing to test for. I think this is a pretty common use case when it comes to toggling the visibility of components in various popover/portals.

kentcdodds commented 4 years ago

I like it in theory. The problem will inevitably be performance issues 😬 We already have issues with something similar with the *ByRole query: #552. Maybe we could make it an opt-in thing and people could apply this same strategy to enabling it. Seems pretty reasonable.

I'll reopen this and I'd be willing to look at a PR for this.

rluvaton commented 1 year ago

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);
    };
}

Tests

Tests were copied from https://github.com/testing-library/dom-testing-library/blob/edffb7c5ec2e4afd7f6bedf842c669ddbfb73297/src/__tests__/role.js and updated to this matcher ```typescript describe('accessibleTextContentMatcher', () => { it('should filter out hidden divs', () => { const { getByText, getByTestId } = render(

hello

); 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(); expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow(); }); test('should still find element that are not hidden', () => { const { getByText } = render(

something

); getByText(accessibleTextContentMatcher('something')); }); test('by default excludes elements that have the html hidden attribute on any of their parents', () => { const { getByText } = render( ); expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow(); }); test('by default excludes elements which have display: none', () => { const { getByText } = render(

something

); expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow(); }); test('by default excludes elements which have display: none on any of their parents', () => { const { getByText } = render(

something

); 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(

something

); expect(() => getByText(accessibleTextContentMatcher('something'))).toThrow(); }); test('by default excludes elements which have aria-hidden="true"', () => { const { getByText } = render(); 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( ); 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(

something

); getByText(accessibleTextContentMatcher('something')); }); }); ```

rluvaton commented 1 year ago

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))
);

renet commented 10 months ago

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 ByRoleMatcher.

@rluvaton Any updates on the PR?

testing-library / dom-testing-library