ui5-webcomponents icon indicating copy to clipboard operation
ui5-webcomponents copied to clipboard

[Toolbar(V2)] ToolbarButton with Modals Menu rendered in the wrong place

Open wvudako opened this issue 1 year ago • 3 comments

Describe the bug

Adding a Menu to a ToolbarButton using state or the Modals API (https://sap.github.io/ui5-webcomponents-react/v2/?path=/docs/user-feedback-modals--docs) doesnt render the menu in the right place.

The above code sample is basically just copied from the doc

import {
  MenuItem,
  Modals,
  Toolbar,
  ToolbarButton,
} from '@ui5/webcomponents-react';
...
<Toolbar>
  <ToolbarButton
    id="modals-show-menu"
    text={'Show menu'}
    onClick={() => {
      Modals.showMenu({
        opener: 'modals-show-menu',
        headerText: 'Menu Title',
        children: (
          <>
            <MenuItem text={'Click me'} onClick={() => {}} />
          </>
        ),
      });
    }}
  />
</Toolbar>

There are several things which might be unrelated that I have also noticed: The behaviour doesnt change where we place the <Modals /> element Toasts are showing a similar behaviour in some cases, dependent on where in the tree the <Modals /> element is placed The menu is displayed properly using @ui5/webcomponents-react-compat Toolbar (Legacy) with a standard button We are having similar issues in the latest 1.x versions of webcomponents(-react)

Isolated Example

https://stackblitz.com/edit/github-f9pvpx

Reproduction steps

No response

Expected Behaviour

The Menu is displayed attached to the ToolbarButton

Screenshots or Videos

You can see the menu attached at the wrong place in the bottom left

image

UI5 Web Components for React Version

2.2.0

UI5 Web Components Version

2.3.0

Browser

Chrome

Operating System

Windows 11

Additional Context

No response

Relevant log output

No response

Organization

No response

Declaration

  • [X] I’m not disclosing any internal or sensitive information.

wvudako avatar Oct 08 '24 11:10 wvudako

Hi @gitgdako

you're using the same id for multiple elements, this is the reason why it's not working for the button outside of the toolbar opening a Menu.

The ToolbarButton itself is an abstract (docs) ui5 web component and therefore the id is not forwarded to the actual element inside the shadow root. Usually you should be able using the getDomRef method of the ui5-toolbar-button to get the reference for the actual element, but it doesn't seem to work for the ToolbarButton. I've created an issue for this: https://github.com/SAP/ui5-webcomponents/issues/9996

Because of this, it's pretty complicated getting a reference to the actual button where the popover should be opened:

https://stackblitz.com/edit/github-f9pvpx-m5rxp2?file=src%2FApp.tsx,package.json

Since the Toolbar and all of its subcomponents are developed by the ui5-webcomponents team, I'll forward this issue to their repo.

The menu is displayed properly using @ui5/webcomponents-react-compat Toolbar (Legacy) with a standard button We are having similar issues in the latest 1.x versions of webcomponents(-react)

Please create a dedicated issue for this, since the Toolbar v1 is a completly different component, maintained in the ui5-webcomponents-react repo.


Hi Colleagues,

could you please check what the intended way of opening a popover via a ui5-toolbar-button should be? Currently it's only possible by entering the shadowRoot and referencing the respective internal button as opener. in the example above the click handler should mainly be the same as with plain js. If you still need an example without our wrapper or React please let me know.

For the ShellBarItem there was a targetRef event detail introduced for the same reason, maybe this is something to consider here as well.

type ShellBarItemClickEventDetail = {
	targetRef: HTMLElement,
};

Lukas742 avatar Oct 09 '24 09:10 Lukas742

Sorry for the faulty Code sample. Thanks for forwarding to the relevant people. As for the mention of 1.x: I meant the ToolbarV2 in 1.x, which I assume faces the same issue. For us it does not need to be fixed there as we will move to 2.x

wvudako avatar Oct 10 '24 06:10 wvudako

Hello @ui5-webcomponents-topic-p,

It seems the menu is not opened because of undefined getDomRef() returned by ui5-toolbar-button, alse see the related issue #9996

Best Regards, Nikolay Hristov

NHristov-sap avatar Oct 11 '24 09:10 NHristov-sap

Hi @kgogov @Lukas742, when will this be released? I assumed it would be in 2.4 and tested it and have some new findings that can be seen in the adapted Sample https://stackblitz.com/edit/github-f9pvpx The menu is now rendered in the top left. Am I using the opener reference wrong? The compat toolbar now does the same intially, but the menu then jumps to the opener

wvudako avatar Nov 05 '24 15:11 wvudako

Hi @gitgdako

you can't set an id there - please see my answer above about abstract elements. Also, the StackBlitz example isn't updated to 2.4, so the fix isn't available there. I've updated your example and used the internal ui5-button element (via getDomRef) as opener and now it's working as intended.

https://stackblitz.com/edit/github-f9pvpx-ynsqxn?file=src%2FApp.tsx,package.json


Hi @kgogov

I’d argue that the issue isn’t resolved by the linked PR. The PR only addresses the problem with getDomRef (corresponding issue), it doesn’t provide a way to obtain the correct element reference via the click event (similar to the click event of the ShellBar) or outlines the recommended way to open a popover in the docs. This could lead to confusion for other developers, as I think that getDomRef isn’t widely recognized, especially for abstract components.

Lukas742 avatar Nov 06 '24 13:11 Lukas742

The documentation of the modals https://sap.github.io/ui5-webcomponents-react/v2/?path=/docs/user-feedback-modals--docs doesnt really make it clear that in this specific case the usage is different (and also not at all intuitive). This need to be addressed by updating the doc accordingly or having a consistent usage (which would obviosuly be preferrable)

wvudako avatar Nov 11 '24 09:11 wvudako

Hi @gitgdako this behavior occurs with or without using our Modals API. Abstract components are a general concept, which is why we can't outline every situation where they might be relevant. We describe this particularity in our FAQ. If you feel like something is missing there, please let us know.

Lukas742 avatar Nov 11 '24 10:11 Lukas742

This might be just my gut feeling, but using a Menu on a button (which is "standard" functionality covered in your storybook) within a toolbar isnt such an edge case. I dont think without raising an issue or stumbling across this one I would never be able to get this to work. The alternative solution would be a component explicitly meant for usage in a Toolbar [0]

[0] https://github.com/SAP/ui5-webcomponents/issues/9851

wvudako avatar Nov 11 '24 10:11 wvudako

@Lukas742 There is an other issue with the given solution: When clicking the button the menu pops up in the top left corner before it snaps to the opener. So it is set to open in its placeholder state before being moved. This isnt a major issue but not really beautiful Reproduce: Your updated stackblitz

  • https://stackblitz.com/edit/github-f9pvpx-ynsqxn?file=src%2FApp.tsx,package.json
  • Press the button to see menu pop up in the top left corner (can be done multiple times)

wvudako avatar Nov 12 '24 11:11 wvudako

Hi @kgogov

the behavior described by @gitgdako occurs when the Menu is conditionally rendered. You can find an example using only ui5wc without our wrapper or React here: https://stackblitz.com/edit/github-8upxfb?file=index.html,main.js

Should this be discussed in this GitHub issue, or would it be better to create a new one?

Lukas742 avatar Nov 12 '24 13:11 Lukas742

Hi @Lukas742,

Thank you for providing the example and for outlining the behavior when the Menu is conditionally rendered. To keep things organized and clear, I suggest creating a new GitHub issue to discuss this specific behavior separately.

kgogov avatar Nov 12 '24 14:11 kgogov

As I see it, there are 3 issues at hand here:

  1. the original issue of the component not working at all, which is "fixed" imo, although not perfectly
  2. documentation and reproducability: As stated above this isnt intuitive at all and can hardly be recreated by a dev. Either document it well in your storybook or create a different way of using a menu with a toolbar button)
  3. the menu poping up in the corner: that can be moved to a separate issue. Please do create it yourself because you can probably attach more relevant information

wvudako avatar Nov 12 '24 15:11 wvudako

Hello! I’d like to share a quick update on the progress: We’ve been working on enabling popovers to open through abstract elements, such as ui5-toolbar-button. You can find the details here: https://github.com/SAP/ui5-webcomponents/pull/10140

kgogov avatar Nov 15 '24 12:11 kgogov

Hello,

Based on the latest changes, I believe we can close this ticket because:

  • With the recent update, it is now possible to use abstract components, such as ui5-toolbar-button, as an opener.
  • This change also allows calling getDomRef() on specific abstract elements within the Toolbar.
  • Lastly, we added the targetRef property to the event.details object, which can now be used to open a Popover.

If there are still issues related to the Toolbar component, please let us know.

Thank you to @Lukas742 for the detailed analysis and to @wvudako for reporting the issue.

kgogov avatar Nov 28 '24 14:11 kgogov