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

Support passive event listeners #428

Open
rektide opened this issue Dec 2, 2016 · 15 comments · May be fixed by #3769
Open

Support passive event listeners #428

rektide opened this issue Dec 2, 2016 · 15 comments · May be fixed by #3769

Comments

@rektide
Copy link

rektide commented Dec 2, 2016

React is trying to figure out how to handle this too, in facebook/react#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

@developit
Copy link
Member

developit commented Dec 2, 2016

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>
    );
  }
}

@rektide
Copy link
Author

rektide commented Dec 30, 2016

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}) .

@developit
Copy link
Member

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

@jeyj0
Copy link

jeyj0 commented Jan 26, 2018

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?

@zouhir
Copy link
Member

zouhir commented Jun 21, 2018

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.

@adonig
Copy link

adonig commented Jul 8, 2020

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

@Alorel
Copy link

Alorel commented Aug 28, 2020

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>
}

@adonig
Copy link

adonig commented Oct 24, 2020

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

if (!oldValue) dom.addEventListener(name, proxy, useCapture);

Will it break something? 😅

@developit
Copy link
Member

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

@adonig
Copy link

adonig commented Oct 25, 2020

@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: GoogleChrome/lighthouse#9315, GoogleChrome/lighthouse#11100

@adonig
Copy link

adonig commented Oct 25, 2020

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: facebook/react#6436, facebook/react#19651.

@krskrs
Copy link

krskrs commented Apr 1, 2022

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! ;)

@amalitsky
Copy link

amalitsky commented Apr 5, 2022

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

@nicksrandall
Copy link

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.

@marvinhagemeister
Copy link
Member

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

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

Successfully merging a pull request may close this issue.

10 participants