[Material][Menu] Fix MenuPaper class composition precedence

[DiegoAndai]

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

Summary

This bug is closely related to #31013. The issue is that for the Menu component's Paper, PopoverPaper styles are being applied on top of MenuPaper styles, when it should be the other way around.

This is because the composition is not in the correct order: We use as to provide the MenuPaper to the Popover, so PopoverPaper is built on top of MenuPaper, instead of MenuPaper being built on top of PopoverPaper, which causes:

• Properties defined in MenuPaper that are also defined in PopoverPaper to get overridden, which caused [material][Menu] Menu popover not leaving enough space to allow exiting out from long lists #36838
• Theme customization not to work for MenuPaper properties that are also defined in PopoverPaper: Codesandbox

Solution

To solve this, I implemented the slots and slotProps props on the Popover component with the paper key. With that, the new composition works as:

• PopoverPaper is imported into the Menu component's file
• MenuPaper is built on top of PopoverPaper
• MenuPaper is passed to Popover using the paper slot

Some other changes that I did were

• Refactor Menu to use slotProps instead of PaperProps, for consistency as we're now using the slot architecture. PaperProps is still supported on the Popover component.
• Refactor tests and add tests for the new implementations as well as missing cases for both Popover and Menu

Pending

Some questions I have that should be resolved before merging:

• Should we document Popover's new slots and slotProps props? Maybe we want to leave them as internal, although I don't see the harm in making them public
• Should I implement the components and componentProps equivalence? I didn't because slots are the new convention
• When implementing the paper slot, I had to refactor the way the props are passed to the Paper component inside the Popover to use useSlotProps. Every prop that I moved had a test associated (or I added it), except for the usage of style: isPositioned ? paperSlotProps.style : { ...paperSlotProps.style, opacity: 0 } which was added on this PR: [Popover] Fix paper position flash on open #34546. I could find a way to create a test for it, is there a way to test it or should I just leave it?
• Should I implement the root key on the slots prop as well? I didn't because it's not required for the fix

Please add any other suggestions you might have!

closses #36838

[mui-bot]

Netlify deploy preview

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

@material-ui/core: parsed: +0.10% , gzip: +0.12%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 287dff7

[DiegoAndai]

The failing test is because of the missing documentation for slots and slotProps, which is one of my questions in the description:

• Should we document Popover's new slots and slotProps props? Maybe we want to leave them as internal, although I don't see the harm in making them public

That's why I haven't fixed it yet

[mnajdova]

Should we document Popover's new slots and slotProps props? Maybe we want to leave them as internal, although I don't see the harm in making them public

Yes, we should document them.

Should I implement the components and componentProps equivalence? I didn't because slots are the new convention

No, we don't want to add props we plan to remove :) It's best to teach developers to move to the new API

When implementing the paper slot, I had to refactor the way the props are passed to the Paper component inside the Popover to use useSlotProps. Every prop that I moved had a test associated (or I added it), except for the usage of style: isPositioned ? paperSlotProps.style : { ...paperSlotProps.style, opacity: 0 } which was added on this PR: #34546. I could find a way to create a test for it, is there a way to test it or should I just leave it?

You can use toHaveComputedStyle to test specific styles. Note, we don't test this in JSDom, so you will need a check like this at the start of the test:

    if (/jsdom/.test(window.navigator.userAgent)) {
      this.skip();
    }

Should I implement the root key on the slots prop as well? I didn't because it's not required for the fix

Yes, let's add all slots that makes sense in one go.

[DiegoAndai]

Hey @mj12albert @mnajdova thanks for you comments! I implemented the suggested changes:

• Added test for the style: isPositioned ? paperSlotProps.style : { ...paperSlotProps.style, opacity: 0 } paper prop ([Popover] Fix paper position flash on open #34546). The tricky part was figuring out that that prop forces the opacity to be set to 1 only after onEntering has been called, which I finally tested.
• Made PaperProps and slotProps.paper actually interchangeable. I documented that slotProps.paper should be used and that it overrides PaperProps, which I marked as deprecated.
• Implemented root key for slots and slotProps. I'm unsure if the types I defined in Popover.d.ts for these props are correct, please help me check this.
• Added tests and documentation for everything.

Please review when you have time to do so 😃

[mj12albert]

Looks good 👍

[ethaizone]

Hi @DiegoAndai

I found issue in here. When SelectInput component pass slotProps to this component. It will make slotProps appear in other variable at line 80. so we can lose value.

Here is example to give picture when it can happen.

<Select
  ...
  MenuProps={{
    slotProps: {
      root: {
        slotProps: {
          backdrop: {
            invisible: true,
          },
        },
      },
    },
  }}
>
  some item...
</Select>

I think you should extract slotProps and merge at this line.

[DiegoAndai]

Hi @ethaizone, you're correct!

I overlooked that when adding the slots and slotProps to the Popover component, those would also be made available to the Menu component. If a user provides either, we would lose the values provided in the Menu component.

I created an issue and will be working on it next week: #37612

Thanks for spotting this 😊