1276129 - [webvtt] Use PositionAlignSetting type for VTTCue's positionAlign

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Alastor Wu [:alwu]]

https://w3c.github.io/webvtt/#enumdef-positionalignsetting

Now we use AlignSetting for the position, and it's wrong.

Comment 1

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

Review commit: https://reviewboard.mozilla.org/r/56532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56532/

Comment 2

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1276129 - part2 : change PositionAlign value in vtt.jsm.

Review commit: https://reviewboard.mozilla.org/r/56534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56534/

Comment 3

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1276129 - part3 : add & modify test.

* * *
Bug 1276129 - part : modify test.

Review commit: https://reviewboard.mozilla.org/r/56536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56536/

Comment 4

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1276129 - part4 : add webvtt tag

Review commit: https://reviewboard.mozilla.org/r/56538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56538/

Comment 5

[Alastor Wu [:alwu]]

Comment on attachment 8758197 [details]
MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56532/diff/1-2/

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8758198 [details]
MozReview Request: Bug 1276129 - part2 : change PositionAlign value in vtt.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56534/diff/1-2/

Comment 7

[Alastor Wu [:alwu]]

Comment on attachment 8758199 [details]
MozReview Request: Bug 1276129 - part3 : add & modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56536/diff/1-2/

Comment 8

[Alastor Wu [:alwu]]

Comment on attachment 8758200 [details]
MozReview Request: Bug 1276129 - part4 : add webvtt tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56538/diff/1-2/

Comment 9

[Alastor Wu [:alwu]]

Hi, Ralph,
Could you help me review these patches?
The purpose of this bug is to use PositionAlignSetting for VTTCue's positionAlign, instead of using AlignSetting [1].
Thanks!

[1] https://w3c.github.io/webvtt/#enumdef-positionalignsetting

Comment 10

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=928184f60e91&selectedJob=21773071

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8758197 [details]
MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

https://reviewboard.mozilla.org/r/56532/#review53782

Looks good to me. This will need dom peer review as well.

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8758197 [details]
MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

Adding smaug for dom peer review.

Note that similar changes are needed to add the LineAlignSetting enum. That's being tracked in bug 1276830.

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8758198 [details]
MozReview Request: Bug 1276129 - part2 : change PositionAlign value in vtt.jsm.

https://reviewboard.mozilla.org/r/56534/#review53784

::: dom/media/webvtt/vtt.jsm:787
(Diff revision 2)
>      // position of the cue box. The reference edge will be resolved later when
>      // the box orientation styles are applied.
>      var textPos = 0;
> -    switch (cue.positionAlign) {
> -    case "start":
> +    switch (cue.computedPositionAlign) {
> +      // TODO : modify these fomula to follow the spec.
> +      case "line-left":

Please file a new bug now and refernce the bug number in the comment here to make sure it's easy to follow-up.

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 8758199 [details]
MozReview Request: Bug 1276129 - part3 : add & modify test.

https://reviewboard.mozilla.org/r/56536/#review53790

::: dom/media/test/chrome.ini:8
(Diff revision 2)
> +  seek.webm
> +  seek.webm^headers^
> +  vttPositionAlign.vtt
> +
> +[test_webvtt_positionalign.html]
> +tags = webvtt

chrome.ini tests are not compatible with e10s. Please remove the chrome.ini reference and use `SpecialPowers.wrap()` to access chrome-only attributes like `computedPositionAlign` inside a normal mochitest.

See test_texttrack_cue_moz.html for an example.

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8758200 [details]
MozReview Request: Bug 1276129 - part4 : add webvtt tag

https://reviewboard.mozilla.org/r/56538/#review53792

Good idea! Make sure to update this after moving the test in Part 3.

Comment 16

[Alastor Wu [:alwu]]

Comment on attachment 8758197 [details]
MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56532/diff/2-3/

Comment 17

[Alastor Wu [:alwu]]

Comment on attachment 8758198 [details]
MozReview Request: Bug 1276129 - part2 : change PositionAlign value in vtt.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56534/diff/2-3/

Comment 18

[Alastor Wu [:alwu]]

Comment on attachment 8758199 [details]
MozReview Request: Bug 1276129 - part3 : add & modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56536/diff/2-3/

Comment 19

[Alastor Wu [:alwu]]

Comment on attachment 8758200 [details]
MozReview Request: Bug 1276129 - part4 : add webvtt tag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56538/diff/2-3/

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Is this a recent spec change? It is after all not backwards compatible change.

Comment 21

[Alastor Wu [:alwu]]

Our implementation didn't follow the spec for a long while, so we need to update it.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8758197 [details]
MozReview Request: Bug 1276129 - part1 : introduce PositionAlignSetting.

https://reviewboard.mozilla.org/r/56532/#review53922

ok, this is worrisome change. If anyone is using the attribute, this will break the usage.
And if no one is using the attribute, why to have it at all?
though, I guess the API still rather new, so I guess that could explain if it isn't being used too much.
So fine, but be careful if we see some regressions because of this.

Comment 23

[Ralph Giles (:rillian)]

Comment on attachment 8758199 [details]
MozReview Request: Bug 1276129 - part3 : add & modify test.

https://reviewboard.mozilla.org/r/56536/#review54012

Looks good, thanks!

Comment 24

[Alastor Wu [:alwu]]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #22)
> ok, this is worrisome change. If anyone is using the attribute, this will
> break the usage.
> And if no one is using the attribute, why to have it at all?
> though, I guess the API still rather new, so I guess that could explain if
> it isn't being used too much.
> So fine, but be careful if we see some regressions because of this.

Thanks for your suggestion! 

FYI, the Blink [1] and Webkit [2] doesn't support the "positionAlign" yet.
Blink has the implementation, but it's out-of-date and doesn't be enable.
Webkit doesn't have the implementation.

Therefore, I guess we might not have the regression (because no one use now :p) if we're the only one to support this attribute. 

[1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/track/vtt/VTTCue.idl&q=VTTCue&sq=package:chromium&type=cs&l=30
[2] https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/track/VTTCue.idl

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3cf9f70e5fe
part1 : introduce PositionAlignSetting. r=rillian,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7eb0adafc6
part2 : change PositionAlign value in vtt.jsm. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb00e0208622
part3 : add & modify test. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa464d5d11b
part4 : add webvtt tag r=rillian

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3cf9f70e5fe
https://hg.mozilla.org/mozilla-central/rev/1d7eb0adafc6
https://hg.mozilla.org/mozilla-central/rev/fb00e0208622
https://hg.mozilla.org/mozilla-central/rev/efa464d5d11b

Comment 27

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/vttcue-positionalign-now-uses-positionalignsetting-instead-of-alignsetting/