WordPress.org

Make WordPress Core

Opened 2 weeks ago

Closed 3 days 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.0 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 13 days ago.

Download all attachments as: .zip

Change History (6)

@obenland
13 days ago

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


11 days ago

#2 @pento
6 days 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 days 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
4 days 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
3 days 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.

Note: See TracTickets for help on using tickets.