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

<select> doesn't reflect new "select text" in response to change event #115

Closed
theneva opened this issue Oct 9, 2018 · 18 comments · Fixed by #124
Closed

<select> doesn't reflect new "select text" in response to change event #115

theneva opened this issue Oct 9, 2018 · 18 comments · Fixed by #124
Labels
jsdom Issue with JSDOM environment

Comments

@theneva
Copy link

theneva commented Oct 9, 2018

  • dom-testing-library version: 3.8.2
  • react version: N/A
  • node version: v10.11.0 (but it doesn't matter)
  • npm (or yarn) version: yarn 1.10.1 (but it doesn't matter)

Relevant code or config:

See the reproduction section below.

What you did:

  1. Created a <div> containing a <select> with two <option>s.
  2. Fetched the <select> using getBySelectText
  3. Fired a change event on the fetched <select> to select another valid option
  4. Verified that the value was changed
  5. Attempted to fetch the <select> from the <div> again using the newly selected option's label (child element)

What happened:

The <select> cannot be fetched by its new "select text":

 FAIL  ./index.test.js
  ✕ should respond to changes (4520ms)

  ● should respond to changes

    Unable to find a <select> element with the selected option's text: Option 2

    <div>
      <select>


        <option
          value="option-1"
        >
          Option 1
        </option>


        <option
          value="option-2"
        >
          Option 2
        </option>


      </select>
    </div>

      22 |     // but this times out because the text hasn't been updated.
      23 |     // No changes are reflected in the DOM, which is probably why…
    > 24 |     return waitForElement(() => getBySelectText(container, 'Option 2'));
         |                                 ^
      25 | });
      26 |

      at getElementError (node_modules/dom-testing-library/dist/query-helpers.js:29:10)
      at getAllBySelectText (node_modules/dom-testing-library/dist/queries.js:375:45)
      at firstResultOrNull (node_modules/dom-testing-library/dist/query-helpers.js:37:30)
      at getBySelectText (node_modules/dom-testing-library/dist/queries.js:385:42)
      at getBySelectText (index.test.js:24:33)
      at onMutation (node_modules/dom-testing-library/dist/wait-for-element.js:48:22)
      at node_modules/dom-testing-library/dist/wait-for-element.js:66:7
      at waitForElement (node_modules/dom-testing-library/dist/wait-for-element.js:26:10)
      at Object.waitForElement (index.test.js:24:12)

Reproduction:

const { getBySelectText, fireEvent, wait, waitForElement } = require('dom-testing-library');

it('should respond to changes', () => {
    const select = document.createElement('select');
    select.innerHTML = `
        <option value="option-1">Option 1</option>
        <option value="option-2">Option 2</option>
    `;

    const container = document.createElement('div');
    container.appendChild(select);

    const foundSelectBeforeEvent = getBySelectText(container, 'Option 1');
    expect(foundSelectBeforeEvent.value).toBe('option-1');

    fireEvent.change(foundSelectBeforeEvent, { target: { value: 'option-2' }});

    // It has been updated, and this passes
    expect(foundSelectBeforeEvent.value).toBe('option-2');

    // I would then expect to be able to get the <select> again by the new text,
    // but this times out because the text hasn't been updated.
    // No changes are reflected in the DOM, which is probably why…
    return waitForElement(() => getBySelectText(container, 'Option 2'));
});

I've tried (among other things):

  • waiting for a tick to let updates happen
  • Firing events for focus and blur before and after firing the change to force a rerender
  • Not wrapping the fetching in waitForElement (it just fails in the same way, but faster)

Problem description:

The <select> element doesn't seem to update its displayed value, so it behaves differently from what a user experiences in a browser.

Suggested solution:

The <select> should be selectable using the label of the selected option. I don't know how to fix it, though 😄

@kentcdodds
Copy link
Member

Hmmm... I'm not sure what's going on here. Would anyone care to dig deeper into this?

I created this and I'm unable to reproduce: https://jsbin.com/lukecehisa/edit?html,js,console

@theneva
Copy link
Author

theneva commented Oct 9, 2018

Thanks for checking it out

I'm pretty sure the jsbin snippet works because it executes in the browser, and as I mentioned in the OP, this behaviour for the select is different from the browser.

Would be great if someone could take a look! 😄

@alexkrolick
Copy link
Collaborator

I can reproduce the same error in a Jest/JSDOM environment

@kentcdodds
Copy link
Member

Hmmm... Maybe it's a bug in JSDOM :-(

@alexkrolick
Copy link
Collaborator

alexkrolick commented Oct 9, 2018

Probably this: jsdom/jsdom#2326, fixed Aug 18

https://github.com/kentcdodds/dom-testing-library/blob/master/src/queries.js#L117

The fix went out in JSDOM 12.0.0 https://github.com/jsdom/jsdom/blob/master/Changelog.md#1200

But I'm seeing jsdom 11.5.1 coming in via jest-environment-jsdom

@kentcdodds
Copy link
Member

Ah, in that case I'm pretty sure we'll need an updated version of jsdom which I'm guessing is already in the latest version of Jest 🤔

@alexkrolick
Copy link
Collaborator

@kentcdodds
Copy link
Member

@theneva perhaps you could open a PR on Jest to upgrade JSDOM?

@kentcdodds
Copy link
Member

In any case, there's not much we can do in this project, so I'm going to go ahead and close this issue. Sorry! 😬

@alexkrolick
Copy link
Collaborator

Try adding this to package.json

  "resolutions": {
    "jsdom": "12.2.0"
  }

Didn't seem to work for me but maybe you can figure something out.

@alexkrolick alexkrolick added the jsdom Issue with JSDOM environment label Oct 9, 2018
@SimenB
Copy link
Contributor

SimenB commented Oct 9, 2018

We won't upgrade jsdom in jest (for some time) as they've dropped node 6. See e.g. jestjs/jest#7122

@kentcdodds
Copy link
Member

Ah, that's unfortunate. Any timeframe on when jest can drop node < 8?

@SimenB
Copy link
Contributor

SimenB commented Oct 9, 2018

Probably not before it's EOL-ed, although we dropped 4 in December 2017. But I wouldn't bet on a PR dropping 6 landing before Christmas

@kentcdodds
Copy link
Member

Ok, thanks!

@theneva
Copy link
Author

theneva commented Oct 10, 2018

With help from @SimenB I put together a version of jest-environment-jsdom that ships JSDOM version 12:

GitHub: https://github.com/theneva/jest-environment-jsdom-twelve
npm: https://www.npmjs.com/package/jest-environment-jsdom-twelve

Using that solves my issue 😄

Thanks for looking into this!

@matchai
Copy link
Contributor

matchai commented Oct 12, 2018

Though HTMLSelectElement.selectedOptions is not reactive in the version of JSDOM bundled with Jest, HTMLSelectElement.selectedIndex is.

I wouldn't mind opening a PR to change it to selectedIndex, but that would drop compatibility with the multiple attribute.

@kentcdodds What would you prefer?

@kentcdodds
Copy link
Member

Hmmmm.... What if we get the best of all worlds. If the select has multiple then let's use selectedOptions (hopefully they don't need it to be reactive), if it's not multiple then we can use selectedIndex.

Is there any way we could iterate through the...

Yeah, we can do this:

const selectedOptions = Array.from(selectElement.options).filter(option => option.selected)

@kentcdodds kentcdodds reopened this Oct 12, 2018
kentcdodds pushed a commit that referenced this issue Oct 12, 2018
<!--
Thanks for your interest in the project. Bugs filed and PRs submitted are appreciated!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

If you're new to contributing to open source projects, you might find this free
video course helpful: http://kcd.im/pull-request

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->

**What**:

Replace the existing select query mechanism which uses `HTMLSelectElement .selectedOptions` with a query that searches for the `selected` attribute of an `HTMLOptionElement`.

<!-- Why are these changes necessary? -->

**Why**:

Closes #115.

Since `selectedOptions` is not a reactive property, JSDOM was not updating to match the selected option in a select Element. The `selected` attribute is reactive.

<!-- How were these changes implemented? -->

**How**:

Instead of iterating through `selectedOptions` of an HTMLSelectElement, we iterate through all HTMLSelectOptions and isolate the ones that are selected.
<!-- Have you done all of these things?  -->

I've gone ahead and tested this locally to make sure that it would be reactive! 👍 

**Checklist**:

<!-- add "N/A" to the end of each line that's irrelevant to your changes -->

<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->

- [ ] Documentation N/A
- [ ] Tests N/A
- [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [x] Added myself to contributors table <!-- this is optional, see the contributing guidelines for instructions -->

<!-- feel free to add additional comments -->
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 3.10.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jsdom Issue with JSDOM environment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants