[webvtt] Correct the enum value for VTTCue's AlignSetting

RESOLVED FIXED in Firefox 50

Status

()

P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Other Branch
mozilla50
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

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

We use "middle", but the value should be "center".
(Assignee)

Updated

3 years ago
Assignee: nobody → alwu
Priority: -- → P2
(Assignee)

Updated

3 years ago
Summary: Correct the enum value for VTTCue's AlignSetting → [webvtt] Correct the enum value for VTTCue's AlignSetting
(Assignee)

Comment 1

3 years ago
Created attachment 8758617 [details]
Bug 1276130 - part1 : replace the value 'middle' with 'center.

Review commit: https://reviewboard.mozilla.org/r/56874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56874/
(Assignee)

Comment 4

3 years ago
Created attachment 8759077 [details]
Bug 1276130 - part3 : modify web-platform test.

Review commit: https://reviewboard.mozilla.org/r/57160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57160/
(Assignee)

Comment 5

3 years ago
Comment on attachment 8758617 [details]
Bug 1276130 - part1 : replace the value 'middle' with 'center.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56874/diff/1-2/
(Assignee)

Comment 6

3 years ago
Comment on attachment 8758618 [details]
Bug 1276130 - part2 : modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56876/diff/1-2/
(Assignee)

Updated

3 years ago
Attachment #8758617 - Attachment description: MozReview Request: Bug 1276130 - part1 : replace the value 'middle' with 'center. → Bug 1276130 - part1 : replace the value 'middle' with 'center.
Attachment #8758618 - Attachment description: MozReview Request: Bug 1276130 - part2 : modify test. → Bug 1276130 - part2 : modify test.
Attachment #8759077 - Attachment description: MozReview Request: Bug 1276130 - part3 : modify web-platform test. → Bug 1276130 - part3 : modify web-platform test.
Attachment #8758617 - Flags: review?(giles)
Attachment #8758617 - Flags: review?(bugs)
Attachment #8758618 - Flags: review?(giles)
Attachment #8759077 - Flags: review?(giles)
Comment on attachment 8758617 [details]
Bug 1276130 - part1 : replace the value 'middle' with 'center.

https://reviewboard.mozilla.org/r/56874/#review54296

r+ for the .webidl
Attachment #8758617 - Flags: review?(bugs) → review+
Comment on attachment 8758617 [details]
Bug 1276130 - part1 : replace the value 'middle' with 'center.

https://reviewboard.mozilla.org/r/56874/#review54300

Actually, not so sure about this want. Also other browsers use dictionary with "middle".
Attachment #8758617 - Flags: review+ → review?(bugs)
(Assignee)

Comment 13

3 years ago
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #11)
> Actually, not so sure about this. Also other browsers use dictionary with "middle".

OK, if other browsers still use "middle", we can put this bug on hold.

> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/
> track/vtt/VTTCue.idl?q=VTTCue.idl&sq=package:chromium&l=52
> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/
> track/vtt/VTTCue.idl?q=VTTCue.idl&sq=package:chromium&l=36

Yes, the AlignSetting is used for text alignment [1].

[1] https://w3c.github.io/webvtt/#webvtt-cue-text-alignment
Comment on attachment 8758617 [details]
Bug 1276130 - part1 : replace the value 'middle' with 'center.

https://reviewboard.mozilla.org/r/56874/#review54310

Yeah, I think we should wait for some response to the spec bug.
Comment on attachment 8758617 [details]
Bug 1276130 - part1 : replace the value 'middle' with 'center.

https://reviewboard.mozilla.org/r/56874/#review54530

I agree waiting for consensus on the spec bug is prudent. r=me as far as the implementation goes.
Attachment #8758617 - Flags: review?(giles) → review+
Comment on attachment 8759077 [details]
Bug 1276130 - part3 : modify web-platform test.

https://reviewboard.mozilla.org/r/57160/#review54534

testing/web-platform/README.md actually says we prefer to have the tests match the spec and x-fail them if our implementation is different, so sounds like we can do this either way, but would need to revert it if the spec changes back to 'middle'.
Attachment #8759077 - Flags: review?(giles) → review+
(Assignee)

Comment 18

3 years ago
Hi, Ralph & Smaug,
It seems that Chromium and Webkit would also modify this property to follow the spec [1].
So, should we keep this bug moving forward :)?
Thanks!

[1] https://github.com/w3c/webvtt/issues/301#issuecomment-224332702
Flags: needinfo?(giles)
Flags: needinfo?(bugs)
Sure. Need to just keep an eye that their bugs actually get fixed. It wouldn't be the first time it is said "yes, we'll change the behavior" and then years later we notice FF is broken on some sites because others haven't changed the behavior after all.
Flags: needinfo?(bugs)
Works for me too.
Flags: needinfo?(giles)
Keywords: dev-doc-needed
(Assignee)

Comment 22

3 years ago
Comment on attachment 8758617 [details]
Bug 1276130 - part1 : replace the value 'middle' with 'center.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56874/diff/2-3/
(Assignee)

Comment 23

3 years ago
Comment on attachment 8758618 [details]
Bug 1276130 - part2 : modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56876/diff/2-3/
(Assignee)

Comment 24

3 years ago
Comment on attachment 8759077 [details]
Bug 1276130 - part3 : modify web-platform test.

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

Comment 25

3 years ago
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f17eaa46dd9
part1 : replace the value 'middle' with 'center. r=rillian,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb956a4232b0
part2 : modify test. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/04b5629f3008
part3 : modify web-platform test. r=rillian
(Assignee)

Updated

3 years ago
Flags: needinfo?(alwu)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8758617 [details]
Bug 1276130 - part1 : replace the value 'middle' with 'center.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56874/diff/3-4/
(Assignee)

Comment 28

3 years ago
Comment on attachment 8758618 [details]
Bug 1276130 - part2 : modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56876/diff/3-4/
(Assignee)

Comment 29

3 years ago
Comment on attachment 8759077 [details]
Bug 1276130 - part3 : modify web-platform test.

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

Comment 30

3 years ago
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad647bf5727
part1 : replace the value 'middle' with 'center. r=rillian,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5ef5c3c8ac
part2 : modify test. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/212533217731
part3 : modify web-platform test. r=rillian
Backed out for web-platform-test failure invttcue-interface/align.html.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dafb7a602e103ca54423ac342a7fbb9ca67a813d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0271888ce063a91cf234ba5be97309c2be4f31df
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e33cc58b91bed31ea605830b16311272de8db8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2125332177315968bfe9b26bdef9b9b2989f4e66
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=30120533&repo=mozilla-inbound

07:50:02     INFO - TEST-START | /webvtt/webvtt-api-for-browsers/vttcue-interface/align.html
07:50:02     INFO - PROCESS | 3024 | 1465915802739	Marionette	INFO	sendAsync 08358678-d828-4775-8280-16d3719a2d07
07:50:02     INFO - PROCESS | 3024 | 1465915802769	Marionette	INFO	sendAsync 08358678-d828-4775-8280-16d3719a2d07
07:50:02     INFO - 
07:50:02     INFO - TEST-UNEXPECTED-FAIL | /webvtt/webvtt-api-for-browsers/vttcue-interface/align.html | VTTCue.align, script-created cue - assert_equals: expected "middle" but got "center"
07:50:02     INFO - @http://web-platform.test:8000/webvtt/webvtt-api-for-browsers/vttcue-interface/align.html:11:5
07:50:02     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1398:20
07:50:02     INFO - test@http://web-platform.test:8000/resources/testharness.js:496:9
07:50:02     INFO - @http://web-platform.test:8000/webvtt/webvtt-api-for-browsers/vttcue-interface/align.html:7:1
07:50:02     INFO - 
07:50:02     INFO - TEST-UNEXPECTED-FAIL | /webvtt/webvtt-api-for-browsers/vttcue-interface/align.html | VTTCue.align, parsed cue - assert_equals: expected "middle" but got "center"

mochitest-media is also failing again: https://treeherder.mozilla.org/logviewer.html#?job_id=30121731&repo=mozilla-inbound
Flags: needinfo?(alwu)
(Assignee)

Comment 32

3 years ago
Comment on attachment 8758618 [details]
Bug 1276130 - part2 : modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56876/diff/4-5/
(Assignee)

Comment 33

3 years ago
Comment on attachment 8759077 [details]
Bug 1276130 - part3 : modify web-platform test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57160/diff/3-4/
(Assignee)

Comment 34

3 years ago
Comment on attachment 8759077 [details]
Bug 1276130 - part3 : modify web-platform test.

Hi, Ralph,
Could you help me review it again?
I re-enable the "align.html" in bug1016925, so I need to modify this file as well.
Thanks!
Flags: needinfo?(alwu)
Attachment #8759077 - Flags: review+ → review?(giles)
Comment on attachment 8759077 [details]
Bug 1276130 - part3 : modify web-platform test.

https://reviewboard.mozilla.org/r/57160/#review56362

Looks fine.
Attachment #8759077 - Flags: review?(giles) → review+

Comment 37

3 years ago
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab146e4b75ae
part1 : replace the value 'middle' with 'center. r=rillian,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/af6531f23c88
part2 : modify test. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b143d90a9f
part3 : modify web-platform test. r=rillian

Comment 38

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab146e4b75ae
https://hg.mozilla.org/mozilla-central/rev/af6531f23c88
https://hg.mozilla.org/mozilla-central/rev/f3b143d90a9f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/Web_Video_Text_Tracks_Format

Also updated Firefox 50 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.