[webvtt] Use PositionAlignSetting type for VTTCue's positionAlign

RESOLVED FIXED in Firefox 49

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

(Blocks 1 bug, {dev-doc-needed, site-compat})

Other Branch
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

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

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

Updated

3 years ago
Blocks: webvtt
No longer depends on: webvtt
Assignee

Updated

3 years ago
Summary: Use PositionAlignSetting type for VTTCue's position → [webvtt] Use PositionAlignSetting type for VTTCue's position
Assignee

Updated

3 years ago
Summary: [webvtt] Use PositionAlignSetting type for VTTCue's position → [webvtt] Use PositionAlignSetting type for VTTCue's positionAlign
Assignee

Comment 5

3 years ago
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/
Attachment #8758197 - Flags: review?(giles)
Attachment #8758198 - Flags: review?(giles)
Attachment #8758199 - Flags: review?(giles)
Attachment #8758200 - Flags: review?(giles)
Assignee

Comment 6

3 years ago
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/
Assignee

Comment 7

3 years ago
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/
Assignee

Comment 8

3 years ago
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/
Assignee

Comment 9

3 years ago
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 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.
Attachment #8758197 - Flags: review?(giles) → review+
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.
Attachment #8758197 - Flags: review?(bugs)
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.
Attachment #8758198 - Flags: review?(giles) → review+
Attachment #8758199 - Flags: review?(giles)
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 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.
Attachment #8758200 - Flags: review?(giles) → review+
Assignee

Comment 16

3 years ago
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/
Attachment #8758197 - Flags: review?(bugs)
Attachment #8758199 - Flags: review?(giles)
Assignee

Comment 17

3 years ago
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/
Assignee

Comment 18

3 years ago
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/
Assignee

Comment 19

3 years ago
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/
Assignee

Updated

3 years ago
Attachment #8758197 - Flags: review?(bugs)
Is this a recent spec change? It is after all not backwards compatible change.
Assignee

Comment 21

3 years ago
Our implementation didn't follow the spec for a long while, so we need to update it.
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.
Attachment #8758197 - Flags: review?(bugs) → review+
Attachment #8758199 - Flags: review?(giles) → review+
Comment on attachment 8758199 [details]
MozReview Request: Bug 1276129 - part3 : add & modify test.

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

Looks good, thanks!
Assignee

Comment 24

3 years ago
(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

3 years ago
Pushed by alwu@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.