Closed caption rendering of proper cue alignment from a WebVTT file is broken. Left alignment is rendered on the right, right alignment is rendered on the left.
rdar://problem/32788929
Created attachment 350876 [details] Patch
Comment on attachment 350876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350876&action=review > Source/WebCore/html/track/TextTrackCueGeneric.cpp:173 > +ExceptionOr<void> TextTrackCueGeneric::setPosition(const Variant<double, AutoKeyword>& position) Nit: "Variant<double, AutoKeyword>" is used often enough that it would be good to define a type for it: using LineAndPositionSetting = Variant<double, AutoKeyword>; > Source/WebCore/html/track/VTTCue.cpp:401 > + Variant<double, AutoKeyword> pos; > + if (textPositionIsAuto()) > + pos = Auto; > + else > + pos = m_textPosition; > + return pos; Nit: this can be reduced to something like: return textPositionIsAuto() ? Auto : m_textPosition; > Source/WebCore/html/track/VTTCue.cpp:412 > + if (position.index() == 1) { I think WTF::holds_alternative<> would be cleaner: WTF::holds_alternative<AutoKeyword>(position)
(In reply to Eric Carlson from comment #3) > Comment on attachment 350876 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350876&action=review > > > Source/WebCore/html/track/TextTrackCueGeneric.cpp:173 > > +ExceptionOr<void> TextTrackCueGeneric::setPosition(const Variant<double, AutoKeyword>& position) > > Nit: "Variant<double, AutoKeyword>" is used often enough that it would be > good to define a type for it: > > using LineAndPositionSetting = Variant<double, AutoKeyword>; > > > Source/WebCore/html/track/VTTCue.cpp:401 > > + Variant<double, AutoKeyword> pos; > > + if (textPositionIsAuto()) > > + pos = Auto; > > + else > > + pos = m_textPosition; > > + return pos; > > Nit: this can be reduced to something like: > > return textPositionIsAuto() ? Auto : m_textPosition; > > > Source/WebCore/html/track/VTTCue.cpp:412 > > + if (position.index() == 1) { > > I think WTF::holds_alternative<> would be cleaner: > > WTF::holds_alternative<AutoKeyword>(position) Thanks for reviewing! I will update the patch.
Created attachment 350883 [details] Patch
Comment on attachment 350883 [details] Patch Attachment 350883 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9359332 New failing tests: media/track/track-webvtt-tc016-align-positioning.html media/track/track-cue-mutable.html media/track/track-vttcue.html media/track/track-webvtt-tc021-valign.html media/track/track-webvtt-tc018-align-text-line-position.html media/track/track-add-remove-cue.html media/track/track-webvtt-tc015-positioning.html media/track/track-webvtt-tc013-settings.html
Created attachment 350895 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 350883 [details] Patch Attachment 350883 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9359515 New failing tests: media/track/track-webvtt-tc018-align-text-line-position.html media/track/track-cue-mutable.html media/track/track-vttcue.html media/track/track-webvtt-tc021-valign.html media/track/track-webvtt-tc016-align-positioning.html media/track/track-add-remove-cue.html media/track/track-webvtt-tc015-positioning.html media/track/track-webvtt-tc013-settings.html
Created attachment 350901 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 350905 [details] Patch
Comment on attachment 350905 [details] Patch Clearing flags on attachment: 350905 Committed r236531: <https://trac.webkit.org/changeset/236531>
Looks like the new test from https://trac.webkit.org/changeset/236531/webkit media/track/track-cue-left-align.html has been having image failures History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Ftrack%2Ftrack-cue-left-align.html Looks like it has a reference mismatch. https://trac.webkit.org/browser/trunk/LayoutTests/media/track/track-cue-left-align-expected-mismatch.html