toHaveStyle for property `font-size` behaves incorrectly for number values #564

codepath2019 · 2023-12-27T21:18:21Z

@testing-library/jest-dom version: 6.1.5
node version: 20.3.1
jest (or vitest) version: 0.34.1
npm (or yarn) version: 9.6.7

Relevant code or config:

render(<div data-testid="element" style={{ fontSize: 8 }} />)
expect(screen.getByTestId('element')).toHaveStyle({ fontSize: 1 })

What you did:

I expected the above assertion to fail

What happened:

The test passes instead!✅

Reproduction:

Problem description:

the assertion is clearly incorrect and can easily cause bugs

Suggested solution:

I have not thought of a solution yet but would like to investigate this issue during my off hours.

The text was updated successfully, but these errors were encountered:

Given an invalid declaration such as `fontSize: 8`, due to the missing unit, the `toHaveStyle` matcher should not pass the following test: ``` render(<div data-testid="element" style={{ fontSize: 8 }} />) expect(screen.getByTestId('element')).toHaveStyle({ fontSize: 1 }) ``` This PR fixes testing-library#564 by adding a more restrictive guard in the matcher's logic.

EduardoSimon · 2024-02-16T22:43:27Z

Hi @gnapse, I've created a PR that addresses this issue. Please, feel free to comment or request any change, I'm a new contributor to the project (although a long time user 😃) and I'd like your feedback. Cheers.

ludacirs · 2024-03-12T02:13:23Z

I reside in a non-English-speaking region, so my English may be a bit clumsy. If I used incorrect vocabulary or seemed impolite, I apologize in advance.

While EduardoSimon's idea of failing the test in cases where an empty string occurs is valid, to understand why empty strings are occurring, I examined the behavior of CSSStyleDeclaration.

I found two issues:

There is an issue in the getStyleDeclaration function where providing an incorrect value results in an empty string being added to the styles object.
There is an issue in the computedStyle.getPropertyValue(property) function where using camelCase for property always returns an empty string.

1. getStyleDeclaration

function getStyleDeclaration(document, css) {
  const styles = {}

  const copy = document.createElement('div')
  Object.entries(css).forEach(([property, value]) => {
  	// if value is incorrect
    copy.style[property] = value
  	//  then styles[property] have "" value
    styles[property] = copy.style[property]
  })
  return styles
}

Returning an empty string when an incorrect value is provided might be considered expected.

Let's determine what is considered an incorrect value.

const copy = document.createElement('div');
copy.style.borderWidth = '1' // incorrect value => copy.style.borderWidth is ""
copy.style.borderWidth = 1 // incorrect value ""
copy.style.borderWidth = '1px' // correct value "1px"

copy.style.background = 'test' // incorrect value ""
copy.style.background = 1234 // incorrect value ""
copy.style.background = 'red' // correct value "red"

It seems that the rule for determining what is considered incorrect varies for each property.

A point here is that providing a number for borderWidth results in an empty string being returned.

2. CSSStyleDeclaration: getPropertyValue()

The getPropertyValue function returns an empty string when provided with camelCase.

const div = document.createElement('div');

div.style.borderWidth = '2px';

div.style.borderWidth // 2px
div.style.getPropertyValue('borderWidth') // ""
div.style.getPropertyValue('border-width') // 2px

Now that we have identified the two bugs, let's see how these bugs manifest in our tests.

As of the current version, 6.4.2, the following tests all pass…

// passed
const {container} = render(`<div class="border" style="border-width: 2px;/>`)
expect(container.querySelector('.border')).toHaveStyle({ borderWidth: '' })

// passed
const {container} = render(`<div class="border" style="border-width: 2px;/>`)
expect(container.querySelector('.border')).toHaveStyle({ borderWidth: 1 })

// passed
const {container} = render(`<div class="border" style="border-width: 2px;/>`)
expect(container.querySelector('.border')).toHaveStyle({ borderWidth: '2' })

Let's examine the flow based on the second test case.

The getStyleDeclaration function transforms our style object passed to toHaveStyle.

function getStyleDeclaration(document, css) {
  const styles = {}

  // css { borderWidth: 1 } => Remember, providing a number was considered an incorrect value
  const copy = document.createElement('div')
  Object.entries(css).forEach(([property, value]) => {
    copy.style[property] = value
    styles[property] = copy.style[property]
  })
  // styles {borderWidth: ''}
  return styles
}

Consequently, borderWidth became an empty string.

Next, the determination of test passage in isSubset is examined.

// the styles object is our styles returned by the getStyleDeclaration({ borderWidth: '' }).

// The computedStyle object is the CSSStyleDeclaration object of the `<div class="border" style="border-width: 2px;"/>` rendered in the test code.

function isSubset(styles, computedStyle) {
  return (
    !!Object.keys(styles).length &&
    Object.entries(styles).every(([prop, value]) => {
      const spellingVariants = [prop]
      if (!isCustomProperty(prop)) spellingVariants.push(prop.toLowerCase())

      return spellingVariants.some(name => {
        return (
          computedStyle[name] === value ||
          **computedStyle.getPropertyValue(name) === value**
          // Here, getPropertyValue returns an empty string,
          // and the value is also an empty string. Therefore, it returns true.
        )
      })
    })
  )
}

// prop, name
// ⇒ borderWidth

// value
// ⇒ ‘’ // because first case

// computedStyle[name]
// ⇒ 2px

// computedStyle.getPropertyValue(name)
// ⇒ ‘’ // because second case

// Therefore, the comparison values for spellingVariants.some(~) are as follows:
// computedStyle[name] === value
// 2px === ‘’ ⇒ false
// computedStyle.getPropertyValue(name) === value
// ‘’ === ‘’ ⇒ true

We can see why the test that should fail is passing.

@gnapse
Can you review and fix this bug if I submit a pull request with the changes?

codepath2019 mentioned this issue Dec 27, 2023

toHaveStyle does not behave consistently between number and string values #561

Closed

gnapse added bug Something isn't working good first issue Good for newcomers labels Dec 28, 2023

EduardoSimon linked a pull request Feb 16, 2024 that will close this issue

fix: toHaveStyle assertion with invalid style (#564) #582

Open

4 tasks

ludacirs mentioned this issue Mar 14, 2024

fix: add a convertCssObject to create a valid style object within the toHaveStyle. #591

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

toHaveStyle for property `font-size` behaves incorrectly for number values #564

toHaveStyle for property `font-size` behaves incorrectly for number values #564

codepath2019 commented Dec 27, 2023

EduardoSimon commented Feb 16, 2024

ludacirs commented Mar 12, 2024

toHaveStyle for property font-size behaves incorrectly for number values #564

toHaveStyle for property font-size behaves incorrectly for number values #564

Comments

codepath2019 commented Dec 27, 2023

Relevant code or config:

What you did:

What happened:

Reproduction:

Problem description:

Suggested solution:

EduardoSimon commented Feb 16, 2024

ludacirs commented Mar 12, 2024

1. getStyleDeclaration

2. CSSStyleDeclaration: getPropertyValue()

toHaveStyle for property `font-size` behaves incorrectly for number values #564

toHaveStyle for property `font-size` behaves incorrectly for number values #564