Opened 9 years ago
Last modified 4 years 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 needs-refresh |
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:
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:
And so on and on, after 15 times, 15 keydown
events attached, etc.:
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)
Change History (6)
#2
@
9 years ago
- Keywords has-patch added
First pass. Any thoughts, improvements and feedback welcome.
#3
@
9 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.
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: