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

Event directives can only be synchronous without the option for them to be asynchronous/passive #61634

Closed
westonruter opened this issue May 14, 2024 · 8 comments · Fixed by #61885
Assignees
Labels
[Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts

Comments

@westonruter
Copy link
Member

westonruter commented May 14, 2024

Please see discussion in #60574 (comment).

Interactivity performance on the web is heavily impeded by too many event handlers running synchronously in response to a user interaction. This is made evident by poor responsiveness on web pages which is also reflected in poor Interaction to Next Paint (INP) scores. Too much JavaScript running at once results in long tasks (>50ms) which a user can experience as jank (e.g. stuttering animations) or worse: rage clicks. To help guard against this as much as possible, event handlers should be broken up into chunks, each of which take less than 50 ms to complete on modest hardware. Nevertheless, even if one event handler is doing its part to take less than 50 ms, if there are multiple event handlers which are also running (and which may be less than 50 ms), it is most likely that they will all add up to a long task that hurts the user experience. (This is a real possibility as of #60661.)

The issue is that by default an event handler runs synchronously and blocks the main thread. In the Interactivity API, this is the wp-on directive. Sometimes an event handler must be synchronous in order to be able to call event.preventDefault(). However, for many use cases there is no need for synchronous access. For example, a common problem on the web is that analytics trackers will attach their event handlers that run synchronously at the same time as critical 1st party code. These should be safely made asynchronous. The majority of directives in the Image block can also be made asynchronous, particularly those which don't call event.preventDefault():

Directive Handler Async-eligible?
data-wp-on--click actions.hideLightbox
data-wp-on--click actions.showLightbox
data-wp-on--keydown actions.handleKeydown
data-wp-on--load callbacks.setButtonStyles
data-wp-on--touchend actions.handleTouchEnd
data-wp-on--touchmove actions.handleTouchMove
data-wp-on--touchstart actions.handleTouchStart
data-wp-on-window--resize callbacks.setButtonStyles
data-wp-on-window--resize callbacks.setOverlayStyles
data-wp-on-window--scroll actions.handleScroll

As for implementation, it could look very similar to the existing data-wp-on directive logic but simply adding in a statement to yield before invoking each handler:

	// data-wp-on-async--[event]
	directive( 'on-async', ( { directives: { on }, element, evaluate } ) => {
		const events = new Map();
		on.filter( ( { suffix } ) => suffix !== 'default' ).forEach(
			( entry ) => {
				const event = entry.suffix.split( '--' )[ 0 ];
				if ( ! events.has( event ) ) {
					events.set( event, new Set() );
				}
				events.get( event ).add( entry );
			}
		);

		events.forEach( ( entries, eventType ) => {
			element.props[ `on${ eventType }` ] = async ( event ) => {
				entries.forEach( ( entry ) => {
					await yieldToMain(); // 👈👈👈 KEY ADDITION HERE
					evaluate( entry, event );
				} );
			};
		} );
	} );

As discussed in #60574 (comment), having such a yieldToMain() helper function would be useful for actions and callbacks to manually insert their own yield points before or after some expensive logic. For an example implementation of yieldToMain(), see Optimize long tasks, which was already implemented in the Interactivity API during hydration in #58227.

Eventually once Preact supports passive event listeners (#58705), such async event handlers could be made passive as well.

Once this is implemented, the core blocks can be updated to use data-wp-async instead of data-wp. Additionally, @luisherranz noted in #60574 (reply in thread), the Interactivity API docs should be updated to make use of this directive whenever possible, and the create-block template can be updated to use it as well.

@westonruter westonruter added [Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts [Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity labels May 14, 2024
@westonruter westonruter changed the title Introduce wp-on-async directive as performant alternative over synchronous wp-on directive Event directives can only be synchronous without the option for them to be asynchronous/passive May 23, 2024
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label May 23, 2024
@westonruter
Copy link
Member Author

Here's a PR with the proposed changes: #61885

@luisherranz
Copy link
Member

luisherranz commented May 23, 2024

I had an idea to make wp-on synchronous by default. I don't know WordPress's backward compatibility policy, but it is possible to transition wp-on from synchronous to asynchronous by informing users over a period of time in the JS console, similar to a deprecation.

It would work as follows. Instead of adding wp-on-async, we add wp-on-sync. In wp-on, we proxy or wrap the event passed to the callback so that the functions preventDefault, stopPropagation, etc. display a warning in the console urging users to replace wp-on with wp-on-sync. If we maintain this warning message for several WordPress versions (again, I'm not entirely sure what the policy would be here - three major versions? one year?), we could make the default behavior asynchronous. Then, when someone uses a synchronous method on wp-on, an error message would be displayed telling them to change the wp-on directive to wp-on-sync.

The only downside would be that, until several WordPress versions later, wp-on (and wp-on-sync) would need to be synchronous.

@westonruter
Copy link
Member Author

In wp-on, we proxy or wrap the event passed to the callback so that the functions preventDefault, stopPropagation, etc. display a warning in the console urging users to replace wp-on with wp-on-sync.

A couple challenges here are that (1) this requires the user to trigger the event to see the warning, and (2) calling preventDefault() is often conditional, for example based on pressing a specific key. Therefore, it could be difficult for developers to discover the warnings.

@luisherranz
Copy link
Member

Let's use wp-on-async, then 🙂

@westonruter
Copy link
Member Author

Reopening as we should also now leverage wp-on-async for the other core blocks beyond Image.

@westonruter westonruter reopened this May 28, 2024
@adamsilverstein
Copy link
Member

Reopening as we should also now leverage wp-on-async for the other core blocks beyond Image.

Great idea to use this for core blocks! Maybe open a new issue for this since this Issue seems to be resolved?

@westonruter
Copy link
Member Author

I guess so. It's just the initial PR implemented data-wp-on-async for the Image block, so it's partially done already.

@westonruter
Copy link
Member Author

I've opened #62160 to implement data-wp-on-async for the other core blocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts
Projects
None yet
3 participants