-
Notifications
You must be signed in to change notification settings - Fork 156
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
fix(video-player): correct a visual regression with background video in some contexts #12081
fix(video-player): correct a visual regression with background video in some contexts #12081
Conversation
Deploy preview created for package Built with commit: 5fc572f5b8c2781e6ff8931ebdd60a8f433a2f44 |
Deploy preview created for package Built with commit: 5fc572f5b8c2781e6ff8931ebdd60a8f433a2f44 |
Deploy preview created for package Built with commit: 5fc572f5b8c2781e6ff8931ebdd60a8f433a2f44 |
Deploy preview created for package Built with commit: 5fc572f5b8c2781e6ff8931ebdd60a8f433a2f44 |
Deploy preview created for package Built with commit: 5fc572f5b8c2781e6ff8931ebdd60a8f433a2f44 |
The leadspace with video isn't playing the video on page load. If I click pause & play again, it will play, but it won't play on load. |
Also seems like inline videos need to be clicked on twice to start the video? |
On my local I needed to go back to the |
The current video-player suite order of operations video-player
video-player-composite
video-player-contaienr
|
This does cause an accessibility issue - Since we don't override |
Should be fixed now. We need to make sure the below behaviors are all exhibited:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple things to note. Otherwise, nice work sleuthing the bug. I don't find any issues.
packages/web-components/src/components/video-player/video-player-composite.ts
Show resolved
Hide resolved
packages/web-components/src/components/video-player/video-player-container.ts
Show resolved
Hide resolved
✅ This is looking good to me now. Testing in Storybook looks good. I've also used the CDN preview together with Requestly to test issues reported with needing two clicks to play at this link as well as background visual regression at this link. |
### Related Ticket(s) [ADCMS-7129](https://jsw.ibm.com/browse/ADCMS-7129) ### Description Ports a couple of v1 player fixes to v2 that got missed to fix auto-play while maintaining background-video mode: Work that I've cherry-picked for this PR: [ADCMS-6363](https://jsw.ibm.com/browse/ADCMS-6363) #12070 (3d98787) #12081 (863dfdc) ### Changelog **New** - Adds `muted` attribute to `video-player-container` - Adds storybook examples for `auto-play` and `muted` video player usage **Changed** - Fixes `auto-play` attribute on `video-player-container` - Make `background-video` a reflected boolean attribute all the time. - Fix a regression with auto-play video. <!-- React and Web Component deploy previews are enabled by default. --> <!-- To enable additional available deploy previews, apply the following --> <!-- labels for the corresponding package: --> <!-- *** "test: e2e": Codesandbox examples and e2e integration tests --> <!-- *** "package: services": Services --> <!-- *** "package: utilities": Utilities --> <!-- *** "RTL": React / Web Components (RTL) --> <!-- *** "feature flag": React / Web Components (experimental) -->
Related Ticket(s)
Closes # {{Provide issue number link(s) to the related ticket(s) that this pull request addresses}}
Description
Recent changes caused visual regressions due to styles that relied on
[background-video="true"]
. These updates makebackground-video
a pure boolean attribute and simplify the style selectors accordingly.Changelog
Changed
background-video
a reflected boolean attribute all the time.