Closed Bug 1276130 Opened 9 years ago Closed 8 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
Other Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

Bug 1276130 - part1 : replace the value 'middle' with 'center.
58 bytes, text/x-review-board-request
rillian
: review+
smaug
: review+
Details
Bug 1276130 - part2 : modify test.
58 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1276130 - part3 : modify web-platform test.
58 bytes, text/x-review-board-request
rillian
: review+
Details
https://w3c.github.io/webvtt/#enumdef-alignsetting We use "middle", but the value should be "center".
Assignee: nobody → alwu
Summary: Correct the enum value for VTTCue's AlignSetting → [webvtt] Correct the enum value for VTTCue's AlignSetting
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/
Comment on attachment 8758618 [details] Bug 1276130 - part2 : modify test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56876/diff/1-2/
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)
(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+
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
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/
Comment on attachment 8758618 [details] Bug 1276130 - part2 : modify test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56876/diff/2-3/
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/
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
Flags: needinfo?(alwu)
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/
Comment on attachment 8758618 [details] Bug 1276130 - part2 : modify test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56876/diff/3-4/
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/
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)
Comment on attachment 8758618 [details] Bug 1276130 - part2 : modify test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56876/diff/4-5/
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/
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+
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: