#44526 closed defect (bug) (fixed)
Admin Bar: Fail gracefully when adding events to non-existent elements
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (8)
This ticket was mentioned in Slack in #meta by obenland. View the logs.
5 years ago
#3
@
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
@
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.
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 belowaddEvent()
tests that thet
parameter exists before it's used.