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

Fix background being invisible with theme videos #5640

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

mihawk90
Copy link
Contributor

Changes
I noticed that since I updated to 10.9 the skin's background would disappear when a theme video was playing and therefore some text would be unreadable depending on the video in the background.

Before:
image

After:
image

This basically restores pre-10.9 behaviour.

Issues
Couldn't find any open issues for this.


Calling in @grhallenbeck since the cause was introduced in ce4c7ae / #5076 and there might have been a reason I'm not seeing.

The videoPlayerContainer and backgroundContainer elements are the first 2 elements in the DOM structure though. They are already at the lowest layer of elements, making z-index seem redundant here. The issue is that the videoPlayerContainer specifies no z-index at all so the -1 made the backgroundContainer be rendered behind the video (or culled entirely, not sure how the actual rendering pipeline works :P ).

@mihawk90 mihawk90 requested a review from a team as a code owner May 31, 2024 07:44
@dmitrylyzo
Copy link
Contributor

I think this should be rebased on the release-10.9.z branch.
Then edit the target branch using Edit next to the title.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label May 31, 2024
@jellyfin-bot

This comment was marked as outdated.

@mihawk90
Copy link
Contributor Author

Sorry my bad, I didn't think of targeting the release branch.

Welp, something went wrong with that rebase 🤣

github-actions[bot]

This comment was marked as outdated.

@mihawk90 mihawk90 changed the base branch from master to release-10.9.z May 31, 2024 11:33
@mihawk90
Copy link
Contributor Author

mihawk90 commented May 31, 2024

Ah there we go, I should have updated the base before pushing I guess...

Seems CI choked on it :/

@dmitrylyzo dmitrylyzo added this to the v10.9.4 milestone May 31, 2024
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label May 31, 2024
@dmitrylyzo dmitrylyzo added bug Something isn't working regression We broke something ui & ux This PR or issue mainly concerns UI & UX labels May 31, 2024
Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't know what the original purpose of this z-index is. Perhaps it is a leftover.
Seems to work as intended without it.

@thornbill
Copy link
Member

(rebased to trigger CI)

Copy link

sonarcloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@thornbill thornbill merged commit 7854c4b into jellyfin:release-10.9.z Jun 4, 2024
11 checks passed
@thornbill thornbill added the stable backport Backport into the next stable release label Jun 4, 2024
joshuaboniface pushed a commit that referenced this pull request Jun 5, 2024
Fix background being invisible with theme videos

Original-merge: 7854c4b

Merged-by: thornbill <thornbill@users.noreply.github.com>

Backported-by: Joshua M. Boniface <joshua@boniface.me>
@jellyfin-bot jellyfin-bot removed the stable backport Backport into the next stable release label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression We broke something ui & ux This PR or issue mainly concerns UI & UX
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants