Non-Passive Event Listener Performance Warnings

pbower commented 3 months ago

Describe the bug

Hi there,

Hope all is well.

I'm receiving the following browser warnings with Slickgrid (latest version confirmed).

The warnings suggests a possible fix for slick.interactions.ts to increase performance by adding ' { passive: true }' to (at least some) of the event listeners. However, noticing that there are some preventDefaults called I am unsure if this is the correct solution.

Therefore, I'm wondering whether it's a case of adding 'passive: true' to some of the following event listeners?

Please kindly find details below.

Thanks heaps,

Regards, Peter

[Violation] Added non-passive event listener to a scroll-blocking event. Consider marking event handler as 'passive' to make the page more responsive. See GridStore.tsx:127

It is traced back to the following event listeners in slick.interactions.ts (and associated bundle files):

  function init() {
    if (containerElement) {
      containerElement.addEventListener('mousedown', userPressed as EventListener);
      containerElement.addEventListener('touchstart', userPressed as EventListener);
    }
  }

Possibly:

    function userPressed(event: MouseEvent | TouchEvent | KeyboardEvent) {
    if (!preventDrag(event)) {
      element = event.target as HTMLElement;
      const targetEvent: MouseEvent | Touch = (event as TouchEvent)?.touches?.[0] ?? event;
      const { target } = targetEvent;

      if (!options.allowDragFrom || (options.allowDragFrom && (element.matches(options.allowDragFrom)) || (options.allowDragFromClosest && element.closest(options.allowDragFromClosest)))) {
        originaldd.dragHandle = element as HTMLElement;
        const winScrollPos = Utils.windowScrollPosition();
        startX = winScrollPos.left + targetEvent.clientX;
        startY = winScrollPos.top + targetEvent.clientY;
        deltaX = targetEvent.clientX - targetEvent.clientX;
        deltaY = targetEvent.clientY - targetEvent.clientY;
        originaldd = Object.assign(originaldd, { deltaX, deltaY, startX, startY, target });
        const result = executeDragCallbackWhenDefined(onDragInit as (e: DragEvent, dd: DragPosition) => boolean | void, event, originaldd as DragItem);

        if (result !== false) {
          document.body.addEventListener('mousemove', userMoved);
          document.body.addEventListener('touchmove', userMoved);
          document.body.addEventListener('mouseup', userReleased);
          document.body.addEventListener('touchend', userReleased);
          document.body.addEventListener('touchcancel', userReleased);
        }
      }
    }
  }

and (for MouseWheel)


  function init() {
    element.addEventListener('wheel', wheelHandler as EventListener);
    element.addEventListener('mousewheel', wheelHandler as EventListener);
  }

and (for Resizable):

    function init() {
    // add event listeners on the draggable element
    resizeableHandleElement.addEventListener('mousedown', resizeStartHandler);
    resizeableHandleElement.addEventListener('touchstart', resizeStartHandler);
  }

and/or ResizingHandler:

        document.body.addEventListener('mousemove', resizingHandler);
      document.body.addEventListener('mouseup', resizeEndHandler);
      document.body.addEventListener('touchmove', resizingHandler);
      document.body.addEventListener('touchend', resizeEndHandler);

  The items called out specifically in the full stack trace are:
  -touchstart
  -mousewheel
  -wheel

  with 90% of the warnings relating to 'touchstart'.

  The full stack trace is:

      [Violation] Added non-passive event listener to a scroll-blocking <some> event. Consider marking event handler as 'passive' to make the page more responsive. See <URL>
SlickGrid.ts:17 [Violation] Added non-passive event listener to a scroll-blocking 'touchstart' event. Consider marking event handler as 'passive' to make the page more responsive. See https://www.chromestatus.com/feature/5745543795965952
init @ index.js:2301
Resizable @ index.js:2324
setupColumnResize @ index.js:7437
createColumnHeaders @ index.js:7344
finishInitialization @ index.js:6952
initialize @ index.js:6945
SlickGrid @ index.js:6895
SlickGrid @ SlickGrid.ts:17
initialiseSlickGrid @ GridStore.tsx:121
(anonymous) @ GridStore.tsx:77
executeAction @ mobx.esm.js:1249
Reaction@6 @ mobx.esm.js:1234
reactionRunner @ mobx.esm.js:2874
(anonymous) @ mobx.esm.js:2851
runReaction_ @ mobx.esm.js:2443
runReactionsHelper @ mobx.esm.js:2639
reactionScheduler @ mobx.esm.js:2616
(anonymous) @ mobx.esm.js:2649
batchedUpdates$1 @ react-dom.development.js:26179
reactionScheduler @ mobx.esm.js:2648
runReactions @ mobx.esm.js:2623
endBatch @ mobx.esm.js:2256
reportChanged @ mobx.esm.js:468
setNewValue_ @ mobx.esm.js:1419
setObservablePropValue_ @ mobx.esm.js:4970
set_ @ mobx.esm.js:5003
set @ mobx.esm.js:3575
commitAttachRef @ react-dom.development.js:23704
commitLayoutEffectOnFiber @ react-dom.development.js:23542
commitLayoutMountEffects_complete @ react-dom.development.js:24727
commitLayoutEffects_begin @ react-dom.development.js:24713
commitLayoutEffects @ react-dom.development.js:24651
commitRootImpl @ react-dom.development.js:26862
commitRoot @ react-dom.development.js:26721
performSyncWorkOnRoot @ react-dom.development.js:26156
flushSyncCallbacks @ react-dom.development.js:12042
(anonymous) @ react-dom.development.js:25690
Show 34 more frames
Show less
SlickGrid.ts:17 [Violation] Added non-passive event listener to a scroll-blocking 'touchstart' event. Consider marking event handler as 'passive' to make the page more responsive. See https://www.chromestatus.com/feature/5745543795965952
init @ index.js:2301
Resizable @ index.js:2324
setupColumnResize @ index.js:7437
createColumnHeaders @ index.js:7344
finishInitialization @ index.js:6952
initialize @ index.js:6945
SlickGrid @ index.js:6895
SlickGrid @ SlickGrid.ts:17
initialiseSlickGrid @ GridStore.tsx:121
(anonymous) @ GridStore.tsx:77
executeAction @ mobx.esm.js:1249
Reaction@6 @ mobx.esm.js:1234
reactionRunner @ mobx.esm.js:2874
(anonymous) @ mobx.esm.js:2851
runReaction_ @ mobx.esm.js:2443
runReactionsHelper @ mobx.esm.js:2639
reactionScheduler @ mobx.esm.js:2616
(anonymous) @ mobx.esm.js:2649
batchedUpdates$1 @ react-dom.development.js:26179
reactionScheduler @ mobx.esm.js:2648
runReactions @ mobx.esm.js:2623
endBatch @ mobx.esm.js:2256
reportChanged @ mobx.esm.js:468
setNewValue_ @ mobx.esm.js:1419
... etc.

Reproduction

Activate all available event listeners for Slickgrid and the warning shows (at least) in Chrome browser.

Which Framework are you using?

React

Environment Info

System:
    OS: Linux 6.8 Ubuntu 24.04 LTS 24.04 LTS (Noble Numbat)
    CPU: (24) x64 12th Gen Intel(R) Core(TM) i9-12900KF
    Memory: 52.68 GB / 62.58 GB
    Container: Yes
    Shell: 5.9 - /usr/bin/zsh
  Binaries:
    Node: 20.15.1 - ~/.nvm/versions/node/v20.15.1/bin/node
    Yarn: 1.22.22 - ~/.nvm/versions/node/v20.15.1/bin/yarn
    npm: 10.8.2 - ~/.nvm/versions/node/v20.15.1/bin/npm
  Browsers:
    Chrome: 126.0.6478.182

Validations

[X] Follow our Code of Conduct
[X] Read the Wikis.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion.
[X] The provided reproduction is a minimal reproducible example of the bug.

ghiscoding commented 3 months ago

I've already done some changes for some of them, but it's just impossible to do it for all event listeners. Chrome doesn't make a distinction between what's possible and what's not, so I don't think that there's anything more that we can do to remove all warnings. In fact, I had tried in the past to apply the change to all events to remove the warnings but quickly realized it caused huge problems and even broke SlickGrid and so I had to revert the changes. So we can't always rely on Chrome warnings to do code changes.

see previous list of past PRs which I worked on and in the end, I had to revert all changes because Passive mode for event listeners should only be used when you are 100% sure that you won't ever call e.preventDefault() and for many SlickGrid events, it's just impossible to know for sure. For example when editing, we will often use prevent default to avoid event bubbling.

Chrome warnings are not always valid, it only provides suggestions that if possible might improve perf (but in our case, it's not possible to apply the changes)

pbower commented 3 months ago

No worries, thanks for this.

6pac / SlickGrid