Bug 136627 – WebVTT: vertical cue text alignment is the wrong way around

• Component: Media
• Status: REOPENED

[Silvia Pfeiffer]

I'm using a WebVTT file like this:

WEBVTT

00:00:15.042 --> 00:00:18.042 vertical:lr
<ruby>左<rt>ひだり</rt></ruby>に<ruby> 見<rt>み</rt></ruby>えるのは...

00:00:18.750 --> 00:00:20.333 vertical:rl 
<ruby>右<rt>みぎ</rt></ruby>に<ruby> 見<rt>み</rt></ruby>えるのは...

However, the first cue is rendered on the right edge of the video viewport (it should be growing from left to right), and the second one is rendered on the left edge (it should be growing from right to left). I think they are the wrong way around.

[Gary Katsevman]

I've created a live test case for this: https://run.plnkr.co/plunks/odXryMPG4CZuIaUd/

[Radar WebKit Bug Importer]

<rdar://problem/49725538>

[Gary Katsevman]

Created attachment 368261 [details]
Patch

[EWS Watchlist]

Comment on attachment 368261 [details]
Patch

Attachment 368261 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12000420

New failing tests:
media/track/track-cue-rendering-vertical.html

[EWS Watchlist]

Created attachment 368294 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Eric Carlson]

Created attachment 368784 [details]
Patch

[EWS Watchlist]

Comment on attachment 368784 [details]
Patch

Attachment 368784 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12063531

New failing tests:
media/track/track-cue-rendering-vertical.html

[EWS Watchlist]

Created attachment 368789 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 368784 [details]
Patch

Attachment 368784 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12063537

New failing tests:
media/track/track-cue-rendering-vertical.html

[EWS Watchlist]

Created attachment 368792 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 368784 [details]
Patch

Attachment 368784 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12063744

New failing tests:
media/track/track-cue-rendering-vertical.html

[EWS Watchlist]

Created attachment 368802 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 368784 [details]
Patch

Attachment 368784 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12063946

New failing tests:
svg/dynamic-updates/SVGLinearGradientElement-svgdom-href-prop.html

[EWS Watchlist]

Created attachment 368808 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Eric Carlson]

Created attachment 368811 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 368811 [details]
Patch

Clearing flags on attachment: 368811

Committed r244891: <https://trac.webkit.org/changeset/244891>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 368811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368811&action=review

> Source/WebCore/html/track/VTTCue.cpp:184
> +        setInlineStyleProperty(CSSPropertyLeft, makeString("calc(-", FormattedNumber::fixedWidth(videoSize.width(), 2), "px - ", FormattedNumber::fixedWidth(cue->getCSSSize(), 2), "px)"));

You should just be able to write videoSize.width() and cue->getCSSSize() and not use FormattedNumber. That should use "shortest" floating point formatting, which should do what we want, even better than fixed width would, without any unnecessary rounding.

Would you be willing to try that?

[Eric Carlson]

Reopening to attach new patch.

[Eric Carlson]

Created attachment 368944 [details]
Address post review comments

[WebKit Commit Bot]

Comment on attachment 368944 [details]
Address post review comments

Rejecting attachment 368944 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 368944, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/12089014

[Shawn Roberts]

After changes in https://trac.webkit.org/changeset/244891

media/track/track-cue-rendering-vertical.html is passing on High Sierra, but after being unmarked as a failure it is failing on Mojave testers still.

Possibly just need a rebaseline for Mojave?

Diff:

--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/media/track/track-cue-rendering-vertical-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/media/track/track-cue-rendering-vertical-actual.txt
@@ -61,4 +61,4 @@
         RenderText {#text} at (0,0) size 14x111
           text run at (0,0) width 111: "Cue 6: \x{79C1}\x{306F}\x{7ACB}\x{6D3E}\x{306A}\x{4EBA}"
 layer at (8,238) size 320x10
-  RenderButton {BUTTON} at (0,230) size 320x10
+  RenderButton {BUTTON} at (0,230) size 320x10 [color=#000000D8]

[Eric Carlson]

Created attachment 368947 [details]
Patch

[Eric Carlson]

(In reply to Darin Adler from comment #18)
> Comment on attachment 368811 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368811&action=review
> 
> > Source/WebCore/html/track/VTTCue.cpp:184
> > +        setInlineStyleProperty(CSSPropertyLeft, makeString("calc(-", FormattedNumber::fixedWidth(videoSize.width(), 2), "px - ", FormattedNumber::fixedWidth(cue->getCSSSize(), 2), "px)"));
> 
> You should just be able to write videoSize.width() and cue->getCSSSize() and
> not use FormattedNumber. That should use "shortest" floating point
> formatting, which should do what we want, even better than fixed width
> would, without any unnecessary rounding.
> 
> Would you be willing to try that?

That does work, thanks for the suggestion

[WebKit Commit Bot]

Comment on attachment 368947 [details]
Patch

Rejecting attachment 368947 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 368947, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/12089312

[Truitt Savell]

Remarked test as failing for Mojave in: https://trac.webkit.org/changeset/244962/webkit while test is investigated further.