WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 15 months ago

#33267 new defect (bug)

Customizer Theme details: too many events

Reported by: afercia Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.2
Component: Customize Keywords: has-patch
Focuses: javascript Cc:

Description

This happen just in the Customizer Theme details modal, not in the Themes screen. To reproduce, go in the Customizer and press the "Change" button next to the Active Theme title. Choose one of the themes in the left sidebar and press the "Theme Details" button.
When the Theme Details modal opens the first time, there's just one keydown event attached to the theme overlay:

https://cldup.com/66ZTntnIuP.png

Now, navigate forth and back clicking on the next and previous arrows in the top left to see more themes details. After 10 times, there will be 10 keydown events attached:

https://cldup.com/kNtc6D9dsy.png

And so on and on, after 15 times, 15 keydown events attached, etc.:

https://cldup.com/5tgfv95ieb.png

What's happening? Every time next and previous are clicked, api.ThemesSection.showDetails() will replace the html in the modal to show the new details and it will also call each time api.ThemesSection.containFocus().

Then, containFocus() will attach a new keydown event each time it runs :( and it will also run each time jQuery UI :tabbable. At the end, after you navigate a few times in the Themes Details and then you tab inside the modal, it's very easy to run :tabbable literally hundreds of times for each "Tab" key press.

Notice all this doesn't happen in the Themes screen because the div where events are attached to gets completely replaced by jQuery html() which removes also any data and events. This is not the case in the Customizer where events are attached to the theme overlay div which doesn't get replaced.

To my understanding this should be refactored a bit:

  • attach the keydown event just once, in the attachEvents() function
  • split containFocus() in two separate functions:
    • a simplified containFocus() to just check the event keys and move focus
    • a new getTabbables() to get the tabbable elements just once for each theme details.
  • store the tabbables in a variable to be updated with getTabbables()

Attachments (1)

33267.patch (3.1 KB) - added by afercia 2 years ago.

Download all attachments as: .zip

Change History (5)

#1 @afercia
2 years ago

Dumping $._data( el[0], 'events' ) in the console to double check the events attached to the theme overlay, after 10 clicks on the Next button:

https://cldup.com/Kq54xIiIGK.jpg

@afercia
2 years ago

#2 @afercia
2 years ago

  • Keywords has-patch added

First pass. Any thoughts, improvements and feedback welcome.

#3 @celloexpressions
2 years ago

I'm guessing this also happens on themes.php, as this code was taken directly from there. I believe this code was written in a last-minute scramble to make the themes redesign in 3.8 keyboard accessible, so improvements are probably much-needed and I wouldn't be surprised if there were additional issues like this.

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


15 months ago

Note: See TracTickets for help on using tickets.