preact icon indicating copy to clipboard operation
preact copied to clipboard

Support passive event listeners

Open rektide opened this issue 7 years ago • 14 comments

React is trying to figure out how to handle this too, in https://github.com/facebook/react/issues/6436 . Would love to see this added in somehow in preact. It's a fantastic capability for battling jank, and keeping things interactive and buttery smooth.

Explainer doc on passive event listeners: https://github.com/WICG/EventListenerOptions/blob/gh-pages/explainer.md

rektide avatar Dec 02 '16 18:12 rektide

Which event types would be useful for this though? I use passive event listeners, but only at the document level - scroll mainly. A compositional component like this is where I implement them, and they circumvent VDOM entirely:

const OPTS = { passive:true };
const EVENT = {
  get offset() { return document.scrollingElement.scrollTop; },
  get max() { return document.scrollingElement.scrollHeight }
};
class ScrollObserver {
  update = () => {
    this.props.onScroll(EVENT);
  };
  componentDidMount() {
    addEventListener('scroll', this.update, OPTS);
  }
  componentWillUnmount() {
    removeEventListener('scroll', this.update, OPTS);
  }
  render({ children: [child] }) {
    return typeof child==='function' ? child() : child;
  }
}

Example usage:

class Example extends Component {
  onScroll = e => {
    // accessing these is what triggers relayout, so be careful!
    console.log(e.offset, e.max);
  };
  render() {
    return (
      <ScrollObserver onScroll={this.handleScroll}>
        <div style={{ height:10000 }} />
      </ScrollObserver>
    );
  }
}

developit avatar Dec 02 '16 18:12 developit

One of the things I really appreciate about Preact is that it's not whitelist based. Unlike React, it doesn't pick what it wants to implement, it just provides broad support for what an author sees fit to do. I apologize for the non-answer, but I don't think I could ever come up with a comprehensive, adequate list of places where Passive might be useful and generally feel any such attempt will be flawed and result in trouble and unnecessary hinderance.

It's crude but just adding post-script modifiers could be adequate. onScrollPassive becomes an .addEventListener(foo, "scroll", {passive: true}) .

rektide avatar Dec 30 '16 02:12 rektide

I like making it an option/suffix like that, but I worry about how hard it might be to implement addition/removal.

developit avatar Dec 30 '16 03:12 developit

As discussed in React's issue @rektide provided so kindly, the suffix is probably not the best option, as it does not scale (only...). I'd like to see this implemented.

I currently need to use passive listeners on a specific component, not the entire body. I am forced to manually addEventListener and removeEventListener for now, though that option does seem like an antipattern to me.

Any plans to implement it even though React seems to stall on it as well?

jeyj0 avatar Jan 26 '18 14:01 jeyj0

I like the idea of having onScrollPassive however, my 2cents:

  • the options object accept multiple things {capture, passive, once} and I don't think we should maintain a shorthand on the element for each option.

  • providing a default to true that is not a web standard default is something we shouldn't be doing.

  • addEventListsner is not an antipattern at all, as long as you remember to removeEventListsner once the component will unmount.

My favourite out of those 3 is actually addEventListsner if you want custom options.

zouhir avatar Jun 21 '18 16:06 zouhir

This is mainly an issue with Lighthouse. Are there any known workarounds to make react/preact pass?

adonig avatar Jul 08 '20 11:07 adonig

This is mainly an issue with Lighthouse. Are there any known workarounds to make react/preact pass?

The workaround I use involves refs, which is similar to what @developit was doing in their comment, but not very convenient if you need to use it often.

import {useEffect, useRef} from 'preact/hooks';
import {h, VNode} from 'preact';

function MyComponent(): VNode {
  const divRef = useRef<HTMLDivElement>();

  useEffect(() => {
    const element = divRef.current;
    if (!element) {
      return;
    }

    const opts: any = {passive: true};
    const listener = () => {
      console.log('I do things');
    };
    element.addEventListener('click', listener, opts);

    return () => {
      element.removeEventListener('click', listener, opts);
    };
  }, [divRef.current]);

  return <div ref={divRef}>Foo</div>
}

Alorel avatar Aug 28 '20 13:08 Alorel

What will happen if someone replaces useCapture with { capture: useCapture, passive: true } here?

https://github.com/preactjs/preact/blob/0c14a92a98e8ed8427aa4a9dc9db09045fc5b27a/src/diff/props.js#L102

Will it break something? 😅

adonig avatar Oct 24 '20 18:10 adonig

@adonig yes - passive events cannot use preventDefault(), which is extremely common.

developit avatar Oct 25 '20 05:10 developit

@developit I see. So even when we differentiate between certain types of events, the lighthouse check would complain about the one addEventListener call without the truthy passive option. I think this is a problem with lighthouse, potentially leading to over-optimization. I found two open issues related to passive event listeners there: https://github.com/GoogleChrome/lighthouse/issues/9315, https://github.com/GoogleChrome/lighthouse/issues/11100

adonig avatar Oct 25 '20 11:10 adonig

Correction: It looks like it worked like this

  1. Collect all event listeners on the page.
  2. Filter out non-touch and non-wheel listeners.
  3. Filter out listeners that call preventDefault().
  4. Filter out listeners that are from a different host than the page.

but then Chrome changed the policy to warn on any touchevent with a non-passive listener.

The react team has multiple issues on the topic too: https://github.com/facebook/react/issues/6436, https://github.com/facebook/react/issues/19651.

adonig avatar Oct 25 '20 11:10 adonig

Hi guys, I ended up here after noticing that with the DatePicker component of React mui/material (which, apart from this glitch, works flawlessly on Preact) I was getting a lot of:

image

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

It looks like the issue is not present on the React examples on mui/material site (https://mui.com/components/pickers/#react-components). I tried to track the progress of this issue but I haven't been able to understand exactly what's its current state on Preact (and neither how React is exactly handling this), because there has been quite a lot of discussion about it. I mean: the clear part is that a few events should be used with the passive flag set to true, for performance reasons, and that Chrome is reporting when such events are attached without the passive: true flag (or when it's not expressly set as well?).

Anyway, the practical conclusion is that such violations are not reported for React, while it looks like they are for Preact. So, I was wondering what's the current state of Preact regarding the handling of such events, and if there's the possibility to 'fix' them on Preact as well (maybe implementing the same behavior as React?).

Long live Preact! ;)

krskrs avatar Apr 01 '22 23:04 krskrs

This seems to fail one of the recommended lighthouse assertions:

  ✘  uses-passive-event-listeners failure for minScore assertion
       Does not use passive listeners to improve scrolling performance

In my case it is used by the GatsbyJs website but the error description points to preact.module.js file:

Screen Shot 2022-04-04 at 19 09 54

amalitsky avatar Apr 05 '22 02:04 amalitsky

One event that passive events are common for (that is not on the document) is touchstart events. React 18 always uses "passive" for touchstart events.

nicksrandall avatar Sep 16 '22 21:09 nicksrandall

Can confirm. For reference the code in React that does that can be found here: https://github.com/facebook/react/blob/main/packages/react-dom-bindings/src/events/DOMPluginEventSystem.js#L432-L447

marvinhagemeister avatar Oct 15 '22 11:10 marvinhagemeister