Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OP / Toolbar(Button) V2]: Hydration of Header Toolbar Actions #10415

Closed
1 task done
wvudako opened this issue Nov 27, 2024 · 6 comments · Fixed by #10546
Closed
1 task done

[OP / Toolbar(Button) V2]: Hydration of Header Toolbar Actions #10415

wvudako opened this issue Nov 27, 2024 · 6 comments · Fixed by #10546
Assignees
Labels
bug This issue is a bug in the code Medium Prio released TOPIC P

Comments

@wvudako
Copy link

wvudako commented Nov 27, 2024

Describe the bug

Like in the code samples in
https://sap.github.io/ui5-webcomponents-react/v2/?path=/docs/layouts-floorplans-objectpage--docs

we are using a Toolbar V2 to display Actions in the OP Header.

[All Imports] from '@ui5/webcomponents-react';

<ObjectPage...
  titleArea={
    <ObjectPageTitle...
      actionsBar={
        <Toolbar...
          <ToolbarButton

Theoretically by a user, but more consistently by automated tests, it is possible to click the button before the button handler is attached.

Our solution was to retry clicking the button until it actually does something
Our preferred solution would be that the buttons are disabled until the handler is attached

https://www.youtube.com/watch?v=8g7FvoRToGo perfectly describes the status quo
We are using Playwright with Chromium, which clicks the button once its visible and enabled
Because of the flakiness of the issue we are unsure which other components are affected by this, but we believe to have had the same issue using the compat Toolbar with standard Buttons, starting with the upgrade to webcomponents 2.x

Playwright 1.47.2
Webcomponents(-react) 2.4.0, but believed to be since 2.0

Isolated Example

No response

Reproduction steps

Using the codesample, click the button immediately after its visible and enabled

Expected Behaviour

Button is not clickable until click handler is attached

Screenshots or Videos

No response

UI5 Web Components for React Version

2.4.0

UI5 Web Components Version

2.4.0

Browser

Chrome

Operating System

Windows 11

Additional Context

No response

Relevant log output

No response

Organization

No response

Declaration

  • I’m not disclosing any internal or sensitive information.
@Lukas742
Copy link
Collaborator

Lukas742 commented Dec 18, 2024

Hi @wvudako,
using waitForDefine could help improve this behavior, as it ensures the web component is only mounted once the customElement is defined. Please note that this prop is only added by our wrapper and is not available on the web component itself.
However, since UI5 web components themselves are often asynchronous, there’s still a chance that some events, attributes or properties might not be fully initialized.

That said, we’ve never received complaints about this behavior with real user interactions.

Nevertheless, I’ll forward this issue to the ui5-webcomponents team, as all UI5 web components are developed in their repository.


Hi Colleagues,
could you please take over?

@Lukas742 Lukas742 transferred this issue from SAP/ui5-webcomponents-react Dec 18, 2024
@stephania87 stephania87 self-assigned this Dec 19, 2024
@ivoplashkov
Copy link
Member

Hello colleagues,

The reporter says that after updating to version 2.0, they encounter an issue where buttons in the Toolbar component can not be clicked before their event handlers are attached.

The proposal is that buttons should remain disabled until their handlers are fully initialized to address this.

Could you please have a look and provide insights on whether this behavior is expected or if adjustments are needed at the framework level?

Thank you in advance!
Best regards,
Ivaylo

@pskelin
Copy link
Contributor

pskelin commented Dec 23, 2024

@wvudako can you share your test code please? How are you querying for the button? the ui5-toolbar-button is an abstract element, you can't really click it. We recently switched to cypress, and there we will provide helpers how to trigger actions on abstract elements. You have to find the actual ui5-button that is rendered by the ui5-toolbar-button and click it.

Now the second part - when you say the event handlers are not attached - I understand it as the application event handlers. The component has no way of knowing what handlers are attached, or when they are coming. The way the video explains it, the application has to set them as disabled.

In the end, I would like to understand why exactly this happens. The test is probably clicking something that a real user cannot click and this is what we need to address.

@pskelin pskelin self-assigned this Dec 23, 2024
@wvudako
Copy link
Author

wvudako commented Dec 23, 2024

App setup
import {Toolbar, ToolbarButton} from '@ui5/webcomponents-react';
We set the event handler via ToolbarButton#onClick

Playwright setup

this.page.locator('[data-component-name="ObjectPageTitleMiddleSection"]').getByRole('button', {
  name,
  exact: true,
});

in an async function which we then await in that specific test.
The element that is being targeted:

Image

To test we have added a console.log to the handler and sometimes although - according to playwright - the button is correctly pressed, the click handler is not executed.

PS: A colleague of mine can probably add some more details once he is back from vacation if this information is not sufficient

@pskelin
Copy link
Contributor

pskelin commented Jan 7, 2025

@wvudako I managed to reproduce this locally and it is a problem with the toolbar code that has to be fixed.

For your test, please add a delay before the click action, until a fix is ready. Anything above 200 ms should be ok, 300 to be sure.

  await page.waitForTimeout(300);

@SAP/ui5-webcomponents-topic-p

  1. The toolbar attaches the handlers in onBeforeRendering. This means the first time the Toolbar code runs, there is nothing to attach.

  2. After the first rendering, there is an invalidation and the Toolbar runs onBeforeRendering again, this time attaching the handlers in the DOM.

The problem is that tests wait for something to appear in the DOM, which happens between step 1 and step 2, then the test executes a click but the handlers are not attached.

Here is a test that demonstrates this, simply put it in Toolbar.cy.ts

it("Should call click handler on abstract item", () => {
	cy.mount(html`
		<ui5-toolbar>
			<ui5-toolbar-button text="Button 1"></ui5-toolbar-button>
		</ui5-toolbar>
	`);

	cy.get("ui5-toolbar-button[text='Button 1']")
		.then(button => {
			console.log("button1 click handler added")
			button.get(0).addEventListener("click", cy.stub().as("clicked"));
		});

	// test passes when the following line is commented out
	// cy.wait(300);
	cy.get("ui5-button", { includeShadowDom: true }).contains("Button 1")
		.click();

	cy.get("@clicked")
		.should("have.been.calledOnce");
});

If you add a console.log in the Toolbar.ts, you can observe this behaviour together with the test code:

// Toolbar.ts
	attachListeners() {
		const popover = this.getOverflowPopover();

		this.subscribedEvents.forEach((e: string) => {
+			// log the shadowRoot items
+			console.log(this.itemsDOM);
			this.itemsDOM?.addEventListener(e, this._onInteract);
			popover?.addEventListener(e, this._onInteract);
		});
	}

Output in the test console showing the test click handler is added before the toolbar attaches the actual handlers in the DOM

null                               VM19003 Toolbar.ts:328 
button1 click handler added        Toolbar.cy.ts:20 
<div class=​"ui5-tb-items">​…​</div>​  VM19003 Toolbar.ts:328 

Ideally, attaching the events should happen in the template of the Button/Select, or in its own onAfterRendering. onBeforeRendering is too flaky - it might not even be called a second time, and there is a big delay beween renderings (200 ms)

@ui5-webcomponents-bot
Copy link
Collaborator

🎉 This issue has been resolved in version v2.7.0-rc.1 🎉

The release is available on v2.7.0-rc.1

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug in the code Medium Prio released TOPIC P
Projects
Status: Completed
Status: Completed
Development

Successfully merging a pull request may close this issue.

8 participants