[Rating] Fix Rating returns NaN when using user event

[NooBat]

• I have followed (at least) the PR section of the contributing guide.

Closes #38828

[mui-bot]

Netlify deploy preview

https://deploy-preview-45054--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 6eba2a4

[siriwatknp]

👍 Thanks for the fix and welcome to MUI contributors.

[ZeeshanTamboli]

Why is this logic needed?

[NooBat]

The userEvent.click emits a mousemove event with clientX = 0, while in Karma, the Rating's bounding rect has left = 8. If we don't use this logic, the newHover would be 1 regardless of where we click. This also not align with real-world scenario, where if you not hovering over the Rating, it should not trigger the onMouseMove handler.

[ZeeshanTamboli]

if you not hovering over the Rating, it should not trigger the onMouseMove handler.

I didn't understand. Doesn't mousemove trigger over the rating container when the user clicks the rating? Shouldn't we simulate it in the test for browsers?

[NooBat]

I have been trying to find a way to correctly simulate a mousemove event with the correct event.clientX but no success so far. Currently, user-event doesn't seem to support that.

[ZeeshanTamboli]

I am reluctant to have this logic just for a test. Maybe simulate a mousemove using fireEvent.mouseMove above user.click.

[NooBat]

@DiegoAndai if we mock the getBoundingClientRect, the clientX from MouseMove event will still be 0.
Hence, when actually setting the value in handleChange:

  const handleChange = (event) => {
    let newValue = event.target.value === '' ? null : parseFloat(event.target.value);

    // Give mouse priority over keyboard
    // Fix https://github.com/mui/material-ui/issues/22827
    if (hover !== -1) {
      newValue = hover;
    }

    setValueState(newValue);

    if (onChange) {
      onChange(event, newValue);
    }
  };

The hover state variable will always be 1 while the newValue will have the actual value

[DiegoAndai]

Understood.

Again, we shouldn't modify the component to handle a user event limitation. If we can't find a way around it, I prefer to document it and let users know not to use the user event for this instead of modifying the component.

Does this make sense @NooBat?

[NooBat]

@DiegoAndai From what I understand we should remove this

if (event.clientX < left || event.clientX > right) {
    return;
}

and keep this in, correct?

if (!containerWidth) {
    // Workaround for test scenario using userEvent since jsdom defaults getBoundingClientRect to 0
    // Fix https://github.com/mui/material-ui/issues/38828
    return;
}

[DiegoAndai]

No, I think we shouldn't have this:

if (!containerWidth) {
    // Workaround for test scenario using userEvent since jsdom defaults getBoundingClientRect to 0
    // Fix https://github.com/mui/material-ui/issues/38828
    return;
}

Only to fix a userEvent limitation. If there's no other way around it, I would keep the component as is and add a warning in the documentation.

[NooBat]

@DiegoAndai So we just skip the suggestions from Oliver in #38828?
He did said:

3. Consider special handling in the logic if getBoundingClientRect is not available, effectively for tests, we have a couple of those already in the codebase.