WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 3 months ago

#47510 new defect (bug)

Developer tool - Best Practice - input comment field Does not use passive listeners

Reported by: efilippucci Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.2.1
Component: Comments Keywords: has-patch needs-refresh
Focuses: javascript, performance Cc:

Description

Using chrome developer tool, I noticed that, when is displayed the input comment field in standard post, one best Practice is not satisfied:

Does not use passive listeners to improve scrolling performance.
Consider marking your touch and wheel event listeners as passive to improve your page’s scroll performance.
js/comment-reply.min.js?ver=5.2.1

One theme developer confirmed that this is a WordPress core problem: https://generatepress.com/forums/topic/gp-2-3-alpha-3-consider-using-passive-listeners/#post-923468

Attachments (1)

47510.diff (925 bytes) - added by eherman24 8 months ago.
Set event listeners to passive

Download all attachments as: .zip

Change History (35)

#1 @SergeyBiryukov
13 months ago

  • Component changed from General to Comments

Related: #46713

#2 @efilippucci
13 months ago

  • Severity changed from minor to normal

#3 @mwanmedia
10 months ago

  • Focuses javascript added

waiting this ticket

#4 @slkfsdf8y34ljhsfsdfkuhfkl84hj
9 months ago

  • Severity changed from normal to major

This is hurting the wordpress performance. Please consider rising it up as a priority.

#5 @eherman24
8 months ago

Do we need a patch for this? I've also encountered this issue this morning and since it's outside of my control there is nothing I can do.

@eherman24
8 months ago

Set event listeners to passive

#6 @eherman24
8 months ago

  • Keywords has-patch added

I've attached a patch which sets the event listeners to passive, and ultimately resolves the notice thrown in Lighthouse.

Before Patch:
https://cldup.com/s1YO-ay-Gd.png

After Patch:
https://cldup.com/wBq0Ha6LO4.png

Tests:
✅Lighthouse audits run using TwentyNineteen
✅Lighthouse audits run using TwentyTwenty

#8 @efilippucci
8 months ago

  • Keywords reporter-feedback added

Thanks a lot, any chance to put it in the next WordPress 5.3 release?

#9 follow-up: @eherman24
8 months ago

@efilippucci I don't think that it will make the 5.3 release. That is slated for 11/12, which is only 3 days away. I'm guessing things are in a freeze for testing right now before that release, and (if the patch is accepted) it will make it into the next security release (5.3.1).

Last edited 8 months ago by eherman24 (previous) (diff)

#10 in reply to: ↑ 9 @soulkitchen
7 months ago

Hello
your patch is for the normal js/comment-reply.js file. please consider that is this file: js/comment-reply.min.js (sorry if I say any wrong words or do not understand how things work. just a rookie I am. :) )
Thank you.
Best regards

Replying to eherman24:

@efilippucci I don't think that it will make the 5.3 release. That is slated for 11/12, which is only 3 days away. I'm guessing things are in a freeze for testing right now before that release, and (if the patch is accepted) it will make it into the next security release (5.3.1).

Last edited 7 months ago by soulkitchen (previous) (diff)

#11 @andraganescu
7 months ago

  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to 5.3.1
  • Type changed from enhancement to defect (bug)

#12 @andraganescu
7 months ago

  • Keywords needs-refresh added

Hi @eherman24 the patch looks good but I checked and since we still support IE11 there might be a need to check if passive is supported.

Can I use tells us IE11 doesn't support passive listeners and MDN advises we create some way to detect prior to setting the option.

Could you please add this to the patch?

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


7 months ago

#14 @audrasjb
7 months ago

  • Milestone changed from 5.3.1 to 5.4
  • Severity changed from major to normal

Given this issue is not a regression caused by WordPress 5.3, let's handle this in the next major release, 5.4.

#15 follow-up: @slkfsdf8y34ljhsfsdfkuhfkl84hj
7 months ago

In my opinion it is small enough and critical enough to improve the performance that I think it can be released in 5.3.1.

Is there a way I could apply the patch manually by downloading the files?

#16 in reply to: ↑ 15 @audrasjb
7 months ago

Hi @slkfsdf8y34ljhsfsdfkuhfkl84hj ,

Replying to slkfsdf8y34ljhsfsdfkuhfkl84hj:

In my opinion it is small enough and critical enough to improve the performance that I think it can be released in 5.3.1.

Unfortunately, 5.3.1 is scheduled for Thursday 12 with a Release Candidate at the beginning of the week, and this ticket still needs a patch that addresses the above issues (see previous comments).
To milestone it again to 5.3.1, we need a new patch by tomorrow and people available to test this patch. That could still happen but doesn't sounds very realistic to me.

Is there a way I could apply the patch manually by downloading the files?

Since it is a JS change, you'll need to run npm run build command on wordpress-develop after applying the patch on your development instance of WordPress. See related docs for reference: https://make.wordpress.org/core/handbook/tutorials/working-with-patches/

Cheers,
Jb

#17 follow-up: @chadrew
7 months ago

After applying the patch, a new error appears in Chrome's developer tools console when clicking the reply link:

comment-reply.js:1 Unable to preventDefault inside passive event listener invocation.

Edit: there's also a change in behavior when you touch the reply link and immediately begin to scroll on mobile. Before the form wouldn't open, now it opens and you scroll away. Just an observation, no clue if this is undesirable.

Last edited 7 months ago by chadrew (previous) (diff)

#18 @slkfsdf8y34ljhsfsdfkuhfkl84hj
7 months ago

Since Windows 7 support will end on January 14, 2020 and after that Google will display full screen warnings and won't even ship security software to it, I think it is safe to say that WordPress should ditch IE11 support after Jan 2020.

There is no point to support a browser which won't be supported even by its creators.

Last edited 7 months ago by slkfsdf8y34ljhsfsdfkuhfkl84hj (previous) (diff)

#19 in reply to: ↑ 17 @slkfsdf8y34ljhsfsdfkuhfkl84hj
7 months ago

Replying to chadrew:

After applying the patch, a new error appears in Chrome's developer tools console when clicking the reply link:

comment-reply.js:1 Unable to preventDefault inside passive event listener invocation.

Any update on this?

Last edited 7 months ago by slkfsdf8y34ljhsfsdfkuhfkl84hj (previous) (diff)

#20 @audrasjb
5 months ago

  • Milestone changed from 5.4 to Future Release

Hi,

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#21 @slkfsdf8y34ljhsfsdfkuhfkl84hj
5 months ago

And this is how WordPress created a huge backlog that is growing bigger and bigger. It is preposterous that an issue of that scale is left to be postponed in that way in the future so many times.

#22 @audrasjb
5 months ago

Right, but to be fixed, the issue needs contributors to propose a patch.
The current patch needs to be refreshed first, and it's too late for 5.4, that's why this issue won't be fixed in the incoming release, unfortunately.

#23 @slkfsdf8y34ljhsfsdfkuhfkl84hj
5 months ago

I understand, but that was said the previous time as well. I am sure something like that would happen in a next version as well and so on. The old tickets are growing.

To be honest as a side viewer of all this, I think everything is not streamlined. If I was someone responsible for WordPress.org I would significantly streamlined and simplify everything, including the structure of the site, so it gets as minimal as possible and every focus is on developing.

I think the whole website needs to become significantly simpler, streamlined, modernized and these tickets should be put in front of everything.

Currently what's happening is bureaucracy building more bureaucracy leading to more and more backlog.

If the environment is good, then the participants would be more active.

#24 @peterwilsoncc
5 months ago

Both the reply and cancel button event handlers use event.preventDefault(); so switching to passive handlers can't be made without other refactoring.

I can't see how the event handlers can be rewritten to avoid event.preventDefault(); for a couple of reasons:

  • without preventing the default action, clicking the links will cause them to be followed by the browser
  • backward compatibility with custom comment engine plugins overwriting the moveForm() function

While a high lighthouse score feels nice, it's an automated score-card. Changes such as this would need to come with substantiated performance increases.

#25 @slkfsdf8y34ljhsfsdfkuhfkl84hj
5 months ago

The current state of the WordPress development:

  • WordPress.org - outdated site with cluttered sections;
  • Tickets - pilling up more and more while less and less are resolved;
  • WYSIWYG editing that Web.com, Wix, Squarespace, Shopify did comes to Gutenberg which is 1/10th of the way there far too little, far too late.
  • Static mindset - "things are good the way they are and should stay like that for compatibility reasons, aka Windows-y mindset.

I am sorry I don't see a future in this CMS in the longrun, Squarespace, Wix and Shopify would take even more market share because of this WRONG anti-consumer mindset.

P.S. IE11 was released 7 years ago, it's primarily used for oldschool banking and government entities since Microsoft moved to Edge, now Edge is even re-released with the same name as a brand and with Chromium as an engine. IE11 is just a mode inside Edge now on Windows platforms. Nobody would use that mode apart from some old government site that was still not updated. It doesn't make any sense for us to seek for such compatibility. Nothing works properly on the IE11 anymore, it's just a legacy mode for just few sites out there that large institutions cannot change just now.

My whole point is - let's move forward!

Last edited 5 months ago by slkfsdf8y34ljhsfsdfkuhfkl84hj (previous) (diff)

#26 @slkfsdf8y34ljhsfsdfkuhfkl84hj
5 months ago

This ticket was opened 9 months ago. The pile of tickets is growing more and more and less is resolved. What's needed and what are we waiting for? AI to solve all of them? It's disastrous, while in the meantime Squarespace, Wix, Shopify and others are having clean, simple, streamlined WYSIWYG editing that makes a website popups up in seconds.

I am starting to think WordPress is not competitive in the long run anymore!

#27 @peterwilsoncc
5 months ago

@slkfsdf8y34ljhsfsdfkuhfkl84hj please refrain from snarky comments on trac and WordPress.org generally.

I'm happy for this to move this ticket forward if I am missing how these functions can be rewritten without relying on event.preventDefault() and a performance gain can be shown.

#28 @slkfsdf8y34ljhsfsdfkuhfkl84hj
5 months ago

We have internal split tests of ranking advantages in the search engines before/after optimization of the Lighthouse scores are made.

Last edited 3 months ago by slkfsdf8y34ljhsfsdfkuhfkl84hj (previous) (diff)

#29 follow-up: @slkfsdf8y34ljhsfsdfkuhfkl84hj
3 months ago

What kind of action is needed for this to move forward and not become one of the thousands opened 10+ year old tickets that are just piling up?

#30 in reply to: ↑ 29 @peterwilsoncc
3 months ago

Replying to slkfsdf8y34ljhsfsdfkuhfkl84hj:

What kind of action is needed for this to move forward....?

In order to use passive listeners the handlers need to be written in a way that does not rely on event.preventDefault(). I don't think this is possible as the handlers prevent the browser from following the link (a non-JavaScript fallback) but I'm happy to hear from other contributors.

Lighthouse is an automated tool that warns based on rules of thumb. By avoiding the need to visit a new page to reply to a comment, the commenter is able to reply immediately rather than waiting for a new page to load and render.

I'd caution against blindly following the recommendations of any automated tool, as it can lead to problems in itself.

As I said, if the listeners can be rewritten so they don't need event.preventDefault() without removing the fallback then I am happy for this to move forward.

#31 follow-up: @slkfsdf8y34ljhsfsdfkuhfkl84hj
3 months ago

"In order to use passive listeners the handlers need to be written in a way that does not rely on event.preventDefault(). "

Otherwise it won't be compatible with the minority of IE11 users, otherwise it would be working as expected?

I did some tests with the lighthouse scores in terms of ranking and I found that on many occasions it helped rankings overall.

The current situation is decreasing the scores a lot.

Last edited 3 months ago by slkfsdf8y34ljhsfsdfkuhfkl84hj (previous) (diff)

#32 in reply to: ↑ 31 @peterwilsoncc
3 months ago

Replying to slkfsdf8y34ljhsfsdfkuhfkl84hj:

"In order to use passive listeners the handlers need to be written in a way that does not rely on event.preventDefault(). "

Otherwise it won't be compatible with the minority of IE11 users, otherwise it would be working as expected?

I did some tests with the lighthouse scores in terms of ranking and I found that on many occasions it helped rankings overall.

The current situation is decreasing the scores a lot.

This is unrelated to IE11 support, per the MDN addEventListener documentation using a passive listener will cause the event to fail and throw a console error if it contains event.preventDefault():

A Boolean which, if true, indicates that the function specified by listener will never call preventDefault(). If a passive listener does call preventDefault(), the user agent will do nothing other than generate a console warning.

#33 @jr3074
3 months ago

is there any patch?

#34 @slkfsdf8y34ljhsfsdfkuhfkl84hj
3 months ago

Is there a way to make the comments use passive listeners without creating issues or find another way to patch this performance issue?

Note: See TracTickets for help on using tickets.