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(tests): Fixes for old Safari #8368

Merged
merged 1 commit into from
Jul 21, 2023
Merged

Conversation

mister-ben
Copy link
Contributor

Description

Followup to #8356. The issue with older Safari with document.styleSheets is a little different than thought. The problem is that Safari doesn't necessarily return rules verbatim, so a text comparison wasn't working. A rule with a shorthand property like background: white; is returned as background-color: white;.

Specific Changes proposed

Changes the test to use a rule that won't be changed.
Revert skipping the test on Safari from #8356 as we can make that test work after all.
Change copyStyleSheets to use styleSheet.media.mediaText when setting media. On old Safari this has to be a string, other browsers allow the MediaList object to be set.

Test ran on browserstack passed here:
https://github.com/mister-ben/video.js/actions/runs/5600546784/jobs/10243092934

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #8368 (feee88d) into main (452a918) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #8368   +/-   ##
=======================================
  Coverage   82.71%   82.71%           
=======================================
  Files         113      113           
  Lines        7578     7578           
  Branches     1821     1821           
=======================================
  Hits         6268     6268           
  Misses       1310     1310           
Impacted Files Coverage Δ
src/js/utils/dom.js 66.41% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@wseymour15 wseymour15 left a comment

Choose a reason for hiding this comment

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

+1, good comment describing the issue with Safari!

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Nice, was just looking at these test failures.

@mister-ben mister-ben merged commit 6fc1fd4 into videojs:main Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants