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

perf: skip reapplying empty html when target is already empty #932

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

ghiscoding
Copy link
Collaborator

@ghiscoding ghiscoding commented Nov 28, 2023

  • when calling applyHtmlCode() method and our target element is empty and we try to reapply empty, it should always be ok to simply skip this reassignment because the end result is exactly the same
  • this should also help with performance since trying to reapply empty using innerHTML will probably cause a canvas repaint but if we manage to skip this assignment then a repaint will also be skipped
  • it should help as well for CSP because we could write a Formatter with native element while still returning empty string when there's no value to return, that will then call applyHtmlCode and skip the assignment if the cell is already empty

for example the code below would be 100% CSP Safe and iinterpreted by SlickGrid as native without using innerHTML at all

function myNativePercentFormatter(cell, row, val) {
  if (!val) {
    return ''; // we still want to accept empty string (or `null`) without calling innerHTML in SlickGrid
  }
  // when value is defined, return a native HTML element
  const div = document.createElement('div');
  div.textContent = `${val / 100}%`;
  return div;
}

- when calling `applyHtmlCode()` method and our target element is empty and we try to reapply empty, it should always be ok to simply skip this reassignment.
- this should also help with performance since trying to reapply empty using `innerHTML` will probably cause a canvas repaint but if we manage to skip this then a repaint will also be skipped
- this should also help with CSP because we could write a Formatter with native element while still returning empty string when there's no value to return, that will then call `applyHtmlCode` and skip the assignment if the cell is already empty
@ghiscoding
Copy link
Collaborator Author

ghiscoding commented Nov 28, 2023

@JesperJakobsenCIM did you manage to be fully CSP compliant in your application?

I'm still working on my end in my other Slickgrid-Universal project, and I saw that this PR could help a bit with CSP when creating a Formatter with native element but return empty string when there is no value to display in the cell formatter. With this PR it will do 2 things:

  1. skip reapplying empty to an empty target (which is good for perf reasons)
  2. and it will use .textContent instead of .innerHTML when it sees that the HTML to apply is an empty string .textContent = '' will simply empty the element and would provide the same result as if we would have used .innerHTML = ''. The goal is to use as less as possible .innerHTML

Anyway I was just curious to know if managed to be fully CSP compliant in your project? I'm assuming unsafe-inline is needed for style-src because we still use inline style for some elements like the slick row with margin top, or maybe you're passing a nonce on it?

EDIT

Taking another look at the unsafe-inline for the style and I guess we'll have a few non-compliance since dynamically changing .style is used in a few plugins like Grid Menu, Column Pickers and probably all of them.... actually it might be ok given this SO

@JesperJakobsenCIM
Copy link
Contributor

Sadly haven't gotten by to be fully CSP compliant yet, however the minor testing i have done looks promising so far. I'm expecting to properly do it over the Christmas, since we have quite a bit of maintenance work in that period with package updating too.

@ghiscoding ghiscoding merged commit 564c38d into master Nov 30, 2023
2 checks passed
@ghiscoding ghiscoding deleted the perf/skip-reapplying-empty-html branch November 30, 2023 05:02
@ghiscoding
Copy link
Collaborator Author

@JesperJakobsenCIM I found another dynamic function creation that is not CSP safe, do you think you can work your magic and provide a duplicate function that is CSP safe? That dynamic function is used with Grouping/DraggableGrouping

protected compileAccumulatorLoop(aggregator: Aggregator) {
if (aggregator.accumulate) {
const accumulatorInfo = this.getFunctionInfo(aggregator.accumulate);
const fn: any = new Function(
'_items',
'for (var ' + accumulatorInfo.params[0] + ', _i=0, _il=_items.length; _i<_il; _i++) {' +
accumulatorInfo.params[0] + ' = _items[_i]; ' +
accumulatorInfo.body +
'}'
);
const fnName = 'compiledAccumulatorLoop';
fn.displayName = fnName;
fn.name = this.setFunctionName(fn, fnName);
return fn;
} else {
return function noAccumulator() { };
}
}

@JesperJakobsenCIM
Copy link
Contributor

I can look into it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants