Use innerText rather than textContent for label queries. #190

ismail-codar · 2019-01-09T11:55:06Z

What:
Using label.innerText rather than label.textContent

Why:
An example dom:

<label>
Options
<select>
    <option>Option 1</option>
    <option>Option 2</option>
</select>
</label>

textContent returns incorrect result for label text

Options

    Option 1
    Option 2

but innerText returns correct result "Options".

But I can not be simulate with jest tests also label.innerText || label.textContent used because jsdom does not support innerText jsdom/jsdom#1245

How:

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table

label.textContent usage is problematic for select box in label

kentcdodds · 2019-01-09T20:54:54Z

Interesting. I've never seen a select inside a label like that... 🤔

I remember writing this code and I remember researching and making the decision to use textContent rather than innerText. In retrospect I wish I had written a comment about that decision, but I didn't. I also didn't write a test to illustrate the issue that I saw with using innerText. I worry that if we merge this we'll break a bunch of tests 😬

ismail-codar · 2019-01-10T00:28:20Z

Currently (using textContent) -> select inside label gives false results. It explained above.
Using innerText rather than textContent is correct solution but a lot of test are break because test runner (jest) using jsdom and jsom does not support innerText.
label.innerText || label.textContent usage is intermediate solution.

Also migrating jest to karma is another stuff to avoid jsdom related problems.

kentcdodds · 2019-01-10T02:34:13Z

Ah, the fact that jsdom doesn't support innerText anyway may be the reason I landed with textContent actually 😅

I'm good with this PR 👍 Thanks for explaining!

kentcdodds · 2019-01-10T02:37:16Z

src/queries.js

@@ -20,7 +20,7 @@ function queryAllLabelsByText(
  const matcher = exact ? matches : fuzzyMatches
  const matchNormalizer = makeNormalizer({collapseWhitespace, trim, normalizer})
  return Array.from(container.querySelectorAll('label')).filter(label =>
-    matcher(label.textContent, label, text, matchNormalizer),
+    matcher(label.innerText || label.textContent, label, text, matchNormalizer),


Actually, I think it may be better to:

Change this to: matcher(getNodeText(label), label, text, matchNormalizer),

Update getNodeText to try innerText before textContent

This way we get the benefit of using innerText for all text selectors.

Could you make that adjustment here please?

I will sent it with a test.

alexkrolick · 2019-01-10T02:37:20Z

Does this still work if you have a nested span or something inside the label?

<label><span>Translation framework added a span</span></label>

I think we need to minimally add tests in JSDOM that mock out and test the different behavior.

kentcdodds · 2019-01-10T02:42:31Z

Does this still work if you have a nested span or something inside the label?

Yep 👍

I think we need to minimally add tests in JSDOM that mock out and test the different behavior.

I think I agree with you, though I'm not sure what you're suggesting.

alexkrolick · 2019-01-10T02:50:45Z

I think I agree with you, though I'm not sure what you're suggesting.

Something like a stubbing out the .innerText method in the environment or at least for one test: jsdom/jsdom#1245 (comment)

Then verifying that form elements (or whatever else is excluded from innerTextification) are omitted.

? Due to the stubbing, this is probably more like a documentation case than something that is actually testing functionality. I guess you could just assert .innerText is called if present.

kentcdodds · 2019-01-10T02:52:50Z

I guess you could just assert .innerText is called if present.

That's probably the best we could do by way of a test and I'm not sure how valuable that test would be but I'd definitely appreciate having it if @ismail-codar wants to add it.

getNodeText(label) is results to label.innerText

kentcdodds · 2019-01-10T05:32:04Z

src/queries.js

@@ -20,7 +20,7 @@ function queryAllLabelsByText(
  const matcher = exact ? matches : fuzzyMatches
  const matchNormalizer = makeNormalizer({collapseWhitespace, trim, normalizer})
  return Array.from(container.querySelectorAll('label')).filter(label =>
-    matcher(label.textContent, label, text, matchNormalizer),
+    matcher(getNodeText(label), label, text, matchNormalizer),


Don't forget that you'll need to update the getNodeText function to use innerText 👍

ismail-codar · 2019-01-10T08:32:34Z

src/__tests__/elements-inside-label.js

+
+  // this fails because getNodeText function returns incorrect result on JSDOM but it good for browser
+  // const element2 = getByLabelText(container, 'States');
+  // expect(element2.tagName).toBe('SELECT');


This lines added temporarily added until switching jest to karma.

This lines added temporarily added until switching jest to karma.

Wait.... Are you attempting to migrate our tests to karma? If so then please don't take the time, it will not be merged.

If you want to add a few tests in karma and leave the existing tests as they are that's fine. The bill of the tests should be in jest, but I'd be happy to have a few tests that run in a real browser with karma.

Just want to make sure we're clear on expectations here.

Ok I understand.

itsravenous · 2019-02-20T07:57:11Z

What needs to happen to get this merged? I would love to be able to use getByText to select an element whose text is split within several child nodes.

alexkrolick · 2019-02-20T08:17:23Z

What needs to happen to get this merged? I would love to be able to use getByText to select an element whose text is split within several child nodes.

Not sure. It sounds like there a ton of layout edge cases causing innerText not to be practical in JSDOM. I don't know if working around/semi-polyfilling the behavior is a good idea if it deviates from the spec.

And naively doing a recursive join of the text content of all the nodes would be pretty memory-intensive and probably not what you want most of the time... 🤔

<body>
	<button>
		<span>Click Me</span>
	</button>
</body>

What gets returned here? Outermost element is body...?

Cypress uses the native innerText and textContent methods: https://github.com/cypress-io/cypress/blob/55b352a11d0a664517ae054c67432d98abc2389e/packages/driver/src/cy/commands/querying.coffee#L330-L337

itsravenous-sky · 2019-03-06T07:35:23Z

I think I misunderstood the purpose of this PR. I'm after being able to query for an element with text split over multiple children, e.g:

<div>Hi <span>there</span></div>

getByText('Hi there')

I'll look into it and raise an issue/PR

kentcdodds · 2019-03-19T21:07:45Z

Ok, so in investigating this, I realized there's a flaw in the original assumption:

but innerText returns correct result "Options".

That is not correct. This is what I've learned:

labelNode.innerText
"Options 
Option 1
Option 2"


labelNode.textContent
"
Options

    Option 1
    Option 2
"

So, our current implementation is great because when running this code with getNodeText (without changes), we have:

getNodeText(labelNode)
"
Options

"

Which is correct I believe. So I'll go ahead and make that fix and we should be good to go.

Closes #190

kentcdodds · 2019-03-19T21:23:34Z

🎉 This issue has been resolved in version 3.18.1 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

mcmillion · 2019-03-20T01:02:37Z

Is this change why I suddenly have failing tests with getByLabelText for labels with nested spans? Just started happening today on a new machine with a fresh yarn install.

kentcdodds · 2019-03-20T01:04:20Z

Ah yes, it might be. I think there's a solution to this. I'll have to work on it tomorrow. Could you make a new issue?

NehrDani · 2019-03-20T10:17:55Z

Ah yes, it might be. I think there's a solution to this. I'll have to work on it tomorrow. Could you make a new issue?

I did: #230

Use innerText rather than textContent for label queries.

fab6064

label.textContent usage is problematic for select box in label

kentcdodds reviewed Jan 10, 2019

View reviewed changes

Using getNodeText

8d81c51

getNodeText(label) is results to label.innerText

kentcdodds reviewed Jan 10, 2019

View reviewed changes

elements-inside-label test added

b0ae02c

ismail-codar commented Jan 10, 2019

View reviewed changes

Removed code formatting

c148ce3

alexkrolick mentioned this pull request Feb 5, 2019

getByText()/queryByText() (or some new query) should search across nested elements #201

Closed

kentcdodds mentioned this pull request Feb 8, 2019

Click on a disabled button with nested elements is silently ignored testing-library/cypress-testing-library#34

Closed

alexkrolick added the help wanted Extra attention is needed label Feb 15, 2019

kentcdodds closed this Mar 19, 2019

kentcdodds pushed a commit that referenced this pull request Mar 19, 2019

fix(ByLabel): use getNodeText abstraction instead of textContent

c337ea1

Closes #190

kentcdodds mentioned this pull request Mar 19, 2019

fix(ByLabel): use getNodeText abstraction instead of textContent #227

Merged

4 tasks

kentcdodds pushed a commit that referenced this pull request Mar 19, 2019

fix(ByLabel): use getNodeText abstraction instead of textContent (#227)

96f21a5

Closes #190

kentcdodds added the released label Mar 19, 2019

NehrDani mentioned this pull request Mar 20, 2019

Latest release (3.18.1) breaks labels with nested spans #230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use innerText rather than textContent for label queries. #190

Use innerText rather than textContent for label queries. #190

ismail-codar commented Jan 9, 2019 •

edited

kentcdodds commented Jan 9, 2019

ismail-codar commented Jan 10, 2019 •

edited

kentcdodds commented Jan 10, 2019

kentcdodds Jan 10, 2019

ismail-codar Jan 10, 2019

alexkrolick commented Jan 10, 2019 •

edited

kentcdodds commented Jan 10, 2019

alexkrolick commented Jan 10, 2019

kentcdodds commented Jan 10, 2019

kentcdodds Jan 10, 2019

ismail-codar Jan 10, 2019

kentcdodds Jan 10, 2019

ismail-codar Jan 10, 2019

itsravenous commented Feb 20, 2019

alexkrolick commented Feb 20, 2019 •

edited

itsravenous-sky commented Mar 6, 2019

kentcdodds commented Mar 19, 2019

kentcdodds commented Mar 19, 2019

mcmillion commented Mar 20, 2019

kentcdodds commented Mar 20, 2019

NehrDani commented Mar 20, 2019

Use innerText rather than textContent for label queries. #190

Use innerText rather than textContent for label queries. #190

Conversation

ismail-codar commented Jan 9, 2019 • edited

kentcdodds commented Jan 9, 2019

ismail-codar commented Jan 10, 2019 • edited

kentcdodds commented Jan 10, 2019

kentcdodds Jan 10, 2019

Choose a reason for hiding this comment

ismail-codar Jan 10, 2019

Choose a reason for hiding this comment

alexkrolick commented Jan 10, 2019 • edited

kentcdodds commented Jan 10, 2019

alexkrolick commented Jan 10, 2019

kentcdodds commented Jan 10, 2019

kentcdodds Jan 10, 2019

Choose a reason for hiding this comment

ismail-codar Jan 10, 2019

Choose a reason for hiding this comment

kentcdodds Jan 10, 2019

Choose a reason for hiding this comment

ismail-codar Jan 10, 2019

Choose a reason for hiding this comment

itsravenous commented Feb 20, 2019

alexkrolick commented Feb 20, 2019 • edited

itsravenous-sky commented Mar 6, 2019

kentcdodds commented Mar 19, 2019

kentcdodds commented Mar 19, 2019

mcmillion commented Mar 20, 2019

kentcdodds commented Mar 20, 2019

NehrDani commented Mar 20, 2019

ismail-codar commented Jan 9, 2019 •

edited

ismail-codar commented Jan 10, 2019 •

edited

alexkrolick commented Jan 10, 2019 •

edited

alexkrolick commented Feb 20, 2019 •

edited