From 0cf5f9f8c5f1d266bf3f5c2e5593c80c7825c04c Mon Sep 17 00:00:00 2001 From: NooBat Date: Mon, 27 Jan 2025 18:41:42 +0700 Subject: [PATCH 1/9] [fix] Fix Rating return NaN when using user event --- packages/mui-material/src/Rating/Rating.js | 10 ++++++++-- packages/mui-material/src/Rating/Rating.test.js | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/mui-material/src/Rating/Rating.js b/packages/mui-material/src/Rating/Rating.js index d6b5f761603982..6efc9854dbe823 100644 --- a/packages/mui-material/src/Rating/Rating.js +++ b/packages/mui-material/src/Rating/Rating.js @@ -404,9 +404,17 @@ const Rating = React.forwardRef(function Rating(inProps, ref) { percent = (event.clientX - left) / containerWidth; } + setFocusVisible(false); + let newHover = roundValueToPrecision(max * percent + precision / 2, precision); newHover = clamp(newHover, precision, max); + if (Number.isNaN(newHover)) { + // Workaround for test scenario using userEvent since jsdom defaults getBoundingClientRect to 0 + // Fix https://github.com/mui/material-ui/issues/38828 + return; + } + setState((prev) => prev.hover === newHover && prev.focus === newHover ? prev @@ -416,8 +424,6 @@ const Rating = React.forwardRef(function Rating(inProps, ref) { }, ); - setFocusVisible(false); - if (onChangeActive && hover !== newHover) { onChangeActive(event, newHover); } diff --git a/packages/mui-material/src/Rating/Rating.test.js b/packages/mui-material/src/Rating/Rating.test.js index 7c84b7e72ce4fb..d4334e14bc048d 100644 --- a/packages/mui-material/src/Rating/Rating.test.js +++ b/packages/mui-material/src/Rating/Rating.test.js @@ -4,6 +4,7 @@ import { stub, spy } from 'sinon'; import { act, createRenderer, fireEvent, screen } from '@mui/internal-test-utils'; import Rating, { ratingClasses as classes } from '@mui/material/Rating'; import { createTheme, ThemeProvider } from '@mui/material/styles'; +import userEvent from '@testing-library/user-event'; import describeConformance from '../../test/describeConformance'; describe('', () => { @@ -121,6 +122,21 @@ describe('', () => { expect(checked.value).to.equal('2'); }); + it('should select the rating with user clicks', async () => { + const user = userEvent.setup(); + const handleChange = spy(); + + const { container } = render(); + + await user.click(screen.getByLabelText('2 Stars')); + + expect(handleChange.callCount).to.equal(1); + expect(handleChange.args[0][1]).to.equal(2); + + const checked = container.querySelector('input[name="rating-test"]:checked'); + expect(checked.value).to.equal('2'); + }); + it('should change the value to null', () => { const handleChange = spy(); render(); From e40d3085e4a294a8adeb47ca8fec8fe6d73bac1e Mon Sep 17 00:00:00 2001 From: ZeeshanTamboli Date: Thu, 30 Jan 2025 14:01:07 +0530 Subject: [PATCH 2/9] trigger CI From ce7b44bd85d977582ce6a66c44ce5df6347f5f5b Mon Sep 17 00:00:00 2001 From: NooBat Date: Thu, 30 Jan 2025 18:12:04 +0700 Subject: [PATCH 3/9] [fix] Fix Rating return NaN when using user event --- packages/mui-material/src/Rating/Rating.js | 10 ++++++++-- .../mui-material/src/Rating/Rating.test.js | 20 ++++++++++++++++++- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/packages/mui-material/src/Rating/Rating.js b/packages/mui-material/src/Rating/Rating.js index d6b5f761603982..6efc9854dbe823 100644 --- a/packages/mui-material/src/Rating/Rating.js +++ b/packages/mui-material/src/Rating/Rating.js @@ -404,9 +404,17 @@ const Rating = React.forwardRef(function Rating(inProps, ref) { percent = (event.clientX - left) / containerWidth; } + setFocusVisible(false); + let newHover = roundValueToPrecision(max * percent + precision / 2, precision); newHover = clamp(newHover, precision, max); + if (Number.isNaN(newHover)) { + // Workaround for test scenario using userEvent since jsdom defaults getBoundingClientRect to 0 + // Fix https://github.com/mui/material-ui/issues/38828 + return; + } + setState((prev) => prev.hover === newHover && prev.focus === newHover ? prev @@ -416,8 +424,6 @@ const Rating = React.forwardRef(function Rating(inProps, ref) { }, ); - setFocusVisible(false); - if (onChangeActive && hover !== newHover) { onChangeActive(event, newHover); } diff --git a/packages/mui-material/src/Rating/Rating.test.js b/packages/mui-material/src/Rating/Rating.test.js index 7c84b7e72ce4fb..175e5cca98757c 100644 --- a/packages/mui-material/src/Rating/Rating.test.js +++ b/packages/mui-material/src/Rating/Rating.test.js @@ -1,9 +1,10 @@ import * as React from 'react'; import { expect } from 'chai'; import { stub, spy } from 'sinon'; -import { act, createRenderer, fireEvent, screen } from '@mui/internal-test-utils'; +import { act, createRenderer, fireEvent, screen, waitFor } from '@mui/internal-test-utils'; import Rating, { ratingClasses as classes } from '@mui/material/Rating'; import { createTheme, ThemeProvider } from '@mui/material/styles'; +import userEvent from '@testing-library/user-event'; import describeConformance from '../../test/describeConformance'; describe('', () => { @@ -121,6 +122,23 @@ describe('', () => { expect(checked.value).to.equal('2'); }); + it('should select the rating with user clicks', async () => { + const user = userEvent.setup(); + const handleChange = spy(); + + const { container } = render(); + + await user.click(screen.getByLabelText('2 Stars')); + + await waitFor(() => { + expect(handleChange.callCount).to.equal(1); + expect(handleChange.args[0][1]).to.equal(2); + + const checked = container.querySelector('input[name="rating-test"]:checked'); + expect(checked.value).to.equal('2'); + }); + }); + it('should change the value to null', () => { const handleChange = spy(); render(); From fbf335b1ba7a95b2e16c09f2b3f072ea85534ca3 Mon Sep 17 00:00:00 2001 From: ZeeshanTamboli Date: Thu, 30 Jan 2025 17:19:03 +0530 Subject: [PATCH 4/9] rerun CI From da88eea494236d47b06db2f1e4cec6a6645875ba Mon Sep 17 00:00:00 2001 From: NooBat Date: Fri, 31 Jan 2025 14:17:42 +0700 Subject: [PATCH 5/9] [fix] Fix Rating return NaN when using user event --- packages/mui-material/src/Rating/Rating.js | 14 ++++++++++++-- packages/mui-material/src/Rating/Rating.test.js | 16 ++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/packages/mui-material/src/Rating/Rating.js b/packages/mui-material/src/Rating/Rating.js index d6b5f761603982..8609b297cf3eaa 100644 --- a/packages/mui-material/src/Rating/Rating.js +++ b/packages/mui-material/src/Rating/Rating.js @@ -393,9 +393,15 @@ const Rating = React.forwardRef(function Rating(inProps, ref) { onMouseMove(event); } + setFocusVisible(false); + const rootNode = rootRef.current; const { right, left, width: containerWidth } = rootNode.getBoundingClientRect(); + if (event.clientX < left || event.clientX > right) { + return; + } + let percent; if (isRtl) { @@ -407,6 +413,12 @@ const Rating = React.forwardRef(function Rating(inProps, ref) { let newHover = roundValueToPrecision(max * percent + precision / 2, precision); newHover = clamp(newHover, precision, max); + if (Number.isNaN(newHover)) { + // Workaround for test scenario using userEvent since jsdom defaults getBoundingClientRect to 0 + // Fix https://github.com/mui/material-ui/issues/38828 + return; + } + setState((prev) => prev.hover === newHover && prev.focus === newHover ? prev @@ -416,8 +428,6 @@ const Rating = React.forwardRef(function Rating(inProps, ref) { }, ); - setFocusVisible(false); - if (onChangeActive && hover !== newHover) { onChangeActive(event, newHover); } diff --git a/packages/mui-material/src/Rating/Rating.test.js b/packages/mui-material/src/Rating/Rating.test.js index 7c84b7e72ce4fb..ea36a368c1ebd5 100644 --- a/packages/mui-material/src/Rating/Rating.test.js +++ b/packages/mui-material/src/Rating/Rating.test.js @@ -4,6 +4,7 @@ import { stub, spy } from 'sinon'; import { act, createRenderer, fireEvent, screen } from '@mui/internal-test-utils'; import Rating, { ratingClasses as classes } from '@mui/material/Rating'; import { createTheme, ThemeProvider } from '@mui/material/styles'; +import userEvent from '@testing-library/user-event'; import describeConformance from '../../test/describeConformance'; describe('', () => { @@ -121,6 +122,21 @@ describe('', () => { expect(checked.value).to.equal('2'); }); + it('should select the rating with user clicks', async () => { + const user = userEvent.setup(); + const handleChange = spy(); + + const { container } = render(); + + await user.click(screen.getByLabelText('3 Stars')); + + expect(handleChange.callCount).to.equal(1); + expect(handleChange.args[0][1]).to.deep.equal(3); + + const checked = container.querySelector('input[name="rating-test"]:checked'); + expect(checked.value).to.equal('3'); + }); + it('should change the value to null', () => { const handleChange = spy(); render(); From a0683173981015c645f07a70756e8d18204ddcbc Mon Sep 17 00:00:00 2001 From: ZeeshanTamboli Date: Fri, 31 Jan 2025 13:16:58 +0530 Subject: [PATCH 6/9] update test description --- packages/mui-material/src/Rating/Rating.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mui-material/src/Rating/Rating.test.js b/packages/mui-material/src/Rating/Rating.test.js index 7134a8398e5ec0..12889a8ca725d7 100644 --- a/packages/mui-material/src/Rating/Rating.test.js +++ b/packages/mui-material/src/Rating/Rating.test.js @@ -122,7 +122,7 @@ describe('', () => { expect(checked.value).to.equal('2'); }); - it('should select the rating with user clicks', async () => { + it('should select the rating with user clicks using user-event', async () => { const user = userEvent.setup(); const handleChange = spy(); From f683fcc428964bcb9ae16fa52ed8b61d3fa5d447 Mon Sep 17 00:00:00 2001 From: ZeeshanTamboli Date: Fri, 31 Jan 2025 13:17:34 +0530 Subject: [PATCH 7/9] remove waitFor import --- packages/mui-material/src/Rating/Rating.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mui-material/src/Rating/Rating.test.js b/packages/mui-material/src/Rating/Rating.test.js index 12889a8ca725d7..59d1214ce81c4b 100644 --- a/packages/mui-material/src/Rating/Rating.test.js +++ b/packages/mui-material/src/Rating/Rating.test.js @@ -1,7 +1,7 @@ import * as React from 'react'; import { expect } from 'chai'; import { stub, spy } from 'sinon'; -import { act, createRenderer, fireEvent, screen, waitFor } from '@mui/internal-test-utils'; +import { act, createRenderer, fireEvent, screen } from '@mui/internal-test-utils'; import Rating, { ratingClasses as classes } from '@mui/material/Rating'; import { createTheme, ThemeProvider } from '@mui/material/styles'; import userEvent from '@testing-library/user-event'; From c19d2468fea37cca828d333ab4202484fb9f9026 Mon Sep 17 00:00:00 2001 From: NooBat Date: Fri, 31 Jan 2025 14:52:36 +0700 Subject: [PATCH 8/9] remove redundant setFocusVisible --- packages/mui-material/src/Rating/Rating.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/mui-material/src/Rating/Rating.js b/packages/mui-material/src/Rating/Rating.js index cdf07a25d0f616..8609b297cf3eaa 100644 --- a/packages/mui-material/src/Rating/Rating.js +++ b/packages/mui-material/src/Rating/Rating.js @@ -410,8 +410,6 @@ const Rating = React.forwardRef(function Rating(inProps, ref) { percent = (event.clientX - left) / containerWidth; } - setFocusVisible(false); - let newHover = roundValueToPrecision(max * percent + precision / 2, precision); newHover = clamp(newHover, precision, max); From feb8d8b40165a0435014afbd4b3473e63f71a7cf Mon Sep 17 00:00:00 2001 From: ZeeshanTamboli Date: Fri, 31 Jan 2025 14:03:16 +0530 Subject: [PATCH 9/9] trigger CI