Make WordPress Core

Opened 12 years ago

Closed 9 years ago

Last modified 9 years ago

#21389 closed task (blessed) (fixed)

Retina theme custom headers

Reported by: nacin's profile nacin Owned by: joemcgill's profile joemcgill
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Customize Keywords: responsive-images has-patch 2nd-opinion
Focuses: Cc:

Description

We should support retina custom headers. Not sure how — ideas welcome.

Attachments (4)

21389.diff (1.3 KB) - added by joemcgill 9 years ago.
21389.1.diff (2.4 KB) - added by azaozz 9 years ago.
21389.2.diff (2.2 KB) - added by azaozz 9 years ago.
21389.3.diff (2.4 KB) - added by azaozz 9 years ago.

Download all attachments as: .zip

Change History (45)

#1 @sabreuse
12 years ago

  • Cc sabreuse@… added

#2 @desrosj
12 years ago

  • Cc desrosj@… added

#3 @lessbloat
12 years ago

I may be oversimplifying this, but could we just create a 2x header image by default if they've uploaded/cropped a large enough image? Adding an additional retina image form field/button/link, would just add clutter to the UI I would think (and would likely end up confusing the majority of users). Just my 2 cents.

#4 @andy
12 years ago

Assuming core adopts some descendent of devicepx.js, the -1x/-2x filename pattern would work fine here.

If the cropped region of the uploaded image is sufficiently (1.5x) larger than the target area, save the 1x-resized version with the -1x filename tag and also save a -2x version.

(This seems to agree with lessbloat's comment above.)

#5 @saracannon
12 years ago

  • Cc sararcannon@… added

#6 @Otto42
12 years ago

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

#7 @aaroncampbell
12 years ago

  • Cc aaroncampbell added

#8 @Otto42
12 years ago

HiDPI Header image support - proposal

There's actually several cases to be considered, so I'm going to bullet point the major bits.

Enabling Support

  • Theme must enable HiDPI support specifically by including a "hidpi"=>true in their add_theme_support( 'custom-header' ) arguments.

Why: Because not all themes will be capable of taking advantage of HiDPI headers, as you'll see below.

Image Creation

There's a couple cases to consider. I'll take them in order.

Fixed-width headers

  • If hidpi support is enabled, and the uploaded image is > 1.5x the fixed width the theme specifies, then the image resizing code will create two images, one with the proper width, and one with up to 2.0x the width. The image will be saved as part of the same normal attachment, but with the size scaled down in the meta.
  • That is to say that normally creating an image via the header creation causes the "full" image to be the cropped version of the image, while the thumbnail/medium/large sizes get generated as per usual. In this case, a new size even larger than "full" called "hidpi" will be created as well. The filename of this image will reflect the actual size of the image, but the width/height in the imagemeta will be altered to reflect the intended display size. This will have the effect of having the img tag generated to have the smaller width/height pieces, allowing for the hidpi effect on capable screens.

Flexible-width headers

  • With flex-width headers, there's a desired width and a maximum width. If hidpi support is enabled, and the image uploaded width is greater than max-width and also greater than 1.5x the minimum-width, then the same logic as fixed width is applied. For an example for twentytwelve, the image uploaded would need to be greater than 2000px wide to get the above logic applied, however in such a case the resulting image would be half as wide as expected. So uploading a 1999px wide image would result in a 1999px wide header, but uploading a 2002px image would result in a 1001px wide header (with a 2002px wide one for hidpi cases).
  • The proposed flexible width handling may lead to user confusion if the theme doesn't choose sensible max widths (IMO, a 2000px max-width is a bit ridiculous... scale it back, guys). This is one reason why hidpi support must be explicitly enabled by the theme for any of this logic to occur.

Image display

There's two cases to consider here as well.

IMG tags

  • Right now, themes should call get_header_image() to get the bare URL of the header image, then call get_custom_header()->width and get_custom_header()->height; to get the width/height, and then generate the img tag themselves.
  • Proposed approach: get_header_image_tag($size) and associated header_image_tag($size) functions to produce the entire IMG tag, with the necessary width and height parameters. These would be wrappers similar to the various get_attachment functions that do much the same work. A size parameter of "hidpi" could be passed to obtain the hidpi version of the image if needed. The theme would have to do its own detection (or use JS replacement methods that I'm planning on providing in #21038, perhaps making this automatically handled; more on that over there, later).

Make CSS an option

  • Currently, the image tag method above is pretty much the only way to implement custom headers. However, another way to do it would be for the theme to simply provide a DIV with a "custom-header" class, and have WordPress itself output the necessary CSS to do the background-image code and so forth. The advantage in this case is that the HiDPI handling could be done entirely in the CSS, including the alternate image for the device-pixel-ratio case, if such an image was available. The CSS would be output in the wp_head call, similar to how the custom-background CSS is output currently.
  • To indicate that the theme supports this approach, an argument of "output_css" => true or similar could be added to the add-theme-support function call. This would be the preferred way, for obvious reasons. Alternatively, if the class name is chosen wisely, the CSS output could simply be on by default, since a theme that didn't include the necessary DIV with the class would simply not have the CSS take effect anywhere.

Suggestions? Criticism? Comment.

#9 @nacin
12 years ago

  • Milestone changed from 3.5 to Future Release
  • Type changed from task (blessed) to feature request

Per the IRC meetup yesterday, HiDPI support for user-uploaded images is no longer part of 3.5's scope, due to the complexities of implementation. With the media UIs and image APIs improving quite a bit in this release, it ideally becomes more feasible in a future version. It is unfortunately too much at once to try it in 3.5 while everything it would be built upon is also shifting around.

Combined with the fact that HiDPI support everywhere on the internet remains a total hack, this also gives browser makers and standards bodies some more time to come up with a sane way to handle HiDPI support.

Otto42 will be watching #21390 (and related tickets) closely to make sure the right hooks are in place so this can be attempted in a plugin.

This applies to #21038 (user content), #21455 (backgrounds), #21389 (headers).

#10 @tollmanz
11 years ago

  • Cc tollmanz@… added

#11 @kwight
11 years ago

  • Cc kwight@… added

#12 @philiparthurmoore
11 years ago

  • Cc philip@… added

#13 @dreamwhisper
11 years ago

  • Cc dreamwhisper added

#14 @laurenmancke
11 years ago

  • Cc laurenmancke added

#15 @benknight
10 years ago

Bump.

This should totally still happen. Supporting high-dpi devices is mainstream at this point, yet still impossible to do with custom headers. I'm having to fork my theme when I want to use a different header because it's manually coded into the CSS. Not ideal.

#16 @Otto42
10 years ago

Unfortunately, the browser support still isn't quite there.

The picture tag and the srcset element offer the way forward here, because with these, having to use JS to select the displayed image is not necessary, you can specify all of them and have the browser choose, even dynamically.

Currently, only Chrome really offers this full support: http://caniuse.com/picture

Doing this in a plugin is possible, but would need the theme to support it as well, because of the current usage of get_header_image(). New functions (like get_header_image_tag()) would provide themes the means to have core output the new image tags (and/or CSS) needed to provide the responsive header image. The theme shouldn't have to know how it works to make it happen, it should just-work.

Still, this is a decent candidate for one of the next few releases.

Note that while making "fixed-size" header images responsive is relatively easy, flexible width images still needs a bit of thinking. It might be reasonable simply to provide a variety of various image sizes at some breakpoints, then let the browser choose amongst them for the full set of the available images. If this is done, then the difference between "fixed" and "flexible" kind of disappears, since you could do this for any image. However, themes may desire to set those breakpoints themselves. An analysis of various existing theme methods for showing header images may be in order to find out the right way.

#17 @benknight
10 years ago

Given the challenges I agree with you that a plugin would be a good approach here.

Some thoughts:

Given that get_header_image just returns the contents of the src attribute, you could solve this problem by just returning the 2x image always. The tradeoff here would be downloading 4 times the amount of data necessary for users with 1x screens. For icons and small images, this is an acceptable tradeoff in my opinion, but for large header images, obviously it matters.

To reiterate what was said above, if it weren't for the header image markup being at the theme level, it's possible to imagine a CSS-only approach here, which is probably more appropriate anyway since the header image is more design than it is actual content. Then all you need is media queries. This is how I'm doing it in my own themes.

I would watch the srcset part of the Responsive Images implementation more closely than the <picture> element (http://responsiveimages.org/ does a really good job tracking the progress), since this is really all you need to solve the problem. srcset is pretty close: on be default in Chrome & Safari, behind a flag on Firefox. Given the timing on this one I think it's probably a good time to start taking action on this feature now.

#18 @nacin
10 years ago

We don't need <picture> here. <img srcset> is sufficient, it's backwards compatible, and we should use it. I'd love for it to happen, and it's time to do this. Note, there's a #feature-respimg channel in Slack that would be appropriate for this to be coordinated in, especially since the chair of RICG (and other RICG contributors) are using that channel to coordinate a plugin (on a track for core) to handle image uploads.

This ticket was mentioned in Slack in #feature-respimg by nacin. View the logs.


10 years ago

This ticket was mentioned in Slack in #core-customize by valendesigns. View the logs.


9 years ago

#21 @wonderboymusic
9 years ago

  • Keywords responsive-images added
  • Milestone changed from Future Release to 4.4
  • Owner changed from Otto42 to joemcgill

Let's see what we can do here

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


9 years ago

#23 @joemcgill
9 years ago

As @benknight mentioned, get_header_image() just returns a url for a src attribute. However, the customizer creates a new image attachment including intermediate sizes when you set a custom header image, which means that since 34855 you can produce the responsive markup like this (line breaks for readability only):

<img src="<?php header_image(); ?>" 
 srcset="<?php echo esc_attr(wp_get_attachment_image_srcset( get_custom_header()->attachment_id, 'full'); ?>" 
 sizes="{customize as needed}"  
 width="<?php echo esc_attr( get_custom_header()->width ); ?>"
 height="<?php echo esc_attr( get_custom_header()->height ); ?>" 
 alt="<?php echo esc_attr( get_bloginfo( 'name', 'display' ) ); ?>"
/>

I imagine you could often simplify this pattern a bit and do something like...

<?php wp_get_attachment_image( get_custom_header()->attachment_id, 'full' ); ?>

...which would automatically have srcset and sizes attributes added, as long as additional intermediate sizes are available to populate the srcset.

#24 @wonderboymusic
9 years ago

  • Type changed from feature request to task (blessed)

We can let this ride while deciding a final outcome

#25 @azaozz
9 years ago

As far as I see most themes repeat the same code when adding custom header images. I'm a bit surprised that there isn't a "template helper" function for themes that will output the whole image tag. Thinking such function (that also adds srcset) will be the best solution here.

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


9 years ago

#27 @Otto42
9 years ago

The proposal I made 3 years ago was along the same lines as what @azaozz is saying. Basically, we need a wrapper function for themes to use to generate the entire IMG tag. I suggested get_header_image_tag() and header_image_tag(), although those names might not be exactly right.

Having a helper function for themes to use to generate this greatly simplifies the problem for the case of "fixed width" header.

Flexible width headers is still on the table though. If a theme chooses to support these, then having some way for it to specify breakpoints on the width would be nice (essential, really). A theme can specify desired widths, and the header uploader/cropper can generate those specific widths for that header image only, instead of simply using all the normal image widths.

@joemcgill
9 years ago

#28 follow-up: @joemcgill
9 years ago

  • Keywords has-patch 2nd-opinion added

21389.diff adds two new helper functions, get_header_image_tag() and the_header_image_tag() which generate the image markup for the custom header image by getting the get_custom_header() object and passing it to wp_get_attachment_image(), which will automatically include srcset and sizes attributes.

@azaozz
9 years ago

#29 in reply to: ↑ 28 @azaozz
9 years ago

Replying to Otto42:

The proposal I made 3 years ago was along the same lines...

Right, sorry, should have linked to comment 16 in my previous comment.

Replying to joemcgill:

We get pretty much everything we need from get_custom_header(). Not sure if we have to go back and run most of the attachment image functions again (it's slow).

How about we get the srcset and sizes attributes right in get_header_image_tag(), see 21389.1.diff. There is some code duplication but is faster and nearly identical to the code most themes use now.

@azaozz
9 years ago

#30 @azaozz
9 years ago

21389.2.diff is same as 21389.1.diff but always generates srcset and sizes, then lets the theme overwrite them (by setting them in the $attr array).

#31 @joemcgill
9 years ago

As @azaozz noted, the internals of wp_get_attachment_image() is not the most efficient piece of code and 21389.1.diff would be an improvement there. My main concern is that we are essentially duplicating a lot of the functionality between the two functions, which doesn't feel right either. Since the header image is only called once per page, I would probably trade some efficiency for DRYness here, but either approach works.

I do prefer the approach in 21389.1.diff over 21389.2.diff, because we shouldn't bother calculating srcset and sizes attributes if they are going to be overridden later in the same function.

I also believe we would need to generate an alt tag, even if it's empty by default, for a11y reasons.

#32 @azaozz
9 years ago

Yes, we should add an empty alt attribute to the defaults, and yes, 21389.1.diff is more optimized than 21389.2.diff. It uses the same method of adding the srcset and sizes attributes as at the bottom of wp_get_attachment_image(). However think it is better to use the data returned by get_custom_header() instead of running the whole wp_get_attachment_image(). There is also a chance the latter may return different values in some edge cases.

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


9 years ago

This ticket was mentioned in Slack in #accessibility by azaozz. View the logs.


9 years ago

#35 follow-up: @kirasong
9 years ago

Tested the three patches.

21389.diff is about 32ms on a shared server, with 21389.(1 and 2).diff at only 13ms, so they are significantly faster.

However, it looks like 21389.(1 and 2).diff are missing classes in the image's generated HTML, so headers break layout with 21389.(1 and 2).diff

#36 in reply to: ↑ 35 ; follow-up: @azaozz
9 years ago

Replying to DH-Shredder:

However, it looks like 21389.(1 and 2).diff are missing classes in the image's generated HTML, so headers break layout with 21389.(1 and 2).diff

This is a new function. If the themes want to have a class set, they will have to pass it in the (optional) $attr array:

$add_classes = array( 'class' => 'one two three' );
the_header_image_tag( $add_classes );

This works in exactly the same way in all of the patches.

#37 in reply to: ↑ 36 ; follow-up: @kirasong
9 years ago

Replying to azaozz:

This is a new function. If the themes want to have a class set, they will have to pass it in the (optional) $attr array:

$add_classes = array( 'class' => 'one two three' );
the_header_image_tag( $add_classes );

This works in exactly the same way in all of the patches.

Ah, I see what's going on, thanks.

Chatted with @azaozz on this:
The "extra" default styles from wp_get_attachment_image() from the first patch were fixing the display on my install, and shouldn't be there. These are intentionally not included in the second two patches, which in my test case meant an overflowing header.

@azaozz
9 years ago

#38 in reply to: ↑ 37 @azaozz
9 years ago

Replying to DH-Shredder:

Yes, when testing in existing themes that add a class to the image tag, the class has to be passed in the $attr argument.

In 21389.3.diff:

  • Refresh after r35569.
  • Add default alt attribute get_bloginfo( 'name' ).
Last edited 9 years ago by azaozz (previous) (diff)

#39 @kirasong
9 years ago

Thanks @azaozz! I think, given the performance benefits, it makes sense to go this route.

@joemcgill, have any additional thoughts here?

#40 @azaozz
9 years ago

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

In 35594:

Responsive images: add template helper functions to generate the tag for a (responsive) header image that includes srcset and sizes attributes.

Props Otto42, joemcgill, DH-Shredder, azaozz.
Fixes #21389.

#41 @azaozz
9 years ago

In 35595:

Responsive images: fix args order and streamline the srcset and sizes generation and better inline docs in get_header_image_tag().

See #21389.

Note: See TracTickets for help on using tickets.