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

Simplify WebVTT snap-to-line test by avoiding cue alignment settings #46455

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

foolip
Copy link
Member

@foolip foolip commented May 23, 2024

The test passes in Firefox before and after these changes. The rendering
isn't affected because these settings are the defaults.

The test passes in Firefox before and after these changes. The rendering
isn't affected because these settings are the defaults.
@silviapfeiffer
Copy link
Contributor

Is there another check for the passing of these settings?

@foolip foolip changed the title Simplify WebvTT snap-to-line test by avoiding cue alignment settings Simplify WebVTT snap-to-line test by avoiding cue alignment settings May 23, 2024
@foolip
Copy link
Member Author

foolip commented May 23, 2024

@foolip
Copy link
Member Author

foolip commented May 23, 2024

A bit of context for this is that I was working on mapping the tests to the features in https://webstatus.dev/?q=webvtt and web-platform-dx/web-features#1121 which I'm adding. Looking for rendering tests for cue alignment settings I could only find this one, but it's not really a test for that. In the end, mapping the tests to features is easier if all of the tests in this directory are for the base WebVTT feature.

@silviapfeiffer
Copy link
Contributor

Thanks for that clarification. Great to see you are adding specific tests for the line-left and start settings and will be testing these separately. Given this addition of tests, I'm fine with this change.

@foolip
Copy link
Member Author

foolip commented May 24, 2024

@silviapfeiffer to make sure there's no miscommunication, web-platform-dx/web-features#1121 is adding the feature to web-features, and I will find the tests for cue alignment settings in WPT so that they can be shown on wpt.fyi using feature:webvtt-cue-alignment. Unfortunately, there are no rendering tests for cue alignment, not even the test I'm updating here really tested it, it was just there in the syntax.

Without rendering tests the feature isn't really tested well enough. I can file an issue about this, but am not planning to improve the test coverage myself.

@silviapfeiffer
Copy link
Contributor

At minimum, this test was testing the parsing of that feature, even if it didn't do a rendering test.
I understood you were adding additional tests, sorry for the misunderstanding.
Please file the issue.
If you happen to find the time to write the additional tests, that would be amazing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants