Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#38697 closed enhancement (fixed)

Twenty Seventeen: Video header pause button design

Reported by: melchoyce's profile melchoyce Owned by: joemcgill's profile joemcgill
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch needs-testing
Focuses: accessibility Cc:

Description

For the pause button in Twenty Seventeen, I'm thinking 30px offset from the top right corner. The icon is pause from Font Awesome with 15px padding around it. When the video is paused, it should switch to play. Background color is #222 at 30% opacity, icon is #fff at 50% opacity. On hover, background is 80% opacity and icon is 100% opaque.

Depends on #38678.

Attachments (10)

pause-mockup.png (1.7 MB) - added by melchoyce 8 years ago.
pause-default.png (223.4 KB) - added by melchoyce 8 years ago.
pause-hover.png (207.2 KB) - added by melchoyce 8 years ago.
38697.1.diff (8.2 KB) - added by laurelfulford 8 years ago.
38697.2.diff (6.7 KB) - added by laurelfulford 8 years ago.
38697.3.diff (6.1 KB) - added by laurelfulford 8 years ago.
38697.4.diff (6.1 KB) - added by laurelfulford 8 years ago.
38697.5.diff (6.5 KB) - added by joemcgill 8 years ago.
Screen Shot 2016-11-16 at 12.32.50 PM.png (618.9 KB) - added by joemcgill 8 years ago.
Full opacity button borders
38697.6.diff (5.2 KB) - added by joemcgill 8 years ago.

Change History (45)

@melchoyce
8 years ago

#1 @lukecavanagh
8 years ago

@melchoyce

The design mockup looks clean and easy to use.

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


8 years ago

#3 follow-up: @davidakennedy
8 years ago

Asking the critical question here: Would there be a way to keep the entire video clickable and make it more obvious that it's able to be paused – from a design standpoint?

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


8 years ago

#5 @afercia
8 years ago

This is also related to #38678 since probably the button needs to be available also on hover.

#6 in reply to: ↑ 3 @melchoyce
8 years ago

Replying to davidakennedy:

Asking the critical question here: Would there be a way to keep the entire video clickable and make it more obvious that it's able to be paused – from a design standpoint?

From a design standpoint, I don't think so, no. Clicking to pause could be a good enhancement, but I don't think there's anything we can do to make that more clear through visual design. Not keen on overcomplicating it.

#7 @joemcgill
8 years ago

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

#8 @helen
8 years ago

  • Keywords needs-patch added

#9 @laurelfulford
8 years ago

Styling this button was trickier than expected (even with the span), so this is a bit hacky.

38697.1.diff uses JavaScript to insert the SVG play/pause icons. I've wrapped the existing button in a div, and included the SVGs next to that - otherwise, the JS used to toggle the button text removes the SVGs with it.

The icons are switched using sibling CSS selectors; the original button is transparent and layered over the play/pause icon.

Needs to be used with the latest patch from #38678.

I think the easiest way to customize this button from a theme perspective would be to allow customizable strings for the play/pause button - something like:

the_custom_header_markup( array(
	'play' => '<span>' . __( 'Play', 'twentyseventeen' ) . '</span>' . twentyseventeen_get_svg( array( 'icon' => 'play' ) ),
	'pause' => '<span>' . __( 'Pause', 'twentyseventeen' ) . '</span>' . twentyseventeen_get_svg( array( 'icon' => 'pause' ) ),
) );

I totally understand that this could be tricky to add from a code perspective, though.

#10 @laurelfulford
8 years ago

Made a second attempt in 38697.2.diff. I originally overcomplicated how the SVGs needed to be added, so have reduced the markup/styles.

I noticed on further testing that the button wasn't always loaded in time with the wp-custom-header-video-loaded event. I've added a setTimeout to make sure the button is present before injecting the SVG - it gets the job done, but it's not great. Suggestions for improvements are very welcome!

Edited to add: like the original patch, it needs to be tested with the latest patch on #38678 to work.

Last edited 8 years ago by laurelfulford (previous) (diff)

#11 @davidakennedy
8 years ago

@joemcgill @bradyvercher I'm curious to get your take on the latest patch. Is there a way we can make this implementation easier for themers? And/or is there a better way to go about this? Maybe an editable string that can be passed in by themes?

#12 @davidakennedy
8 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

#13 @bradyvercher
8 years ago

@davidakennedy @laurelfulford @joemcgill I see a few options for styling those without using JavaScript:

Filter the Strings
The strings can already be modified using the header_video_settings filter, so this would work similar to @laurelfulford's suggestion:

add_filter( 'header_video_settings', function( $settings ) {
	$settings['l10n']['play'] = '<span>' . __( 'Play', 'twentyseventeen' ) . '</span>' . twentyseventeen_get_svg( array( 'icon' => 'play' ) );
	$settings['l10n']['pause'] = '<span>' . __( 'Pause', 'twentyseventeen' ) . '</span>' . twentyseventeen_get_svg( array( 'icon' => 'pause' ) );
	return $settings;
} );

SVG Backgrounds
If the color of the SVG icons doesn't need to be changed, they can be used as background images. I don't think the extra accessibility markup is needed since that's provided by the button text.

CSS Icons
Play and pause icons are simple enough that they can be created using CSS. Here's an example of how these would work: http://nicolasgallagher.com/pure-css-gui-icons/demo/

Icon Font
Many themes still use icon fonts and can use those to insert the icons with a little CSS.

All of those methods should be available without using JS to inject extra markup, which I'd recommend avoiding if at all possible. If JS is absolutely necessary, we would probably need to trigger another event after the button is inserted and do some testing to make sure the icons persist after selective refresh.

With any of these options, removing the extra span in the button in #38678 is probably a good idea. I think pseudo-elements should provide enough styling hooks for the majority of cases and extra markup can be inserted using the filter if needed.

#14 @joemcgill
8 years ago

If the color of the SVG icons doesn't need to be changed, they can be used as background images. I don't think the extra accessibility markup is needed since that's provided by the button text.

I was thinking the same thing. I'm not sure the actual SVGs need to be injected into the button, however, we could potentially add the button markup as a template file that is filterable, but that will require us to add logic to the custom header JS for template rendering. We could also move the logic for creating the buttons out of the initialization method so it could be replaced by a custom handler (which might be the best option).

#15 @laurelfulford
8 years ago

Thanks @bradyvercher and @joemcgill!

I completely missed that there was a filter available. That looks like the best approach to me, since we do need to inject the SVGs into the markup - we need to change the colour of the icon from white to black via CSS for the dark colour scheme.

Given this, I also agree that the span in the button markup isn't needed - if a theme requires markup to style the button in a specific way, it can added with the filter.

Twenty Seventeen isn't otherwise bundled with an icon font, and uses inline SVGs for other icons. For consistency I'd like to stick with that, but both the icon fonts and CSS icons are solid alternatives to style this button.

I'll update the patch now. Thanks again!

#16 @laurelfulford
8 years ago

38697.3.diff uses the header_video_settings filter to add the icons; unneeded JS has been removed :)

#17 @davidakennedy
8 years ago

This is looking really good! Thanks for all the work everyone. Here's some feedback on the latest design and patch:

@melchoyce Can the button have a higher opacity or similar to increase contrast? I love the design and realize the need to not want to overshadow the video in this case, but right now it's really dependent on whatever the background video is, which could be anything... so I'd love to do better there to help combat that.

@laurelfulford The patch is working nicely and looks good.

  • What do you think about moving the filter function to the inc/custom-header.php function where the other custom header stuff is at?
  • The button text has a display: none; applied, meaning the button can't be read by screen readers. Probably an oversight. :) That can be replaced with screen-reader-text.
  • Is there a reason you styled with the id and not class in CSS? Using the class, wp-custom-header-video-button would make it easier to override in spots if necessary.

#18 @joemcgill
8 years ago

@laurelfulford 38697.3.diff is looking good. I'd +1 @davidakennedy's recommendation to use the class instead of ID for styling if possible. I was also surprised to see that the button scrolled upwards with the content rather than staying fixed to the corner of the video (see image below). Is this the intended behavior?

https://cldup.com/PLJxeLEcx8.gif

Also, re @davidakennedy:

Asking the critical question here: Would there be a way to keep the entire video clickable and make it more obvious that it's able to be paused – from a design standpoint?

It is possible to make clicking the video trigger play/pause as well as the button itself if that's desired. In fact, YouTube videos will have that behavior by default unless we intentionally try to block click events on the YouTube iframe, though it sounds like that could carry some potential legal implications.

#19 @afercia
8 years ago

  • Focuses accessibility added

Worth reminding this should be coordinated with #38678 where the button aria-hidden attribute is going to be removed. Since the button will now be available to assistive technologies, there's the need to don't use display: none on the button text but hide the text just visually, otherwise AT won't announce any text. It should be as simple as removing the current rule and add the .video-button-text selector to the .screen-reader-text rule.

See screenshot with text hidden just visually, using Safari + VoiceOver:

https://cldup.com/tuphZTQtjR.png

I'd also propose to consider to improve a bit the two strings and make them a bit more meaningful, something like:
Pause background video
Play background video

#20 @laurelfulford
8 years ago

Thanks for the reviews and comments @davidakennedy, @joemcgill and @afercia!

In 38697.4.diff I made the following updates:

  • The filter function has been moved to custom-header.php - that makes more sense!
  • The button text has been hidden with screen-reader-text instead.
  • I switched the id to a class in the CSS - there was no reason to use the ID, and I agree the class is better.
  • I've switched the button to stay fixed with the video - I didn't consider it but it did feel a bit odd with it moving with the text, when it's actually video-related.
  • I've updated the strings to 'Play background video' and 'Pause background video' in the theme.

Just let me know if there's anything else - thanks!

#21 @joemcgill
8 years ago

Tested 38697.4.diff and everything is looking good, but I also wanted to reiterate one thing that @davidakennedy mentioned earlier:

@melchoyce Can the button have a higher opacity or similar to increase contrast? I love the design and realize the need to not want to overshadow the video in this case, but right now it's really dependent on whatever the background video is, which could be anything... so I'd love to do better there to help combat that.

The current patch sets the style of the button based on the color scheme setting, but as David points out, the button will overlay the video, which won't follow the same color scheme necessarily. For example, using the light color scheme, with a dark video, the button is pretty hard too pick up (see screenshot below). I question whether the button color should even be related to the color scheme or whether a we can style it with affordances for both dark and light videos.

https://cldup.com/DeVYy1pYGS.png

@joemcgill
8 years ago

#22 @joemcgill
8 years ago

38697.5.diff adds slight borders on the outside of the buttons for both color schemes.

Light video BG with dark color scheme
https://cldup.com/dQv0DT4UiJ-3000x3000.png

Dark video BG with light color scheme
https://cldup.com/St3uISHhWw-3000x3000.png

#23 @davidakennedy
8 years ago

I'm feeling pretty good about the above changes, but curious to hear thoughts from @melchoyce on the design tweaks.

It's entirely possible you could have a dark video header with the light color scheme and vice versa. This is a nice suggestion though @joemcgill! One other idea: we could make the button text color editable – with the default header text color, so if a user has a video background that doesn't work with any of the defaults, they can change it.

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


8 years ago

#25 @melchoyce
8 years ago

I'm really not keen on adding color scheme support — I see the play/pause button similar to the gradients over the images, which should remain dark and neutral with a bright spot of white for the icon. We can up the opacity a little for more contrast.

I'm 100% against adding a new theme option for this.

#26 @melchoyce
8 years ago

Didn't notice the border images — that's fine. What does it look like at full opacity? Can it not be 100% opaque?

#27 @Presskopp
8 years ago

Why not position the button in the center (of the video)? I 'feel' it can be missed more easily in it's "corner of shame"

@joemcgill
8 years ago

Full opacity button borders

#28 follow-up: @joemcgill
8 years ago

Thanks @melchoyce. I agree about color scheme support — particularly this late in the cycle. I was intentionally leaving the border color subtle in my patch. This image shows the border at full opacity, which doesn't seem to match the play/pause icon in the middle of the button well.

@Presskopp This button is meant to be available primarily to assist people who have difficulty with background videos, rather than a general video control. As such, we want to make it unobtrusive. However, each theme will have the ability to style these buttons to match their own preference.

#29 in reply to: ↑ 28 @melchoyce
8 years ago

Replying to joemcgill:

Thanks @melchoyce. I agree about color scheme support — particularly this late in the cycle. I was intentionally leaving the border color subtle in my patch. This image shows the border at full opacity, which doesn't seem to match the play/pause icon in the middle of the button well.

Yeah, something's wrong there — the icon should be 100% opaque on hover, as per my first post. The border, to match the background, can go up to 80% opaque on hover.

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


8 years ago

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


8 years ago

@joemcgill
8 years ago

#32 @joemcgill
8 years ago

  • Keywords needs-testing added; dev-feedback removed

After clarifying direction with @melchoyce via Slack, 38697.6.diff slightly modifies the .wp-custom-header-video-button styles based on the following:

Default:
Background: #222, 50% opacity
Border: #fff, 60% opacity
Icon: #fff, 60% opacity

Hover:
Background: #222, 80% opacity
Border: #fff, 80% opacity
Icon: #fff, 100% opacity

I've also removed the extra styles for dark color schemes for this button, at Mel's request:

I don’t think there should be color scheme support for this button — it should stay dark grey and white despite your color scheme.

#33 @joemcgill
8 years ago

Here's a visual preview of 38697.6.diff

https://cldup.com/diujkIgSS2.gif

#34 @melchoyce
8 years ago

Looks good to me 👍

#35 @joemcgill
8 years ago

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

In 39273:

Twenty Seventeen: Add styles for custom header video controls.

Following [39272], this uses the header_video_settings filter to modify
the default video header control markup and adds theme specific styles
for the play/pause button.

Props melchoyce, laurelfulford, joemcgill, davidakennedy, bradyvercher.
Fixes #38697.

Note: See TracTickets for help on using tickets.