Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<rh-video-embed>: context and play button. #1758

Open
zeroedin opened this issue Aug 13, 2024 · 8 comments
Open

<rh-video-embed>: context and play button. #1758

zeroedin opened this issue Aug 13, 2024 · 8 comments
Assignees
Labels
for a11y review Needs accessibility review for design Design work is requested needs discovery Needs discovery needs discussion This issue needs further discussion

Comments

@zeroedin
Copy link
Collaborator

zeroedin commented Aug 13, 2024

In a dark parent context, <rh-video-embed> may be slotted with a 'light' background placeholder image. The dark context set by this parent would cause the <rh-button> in the shadow root of rh-video-embed to adjust its context and display a light play button over top of the light background image.

Because the background image sets another layer over the dark background set by the parent context this could cause color contrast issues with low sighted users identifying the play button on this image.

Example:

<rh-surface color-palette="darkest"> <!-- dark background -->
  <rh-video-embed>
     <img src="#" slot="thumbnail"> <!-- slotted thumbnail can be a light background image -->
    <!-- shadowroot -->
       ...
       <slot name="thumbnail"></slot>
       ...
       <rh-button variant=play></rh-button> <!-- will have dark context from the ancestor surface and display the light variant of the play button which is a possible accessibility issue overlaying the light slotted background image -->
       ...
    <!-- end shadowroot -->
  </rh-video-embed>
</rh-surface>

Possible solutions:

  • modify the play button to work across both light and dark contexts.
  • set a property that sets the context for the play button so it uses the correct context variant.
@zeroedin
Copy link
Collaborator Author

@coreyvickery @marionnegp let us know your thoughts here.

@bennypowers
Copy link
Member

maybe better to make it a provider then

@zeroedin
Copy link
Collaborator Author

zeroedin commented Aug 14, 2024

Maybe better to make it a provider then

This was my original thought see comment on the merged PR

Doing so has some inherent problems noted in a super quick test of implementation of adding the provider.

As it stands we two overlapping backgrounds.

  1. the parent context (could be set by rh-card or rh-surface etc)
  2. the background image (the play button overlays)

Here is my best ascii text representation of current state

| parent context 
|    | host (context w/ provider)
|    |   | image background 
|    |   |   |  play button
|    | caption text
|

Currently, the caption exists on the parent context but inside the host. So just adding a provider here then will set the caption text color incorrectly. Definitely will have to refactor the way it renders so we can provide support to this.

Can see this issue currently here: https://deploy-preview-1567--red-hat-design-system.netlify.app/elements/video-embed/demo/color-context/

@bennypowers
Copy link
Member

we're in charge of the caption, right?
can't we just set a palette background on the host?

@bennypowers
Copy link
Member

because this has to do with images and art direction, we should consider <rh-picture> as a solution and maybe kick this to cubone

@markcaron markcaron added needs discovery Needs discovery needs discussion This issue needs further discussion labels Oct 31, 2024
@marionnegp
Copy link
Collaborator

  • If we make the implementor responsible for picking the right context/theme to make the play button more visible, the docs will have to be updated. The play button color change is mentioned on the Style page, but I think we'll need to add more usage info.

@markcaron
Copy link
Collaborator

@marionnegp Until we have <rh-picture> let's update the docs to let the implementor know they're responsible for ensuring the color-palette doesn't affect the contrast accessibility.

@marionnegp
Copy link
Collaborator

@marionnegp Until we have <rh-picture> let's update the docs to let the implementor know they're responsible for ensuring the color-palette doesn't affect the contrast accessibility.

@markcaron, I have #2125 open with some copy about this. Tagged you as the reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for a11y review Needs accessibility review for design Design work is requested needs discovery Needs discovery needs discussion This issue needs further discussion
Projects
Status: Backlog
Development

No branches or pull requests

5 participants