Closed Bug 1276130 Opened 8 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: