Twenty Fourteen: Allow pages to have featured images

[iamtakashi]

Currently, featured images are limited to posts in Twenty Fourteen. It maybe a good idea to allow pages also to have featured images.

What I'm not 100% sure about is whether we should display the placeholders like posts in case there is no featured image or not.

[iamtakashi]

First pass for allowing pages to have featured images.

[Frank Klein]

Here is my feedback concerning the patch:

• The patch doesn't apply properly, since it's made against src/wp-content.
• Does the featured image need a link wrapped around it? On a page, it only links to itself, which seems redundant. The same applies to single posts by the way.
• If you set a featured image and use the full-width template, it looks kind of broken (image to short, header not overlapping,.... This is related to/dependent on #25031. I think we might need to check if there is a featured image set on the full width page and then apply the corresponding style (via .has-featured-image or something similar).

[SergeyBiryukov]

Replying to Frank Klein:

The patch doesn't apply properly, since it's made against src/wp-content.

That's the path in the ​new repository, it's used in the majority of patches now.

[iamtakashi]

Replying to SergeyBiryukov:

That's the path in the ​new repository, it's used in the majority of patches now.

I've just re-uploaded my recent patches in other tickets so that they can be applied from the WordPress root because, traditionally, that's been considered as the best practice as opposed to what I did initially. Would you say the best practice is changing since we have the new repo?

[iamtakashi]

Replying to Frank Klein:

• Does the featured image need a link wrapped around it? On a page, it only links to itself, which seems redundant. The same applies to single posts by the way.

We can definitely remove the link wrapper for pages and use a div instead but maybe it's a good idea to remove the link wrapper for both pages and posts altogether? I've just realised featured images are not linked in both Twenty Twelve and Twenty Thirteen.

Many magazine sites in wild usually link a lead image to its single view though. Of course, we can alter the markup with a conditional tag but I'm not sure if it's worth to do for the sake of simplicity.

• If you set a featured image and use the full-width template, it looks kind of broken (image to short, header not overlapping,.... This is related to/dependent on #25031. I think we might need to check if there is a featured image set on the full width page and then apply the corresponding style (via .has-featured-image or something similar).

I'll revise the full-width treatment when #25332 progresses.

Another thing I'd like to hear opinions is that the theme should display the placeholder if there is no featured image on a page. I'm slightly leaning towards to not to display at the moment actually. What do you guys think?

[SergeyBiryukov]

Replying to iamtakashi:

I've just re-uploaded my recent patches in other tickets so that they can be applied from the WordPress root because, traditionally, that's been considered as the best practice as opposed to what I did initially. Would you say the best practice is changing since we have the new repo?

Either way is fine. WP root is probably better to make sure the patches can also be applied by those who don't have the whole trunk directory of the new repo checked out.

[iamtakashi]

Replying to SergeyBiryukov:

Either way is fine. WP root is probably better to make sure the patches can also be applied by those who don't have the whole trunk directory of the new repo checked out.

OK, thank you!

[iamtakashi]

Replying to iamtakashi:

We can definitely remove the link wrapper for pages and use a div instead but maybe it's a good idea to remove the link wrapper for both pages and posts altogether? I've just realised featured images are not linked in both Twenty Twelve and Twenty Thirteen.

Many magazine sites in wild usually link a lead image to its single view though. Of course, we can alter the markup with a conditional tag but I'm not sure if it's worth to do for the sake of simplicity.

Discussed in Twenty Fourteen office hours today. We've decided to link featured images to single on archive views. ​IRC log

Another thing I'd like to hear opinions is that the theme should display the placeholder if there is no featured image on a page. I'm slightly leaning towards to not to display at the moment actually. What do you guys think?

We've decided to not to show the placeholder for pages in the office hours today. ​IRC log.

[iamtakashi]

Updated patch

[lancewillett]

Discussed in ​Tue September 24, 2013 office hours and decide Takashi is going to take another pass at the patch to simplify the logic in content.php if possible.

Instead of all the if/else checks for has_post_thumbnail() using the same markup for all cases and using CSS to adjust the layout.

[iamtakashi]

Here is a new patch.

• Simplify the logic in the template and control the display by CSS.
• Display featured images on other formats when they are attached.
• Add a custom post class has-featured-image when it's appropriate.
• Style adjustments.

[obenland]

I wonder if it would make sense to move the post thumbnail logic into its own partial.

Also:

• Do we keep the title attribute in the post thumbnail link?
• In the function comment to twentyfourteen_post_classes() it says "Adds body classes to denote:"

[iamtakashi]

@obenland, thanks for taking a look. I've updated the patch. Suggested approach is certainly cleaner on each template.

[lancewillett]

Patch is looking great. Two suggested changes:

• Use is_singular() to check for a single post or page.
• Move the password checks into the function so the templates just have a one-line call.

[lancewillett]

In 25735:

Twenty Fourteen: allow pages to have featured images, props iamtakashi. Fixes #25325.

[lancewillett]

Rework logic to show link or div correctly

[lancewillett]

I noticed in testing that this wasn't working quite right -- the link should appear only on index, and not in single. And we shouldn't check for has_post_thumbnail() at all, right? Otherwise the background pattern won't ever show up.

Also noting that in the a11y ticket, see ​http://core.trac.wordpress.org/ticket/25054#comment:31 -- this came up as a possible problem for screen readers if the div or anchor element is empty. Could we add a fallback text or something for when no image exists, and hide it with .screen-reader-text maybe?

[iamtakashi]

Replying to lancewillett:

I noticed in testing that this wasn't working quite right -- the link should appear only on index, and not in single. And we shouldn't check for has_post_thumbnail() at all, right? Otherwise the background pattern won't ever show up.

Good catch. You're right. that's also why the placeholder was clickable in in single.

Also noting that in the a11y ticket, see ​http://core.trac.wordpress.org/ticket/25054#comment:31 -- this came up as a possible problem for screen readers if the div or anchor element is empty. Could we add a fallback text or something for when no image exists, and hide it with .screen-reader-text maybe?

OK, but what would be the appropriate text? Something like "No Featured Image"?

[lancewillett]

Better logic for HTML output and add fallback screen-reader-text message

[lancewillett]

Refreshed patch attached.

[obenland]

Optimal logic for HTML output and template tag rename.

[obenland]

Replying to lancewillett:

Also noting that in the a11y ticket, see ​http://core.trac.wordpress.org/ticket/25054#comment:31 -- this came up as a possible problem for screen readers if the div or anchor element is empty. Could we add a fallback text or something for when no image exists, and hide it with .screen-reader-text maybe?

Is it really an a11y issue? Or is he just surprised to see an empty element there?

[lancewillett]

Replying to obenland:

Replying to lancewillett:
Is it really an a11y issue? Or is he just surprised to see an empty element there?

From my understanding screen readers speak aloud the link text so the user knows what the end result would be for following the link. It's also just bad practice to have an empty HTML element, anyway.

[obenland]

Replying to lancewillett:

From my understanding screen readers speak aloud the link text so the user knows what the end result would be for following the link. It's also just bad practice to have an empty HTML element, anyway.

I understand. The problem with though is that we'd have to have a link in a link, to both have it styled with the background image and explained with a screenreader text. If we'd just add text in the empty post thumbnail link it would not be focusable.

[lancewillett]

Replying to obenland:

Replying to lancewillett:
The problem with though is that we'd have to have a link in a link, to both have it styled with the background image and explained with a screenreader text. If we'd just add text in the empty post thumbnail link it would not be focusable.

With changes in my latest patch, it should work perfectly. Now to test it more. :)

The patch adds the placeholder text so that the div or a is never empty.

[obenland]

Replying to lancewillett:

With changes in my latest patch, it should work perfectly. Now to test it more. :)
The patch adds the placeholder text so that the div or a is never empty.

Are paragraph tags focusable?

[lancewillett]

Merged 6 and 7 changes

[lancewillett]

.8 is my proposed solution, but I'm open to discussing the pros and cons of leaving the HTML element empty versus adding the placeholder text.

[lancewillett]

Asked @joedolson for more feedback on .8 proposed patch (see #25054).

[joedolson]

Hi, Lance - sorry about the delay in getting back to you.

• What effect does an empty element have on a11y?
• If the element has text, but isn't focusable, is that a problem?

An empty element that does not perform an action (such as a div or paragraph) has no impact on accessibility; it will be ignored, as long it doesn't take focus.

If an element that performs an action isn't focusable, that's a problem, but if it doesn't do anything, then that doesn't matter. The important thing is that any actionable element -- anytime you need to activate an element to accomplish a task -- then that element must have text, and must be focusable.

A link with no text will generally be read with the href value, which will possible to understand, when read character-by-character is likely to be fairly useless.

[lancewillett]

Replying to joedolson:

Thanks, Joe. :)

[lancewillett]

In 25802:

Twenty Fourteen: better logic for featured image HTML output, and add fallback message for focusable anchor elements, for accessibility. Fixes #25325.

[lancewillett]

In 26694:

Twenty Fourteen: remove invalid rel attributes from post thumbnail output, and fix a CSS typo. See #25946 and #25325.