1556087 - [webvtt] enable vtt alignment related wpts

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, task, P3)

Description

[Alastor Wu [:alwu]]

Use this bug to enable and fix those alignment related wpts (align_*.html) which are under the directory webvtt/rendering/cues-with-video/processing-model.

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1556087 - part1: use default font for testing comparison.

In the reference file, we have no need to add explicitly <br> to add a break, the text would automatically wrap to another line when it overflows.

And always use the WebVTT's default font for the comparison.

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1556087 - part1 : remove unnecessary break in reference files.

In reference files align_center_position_*.html, the text Awesome!!! doesn't exceed the line box's boundary, so it won't wrap to multiple lines.

In other files, we don't have to set the break explicitly, texts would automatically wrap to multiple lines when it exceed the line box's boundary.

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1556087 - part2 : adjust 'left' property for reference cues.

For align_start.html and align_start_wrapped.html, we didn't set cue's position, so the cue box would be 100% video's width.

The alignment start [1] is to align the start side of the box, so it would align to the left side of the box, which is equal to set left: 0px.

[1] https://www.w3.org/TR/webvtt1/#webvtt-cue-start-alignment

---

For other tests, even if we change cues' position from vtt files, the cue box would still keep its left side on the position left:0 or keep its right ride on the position right:0 depending on cue's position.

If cue's position is smaller than 50, the cue box would stay left and reduce its box size. If cue's position is larger than 50 the cue box would stay right and also reduce its box size [2].

Therefore, for align_center_position_lt_50-ref and align_center_position_lt_50_size_gt_maximum_size-ref, we should set left:0 because the box is in the left side.

For align_center_position_gt_50-ref, we have to set right:0.

[2] https://searchfox.org/mozilla-central/source/dom/media/webvtt/vtt.jsm#641-685

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1556087 - part3 : remove `width`.

For those two tests, we didn't specify the box's size in vtt files, which means the box's width should be the default value, 100% width.

Comment 5

[Alastor Wu [:alwu]]

Attachment: Bug 1556087 - part3 : enable wpts.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53ae0fe008c7
part1 : remove unnecessary break in reference files. r=heycam
https://hg.mozilla.org/integration/autoland/rev/249be5c1e643
part2 : adjust 'left' property for reference cues. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e708fe457a95
part3 : enable wpts. r=heycam

Comment 7

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17332 for changes under testing/web-platform/tests

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/17332
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/BY-ieEOBQ0yJEGE5qt5i9w)

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/53ae0fe008c7
https://hg.mozilla.org/mozilla-central/rev/249be5c1e643
https://hg.mozilla.org/mozilla-central/rev/e708fe457a95

Comment 10

[Alastor Wu [:alwu]]

Hi, Jame,
I found that my PR failed on upstream checks because of small color difference which should be ignored by my FUZZY keyword. I didn't know why the FUZZY setting in those ini files didn't work. Do you have any idea?
Thank you.

Comment 11

[James Graham [:jgraham]]

The ini files from cenral aren't applied upstream. It's possible to annotate fuzziness in the test if we think it's a property of the test rather than of gecko, but I don't know if that applies in this case. Given that this is the only failure I'm happy to force merge the PR here.

Comment 12

[Alastor Wu [:alwu]]

(In reply to James Graham [:jgraham] from comment #11)

The ini files from cenral aren't applied upstream. It's possible to annotate fuzziness in the test if we think it's a property of the test rather than of gecko, but I don't know if that applies in this case. Given that this is the only failure I'm happy to force merge the PR here.

Do you mean we can set something like a fuzzy range in test itself without using ini file? If so, how should I do before merging the PR?

What I'm afraid is that if this fuzzy can't be detected, will it also affect other following PR upstream checking?

Thank you.

Comment 13

[James Graham [:jgraham]]

I already merged the PR :)

But you can set fuzzy in the test, see [1]. However note that it's only really appropriate if there are reasons to think that the fuzziness applies to all browsers.

There are some plans to handle intermittents better in the CI (mostly by labelling them); I suppose we could also consider syncing over fuzzy annotations in some way for the per-browser checks. I'll think about the best approach there.

[1] https://web-platform-tests.org/writing-tests/reftests.html?highlight=fuzzy#fuzzy-matching

Comment 14

[Alastor Wu [:alwu]]

(In reply to James Graham [:jgraham] from comment #13)

I already merged the PR :)

But you can set fuzzy in the test, see [1]. However note that it's only really appropriate if there are reasons to think that the fuzziness applies to all browsers.

There are some plans to handle intermittents better in the CI (mostly by labelling them); I suppose we could also consider syncing over fuzzy annotations in some way for the per-browser checks. I'll think about the best approach there.

[1] https://web-platform-tests.org/writing-tests/reftests.html?highlight=fuzzy#fuzzy-matching

Thank you!