Make WordPress Core

Opened 6 years ago

Last modified 2 months ago

#46713 new defect (bug)

Comment reply link uses inconvenient eventlistener

Reported by: szandman's profile szandman Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.1
Component: Comments Keywords: has-patch close
Focuses: performance Cc:

Description

When comment-reply.js was rewritten recently the behavior of the reply-button changed. It now seems to use 'touchstart' as eventlistener which makes it really "touchy" to use on mobile. It's almost impossible to use scroll now since you always trigger the touchstart event which opens the reply-box.

Change History (17)

This ticket was mentioned in Slack in #core-editor by peterwilsoncc. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-js by peterwilsoncc. View the logs.


6 years ago

#3 @janpaulkleijn
6 years ago

Hi there,
During my efforts to make my website more accessible I was also pointed out by Google LightHouse that the adding of the 'touchstart' event listener in this file lacked the option passive.

This can be corrected by altering the file: wp-includes/js/comment-reply.min.js

First the availability of support for the option parameter in the User Agent must be checked.
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Safely_detecting_option_support

Check option support (minified)

var supportsPassive=false;try{var opts=Object.defineProperty({},'passive',{get:function(){supportsPassive=true;}});window.addEventListener("testPassive", null, opts);window.removeEventListener("testPassive", null, opts);} catch (e) {}

So you insert this code in the comment-reply.min.js file, right above the current code.

Then the option must be added to the call for the 'touchstart' event listener in the code itself. Where you see:

addEventListener("touchstart",f)

you should replace it with:

addEventListener("touchstart",f,supportsPassive?{passive:true}:false)

At the moment this event listener is found only twice in the beginning part of the code.

This will solve your issue.

A good and thorough explanation of passive event listeners:
https://github.com/WICG/EventListenerOptions/blob/gh-pages/explainer.md

#4 @Znuff
6 years ago

@janpaulkleijn thanks for the suggested fix, our users were complaining about hitting the reply button while scrolling continuously, we couldn't figure out what it was initially, as it's not exactly obvious at first.

#5 @janpaulkleijn
6 years ago

No problem. It's true that these days an incredible lot happens on a page, which makes looking for the culprit not always that easy. It's kind of a detective thing...

#7 @afercia
6 years ago

Introduced in [42360] / #31590. Details on the Chrome / Lighthouse warning mentioned in #47510: https://developers.google.com/web/tools/lighthouse/audits/passive-event-listeners

#8 @eherman24
5 years ago

Patch attached to #47510

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#9 @madhazelnut
4 years ago

Do the passive listeners "fixes" actually address the core of this ticket? WordPress is still (5.4.2) displaying the same issue on mobile.

This ticket was mentioned in Slack in #core by madhazelnut. View the logs.


4 years ago

#11 @peterwilsoncc
4 years ago

As I've mentioned in #47510, passive listeners are incompatible with the event handler as it calls Event.preventDefault() -- please continue any discussion on moving to a passive listener on the existing ticket.

This ticket should focus on the reported issue of whether thouchstart is the appropriate place to add the event listener.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


4 years ago

This ticket was mentioned in PR #2818 on WordPress/wordpress-develop by jdevalk.


3 years ago
#13

  • Keywords has-patch added

Brought to light by recent Lighthouse updates, we should make the touchStart events in our comment reply script passive.

See https://web.dev/uses-passive-event-listeners/ for background and explanation.

This might affect (and partially fix) https://core.trac.wordpress.org/ticket/46713

Trac ticket: https://core.trac.wordpress.org/ticket/55988

#14 @westonruter
19 months ago

  • Focuses performance added

#15 follow-up: @Znuff
19 months ago

I love how this is 4 years old and... nothing happened yet.

#16 in reply to: ↑ 15 @pbearne
4 months ago

  • Keywords close added

Replying to Znuff:

I love how this is 4 years old and... nothing happened yet.

It looks like this is mostly fixed in https://core.trac.wordpress.org/ticket/55988

I am close this I don't think any more work will done on this ticket

This ticket was mentioned in Slack in #accessibility by dennysdionigi. View the logs.


2 months ago

Note: See TracTickets for help on using tickets.