WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 6 months ago

Last modified 3 months ago

#44526 closed defect (bug) (fixed)

Admin Bar: Fail gracefully when adding events to non-existent elements

Reported by: obenland Owned by: obenland
Milestone: 5.1 Priority: low
Severity: minor Version: 3.1
Component: Toolbar Keywords: commit
Focuses: javascript Cc:

Description

When testing if addEventListener or attachEvent are available for an object it just checks for it to be truthy (source), which works if it is available, if it's not it creates a type error:

TypeError: Cannot read property 'addEventListener' of null

This can occur in circumstances where jQuery is not available and plugin/themes remove nodes that admin-bar.js expects to be available. To reproduce install & activate Logged Out Admin Bar and visit your test site with a theme enabled that does not use jQuery, like Responsive Kubrick.

While it's technically not a core bug, let's make sure it fails a little more gracefully when it does encounter these circumstances.

Attachments (1)

44526.diff (833 bytes) - added by obenland 7 months ago.

Download all attachments as: .zip

Change History (8)

@obenland
7 months ago

This ticket was mentioned in Slack in #meta by obenland. View the logs.


7 months ago

#2 @pento
6 months ago

Would it be better to test that obj is the type of thing we want, rather than testing the function? It took me a while to understand what the problem was and why 44526.diff fixed it.

For example, the addHoverClass() function a little further below addEvent() tests that the t parameter exists before it's used.

#3 @obenland
6 months ago

I don't think I necessarily have a preference either way.

For argument's sake, in addEvent() we don't care what obj is as long as we're able to attach events to it.

Do you feel like testing for obj first would make the code clearer?

#4 @pento
6 months ago

  • Keywords commit added
  • Owner set to obenland
  • Status changed from new to assigned

Yeah, I'm not really fussed either way. Let's go with 44526.diff.

#5 @obenland
6 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 43517:

Toolbar: Fail gracefully when adding events to non-existent elements

Avoids a type error when obj is not set.

Fixes #44526.

#6 @obenland
6 months ago

In 43556:

Toolbar: Check if obj is set before using it.

Props pento.
See #44526.

#7 @johnbillion
3 months ago

  • Milestone changed from 5.0 to 5.1
Note: See TracTickets for help on using tickets.