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.5k stars 258 forks source link

Table: Filtering items in table crashes app on Safari #6570

Closed AndrianStoikov closed 1 year ago

AndrianStoikov commented 1 year ago

Describe the bug When the items of the Table component change, the whole application crashes on Safari.

Isolated Example sandbox

To Reproduce Steps to reproduce the behavior:

  1. Go to the sandbox
  2. Click on the input
  3. Type something
  4. The page crashes

Expected behavior The Table component should be able to handle element filtering

Screenshots If applicable, add screenshots to help explain your problem.

UI5 Web Components for React Information @ui5/webcomponents version: latest (1.10.3) @ui5/webcomponents-react version: 1.8.1 Operating System: Mac OS Ventura 13.2.1 Browser: Safari Version 16.3 (18614.4.6.1.6)

Additional context When the keys in React are not unique the page works correctly. Unfortunately, once the keys are switched to unique(as it should be) the whole application crashes. The bug happens both on iPhone and Mac.

Lukas742 commented 1 year ago

Hi @AndrianStoikov

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

It seems not related to the issue, but is something I think is worth pointing out. You're recreating the toRender const on every rerender which can have an impact on performance. Here you can find a codeSandbox on how to only update your table rows when necessary: https://codesandbox.io/s/flamboyant-dubinsky-eiq1r6?file=/src/App.js:1086-1241


Hi colleagues,

I'm not sure what the root cause of the issue here is (Safari bug, or bug within the component), as I wasn't able to debug it since the page just refreshes with the following message:

This webpage was reloaded because a problem occurred.

I also noticed that for example typing "SAP" inside the input is working fine, but when I type "SAP C" it always crashes. I tried it in codeSandbox (with and w/o iframe) and also on my locale machine, but the error persisted in all envs.

Here you can find an example using ui5-webcomponents with react, but without our wrapper: https://codesandbox.io/s/frosty-snowflake-07yfln?file=/src/App.js

AndrianStoikov commented 1 year ago

Hi @Lukas742,

Thanks for pointing out the optimization. Also to add to the issue. We also couldn't debug what happens and it was very hard to understand that the table causes the page crash. It's very interesting because the logic is quite simple and it works perfectly on Chrome.

Lukas742 commented 1 year ago

@AndrianStoikov a colleague just pointed out to me that it's possible to find the crash report in the Console app on MacOS, but the error is very cryptic:

Excerpt of the error:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000018
Exception Codes:       0x0000000000000001, 0x0000000000000018

Termination Reason:    Namespace SIGNAL, Code 11 Segmentation fault: 11
Terminating Process:   exc handler [11671]

VM Region Info: 0x18 is not in any region.  Bytes before following region: 4311613416
      REGION TYPE                    START - END         [ VSIZE] PRT/MAX SHRMOD  REGION DETAIL
      UNUSED SPACE AT START
--->  
      __TEXT                      100fe0000-100fe4000    [   16K] r-x/r-x SM=COW  ...it.WebContent
AndrianStoikov commented 1 year ago

Hi @ilhan007, Can someone from the team assist us with this bug? Currently, all our safari users are seriously affected.

niyap commented 1 year ago

Hello Colleagues,

Please, test whether the issue is reproducible from your side and do a pre-analysis before forwarding the issue to our component as there is nothing described and following the steps to reproduce in the description of the issue, I am not able to reproduce the issue. In Safari, the isolated sample works in the same way as in Chrome from my side. Safari version: Version 16.3 (18614.4.6.1.6) MacOS version: Ventura 13.2.1

Kind Regards, Niya

AndrianStoikov commented 1 year ago

Hello @niyap,

The example that I provided is tested and it definitely does not work on Safari. I just tested in on my phone and when I type K in the search box it does break. Multiple colleagues tested it on their Macbooks and it breaks on them. Can you please try again?

Lukas742 commented 1 year ago

Hi @niyap

there is clearly an issue, either with the web component or with Safari. I’ve added a codeSandbox that is using only ui5 web components and not our wrapper. We always preprocess the issues and try to help the user when we have the necessary knowledge about the component even if it’s not related to wcr.

I also tested it in the codeSandbox and in my local environment, posted the crash report and pointed out that it’s not always crashing for me just with specific strings. So I don’t know what else there is to do on preprocessing.

NHristov-sap commented 1 year ago

Hi @niyap,

I have checked it and it is reproducible. Steps to reproduce:

  1. Go to https://codesandbox.io/s/frosty-snowflake-07yfln?file=/src/App.js (Safari browser)
  2. Focus the input
  3. ... 3A. Type "S" inside -> works ... (then delete "S") 3B. Type "C" inside (on empty Input) -> the page is reloaded/crashed

I have checked it with a colleague, he reproduced it too

Best Regards, Nikolay Hristov

hristop commented 1 year ago

Hi @Lukas742 , @AndrianStoikov ,

What is missing for me is a code snippet with only web components and no react. Can you please provide such where the issue can be reproduced only with web-components and no react at all?

Best Regards, Hristo

AndrianStoikov commented 1 year ago

Hello @hristop,

We have always provided examples with react because that is the technology we use. I don't know how to create the sandbox only with html and javascript and it's not even close to our productive setup. @Lukas742 has already provided a sandbox without the react wrapper so the problem is clearly not on the react side. Also as it can be clearly seen the sandbox in this issue is as well on react (https://github.com/SAP/ui5-webcomponents/issues/6591). I don't understand what is the problem at the moment ...

Lukas742 commented 1 year ago

Hi @hristop

since ui5-webcomponents can be used with React (outlined in your docs), it should work with it as well in my opinion. (With React but without the React wrapper).

Since I’m currently out sick, I won’t be able creating another example until I’m feeling better (I’m only on my phone right now).

Lukas742 commented 1 year ago

I wasn't able to reproduce the issue with plain js, but since it is still crashing the page in React, could you please check if "Medium" is really the right priority for this issue? Thanks.

https://codesandbox.io/s/ui5-webcomponents-forked-ftsrul?file=/src/index.js

ilhan007 commented 1 year ago

Hello @Lukas742 @AndrianStoikov

I used the codesandbox with React + pure ui5 web components, provided earlier, thank you for setting it up. https://codesandbox.io/s/frosty-snowflake-07yfln?file=/src/App.js

As Lucas and some of the dispatchers, I also started reproducing the issue every time by typing "C" in the input

I could track down the issue to the usage of key={el.id} on the row:

  <ui5-table-row key={el.id}>

When I remove the key={el.id}, I could not break the page any more.

  <ui5-table-row>

Here is a codesandbox, forked by the previous one with the key removed https://codesandbox.io/s/modern-frog-g3blti?file=/src/App.js

Maybe you can also try it out, remove the key attribute ad observe if something changes, then let me know if I am going into the right direction.

I know setting a key is recommended in React, so we will still have to continue debugging. It's just a small update that may help the team and in case you are able to think of something meanwhile.

Lukas742 commented 1 year ago

I think without the key it won't break as it doesn't force the component to rerender. Every time the key of a given item in the list changes, React knows that something has changed and that it has to update the component. So the issue here has probably something to do with the update cycle of React in combination with how the Table handles updates.

I just tested rendering only the TableRows (without the Table) and then it works fine: https://codesandbox.io/s/keen-paper-eku3n2?file=/src/App.js

ilhan007 commented 1 year ago

Yep, it seems so. We have to dig deeper.

I also checked that the List is working in the same setup + key set (although for the Table we have cell in a row in a table, while here just item in a list)

<List>
        {toRender.map((el) => (
          <StandardListItem key={el.id}>
            {el.id} {el.name}
          </StandardListItem>
        ))}
</List>
Lukas742 commented 1 year ago

I tried two more things: My first try was inlining the filter function, so that we can omit the useEffect. Unfortunately, the result is still the same: https://codesandbox.io/s/great-hill-2et7te

The same applies when adding the event listener without React and removing the value prop from the input: https://codesandbox.io/s/fragrant-dust-jv65fh?file=/src/App.js

ilhan007 commented 1 year ago

Hello colleagues @SAP/ui5-webcomponents-topic-rl We continued working on the issue and have some findings

We created very small, almost blank project, with React + pure ui5 web components (ui5-table, ui5-table-column, ui5-table-row and ui5-table-cell). I hope you can access this link to the project and download it if necessary.

As in all provided samples so far, we filter a table upon typing:

Screenshot 2023-03-09 at 14 26 19

And, in all cases the filtering works as long as the first item is not filtered out before the second, when that happens the Safari browser reloads as described in the issue (and, it breaks when we type "b", because this is the only way to get the first item filtered before the second one). In this scenario React seems to perform much more task of removing, adding DOM elements to the slots.

At first, we stopped injecting any styles to the TableCell|TableRow| shadow Dom as we thought for Safari we use <style> tags (not addoptedStylesheets as in Chrome, because it's still not supported on Safari) and that might causing this weird behaviour. And the issue was gone. The table looked ugly, but the filtering worked properly. Then, we started returning the styles, and removing small parts until we tracked down to "overflow: hidden" of the TableCell styles. Without "overflow: hidden", everything works properly on Safari, with "overflow: hidden" the issue is reproducible.


How we tested it In the simple app I linked the ui5 web components project (the "main" package) and started playing with the styles of the TableCell. They can be found in ui5-webcomponents/packages/main/dist/generated/themes/TableCell.css.js. As I mentioned, we started removing styles until I tracked the root cause down to the "overflow: hidden".

I don't have explanation why this happens. It should be combination of the React + Safari factor. Nevertheless, by design UI5 Web Components (the example is with pure web components) should work in React and I think we need to work on a solution. It seems we need to find workaround for that style on Safari. I will be glad to help you with whatever needed in case you have questions. I hope this helps you continue with the issue.


https://user-images.githubusercontent.com/15702139/224026515-4eb52e85-d8f7-4999-a923-07a8e6a2cb2c.mov

https://user-images.githubusercontent.com/15702139/224025849-c138fb52-d6dc-42ce-8ba3-dcc53584d8e0.mov

BR, ilhan

AndrianStoikov commented 1 year ago

Hello Team,

Can we have an update on this issue? Our product is pretty much unusable at the moment in Safari. Can we please increase the priority of this issue? Also, another option is opening an issue in Safari, if it is possible.

Best, Andrian

Neeeko commented 1 year ago

Hi, any update on this issue? We are also encountering a similar issue in a SuccessFactors project. Thanks.

ilhan007 commented 1 year ago

Hello @SAP/ui5-webcomponents-topic-rl colleagues - changed the priority to High. The Table is not really usable on Safari. You can refer to the findings so far, especially one of the last comments to get you going.

ndeshev commented 1 year ago

I tried to find information for the issue on the web and couldn't, but there are similar issues that have been reported for Safari crashing the same way with other styles/combination of styles in the past.

I've test the removing of the style on the provided sample app and it seems to work fine, also it doesn't seem to break the visual appearance of the component.

I created a pull-request that doesn't add this style in case the detected browser is Safari

Updated: We removed the style for all browsers to avoid writing device specific code and because it has unclear purpose

ilhan007 commented 1 year ago

Hello @AndrianStoikov @Lukas742 the issue has been fixed and released with the latest 1.13.0-rc.1 https://github.com/SAP/ui5-webcomponents/releases/tag/v1.13.0-rc.1

@Lukas742 probably we need to align how to make it available via the ui5-webcompoents-react package