fix: change dynamic html string w/CSP safe code to fix scroll, fix #914

[JesperJakobsenCIM]

This is not ready to get merged, its just to have a place to discuss for now

[JesperJakobsenCIM]

Okay I'm experiencing weird things, if you scroll from left to right fast, the data shows up and then disappears shortly after rendering (seems to be column auto correct) https://streamable.com/2ol752 from 7 seconds and forward

Do you have any ideas where the code for this logic exists? since my guess would be its a change in structure from old to new and there are some code that still expects the old style

Edit: for now it seems like the issue stems from cleanUpAndRenderCells tho will have to check up on how it worked before

[JesperJakobsenCIM]

I have found the issue, ill just do some cleanup then commit it

issue seems to be comming from the new code creating a div inside a div with all the child elements below therefore by just removing the first div it fixes the issue

while (Utils.isDefined(processedRow = processedRows.pop())) {
      cacheEntry = this.rowsCache[processedRow];
      let columnIdx;
      while (Utils.isDefined(columnIdx = cacheEntry.cellRenderQueue.pop())) {
        node = divRow.lastChild as HTMLElement; //issue stems from here

        //no idea why node would be null here but apparently it is..
        if (!node) {
          continue;
        }
        if (this.hasFrozenColumns() && (columnIdx > this._options.frozenColumn!)) {
          cacheEntry.rowNode![1].appendChild(node);
        } else {
          cacheEntry.rowNode![0].appendChild(node);
        }
        cacheEntry.cellNodesByColumnIdx![columnIdx] = node;
      }
    }

[ghiscoding]

oh that's great, never taught of looking at the DOM element it creates, the new Cypress test I added last week for "Example 1 Basic Grid" should cover this use case pretty well now, I made it scroll slowly since it showed more often while performing that action. Out of curiosity, could I see the code you changed since last time? I'd like to see where the div in a div happened.

is it ready to be merged then? I think it looks good for my point of view since all Cypress E2E tests are passing, including the new test I added. Perhaps the only other thing I could would be to test a little more locally.

Also forgot to mention that I had to rename your PR title because your title would be caught by the release process, we rely on Conventional Changelog to automatically process release & changelog, in short it requires a fix:, feat: or perf: to show in changelog and anything else (like chore:) won't show in changelog.

[JesperJakobsenCIM]

This code is ready to get merged

[ghiscoding]

Thanks I tried it locally and see no more issues, good job 👍🏻 I'll go ahead and merge, expect a new release by the weekend

[ghiscoding]

@JesperJakobsenCIM so while I was giving this a try in Slickgrid-Universal (my lib based on slickgrid), I found that I had an issue in my lib after applying your changes, the issue was around the lines shown below. I thought it might interesting for you to know since it's CSP related

https://github.com/6pac/SlickGrid/blob/1a93a64c1021cb74bdeee1859bcbe2440eb26ee2/src/slick.grid.ts#L3934-L3937

mainly the applyHtmlCode is a new method I've added in PR #894 which allow us to code a Formatter with native HTML element or keep using HTML strings and the applyHtmlCode will either .appendChild() when it's a native element or when it's a string it will sanitize it and then use innerHTML.... Anyway, the issue I had was with grid that have Grouping, the row that is a Group has an extra level="1" representing the depth level of the group (0 being at the root level, see image below). So in my case, I use DOMPurify as sanitizer and applyHtmlCode passes HTML string to the sanitizer when it is a string and what took me some time to realize is that DOMPurify strips any attributes that it doesn't know unless we tell them it's allowed, basically I had to change the DOMPurify to use these options DOMPurify.sanitize(val || '', { ADD_ATTR: ['level'], RETURN_TRUSTED_TYPE: true }) and that fixed my issue.

So long story short, the DOMPurify issue brought up another change that should be addressed eventually, I'll probably work at it in the next couple days before the release, there are still many areas of the code that uses Formatters with HTML strings and that could cause other indirect issues like I had, converting the code below to be native HTML element would help with CSP compliance (which is what I will do)... or in other words, there might be other areas of SlickGrid core or plugins that need refactoring to be fully CSP compliant and there are still areas of the code that still require innerHTML, if you find others then please open a PR.

https://github.com/6pac/SlickGrid/blob/1a93a64c1021cb74bdeee1859bcbe2440eb26ee2/src/slick.groupitemmetadataprovider.ts#L62-L78

[ghiscoding]

@JesperJakobsenCIM as per what I wrote in previous paragraph, I created PR #925 to convert Group Formatter to native HTML elements, I also used DocumentFragment which I didn't know that I could use to basically create an empty shell since the previous html string had 2 span inside and I didn't want to create an empty div container, I saw fragment in the past but didn't know I could use it this way, anyway it's a fun fact :) ... you could comment on PR #925 if you want, I'll probably merge and release later tonight since I have other fixes that my team is waiting for and I prefer to do it through an official release.