6pac / SlickGrid

A lightning fast JavaScript grid/spreadsheet
https://github.com/6pac/SlickGrid/wiki
MIT License
1.82k stars 422 forks source link

Horizontal scrolling causes cells to render in wrong location #914

Closed timclutton closed 10 months ago

timclutton commented 10 months ago

Horizontal scrolling a particularly wide SlickGrid causes cell content to be mis-rendered.

This can be seen in example 1 by simply dragging the columns wider and then scrolling horizontally:

Example of horizontal scrolling issue (Recorded in Firefox Dev Edition 120.0b9; same occurs in Edge 119.0.2151.58)

This seems to have been introduced in 5.5.1 as I can't reproduce with 5.4.2.

ghiscoding commented 10 months ago

hmm I'm assuming the regression was introduced in PR #908 when @JesperJakobsenCIM looked at replacing the previously dynamically created html string for all the slick rows. The main goal of this new release was to be more CSP compliant and drop as much as possible the use of innerHTML to be CSP safe and dynamically created html by string was one of these non-compliance (plus dynamic html string is also not ideal). A lot of code refactoring had to be done to be CSP compliant.

@JesperJakobsenCIM any chance you can take a look at this issue? Worst case, we might have to rollback the dynamic html string for creating the slick rows, probably this commit 113b6130 and the next ones after from that PR until this issue is all covered. We don't currently have Cypress E2E tests covering the problem shown above, ~not sure if it's even possible with Cypress~ (done)

EDIT

Yup I can confirm that the commit 113b6130 caused the regression and I will push a PR reverting the code until we come up with better approach without causing a regression. Reverting this will partially remove CSP compliance unless the user add the sanitizer with DOMPurify to return TrustedHTML, so I think that's still acceptable if user want to remain CSP compliant

EDIT 2

I pushed a PR to rollback commit 113b6130 and I also added a Cypress E2E test for the Basic Grid Example to cover the issue shown above. With this new Cypress test, it now passes the test with rolled commit but now fails with regressed code, so at least now we also have E2E tests to cover this. Thanks @timclutton for providing a good simple example to test with.

@JesperJakobsenCIM if you have a chance to look at the commit again and somehow find a way to fix this issue, I would be happy to receive another PR but for now I'm just rolling back. I also think that we have to try to keep PRs smaller and more focused on 1 thing at a time instead of multiple since it's a bit hard to troubleshoot regression with large PRs.

chrome_ImUCi7L0sp

ghiscoding commented 10 months ago

fixed and released as 5.5.2, thanks again for the feedback and a simple repro