carbon-design-system / carbon

A design system built by IBM
https://www.carbondesignsystem.com
Apache License 2.0
7.84k stars 1.81k forks source link

StatefulTable: OverflowMenu should scroll with its container #9082

Closed ravinain closed 3 years ago

ravinain commented 3 years ago

RowAction OverflowMenu in StatefulTable is not scrolling with the container.

This issue is similar to https://github.com/carbon-design-system/carbon/issues/4755 . However, I'm using StatefulTable

When I set the parent div position to relative, it fixes the scroll issue but introduces another. After scroll, the position of overflow menu is not correct. This is exactly same problem mentioned in the aforementioned ticket.

Dependency: carbon-addons-iot-react: 2.145 Browser: Chrome

Expected Behavior: OverflowMenu should scroll with container. Also, after scroll the overflow menu should display at correct position.

Steps to reproduce the issue

  1. .parent div is scrollable
  2. scroll div
  3. Now click on overflow rowAction
  4. Notice the position of overflow rowAction menu

Example:

<div class="parent" data-floating-menu-container="data-floating-menu-container">
        <StatefulTable></StatefulTable>
</div>

.parent {
    position: relative;
}

You can also refer the aforementioned issue https://github.com/carbon-design-system/carbon/issues/4755.

tw15egan commented 3 years ago

Hey there @ravinain, since this may be an issue with the StatefulTable component, would it make more sense to open an issue against the carbon-addons-iot-react repo instead? If you believe it is an issue with the core Carbon library, could you please recreate a reduced test case in CodeSandbox so we can take a look?

ravinain commented 3 years ago

Hi,

I have created an example here: Overflow-Menu-Issue.

Steps to reproduce:

tw15egan commented 3 years ago

I'm unable to reproduce the issue.

2021-08-02 10 07 56

Going to close this as unable to reproduce, but if you can replicate the issue (without the StatefulTable component since that is an IOT add-on) we'd be more than happy to reopen and take another look.

petersandor commented 1 year ago

@tw15egan this is still a problem, please see my slightly modified fork of the previously shared CodeSandbox: https://codesandbox.io/s/overflow-menu-issue-forked-y6f2d9

Nov-08-2022 12-45-40

tw15egan commented 1 year ago

@petersandor If this is an issue with the StatefulTable component, that would need to be fixed in the Carbon IOT add-ons repo. Are you able to reproduce the issue with the Carbon DataTable? I am still unable to reproduce it on our end.

petersandor commented 1 year ago

@tw15egan it doesn't matter what component the OverflowMenu is in (it can even be a plain div), I can set up a different example.

tw15egan commented 1 year ago

@petersandor, if you can reproduce the issue with just the overflow menu, please create a new issue so we can properly track a fix. This one seems to be tied to an addon library.

petersandor commented 1 year ago

@tw15egan see screen recording from Carbon React Storybook page:

Nov-08-2022 16-49-30

All that is needed is the overflow scroll on the parent container, height limitation and a lot of content before and after the overflow menu.

tw15egan commented 1 year ago

From slack:

May need to make sure the OverflowMenu has the data-floating-menu-container set to the parent selector (https://react.carbondesignsystem.com/?path=/docs/components-overflowmenu--default#data-floating-menu-container), but the storybook doesn’t have it set and I’m unable to see the same bug

2022-11-08 10 56 33

If a simplified reproduction can be made, we can work on a fix, but I am unaware of what is causing the issue.

petersandor commented 1 year ago

@tw15egan here's a new CodeSandbox with everything https://codesandbox.io/s/romantic-cloud-lbdm42?file=/src/index.js

Nov-08-2022 17-42-08

tw15egan commented 1 year ago

If you wrap the menu in a div and set that as the container, it should be contained inside the scrollable container. https://codesandbox.io/s/dreamy-heisenberg-n41ncd?file=/src/index.js

petersandor commented 1 year ago

If you wrap the menu in a div and set that as the container, it should be contained inside the scrollable container. https://codesandbox.io/s/dreamy-heisenberg-n41ncd?file=/src/index.js

Right, so basically the overflow menu cannot have any sibling elements otherwise the positioning breaks.

tw15egan commented 1 year ago

It can have sibling elements; it just needs its position set to a container inside the scrolling container rather than the scrolling container itself.

petersandor commented 1 year ago

It can have sibling elements; it just needs its position set to a container inside the scrolling container rather than the scrolling container itself.

I see, this is an undocumented feature of the OverflowMenu though, I can see it is mentioned in the Tooltip which presumably uses the same underlying FloatingMenu component.

Thanks for help @tw15egan!

tw15egan commented 1 year ago

Sorry for the confusion! If we can improve the documentation regarding this functionality, I'd love to update it to reduce headaches in the future, just let me know what should be copied over from theTooltip documentation 🙂

petersandor commented 1 year ago

Actually if I may ask one more question, I noticed that the class is not really necesary for the data-floating-menu-container, it selects by the attribute only:

https://github.com/carbon-design-system/carbon/blob/19778e9e8cf08dedf8a79bddd29337f0d5f63f88/packages/react/src/components/OverflowMenu/OverflowMenu.js#L455

Which means it selects the overflow menu button itself (I confirmed by removing the attribute's value in your CodeSandbox example).

petersandor commented 1 year ago

@tw15egan isn't that an accessibility violation? The entire overflow menu is inside of the trigger button in your example? Also React complains that the overflow menu item (which is a button) is inside of the trigger button:

Screenshot 2022-11-08 at 19 14 10
tw15egan commented 1 year ago

Updated the example https://codesandbox.io/s/dreamy-heisenberg-n41ncd?file=/src/index.js so that it correctly appends to the div and not the OverflowMenu

https://github.com/carbon-design-system/carbon/issues/4755#issuecomment-673093566

petersandor commented 1 year ago

Thanks again @tw15egan.

I still think that this is not ideal and the menu should handle this without forcing the consumers of the library to set position: relative and data-floating-menu-container attribute on the parent element as that may have other unintended side effects.

Other libraries solve this problem by dynamically recalculating the position of the menu which can also be tricky.