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

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

Closed
AndrianStoikov opened this issue Feb 22, 2023 · 22 comments · Fixed by #6888
Closed

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

AndrianStoikov opened this issue Feb 22, 2023 · 22 comments · Fixed by #6888
Assignees
Labels

Comments

@AndrianStoikov
Copy link

AndrianStoikov commented Feb 22, 2023

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.

@AndrianStoikov AndrianStoikov added the bug This issue is a bug in the code label Feb 22, 2023
@Lukas742
Copy link
Collaborator

Lukas742 commented Feb 23, 2023

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

@Lukas742 Lukas742 transferred this issue from SAP/ui5-webcomponents-react Feb 23, 2023
@AndrianStoikov
Copy link
Author

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
Copy link
Collaborator

@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
Copy link
Author

AndrianStoikov commented Feb 27, 2023

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

@niyap
Copy link
Contributor

niyap commented Feb 28, 2023

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

@MapTo0 MapTo0 removed bug This issue is a bug in the code Medium Prio labels Feb 28, 2023
@AndrianStoikov
Copy link
Author

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
Copy link
Collaborator

Lukas742 commented Feb 28, 2023

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 NHristov-sap added bug This issue is a bug in the code consulting Medium Prio TOPIC RL labels Mar 1, 2023
@NHristov-sap
Copy link
Contributor

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
Copy link
Contributor

hristop commented Mar 1, 2023

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

@hristop hristop removed the TOPIC RL label Mar 1, 2023
@AndrianStoikov
Copy link
Author

AndrianStoikov commented Mar 1, 2023

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 (#6591). I don't understand what is the problem at the moment ...

@Lukas742
Copy link
Collaborator

Lukas742 commented Mar 1, 2023

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
Copy link
Collaborator

Lukas742 commented Mar 6, 2023

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
Copy link
Member

ilhan007 commented Mar 8, 2023

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.

@ilhan007 ilhan007 self-assigned this Mar 8, 2023
@ilhan007
Copy link
Member

ilhan007 commented Mar 9, 2023

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.


  • This is how it breaks
breaks.mov
  • This is how it works when removing the overflow: hidden from `ui5-webcomponents/packages/main/dist/generated/themes/TableCell.css.js
works_without_overflow_hidden.mov

BR,
ilhan

@ilhan007 ilhan007 removed their assignment Mar 9, 2023
@AndrianStoikov
Copy link
Author

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
Copy link

Neeeko commented Mar 21, 2023

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

@ilhan007
Copy link
Member

ilhan007 commented Apr 5, 2023

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 pushed a commit that referenced this issue Apr 10, 2023
The combination of display: table-cell and overflow: hidden styles
 was causing the browser to crash upon component re-rendering

Fixes: #6570
@ndeshev
Copy link
Contributor

ndeshev commented Apr 10, 2023

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

ndeshev added a commit that referenced this issue Apr 10, 2023
* fix(ui5-table): prevent Safari from crashing

The combination of display: table-cell and overflow: hidden styles
 was causing the browser to crash upon component re-rendering

Fixes: #6570

* fix(ui5-table): prevent Safari from crashing

* fix(ui5-table): prevent Safari from crashing

Remove overflow: hidden style from the ui5-table-cell for all browsers

---------

Co-authored-by: Deshev <I521896@Deshevs-MacBook-Pro.local>
@ilhan007
Copy link
Member

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

ilhan007 pushed a commit that referenced this issue Apr 13, 2023
* fix(ui5-table): prevent Safari from crashing

The combination of display: table-cell and overflow: hidden styles
 was causing the browser to crash upon component re-rendering

Fixes: #6570

* fix(ui5-table): prevent Safari from crashing

* fix(ui5-table): prevent Safari from crashing

Remove overflow: hidden style from the ui5-table-cell for all browsers

---------

Co-authored-by: Deshev <I521896@Deshevs-MacBook-Pro.local>
ilhan007 pushed a commit that referenced this issue Apr 13, 2023
* fix(ui5-table): prevent Safari from crashing

The combination of display: table-cell and overflow: hidden styles
 was causing the browser to crash upon component re-rendering

Fixes: #6570

* fix(ui5-table): prevent Safari from crashing

* fix(ui5-table): prevent Safari from crashing

Remove overflow: hidden style from the ui5-table-cell for all browsers

---------

Co-authored-by: Deshev <I521896@Deshevs-MacBook-Pro.local>
NHristov-sap pushed a commit that referenced this issue May 4, 2023
* fix(ui5-table): prevent Safari from crashing

The combination of display: table-cell and overflow: hidden styles
 was causing the browser to crash upon component re-rendering

Fixes: #6570

* fix(ui5-table): prevent Safari from crashing

* fix(ui5-table): prevent Safari from crashing

Remove overflow: hidden style from the ui5-table-cell for all browsers

---------

Co-authored-by: Deshev <I521896@Deshevs-MacBook-Pro.local>
PetyaMarkovaBogdanova pushed a commit that referenced this issue Jun 20, 2023
* fix(ui5-table): prevent Safari from crashing

The combination of display: table-cell and overflow: hidden styles
 was causing the browser to crash upon component re-rendering

Fixes: #6570

* fix(ui5-table): prevent Safari from crashing

* fix(ui5-table): prevent Safari from crashing

Remove overflow: hidden style from the ui5-table-cell for all browsers

---------

Co-authored-by: Deshev <I521896@Deshevs-MacBook-Pro.local>
@hristop hristop added the Table label Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

9 participants