RedHat-UX / red-hat-design-system

Red Hat's Design System
https://ux.redhat.com
MIT License
94 stars 18 forks source link

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

Open zeroedin opened 1 month ago

zeroedin commented 1 month ago

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:

zeroedin commented 1 month ago

@coreyvickery @marionnegp let us know your thoughts here.

bennypowers commented 1 month ago

maybe better to make it a provider then

zeroedin commented 1 month ago

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 commented 1 month ago

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

bennypowers commented 5 days ago

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