WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 6 weeks ago

#47510 new defect (bug)

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

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

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 3 months ago.
Set event listeners to passive

Download all attachments as: .zip

Change History (20)

#1 @SergeyBiryukov
8 months ago

  • Component changed from General to Comments

Related: #46713

#2 @efilippucci
8 months ago

  • Severity changed from minor to normal

#3 @mwanmedia
5 months ago

  • Focuses javascript added

waiting this ticket

#4 @slkfsdf8y34ljhsfsdfkuhfkl84hj
4 months ago

  • Severity changed from normal to major

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

#5 @eherman24
3 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
3 months ago

Set event listeners to passive

#6 @eherman24
3 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
3 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
3 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 3 months ago by eherman24 (previous) (diff)

#10 in reply to: ↑ 9 @soulkitchen
2 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 2 months ago by soulkitchen (previous) (diff)

#11 @andraganescu
8 weeks ago

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

#12 @andraganescu
8 weeks 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.


8 weeks ago

#14 @audrasjb
8 weeks 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
8 weeks 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 weeks 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 weeks 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 6 weeks ago by chadrew (previous) (diff)

#18 @slkfsdf8y34ljhsfsdfkuhfkl84hj
7 weeks 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 weeks ago by slkfsdf8y34ljhsfsdfkuhfkl84hj (previous) (diff)

#19 in reply to: ↑ 17 @slkfsdf8y34ljhsfsdfkuhfkl84hj
6 weeks 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 6 weeks ago by slkfsdf8y34ljhsfsdfkuhfkl84hj (previous) (diff)
Note: See TracTickets for help on using tickets.