SAP / ui5-webcomponents

UI5 Web Components - the enterprise-flavored sugar on top of native APIs! Build SAP Fiori user interfaces with the technology of your choice.
https://sap.github.io/ui5-webcomponents/
Apache License 2.0
1.54k stars 265 forks source link

StepInput: error state with valueState=Error is not shown after clicking on "+" button one more time #5130

Closed vlad-ti closed 9 months ago

vlad-ti commented 2 years ago

Describe the bug see title

Isolated Example https://codesandbox.io/s/suspicious-cohen-z6szqc?file=/src/App.js

To Reproduce Steps to reproduce the behavior:

  1. Go to the codesandbox link above
  2. Click on "+" icon of the StepInput 2x times, so that the value will be "3". Notice that right now, the error state is shown (red border around StepInput)
  3. Click one more time on the "+" icon of the StepInput. Notice that the error state has disappeared, although it is still assigned to the StepInput via valueState=Error (see React DevTools)

Expected behavior Whenenver valueState=Error is set, StepInput should show it accordingly.

Screenshots StepInput bug

UI5 Web Components for React Information @ui5/webcomponents version: 0.22.0 @ui5/webcomponents-react version: 0.22.0 Operating System: Windows Browser: Chrome

Additional context I've tried out to use the latest 0.23.0 version in your codesandbox template fork, but it does not work due to compilation errors, something related to menu.js import. Please check it as well.

MarcusNotheis commented 2 years ago

Thanks for reporting! I'll forward this issue to our UI5 Web Components Colleagues as the affected component is developed in their repository.

unazko commented 2 years ago

Hello @vlad-ti,

I couldn't reproduce the issue via the provided snippet. I don't don't have react tools, but at my side when inspecting the DOM - the ui5-step-input and the ui5-input elements have their value-state attribute set to "None" after the last step when the value in the input field is "4". 2022-04-27_17-22-45

Could you please check once more at your side?

Best Regards, Boyan Rakilovski

vlad-ti commented 2 years ago

Hey @unazko Boyan, I confirm, that if I look with Chrome Dev tools into the ui5-step-input with value = 4, the value-state gets set to None. But nevertheless, if you look into the linked codesandbox React code, this should actually not happen, because it sets the value-state = Error to all values > 2. The debugging, as well as the React Dev tools proof that. If the code is incorrect from your perspective, then tell me please, how to use StepInput in React with dynamic value validation, which truly works.

@MarcusNotheis Could the issue lie in the ui5-webcomponents-react project? For instance, one could imagine that the click on "+" icon with value-state being Error could reset the state to None, without notifying react side?

Best regards,

Vlad Tintinger SAP SE - Language Experience Lab

MarcusNotheis commented 2 years ago

I think the root cause is that the Step Input component is changing the value-state attribute by its own: Even if we pass value-state="Error", the component renders with value-state="None".

I've also created a sandbox using plain webcomponents: https://codesandbox.io/s/magical-meitner-8cvjfj?file=/src/App.js

niyap commented 2 years ago

Hello Colleagues,

The issue is reproducible using ui5-step-input component only. Here is an isolated example: https://jsbin.com/gohenaraki/edit?html,console,output

Could you please look over?

Kind Regards, Niya

xpr0gamers commented 2 years ago

I think the root cause is that the Step Input component is changing the value-state attribute by its own: Even if we pass value-state="Error", the component renders with value-state="None".

I've also created a sandbox using plain webcomponents: https://codesandbox.io/s/magical-meitner-8cvjfj?file=/src/App.js

Hello,

Is the implemantation of the releated example correct? The change event will be registered with the additive "{ once: true }". With this additive, the change handler and the code "target.valueState = "Error"" is only called once. Without the additive "{ once: true }", it works without problems and in my opinion correct.

Kind Regards, Maxi

vlad-ti commented 2 years ago

Hi, Any news on this topic? Considering the statement of @xpr0gamers, is the issue related to ui5-webcomponents-react or ui5-webcomponents project? Any forecast on solving of this issue? We in SAP / User Enablement / LX Lab have already received multiple bug reports from our end-users with regards to this problem.

Best regards,

Vlad

NHristov-sap commented 11 months ago

Hello @vlad-ti ,

As I didn't managed to start any of the snippets provided, except one provided by @niyap (https://jsbin.com/gohenaraki/edit?html,console,output), I cannot say what is the reason, but in the @niyap snippet, the change event listener is with parameter once: true, which means it is called only once (at first value change) .

In general, the StepInput has it's own validation mechanism that sets valueState depending on value validation on each change based on its own properties like min/max, etc. So if you want to override the default validation, you must do it on each change, but as I can see in the only snippet I can run, the event listener is called only once, and on second change only the default (built-in) validation happens, respectively the valueState is set to None as the value meets validation conditions (there are no min/max set, so any value is counted as valid).

What is your exact use case?

Best Regards, Nikolay Hristov

vlad-ti commented 11 months ago

Hi @NHristov-sap,

I am not sure whether the description of my real world use case will help to resolve this bug, which I've described in the beginning of this thread using a more simplified use case, which narrows down the issue to its root cause at least on the API level of UI5 Webcomponents for React project. I think, the full resolution of this bug requires more cooperation between you and the developers of the UI5 Webcomponents for React project, because according to your provided information, it seems like there is an integration issue between "UI Webcomponents" and "UI5 Webcomponents for React" projects. Since I am not using UI5 Webcomponents directly without React, I am not the right person to help you with the troubleshooting within the internals, which the React wrapper is using. My aim was to report the bug, which I've run into. The team of the UI5 Webcomponents for React project has forwarded this bug to your project, while thinking that the bug is related to this project. If you have further questions, it might be better to align with the team responsible for UI5 Webcomponents for React project development.

In my real use case, there are two StepInputs with the validation rule, which validates that the values from both StepInputs must not differ by more than 1000. The implementation of this validation is done analogously to the provided code snippet, by setting valueState via conditional logic, which at the end suffers from the same bug as initially described by me in this thread. My team has already found a workaround for this bug. Each time the state of a StepInput gets updated, we assign it a different value for its key attribute, which in turn triggers React to detach the previous StepInput instance from the virtual DOM and add a new StepInput instance to it. It is a hack, but at least it works for us. However, it does not change the fact, that the provided API of UI5 Webcomponents for React is still having a bug related to the valueState attribute handling of StepInput.

Best regards,

Vlad Tintinger SAP User Enablement / LXlab

tsanislavgatev commented 9 months ago

Hello, We've introduced new event called - value-state-change. It is preventable so you can prevent it when no internal validation is needed.