Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#44526 closed defect (bug) (fixed)

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

Reported by: obenland's profile obenland Owned by: obenland's profile 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 5 years ago.

Download all attachments as: .zip

Change History (8)

@obenland
5 years ago

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


5 years ago

#2 @pento
5 years 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
5 years 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
5 years 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
5 years 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
5 years ago

In 43556:

Toolbar: Check if obj is set before using it.

Props pento.
See #44526.

#7 @johnbillion
5 years ago

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