Computed position alignment #421
Conversation
Start and end aligned text need to have a computed position alignment that is not center.
|
May I suggest deleting the master branch? ☺ seems like it's causing trouble still. |
|
Will do - every now and then I accidentally merge master, but it doesn't really show in diff, so I didn't bother changing it. 😀 |
|
Hi there, @foolip and I have been looking at normative spec change without tests every week this quarter, and I noticed this seems to be the case here. web-platform-tests/wpt@5b4716e appears to have added some tests for this behavior, but before the spec was updated. I was wondering if there are any plans to update the tests to match this PR. |
|
There's a couple of new tests missing for new commits and it's for two reasons (and entirely my fault): I have to learn to write the tests and I didn't want to hold up the standard process with these patches. I guess a new issue should be opened. What process are you following? Land patches only after tests have been prepared? |
|
Just like the spec., the test suite is a community effort. If you perceive a test as missing, inadequate, or wrong, please try to contribute the necessary improvements. It's not always the editor's job (nor is improved spec. text - pull requests for spec.-ready text are also appreciated). |
https://github.com/w3c/webvtt/blob/gh-pages/CONTRIBUTING.md#tests (added in #357) is the same wording as we're using for all WHATWG specs and lots of W3C specs too. The way that seems to work best by some margin is actually to block the spec changes on the test changes. Second to that, filing a issue on web-platform-tests that tests are missing, but it's quite likely those issues will sit idle unless they're part of someone's triage queue. |
|
I couldn't find everyone on GitHub, so I asked people on https://chromium-review.googlesource.com/c/chromium/src/+/756142 to take a look here. @fsoder I know though :) |
|
The tests in the commit referenced above are not testing the spec change here (the code change isn't actually landed yet.) We have some existing tests that should cover this (what caused #413), that I hope will be exported when the code change lands. |
|
@fsoder, that's great! Upstreaming the tests with the implementation is really ideal, no extra work for the editors. Is the review for that already up, or a tracking bug? Also found @k2nagaraju via #413. FYI ^ |
|
thanks everyone. the recent clean-up efforts are basically done, so this is a good time to get the test suite and implementations all up to date and squeaky clean! |
|
Yes the review is up, and essentially only missing said test updates (IIRC). |
|
I think https://chromium-review.googlesource.com/c/chromium/src/+/845462 is the review, certainly the code changes look like they match the spec changes in this PR. |
Closes #413