WordPress.org

Make WordPress Core

#22307 closed defect (bug) (fixed)

Twenty Twelve: JavaScript error if navigation wrapper markup is removed from header

Reported by: sswells Owned by: lancewillett
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description (last modified by lancewillett)

I'm getting this error on a page that isn't using the header or nav. [Edit by lancewillett: This only occurs when the entire nav markup is removed.]

Error: TypeError: document.getElementById("site-navigation") is null
Source File: /wp-content/themes/twentytwelve/js/navigation.js?ver=1.0
Line: 7

Can a check be added into the Twenty Twelve theme to make sure #site-navigation exists on the page?

Attachments (2)

22307.diff (626 bytes) - added by lancewillett 18 months ago.
22307.2.diff (1.2 KB) - added by nacin 18 months ago.
Perhaps a nicer way to do this.

Download all attachments as: .zip

Change History (26)

comment:1 SergeyBiryukov18 months ago

  • Component changed from Themes to Bundled Theme
  • Milestone changed from Awaiting Review to 3.5

comment:2 DrewAPicture18 months ago

  • Summary changed from Javascript error in 2012 theme if not using nav to Twenty Twelve: Javascript error if not using nav

comment:3 lancewillett18 months ago

  • Keywords needs-testing added

Hi sswells -- could you please tell us:

  1. Your browser and OS version
  2. What version of Twenty Twelve

The navigation JS file already has a check in place, so it'd be good to try and repeat this with your exact setup.

comment:4 follow-up: sswells18 months ago

  1. I'm using FireFox on Mac OS X Lion
  2. Twenty Twelve is Version 1.0. These are the only two instances of 'site-navigation' I can find in the theme.

(twentytwelve/js/navigation.js)

var button = document.getElementById( 'site-navigation' ).getElementsByTagName( 'h3' )[0],

menu = document.getElementById( 'site-navigation' ).getElementsByTagName( 'ul' )[0];

Last edited 18 months ago by sswells (previous) (diff)

comment:5 lancewillett18 months ago

  • Description modified (diff)
  • Keywords needs-patch added; needs-testing removed
  • Summary changed from Twenty Twelve: Javascript error if not using nav to Twenty Twelve: JavaScript error if not removed navigation wrapper markup

I was able to repeat, but only when removing the entire navigation markup -- not just without a navigation menu since that case was already heavily tested and accounted for.

lancewillett18 months ago

comment:6 lancewillett18 months ago

  • Keywords has-patch added; needs-patch removed
  • Summary changed from Twenty Twelve: JavaScript error if not removed navigation wrapper markup to Twenty Twelve: JavaScript error if navigation wrapper markup is removed from header

comment:7 in reply to: ↑ 4 DrewAPicture18 months ago

@sswells: One of the most painless ways to avoid this kind of thing in the future to is to make sure you just dequeue any scripts from the parent theme you won't be using. In this case, you could use something like this in your child theme's functions file:

function dequeue_unneeded_scripts() {
	wp_dequeue_script( 'twentytwelve-navigation' );
}
add_action( 'wp_enqueue_scripts', 'dequeue_unneeded_scripts' );

This way, you're free to take out the navigation markup or leave it in, reuse it for a different type of nav, etc. And no JS errors :)

comment:8 obenland18 months ago

I agree with @DrewAPicture. When a child theme alters markup, shouldn't it also deal with the dependencies?

comment:9 sswells18 months ago

Yes, that would be good if it were a child theme, but it's a plugin. It would be best not to have to dequeue scripts for every theme that starts with Twenty Twelve as a base, as many new theme developers do. Is it possible to add this check?

comment:10 follow-up: obenland18 months ago

As far as I'm concerned, this includes plugins. You can't expect the theme to handle changes caused by a third party.

So if markup is removed that a script depends on, you should handle that dependency issue.

comment:11 in reply to: ↑ 10 ; follow-up: DrewAPicture18 months ago

Replying to obenland:

As far as I'm concerned, this includes plugins. You can't expect the theme to handle changes caused by a third party.

I think this is a very valid point and it underscores that in this case, the plugin is modifying theme behavior and not core behavior, which the theme shouldn't have to support, parent or not.

That said, it might be worth throwing a more helpful error of the _doing_it_wrong() variety instead just bailing as proposed in 22307.diff.

comment:12 in reply to: ↑ 11 ; follow-up: obenland18 months ago

Replying to DrewAPicture:

That said, it might be worth throwing a more helpful error of the _doing_it_wrong() variety instead just bailing as proposed in 22307.diff.

Is it really something that will happen that frequently, that we would have to build a workaround for it at all?

comment:13 in reply to: ↑ 12 ; follow-up: DrewAPicture18 months ago

Replying to obenland:

Is it really something that will happen that frequently, that we would have to build a workaround for it at all?

I don't think so. It would be an interesting discussion some other time though.

+1 to commit 22307.diff as is.

comment:14 in reply to: ↑ 13 ; follow-up: obenland18 months ago

Replying to DrewAPicture:

Is it really something that will happen that frequently, that we would have to build a workaround for it at all?

I don't think so. It would be an interesting discussion some other time though.

Why not now? :)


+1 to commit 22307.diff as is.

I really not sure, whether there is anything to patch

comment:15 in reply to: ↑ 14 ; follow-up: DrewAPicture18 months ago

  • Cc xoodrew@… added

Replying to obenland:

Why not now? :)

OK, I'll bite.

I'm strongly in agreement with the idea that themes should not have to support outside forces (such as plugins) modifying their markup. However, I do believe it is the job of the theme to fail gracefully, just as most well-coded functions do when absent all their requirements.

If an error is going to get tossed when the selector is no longer available, we might as well toss a helpful one, al a _doing_it_wrong(). OR, we can fail silently and show nothing at all, al a 22307.diff.

From a "best practices" point of view, the former has the ability to educate and improve. The latter allows everything to continue working together without rocking the boat. At this point in the cycle, I'm tempted to go with the second choice even though I think the first sets a better example.

comment:16 in reply to: ↑ 15 ; follow-up: obenland18 months ago

Replying to DrewAPicture:

OK, I'll bite.

Yay! :)


I do believe it is the job of the theme to fail gracefully, just as most well-coded functions do when absent all their requirements.

To an extend maybe. But where do you draw the line? A child theme replacing header.php with a file sans wp_head()? Changing the .entry-content classes to .post-content? It is impossible to cover everything child theme authors might throw at it. And I would include "removing the menu markup completely" with that.

I still believe this is not something that requires fixing.

comment:17 in reply to: ↑ 16 ; follow-up: DrewAPicture18 months ago

Replying to obenland:

Replying to DrewAPicture:

I do believe it is the job of the theme to fail gracefully, just as most well-coded functions do when absent all their requirements.

Changing the .entry-content classes to .post-content? It is impossible to cover everything child theme authors might throw at it. And I would include "removing the menu markup completely" with that.

IMHO, all of the edge cases are outside the point. The script should fail gracefully regardless of outside factors. Fault tolerance is not a new idea and as it is already widely applied in core and plugins, I don't see why themes shouldn't be held to the same standards.

comment:18 in reply to: ↑ 17 ; follow-up: obenland18 months ago

Replying to DrewAPicture:

IMHO, all of the edge cases are outside the point.

You consider removing the menu markup and neglecting to dequeue the script not an edge case?

comment:19 in reply to: ↑ 18 ; follow-up: DrewAPicture18 months ago

Replying to obenland:

Replying to DrewAPicture:

IMHO, all of the edge cases are outside the point.

You consider removing the menu markup and neglecting to dequeue the script not an edge case?

I consider graceful error-handling to be the point of the discussion.

It's always been my impression that in order to promote best-practice, you have to actually practice best-practice. I think this may be the first time I've ever seen an argument against graceful error-handling. My position hasn't changed and I'm starting to repeat myself: Either make the error helpful or add the null check and don't show it at all.

I've made my points, take from them what you will.

comment:20 in reply to: ↑ 19 obenland18 months ago

Replying to DrewAPicture:

I consider graceful error-handling to be the point of the discussion.

If this is error-handling for a theme feature (like: the assigned menu is empty but certain menu styles are still displayed #21562), I'm 100% with you!

To me the discussion is rather about whether a theme is supposed to account for all possible mishaps caused by child themes/plugins.
In this ticket, we try to correct an error that results from a third-party change to the theme which apparently wouldn't be an issue if the change would have been followed through (by dequeueing the script).

comment:21 lancewillett18 months ago

Good discussion so far.

My take: while this should be the duty of the plugin or child theme to do things right, I think it's worth adding the JS check to make the theme more bulletproof. It's not a lot of code and helps it fail gracefully.

nacin18 months ago

Perhaps a nicer way to do this.

comment:22 georgestephanis18 months ago

I was thinking along the same lines. We shouldn't assume things, and be okay with breaking if it happens to change. JS breaking on load from this can be a pretty big deal to some sites -- sliders may not work, layouts may not load, event listeners may not attach.

Reaching out and grabbing an element from the DOM and doing stuff to it in the same line, without making sure it exists first, is like ... in PHP, grabbing a global, and calling a method on it, without making sure the global is even an object or would have that method.

+1 for verifying what we're grabbing and not making assumptions -- especially as it's a pretty easy ifcheck to slip in.

comment:23 lancewillett18 months ago

22307.2.diff passes my tests, and is nicer in that it simplifies the logic a bit, thanks for the patch.

comment:24 lancewillett17 months ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from new to closed

In 22574:

Twenty Twelve: avoid JavaScript errors if navigation wrapper markup is removed from header. Fixes #22307.

Note: See TracTickets for help on using tickets.