WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#39035 closed defect (bug) (fixed)

Twenty Seventeen: Header media (images and video) appear zoomed in

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

Description

This ticket came out of a discussion with @melchoyce:

The video and header image don't scale as well as they could. They fit the available header space, which is intended, but the current styles also zoom them in noticeably. This results in a lot of unnecessary cropping, and makes it seem like the recommended image sizes are wrong.

The header image is displayed with an img tag, and the video a video or iframe. Right now, these are positioned via CSS with the following:

.has-header-image .custom-header-media img,
.has-header-video .custom-header-media video,
.has-header-video .custom-header-media iframe {
	position: fixed;
	height: auto;
	left: 50%;
	max-width: 1000%;
	min-height: 100%;
	min-width: 100%;
	min-width: 100vw; /* vw prevents 1px gap on left that 100% has */
	width: auto;
	top: 50%;
	padding-bottom: 1px; /* Prevent header from extending beyond the footer */
	-ms-transform: translateX(-50%) translateY(-50%);
	-moz-transform: translateX(-50%) translateY(-50%);
	-webkit-transform: translateX(-50%) translateY(-50%);
	transform: translateX(-50%) translateY(-50%);
}

Originally, when the theme only had a header image, it was set up as a background image and the scaling was done with background-size: cover and background-position: center center. This scaled the image to fit the available space, but without the extra zoom.

I've included screenshots of the default image with original styles and the current styles, to show the difference.

Current styles:

https://cldup.com/g_PpaoIGPn.thumb.jpg

Original background-image styles:

https://cldup.com/SXDysUo40c-3000x3000.png

It looks like we can get close to the original scaling using object-fit: cover... which unfortunately, is not currently supported by any version of IE, even IE Edge.

Example of styles:

.has-header-image .custom-header-media img,
.has-header-video .custom-header-media video,
.has-header-video .custom-header-media iframe {
	position: fixed;
	height: 100%;
	left: 0;
	object-fit: cover;
	top: 0;
	width: 100%;
}

For comparison, here's the header image scaled/positioned with object-fit: cover:

https://cldup.com/im_SeBxDqN.png

For the video, here's two as-close-as-I-could-get-them screenshots of the same video used on 2017.wordpress.net:

With current styles:

https://cldup.com/mWJDUFPQwB.thumb.jpg

With object-fit: cover styles:

https://cldup.com/U_rzLJYiBj.thumb.jpg

object-fit may not be the best fix - I'm not super-familiar with it myself so it could have issues I'm not aware of, and it will need more testing and a polyfill to cover unsupported browsers. Other suggestions or recommendations for fixing are welcome!

Attachments (3)

39035.0.patch (2.0 KB) - added by laurelfulford 14 months ago.
39035.diff (977 bytes) - added by peterwilsoncc 14 months ago.
39035.2.diff (1002 bytes) - added by peterwilsoncc 14 months ago.

Download all attachments as: .zip

Change History (15)

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


14 months ago

#2 @helen
14 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.7
  • Version set to trunk

I think it's important to do this with the initial release of the theme, as the difference is pretty striking and users will be cropping and otherwise adjusting their media choices to best complement the area and suddenly changing that in the future would be jarring.

object-fit for IE appears to require a polyfill. It's currently a fairly high priority task in the queue for Edge, but of course it also affects older IE (as well as some other browsers, though none of them really concern me). What if we switch to object-fit and then use the current method as an IE fallback for 4.7, improving it for those browsers in the (near) future? What does this look like with object-fit in various IEs?

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


14 months ago

#4 @laurelfulford
14 months ago

39035.0.patch is a first crack at one possible solution: I added a JavaScript check to see if object-fit is supported, and if it is, a CSS class is added that updates the styles to use it.

One drawback is that it adds a flash of 'incorrect' styles - right now, in browsers that support object-fit you see a flash of a zoomed image/video that then corrects itself. It can be switched the other way, but then browsers that don't support object-fit (shakes fist at IE) will show a flash of a pretty broken image/video placement before correcting.

#5 @peterwilsoncc
14 months ago

  • Owner set to davidakennedy
  • Status changed from new to assigned

In 39035.diff:

Uses the same styles in the original patch but replaces the JS test with an @supports ( object-fit: cover ) feature query in CSS.

In older browsers, the entire @supports block is dropped.

#6 @joemcgill
14 months ago

  • Keywords has-patch added; needs-patch removed

I really like the idea of using @supports in this case. Browser support for CSS @supports is quite good, particularly when compared with object-fit. Using this feature in a default theme potentially exposes it to a number of theme developers as a way to do support checks without adding additional JS feature tests.

If we go either of the directions proposed in 39035.0.patch or 39035.2.diff, we should still think of ways to avoid unnecessary zooming for browsers without support for object-fit: cover.

#7 @davidakennedy
14 months ago

  • Keywords commit added

Thanks for the patches @laurelfulford and @peterwilsoncc!

Both of these patches do the job. But I like 39035.2.diff better, because as @joemcgill pointed out, it exposes theme developers to a fairly new CSS feature with good browser support. It's also fixes a CSS problem with just CSS, which is a cleaner solution to me. The browser support is still solid for @supports, see: http://caniuse.com/#feat=css-featurequeries

I tested in all the modern browsers, including Edge, IE 11, 10, 9 and 8. I looked at both the front end and Customizer view and it looks good. I think the feature queries patch is the way to go. And I agree we should try to improve the zooming in future versions of the theme.

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


14 months ago

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


14 months ago

#10 @helen
14 months ago

  • Keywords dev-reviewed added

I also like the usage of @supports, so +1 for 39035.2.diff. The difference really is striking.

#11 @davidakennedy
14 months ago

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

In 39483:

Twenty Seventeen: Improve display of video header and header image in modern browsers

The theme uses a hack to help the video header and header image fill the available space. This centers around max-width: 1000%;. It causes visual issues (zooming of the header video or image) for all users though.

This fixes that with CSS Features Queries. The hack remains for browsers that don't support Feature Queries and object-fit. Browsers that do support both get a better experience with a more reliable styling of the video and image header container.

Props laurelfulford, peterwilsoncc.

Fixes #39035.

#12 @helen
14 months ago

In 39485:

Twenty Seventeen: Improve display of video header and header image in modern browsers

The theme uses a hack to help the video header and header image fill the available space. This centers around max-width: 1000%;. It causes visual issues (zooming of the header video or image) for all users though.

This fixes that with CSS Features Queries. The hack remains for browsers that don't support Feature Queries and object-fit. Browsers that do support both get a better experience with a more reliable styling of the video and image header container.

Props laurelfulford, peterwilsoncc.

Fixes #39035 for the 4.7 branch.

Note: See TracTickets for help on using tickets.