WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#30770 closed defect (bug) (fixed)

Twenty Fifteen: Theme Review: Stylesheets and Scripts

Reported by: emiluzelac Owned by: lancewillett
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.1
Component: Bundled Theme Keywords: has-patch needs-testing
Focuses: Cc:

Description

Quick heads up guys:

  • No hard coding of scripts, styles and Favicons unless a browser workaround script. Everything should be enqueued.
<script>(function(){document.documentElement.className='js'})();</script>

Please see

The workaround noted above refers to e.g.

<script src="<?php echo esc_url( get_template_directory_uri() ); ?>/js/html5.js"></script>

Yep, it's small, but it still needs to be moved and added via functions.php :)

Thanks for the great theme!

Attachments (6)

enqueue_js_fix_script.patch (950 bytes) - added by chipbennett 6 years ago.
Moves js flash fix script to a callback, enqueued at wp_head, priority 0. Output should render exactly as current <head> output in header.php.
30770.diff (1.2 KB) - added by valendesigns 6 years ago.
30770.1.diff (1.2 KB) - added by peterwilsoncc 6 years ago.
Append JS class to HTML element
30770.2.diff (1.2 KB) - added by peterwilsoncc 6 years ago.
30770.3.diff (1.3 KB) - added by valendesigns 6 years ago.
30770.4.diff (1.3 KB) - added by valendesigns 6 years ago.

Download all attachments as: .zip

Change History (54)

#1 @obenland
6 years ago

  • Component changed from Themes to Bundled Theme
  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 4.2
  • Summary changed from Theme Review: Stylesheets and Scripts to Twenty Fifteen: Theme Review: Stylesheets and Scripts

#2 in reply to: ↑ description @iamtakashi
6 years ago

Replying to emiluzelac:

  • No hard coding of scripts, styles and Favicons unless a browser workaround script. Everything should be enqueued.
<script>(function(){document.documentElement.className='js'})();</script>

See this discussion about this. #30056

The workaround noted above refers to e.g.

<script src="<?php echo esc_url( get_template_directory_uri() ); ?>/js/html5.js"></script>

See this ticket. #16024

How do you approach to these issues?

Last edited 6 years ago by iamtakashi (previous) (diff)

#3 follow-up: @philiparthurmoore
6 years ago

I'm not convinced that moving this into an enqueue is better. Better for guidelines? Maybe. But the flashing is very annoying and very real. What reason other than "doesn't meet theme review guidelines" is there to make this change?

#4 in reply to: ↑ 3 @iamtakashi
6 years ago

Replying to philiparthurmoore:

I'm not convinced that moving this into an enqueue is better. Better for guidelines? Maybe. But the flashing is very annoying and very real. What reason other than "doesn't meet theme review guidelines" is there to make this change?

I feel the same way. I don't see a good reason to not making these exceptions until we have a good solution for each issue.

#5 @ocean90
6 years ago

  • Keywords needs-patch good-first-bug removed
  • Milestone 4.2 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

That's all fine as it is. See also the linked tickets in comment:2.

#6 follow-up: @emiluzelac
6 years ago

Help me understand please: with this in header, theme does not really meet the guidelines, how is that not good reason?

#7 in reply to: ↑ 6 @philiparthurmoore
6 years ago

Replying to emiluzelac:

Help me understand please: with this in header, theme does not really meet the guidelines, how is that not good reason?

Another way to put it would be that if there were no guidelines currently in place what would be the best way about this? I think that Twenty Fifteen in its current state does this correctly.

#8 follow-ups: @emiluzelac
6 years ago

Not really sure what is the best way, you can see: https://themes.trac.wordpress.org/ticket/20079#comment:3 for example.

This is not a question how to improve or do this better, guidelines should apply to all themes equally and if it does not, it will came back on us the reviewers :)

#9 in reply to: ↑ 8 @cais
6 years ago

Replying to emiluzelac:

Not really sure what is the best way, you can see: https://themes.trac.wordpress.org/ticket/20079#comment:3 for example.

This is not a question how to improve or do this better, guidelines should apply to all themes equally and if it does not, it will came back on us the reviewers :)

I would have to agree with this, especially with the reference to another "Automattic" theme using the same approach being happy to make the changes to the code that reflects current theme review team requirements and recommendations.

It is also very important that bundled themes follow these requirements and recommendations as many theme developers, new and old, will often use the bundled theme's code as a basis for their own works applying the same methods and constructs with the belief, "If Twenty X uses it, then it must be perfectly fine for me to use it too"

I can accept (although not like at all) a bundled theme being pushed through the queues ahead of all others (it is a WordPress repository after all) but I also believe it is still vitally important for the theme review team and the bundled themes authors to work together to find the best go forward approach; and, ignoring a guideline because you don't think its appropriate is not the best way of doing it.

Exceptions are fine, and there will always be considerations for them ... but wouldn't it be better to discuss them before rather than after the fact?

#10 in reply to: ↑ 8 @philiparthurmoore
6 years ago

Replying to emiluzelac:

Not really sure what is the best way, you can see: https://themes.trac.wordpress.org/ticket/20079#comment:3 for example.

This is not a question how to improve or do this better, guidelines should apply to all themes equally and if it does not, it will came back on us the reviewers :)

My apologies. I thought this was a code discussion, not a theme review discussion. (Seriously, no sarcasm intended.) I have no idea what the submission process for default themes is like so I cannot address that. My points above were specifically about the code.

If this is causing an issue with review guidelines, why not change them? What's wrong with adjusting the guidelines? Because it was done one way 4 months ago doesn't mean that it has to be done that way now. Jankiness and flashing does absolutely occur when the JS detection happens too late; shouldn't we be trying to address that performance issue rather than the issue of whether or not a theme is in line with review guidelines?

Last edited 6 years ago by philiparthurmoore (previous) (diff)

#11 @iamtakashi
6 years ago

We just need to change the guideline then.

It's because If we think about performance of WordPress themes, I believe how Twenty Fifteen does is the best way to address these issues.

But if somebody has better solutions that meet the current guideline, let us know. Otherwise, we just need to change the guideline.

#12 @emiluzelac
6 years ago

Do you guys have any idea what TRT even does and how much we do to make this work for all?

We can't just change the guideline because of one theme, that is not what team does.

Imagine we adjust the guidelines to meet the theme codes, where would that lead us to?

This ticket was mentioned in Slack in #core-themes by iamtakashi. View the logs.


6 years ago

#14 @iamtakashi
6 years ago

As Drew pointed in Slack, these would fall into the category of "workaround", so we might not need to adjust the guideline after all.

I've suggested to change the guideline because I strongly believe the guideline should make WordPress make better, not sacrifice performance and compatibility to old browsers.

#15 follow-up: @greenshady
6 years ago

I think some people may be misunderstanding what TRT is asking. I'll explain it in terms of code.

This line placed in header.php:

<script>(function(){document.documentElement.className='js'})();</script>

Should be moved to a callback hooked to wp_head(). I'd actually argue both should be moved. I'm not sure how that would mess up anything with the theme. It doesn't have anything to do with enqueueing the script.

One of the big reasons for this particular guideline to me is that child themes (or plugins) can't remove the JS for whatever reason without overwriting the entire header.php.

I do recognize that JS should be added when it's needed, not necessarily enqueued. Hardcoding JS right in the middle of a page is sometimes valid. But, for things that are being output directly into the header or the footer, we should try to stick to the core header and footer hooks.

#16 @emiluzelac
6 years ago

@iamtakashi guidelines accepts only one script in the header.php and that is HTML5

Rough example to make this work: (disregard the priority)

function twentyfifteen_js_class () {
    echo '<script>(function(){document.documentElement.className='js'})();</script>'. "\n";
}
add_action( 'wp_head', twentyfifteen_js_class', 1 );
Last edited 6 years ago by emiluzelac (previous) (diff)

#17 @iamtakashi
6 years ago

@emiluzelac, Thanks for the clarification about html5.js.

Also thanks for your suggestion about another. Could you make a patch with your suggestion so that it's easy for everyone to test the performance?

#18 @emiluzelac
6 years ago

@iamtakashi You got me, I've never created or submitted a patch before :( I can try.

#20 @iamtakashi
6 years ago

@emiluzelac, no worries! You can find more info about creating and submitting patches here:

https://make.wordpress.org/core/handbook/working-with-patches/patches-with-command-line/
https://make.wordpress.org/core/handbook/working-with-trac/submitting-a-patch/

As the first link states, make sure you create a patch from the root. This makes a lot easier for people to test.

Patches should be created from the root directory of your WordPress SVN install.

Last edited 6 years ago by iamtakashi (previous) (diff)

#21 in reply to: ↑ 15 @obenland
6 years ago

Replying to greenshady:

This line placed in header.php:

<script>(function(){document.documentElement.className='js'})();</script>

Should be moved to a callback hooked to wp_head().

I think that's reasonable. It will not change the way it works now and child themes will have it easier to modify it, as Justin mentioned.

Last edited 6 years ago by obenland (previous) (diff)

#22 @chipbennett
6 years ago

I'll have a patch submitted this evening, if someone doesn't beat me to it.

#23 @emiluzelac
6 years ago

Chip, I was thinking that this would be good for me, but no worries, as long as we get things going. We can wait till this evening for sure :)

@chipbennett
6 years ago

Moves js flash fix script to a callback, enqueued at wp_head, priority 0. Output should render exactly as current <head> output in header.php.

This ticket was mentioned in Slack in #themereview by jcastaneda. View the logs.


6 years ago

#25 @philiparthurmoore
6 years ago

Chip, this is very minor but what are your thoughts on changing Enqueue script to fix JS flash to something more along the lines of JavaScript Feature Detection or JavaScript Detection or something a bit more general? I'm not married to this at all; would love your thoughts.

#26 @chipbennett
6 years ago

I'm open to any wordsmithing deemed appropriate. I'm likewise not married to the phpDoc text, callback name, etc.

#27 @iamtakashi
6 years ago

Thanks for the patch, Chip.

Like mentioned, the naming and PHPDoc could be better but the patch is working well in my testing. And a patch should be generated from the root :)

#28 @philiparthurmoore
6 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

I have absolutely no idea what I was going on about two weeks ago. I just ran a bunch of no JS tests with a different theme that I'm working on and using wp_enqueue_scripts was totally fine. The only real difference between wp_head and wp_enqueue_scripts was that the no JS detection came under the page title when wp_enqueue_scripts was used.

Chip's intent was good I think, given the discussion, but I tend to think that we could even use wp_enqueue_scripts with Chip's approach just fine. So this:

add_action( 'wp_head', 'twentyfifteen_enqueue_js_fix', 0 );

Becomes this:

add_action( 'wp_enqueue_scripts', 'twentyfifteen_enqueue_js_fix' );

If I could go back in time I'd probably kick myself, but here we are. Sorry for the stubbornness earlier.

This ticket was mentioned in Slack in #core-themes by philip. View the logs.


6 years ago

@valendesigns
6 years ago

#30 follow-up: @valendesigns
6 years ago

  • Keywords has-patch added

Refreshed the patch to be created from the root. Also did a little tidying up, renaming, and added additional docs. I know some of you don't want to move this out of the header, but I don't see why when it's location in the DOM is not changing. What is the downside here?

#31 in reply to: ↑ 30 ; follow-up: @philiparthurmoore
6 years ago

Replying to valendesigns:

Refreshed the patch to be created from the root. Also did a little tidying up, renaming, and added additional docs. I know some of you don't want to move this out of the header, but I don't see why when it's location in the DOM is not changing. What is the downside here?

This definitely needs to be in the header, not the footer. The entire issue here is to prevent flashing of a non-JS menu before the JS detection kicks in.

#32 in reply to: ↑ 31 ; follow-up: @valendesigns
6 years ago

Replying to philiparthurmoore:

This definitely needs to be in the header, not the footer. The entire issue here is to prevent flashing of a non-JS menu before the JS detection kicks in.

Sorry if you misunderstood me, but I didn't mention anything about the footer. I'm saying I don't see the problem with moving into a filter if it's stays in the exact same place in the DOM. Meets the theme guidelines and works as expected.

#33 in reply to: ↑ 32 @philiparthurmoore
6 years ago

Replying to valendesigns:

Replying to philiparthurmoore:

This definitely needs to be in the header, not the footer. The entire issue here is to prevent flashing of a non-JS menu before the JS detection kicks in.

Sorry if you misunderstood me, but I didn't mention anything about the footer. I'm saying I don't see the problem with moving into a filter if it's stays in the exact same place in the DOM. Meets the theme guidelines and works as expected.

"I know some of you don't want to move this out of the header..."

I read that wrong. :)

#34 @valendesigns
6 years ago

No worries. I see the confusion, should have wrote header.php instead.

Cheers,
Derek

#35 @lancewillett
6 years ago

  • Milestone set to 4.2

@peterwilsoncc
6 years ago

Append JS class to HTML element

#36 follow-up: @peterwilsoncc
6 years ago

In 30770.1.diff, I've modified @valendesigns' code to append a js class to the HTML element rather than replace all classes on the HTML element.

This allows child themes to add classes to the HTML element either directly or by using modernizr or similar.

#37 in reply to: ↑ 36 ; follow-up: @iamtakashi
6 years ago

Replying to peterwilsoncc:

We do need to replace no-js class with the current CSS:

.no-js .main-navigation ul ul {
	display: block;
}

#38 in reply to: ↑ 37 ; follow-up: @peterwilsoncc
6 years ago

Replying to iamtakashi:

We do need to replace no-js class with the current CSS:

Uploaded 30770.2.diff with a modified version of the Paul Irish technique.

#39 in reply to: ↑ 38 ; follow-up: @valendesigns
6 years ago

Replying to peterwilsoncc:

Replying to iamtakashi:

We do need to replace no-js class with the current CSS:

Uploaded 30770.2.diff with a modified version of the Paul Irish technique.

Thank you for adding an updated patch. However, I think 30770.diff is still the correct course of action here. The code is being loaded before all other scripts and in the case of child themes or modernizr they would still be able to add classes to the html element by overriding the modification in a separate script, since they are called after wp_head via the wp_enqueue_scripts action. Please correct me if I'm wrong, but I don't see a reason to change the JavaScript to make it any more compatible. The issue here (IMO) is how this particular piece of code is being loaded into header.php and not how it changes no-js to js in the html element.

#40 in reply to: ↑ 39 ; follow-up: @peterwilsoncc
6 years ago

Replying to valendesigns:

Thank you for adding an updated patch. However, I think 30770.diff is still the correct course of action here. The code is being loaded before all other scripts and in the case of child themes or modernizr they would still be able to add classes to the html element by overriding the modification in a separate script, since they are called after wp_head via the wp_enqueue_scripts action. Please correct me if I'm wrong, but I don't see a reason to change the JavaScript to make it any more compatible. The issue here (IMO) is how this particular piece of code is being loaded into header.php and not how it changes no-js to js in the html element.

That's correct, any enqueued javascript runs after the code inserted into wp_head

My aim is to protect against two things:

  • child themers adding a class to the HTML element directly into a header.php override.
  • child themers doing_it_wrong by hard coding modernizr or similar into a header.php override.

The former being more important than the latter.

In terms of opinionated coding, the purpose is to replace no-js with js so that's what I think it should do.

Anyway, that was my poorly explained logic - I'll bow out now.

@valendesigns
6 years ago

#41 in reply to: ↑ 40 @valendesigns
6 years ago

Replying to peterwilsoncc:

My aim is to protect against two things:

  • child themers adding a class to the HTML element directly into a header.php override.
  • child themers doing_it_wrong by hard coding modernizr or similar into a header.php override.

If we're trying to fix those use cases in this ticket, I think the JavaScript should be changed to the code below. I've added a new patch.

document.documentElement.className = document.documentElement.className.replace('no-js','js')

#42 @peterwilsoncc
6 years ago

That works too.

You will need to use the regexp /\bno-js\b/ to ensure it only replaces the no-js class. ATM you'll catch anything containing the sub-string, say casino-jschiffer.

#43 @iandstewart
6 years ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #themereview by karmatosed. View the logs.


6 years ago

@valendesigns
6 years ago

#45 @valendesigns
6 years ago

Added a refreshed patch.

#47 @iamtakashi
6 years ago

I've tested 30770.4.diff. The flash doesn't occur and an additional CSS class to the html tag in child themes stays intact.

#48 @lancewillett
6 years ago

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

In 31184:

Twenty Fifteen: move js and no-js class name functionality out of header template and into a wp_head hook in functions.php file so that the JavaScript functionality isn't hard-coded into a template file.

Fixes #30770, props chipbennett, valendesigns, and peterwilsoncc.

Note: See TracTickets for help on using tickets.