Make WordPress Core

Opened 6 years ago

Last modified 6 months ago

#45903 assigned defect (bug)

Twenty Nineteen: Some menu clicks trigger console errors

Reported by: kjellr's profile kjellr Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.0
Component: Bundled Theme Keywords: has-screenshots has-patch needs-refresh
Focuses: javascript Cc:

Description

There are two scenarios where the Twenty Nineteen menu javascript triggers console errors:


First, on desktop screens, clicking the main navigation arrows produce the following error:

TypeError: event.target.matches is not a function[Learn More] 4 touch-keyboard-navigation.js:285:9
	toggleSubmenuDisplay/< ../wp-content/themes/twentynineteen/js/touch-keyboard-navigation.js:285:9

https://cldup.com/PqsNUSgudV.gif

Originally reported by @joyously in: https://github.com/WordPress/twentynineteen/issues/596


Second, on touch-enabled screens, tapping menu items that have empty (#) href attributes result in the following console errors:

TypeError: event.target.nextSibling is null[Learn More] touch-keyboard-navigation.js:223:5 
	toggleSubmenuDisplay/<	../wp-content/themes/twentynineteen/js/touch-keyboard-navigation.js:223:5
	sendTouchEvent	resource://devtools/server/actors/emulation/touch-simulator.js:301:5
	handleEvent	resource://devtools/server/actors/emulation/touch-simulator.js:212:7
TypeError: event.target.matches is not a function[Learn More] touch-keyboard-navigation.js:277:9
	toggleSubmenuDisplay/<	http://core.test/wp-content/themes/twentynineteen/js/touch-keyboard-navigation.js:277:9

Originally reported in: https://github.com/WordPress/twentynineteen/issues/726


It looks like a possible fix for one of these was explored by @jmau in the following PR, but it has not yet been tested:

https://github.com/WordPress/twentynineteen/pull/753/files

Attachments (3)

45903.patch (2.0 KB) - added by laurelfulford 6 years ago.
45903.1.diff (1.9 KB) - added by audrasjb 4 years ago.
Patch refresh
45903.2.diff (1.9 KB) - added by audrasjb 4 years ago.
Coding standards fixes

Download all attachments as: .zip

Change History (26)

#1 @laurelfulford
6 years ago

  • Keywords has-screenshots added
  • Milestone changed from Awaiting Review to Future Release

Thanks for moving this over, @kjellr! I can recreate both of these issues.

@laurelfulford
6 years ago

#2 @laurelfulford
6 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

45903.patch copies over the changes from @jmlapam's GitHub PR.

#3 @ianbelanger
5 years ago

  • Milestone changed from Future Release to 5.5
  • Version changed from 5.0.2 to 5.0

@audrasjb
4 years ago

Patch refresh

#4 @audrasjb
4 years ago

  • Keywords needs-testing removed

In 45903.1.diff, I refreshed the existing patch and I moved the pre-return checks after vars declarations.

Tested on my side: no error is showing.

#5 @audrasjb
4 years ago

  • Owner set to ianbelanger
  • Status changed from new to assigned

@audrasjb
4 years ago

Coding standards fixes

#6 @dimijazz
4 years ago

@audrasjb I tried your code but still get the issue please have a look at screenshot.
I get the error if I leave the window on firefox and IE

https://www.dropbox.com/s/3ugx9nvcknh6l2f/Screen%20Shot%202020-06-17%20at%204.43.09%20PM.png

#7 @ianbelanger
4 years ago

  • Keywords needs-refresh added
  • Owner changed from ianbelanger to audrasjb

I can confirm that the most recent patch does not fix the issue. I don't get any errors in Chrome, but do in Firefox and IE.

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


4 years ago

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


4 years ago

#10 @pbiron
4 years ago

With 45903.2.diff applied, I can confirm that I'm still getting the console errors in FF 78.0.2 (Windows) and IE 11.

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


4 years ago

#12 @davidbaumwald
4 years ago

  • Milestone changed from 5.5 to 5.6

With 5.5 RC1 releasing tomorrow, this is being moved to 5.6. If any maintainer or committer feel this can be resolved in time or wishes to assume ownership during a specific cycle, feel free to update the milestone.

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


4 years ago

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


4 years ago

#15 @hellofromTonya
4 years ago

  • Milestone changed from 5.6 to 5.7

5.6 is deep into the beta cycle with Beta 4 next week. This ticket hasn't had activity in months. Punting to 5.7.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

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


4 years ago

#17 @poena
4 years ago

I spent a significant amount of time trying to troubleshoot this but I was not able to solve it.

The file twentynineteen\js\touch-keyboard-navigation.js is missing essential documentation for
the choices made regarding the focus event listener for the menu links.
It seems to be triggered on any focus, not only when focus is on elements inside the main menu.

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

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


4 years ago

#19 @hellofromTonya
4 years ago

  • Milestone changed from 5.7 to Future Release

With 5.7 RC1 landing tomorrow and no solution yet, punting this ticket to Future Release.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#20 @McAlyster
3 years ago

As a quick fix to get rid of the flowing annoying errors, I added a conditional test for the existence of
event.target.matches

if ( event.target.matches && event.target.matches('.main-navigation > div > ul > li a') ) {

I did not test any further for potential adversarial consequences but my child theme is working like a charm now.

#21 @audrasjb
18 months ago

In 55970:

Twenty Nineteen: Prevent a console error related to the main navigation on Firefox.

This changeset improves a JS conditional statement to fix a console error thrown by Firefox on event.target.matches.

Props kjellr, laurelfulford, audrasjb, dimijazz, ianbelanger, pbiron, poena, McAlyster, janpaulkleijn, swissspidy, mukesh27, rehanali.
Fixes #46474.
See #45903 (fixes one of the two issues of this ticket).

#22 @karmatosed
6 months ago

  • Owner audrasjb deleted

@audrasjb you are still owner of this. I presume that is due to the commit, so I am for now going to remove you to unlock this for the next stage in it's journey.

#23 @audrasjb
6 months ago

Yeah exactly, thanks Tammie.

Note: See TracTickets for help on using tickets.