WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 4 months ago

#38995 closed defect (bug) (fixed)

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

Reported by: joemcgill Owned by: joemcgill
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Bundled Theme Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

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.

Attachments (11)

Screen Shot 2016-11-30 at 11.15.50 AM.png (32.0 KB) - added by joemcgill 4 months ago.
Screenshot of mobile view with header video but no header image.
38995.patch (14.7 KB) - added by laurelfulford 4 months ago.
38995.1.patch (16.0 KB) - added by joemcgill 4 months ago.
38995.2.patch (16.4 KB) - added by laurelfulford 4 months ago.
sticky-header-2017.gif (3.0 MB) - added by laurelfulford 4 months ago.
Example of menu sticking early when there's a video, but no header image, on screen widths 769px - 899px.
38995.3.patch (15.5 KB) - added by joemcgill 4 months ago.
38995.4.patch (15.3 KB) - added by laurelfulford 4 months ago.
38995.5.patch (15.3 KB) - added by laurelfulford 4 months ago.
38995.6.patch (15.6 KB) - added by joemcgill 4 months ago.
38995.7.patch (15.7 KB) - added by joemcgill 4 months ago.
38995.diff (1.2 KB) - added by joemcgill 4 months ago.

Change History (35)

@joemcgill
4 months ago

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

#1 @joemcgill
4 months ago

  • Owner set to joemcgill
  • Status changed from new to accepted

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.

This ticket was mentioned in Slack in #core by helen. View the logs.


4 months ago

#3 @laurelfulford
4 months ago

  • Keywords has-patch added; needs-patch removed

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?

#4 @Anariel Design
4 months ago

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.

Last edited 4 months ago by Anariel Design (previous) (diff)

#5 @davidakennedy
4 months ago

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?

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


4 months ago

@joemcgill
4 months ago

#7 @joemcgill
4 months ago

  • Keywords needs-testing added

@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.

#8 @laurelfulford
4 months ago

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.
  1. 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.
  1. Made similar updates to .has-header-video class in custom-header.php from fix for #38980.
  1. 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!

Last edited 4 months ago by laurelfulford (previous) (diff)

@laurelfulford
4 months ago

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

@joemcgill
4 months ago

#9 @joemcgill
4 months ago

  • Keywords commit added; needs-testing removed

@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.

#10 @laurelfulford
4 months ago

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!

Last edited 4 months ago by laurelfulford (previous) (diff)

#11 @laurelfulford
4 months ago

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

#12 @helen
4 months ago

  • Keywords commit removed

@joemcgill
4 months ago

#13 @joemcgill
4 months ago

  • Keywords dev-feedback added

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.

#14 @laurelfulford
4 months ago

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!

#15 @joemcgill
4 months ago

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

@joemcgill
4 months ago

#16 @joemcgill
4 months ago

  • Keywords commit added

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' ) ) ) { ... }

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


4 months ago

#18 @laurelfulford
4 months ago

Thanks @joemcgill - this looks good to me!

#19 @helen
4 months ago

  • Keywords dev-reviewed added; dev-feedback removed

#20 @joemcgill
4 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#21 @joemcgill
4 months ago

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.

#22 @laurelfulford
4 months ago

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!

Last edited 4 months ago by laurelfulford (previous) (diff)

@joemcgill
4 months ago

#23 @joemcgill
4 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#24 @joemcgill
4 months ago

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

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.

Note: See TracTickets for help on using tickets.