fix: add CSP safe option for DataView filtering and adjusting inline css for CSP #908
Conversation
JesperJakobsenCIM
changed the title
JesperJakobsenCIM
changed the title
src/slick.grid.ts
Outdated
| this.applyTopStyling(elements2); | ||
| console.timeEnd("applying css"); | ||
|
|
||
| if (this._options.nonce) { |
There was a problem hiding this comment.
same as before I don't want to keep 2 implementations to support unless there's a big perf hit which I don't think there is from previous discussions. Have you tried the autoHeight grid yet?
There was a problem hiding this comment.
Interesting tried to add autoHeight: true with 50.000 rows
tho with nonce seems to be faster than the non nonce version, around 1/3 faster sometimes 1/2 faster
ex a search without nonce took 400ms where with nonce it took 200ms
Initial load took 1500 ms without nonce and 1000 ms with nonce (first execution)
second run took 1300 ms without nonce down to 900 with nonce (second execution)
not exactly sure why on initial load it gets called twice tho this might be a concern (not sure current prod does this, havent checked)
Edit:
Hmm interesting the dataView code even on prod requests renderRows twice.
There was a problem hiding this comment.
yeah I can see that the double doesn't make much difference 1 vs 2ms isn't a big deal I guess, but when you add them up in the autoHeight, 400ms is definitely noticeable. I'm not sure what to do at this point but I feel the worst case should always be considered before making a decision and autoHeight is definitely the worst case usage I assume. If we can find a way to remove that double call of renderRows then perhaps it might be more acceptable to go with the nonce (however that won't help the autoHeight example since it doesn't the DataView in that demo). I'm not sure what to go with at this point... I feel we might have to keep both approach but I'm not too keen about it
There was a problem hiding this comment.
I think you might have misunderstood my text, new code is faster than the old version
first set of times is without nonce so old code and second time is with nonce so new code
There was a problem hiding this comment.
oh I got them reversed indeed, if that's the case then yeah let's keep only new code (with nonce) since that will help with CSP compliance. We can also merge my other PR #894 and I think we'll be close enough to be CSP compliant. So I assume you will push the new code change, it's not in the PR yet right?
Hmm interesting the dataView code even on prod requests renderRows twice.
I'm still curious to see if you can find why it does that, but I think that could, and probably should, be a separate PR
There was a problem hiding this comment.
I've commited the cleanup now and everything as far as i know should be in this PR
src/slick.grid.ts
Outdated
| @@ -4526,7 +4530,7 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e | |||
|
|
|||
| protected cleanUpAndRenderCells(range: CellViewportRange) { | |||
| let cacheEntry; | |||
| const stringArray: string[] = []; | |||
| const stringArray: HTMLElement = document.createElement('div'); | |||
There was a problem hiding this comment.
same comment, please rename the stringArray
src/slick.grid.ts
Outdated
| const stringArrayL: string[] = []; | ||
| const stringArrayR: string[] = []; | ||
| const stringArrayL: HTMLElement[] = []; | ||
| const stringArrayR: HTMLElement[] = []; |
There was a problem hiding this comment.
also a rename since it's not string array anymore
src/slick.grid.ts
Outdated
| x.innerHTML = this.sanitizeHtmlString(stringArrayL.join('')); | ||
| xRight.innerHTML = this.sanitizeHtmlString(stringArrayR.join('')); | ||
|
|
||
| stringArrayL.forEach(elm => { |
There was a problem hiding this comment.
same rename for string array
|
@JesperJakobsenCIM if you can just fix the rename of I would also suggest you to review my other PR #894 since I plan to merge it after yours and if you have any feedback that would be great. Thanks |
|
I guess we could rename appendCellHtml to appendCell same with appendRowHtml -> appendRow, tho i dont know if this is important |
I think those are not that bad, we are appending HTML Element, so it's not that far off. I think I'll go ahead and merge |
You can add 'useCSPSafeFilter: true' to the DataView options