Twenty Seventeen: Custom headers incorrect on mobile when no image is set.

[joemcgill]

On the front page of Twenty Seventeen, when a header video is set without also setting a header image, the header is still styled as if an image is present on small screens when the video isn't loaded (screenshot to follow).

Instead, the header should be styled as if no custom header is available.

[joemcgill]

Screenshot of mobile view with header video but no header image.

[joemcgill]

Copying from ​Slack for posterity:

Re: Should we only allow video headers if an image is also set?

...there are valid cases where you want the video header on the homepage but no image headers on other pages that make use of custom headers. Twenty Seventeen currently allows that option if a person wants to style their site that way. Mandating that videos must be accompanied by an image makes that setup impossible.

Re: Determining when to style.

The tricky part is that we don’t know if the video is going to load until the JS on the front end actually runs since the video could fail to load for several reasons: screen is smaller than our minimum requirements, URL is broken, JS breaks (or browser doesn’t support). One way we could approach fixing in this case is that a class like has_header_video should be added to the body once the video is actually loaded, and the theme should use the existence of that class to conditionally style the header.

On this second point, it does look like Twenty Seventeen is styling the header in this case using the .has-header-image class, which is added by the theme in /inc/template-functions.php and is referenced in the site styles below:

.has-header-image.twentyseventeen-front-page .site-branding,
.has-header-image.home.blog .site-branding {
	display: table-cell;
	height: 100%;
	vertical-align: bottom;
}

We should not add the .has-header-image class unless a header image is set, and we should handle .has-header-video separately, or improve the logic so it handles the case where a video is present without an image and use a combined .has-custom-header class.

[laurelfulford]

38995.patch​ adds a class .header-video-loaded via JavaScript when wp-custom-header-video-loaded is fired; this makes sure it's only added when the video is visible.

.has-header-image has been updated so it's only added when a header image is uploaded, and the styles have been updated to reflect these changes.

I'd love to get some thoughts on another potential change to this section: in the theme we have the file template-parts/header-image.php, and in it the class .custom-header-image, which is on a div that holds both the header image and video. Both are left over from before video header support was added, and have been bugging me since they're no longer accurate.

Would it be beneficial to update these to 'header-media.php' and .custom-header-media to prevent confusion? Or better to leave as is at this stage?

[Anariel Design]

Hey,

thanks for opening this ticket, I noticed the bug yesterday when I was working on a new theme where I added this feature. What I used to resolve this one, left everything like it was in Twenty Seventeen and added a class when header image is empty for the .custom-header-image div like this:

<div class="custom-header-image <?php echo esc_attr( liber_additional_class() ); ?>">
	<?php the_custom_header_markup(); ?>
</div>

than I added inside the extras.php file this:

function liber_additional_class() {
	
	if ( empty(has_header_image()) ) {
		$additional_class = 'has-header-image';
	}

	return esc_attr( $additional_class );
}

and line of CSS:

@media only screen and (max-width: 900px)  {
	.custom-header-image.has-header-image {
		height: auto;
	}
}

This way I changed the height just for the smaller screens by adding extra class to the .custom-header-image when header image is empty.

[davidakennedy]

Nice patch, @laurelfulford! And thanks @anariel-design for the additional report and thoughts!

I tested this patch and it works well. We talked about going this route in Slack yesterday, and even though it's a lot of small changes, it makes the most sense because the theme needs to account for its appearance when there is/isn't an image/video and when there is a video but no image. It's best to separate everything out as Laurel has done to make everything easier to understand and work with. This patch looks good to go to me.

I'm good with making the changes to adjust the file name, containerdiv name and class .custom-header-image. It makes more sense in a world of both header images and header videos. But this wouldn't be absolutely necessary... If we want to do it, it needs to be done now because that can't be changed in future releases. I'm curious what others think?

[joemcgill]

@laurelfulford, your patch looked very good and approaches this exactly how I was thinking. In 38995.1.patch​ I've made a few adjustments on your patch, namely:

1. Change the .custom-header-image class on the container div to .custom-header-media, as you suggested.
2. Uses the class name .has-header-video rather than .header-video-loaded when a video loads, to match the .has-header-image (merely an aesthetic change). We also don't need to include a front page check on the event listener since the front page check will be handled by the custom header video handler.
3. There are several places where you were adding the .header-video-loaded class when the element being styled was an img. I don't think we need to add a style for the video class in those cases, because even when the image is used as a fallback, the has-header-image class will still be applied.
4. I didn't see where your patch handled the ie8.css file so I added them. Perhaps we don't need to handle video header styles for IE8 at all however?
5. I also noticed that your patch broke the image headers on internal pages by checking for is_front_page() before adding the has-header-image() in twentyseventeen_body_classes().

Would appreciate testing and further feedback. Particularly on the customize previews changes because I'm not positive I caught everything.

[laurelfulford]

Thanks for this, @joemcgill!

These changes look good overall (and fixed a couple ugly issues I introduced - thanks!!)

I made a couple tweaks in 38995.2.patch​:

1. In a couple cases, I re-added styles for img in .has-header-video because the images are inside of a .custom-logo-link container and aren’t custom header images.

2. I had totally missed updating the ie8.css styles - thanks for catching that! I tested IE8, and you’re right - it doesn’t look like we need video styles here at all. The video doesn’t appear regardless if it’s mp4 or from YouTube. I’ve updated these styles to remove video references all together.

3. Made similar updates to .has-header-video class in custom-header.php from fix for #38980.

4. I re-added a change to adjustScrollClass() in global.js from 38995.patch​:
   
   Originally we were checking for the presence of .custom-header-image (now .custom-header-media) to decide when to make the menu ‘sticky’. But there’s an edge-case issue: it happens when a site has a video but no header image, and you view the site on a screen size just above the tablet breakpoint (769px - 899px wide). The video isn’t visible at this screen size, but the JavaScript thinks it is because .custom-header-media is there, causing it to miscalculate where the menu should be stuck.
   
   I’ve switched it to check for the .has-header-video or .has-header-image classes instead.
   
   I didn’t comment on this specifically with the last patch, so it wasn’t clear why I had made that change - sorry about that!

Further testing is appreciated, to make sure I didn’t miss anything!

[laurelfulford]

Example of menu sticking early when there's a video, but no header image, on screen widths 769px - 899px.

[joemcgill]

@laurelfulford Thanks for these changes. I only noticed two small things which I've addressed in 38995.3.patch​.

a) I removed the logic I had added in customize-previews.js that adds the .has-video-header class to match what you had originally done in 38995.patch​.
b) Looks like 38995.2.patch​ missed changing the classname of the media container div to .custom-header-media.

Let me know if you have any other feedback, otherwise I think this is good to commit.

[laurelfulford]

Thanks @joemcgill!

Looks like 38995.2.patch​ missed changing the classname of the media container div to .custom-header-media.

Oh weird! It looks like it's in the patch. I've been getting a 'Hunk failed' error from that file on the other patches - I wonder if that's why it didn't carry over?

Things look good overall. I found an issue while testing that's unrelated to these changes - the header fully disappears for a few seconds in the preview if you do the following:

1. Remove header image and header video.
2. Save and publish.
3. Refresh the customizer.
4. Add a header image.

The whole header disappears in the preview, and then appears with the image.

It looks to be related to the fix from #38927. The good news is this patch includes some JavaScript that fixes the same thing, outlined in item four here. The check for has_custom_header() in header-image.php is no longer needed, and removing it fixes the disappearing header.

I added this in 38995.4.patch​. When testing this, can you please check that the custom-header-media class made it over intact? Thanks!

[laurelfulford]

I belatedly realized I left two testing console.log lines in 38995.4.patch​; the updated 38995.5.patch​ removes them - sorry about that!

[joemcgill]

Good catch @laurelfulford. I had trouble applying your patch as well, so I've refreshed it in 38995.6.patch​. This should match 38995.5.patch​, so please double check that I haven't missed anything.

[laurelfulford]

Nice - thanks @joemcgill!

The only thing that didn't seem to carry over was the change I made in 38995.5.patch​ - in header-image.php, this bit:

<?php if ( has_custom_header() ) : ?>
	<div class="custom-header-media">
		<?php the_custom_header_markup(); ?>
	</div>
<?php endif; ?>

... should just be:

<div class="custom-header-media">
	<?php the_custom_header_markup(); ?>
</div>

Everything else looks to be there!

[joemcgill]

Ah, my fault @laurelfulford. I mistakenly thought the opposite was the change.

[joemcgill]

38995.7.patch​ updates 38995.6.patch​ as mentioned in Laurel's comment. Worth noting that this reverts the change made in [39392] but doesn't reintroduce the bug from #38927 because of the addition of a check for .has-header-image or .has-header-video in global.js, i.e.:

if ( isFrontPage && ( $body.hasClass( 'has-header-image' ) || $body.hasClass( 'has-header-video' ) ) ) { ... }

[laurelfulford]

Thanks @joemcgill - this looks good to me!

[joemcgill]

In 39413:

Twenty Seventeen: Better handling of custom headers when no image is set.

This ensures that a standard header is shown on the front page whenever
a header video is set without a header image if the video doesn't load,
e.g., on mobile sizes or if the JS doesn't fire.

This adds a new class, .has-header-video that is added whenever the
wp-custom-header-video-loaded event is fired, which is then used to style
the custom headers along with .has-header-image whenever a header image
is available. This also changes the class name on the custom header media
wrapping div from .custom-header-image to .custom-header-media.

Props laurelfulford, joemcgill.
Fixes #38995.

[joemcgill]

In 39414:

Twenty Seventeen: Better handling of custom headers when no image is set.

This ensures that a standard header is shown on the front page whenever
a header video is set without a header image if the video doesn't load,
e.g., on mobile sizes or if the JS doesn't fire.

This adds a new class, .has-header-video that is added whenever the
wp-custom-header-video-loaded event is fired, which is then used to style
the custom headers along with .has-header-image whenever a header image
is available. This also changes the class name on the custom header media
wrapping div from .custom-header-image to .custom-header-media.

Props laurelfulford, joemcgill.
Fixes #38995 for 4.7.

[laurelfulford]

Playing around with this a bit more, I noticed a change from 38995.2.patch​ to custom-header.php was lost in the back and forth.

The styles for the header text colour need to have versions with the .has-header-video class explicitly set in front of them, like:

.site-title a,
.colors-dark .site-title a,
.colors-custom .site-title a,
body.has-header-image .site-title a,
body.has-header-video .site-title a,
body.has-header-image.colors-dark .site-title a,
body.has-header-video.colors-dark .site-title a,
body.has-header-image.colors-custom .site-title a,
body.has-header-video.colors-custom .site-title a,
.site-description,
.colors-dark .site-description,
.colors-custom .site-description,
body.has-header-image .site-description,
body.has-header-video .site-description,
body.has-header-image.colors-dark .site-description,
body.has-header-video.colors-dark .site-description,
body.has-header-image.colors-custom .site-description,
body.has-header-video.colors-custom .site-description {
	color: #<?php echo esc_attr( $header_text_color ); ?>;
}

I'm sorry I didn't spot this sooner!

[joemcgill]

In 39416:

Twenty Seventeen: Add .has-header-video styles for custom color schemes.

Following [39413], this adds .has-header-video style definitions in
/inc/custom-header.php to support custom color schemes.

Props laurelfulford.
Fixes #38995 for 4.7.