Bug 136627

Summary: WebVTT: vertical cue text alignment is the wrong way around
Product: WebKit Reporter: Silvia Pfeiffer <silviapf>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: REOPENED ---    
Severity: Normal CC: calvaris, cdumez, commit-queue, darin, eoconnor, eric.carlson, esprehn+autocc, ews, gyuyoung.kim, jonlee, philipj, rniwa, sroberts, tsavell, vcarbune, webkit-bug-importer, webkit
Priority: P2 Keywords: InRadar
Version: 525.x (Safari 3.2)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews105 for mac-highsierra-wk2
none
Archive of layout-test-results from ews116 for mac-highsierra
none
Archive of layout-test-results from ews211 for win-future
none
Patch
none
Address post review comments
commit-queue: commit-queue-
Patch commit-queue: commit-queue-

Description Silvia Pfeiffer 2014-09-08 06:22:48 PDT
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.
Comment 1 Gary Katsevman 2019-04-08 15:01:43 PDT
I've created a live test case for this: https://run.plnkr.co/plunks/odXryMPG4CZuIaUd/
Comment 2 Radar WebKit Bug Importer 2019-04-08 23:21:20 PDT
<rdar://problem/49725538>
Comment 3 Gary Katsevman 2019-04-25 13:40:20 PDT
Created attachment 368261 [details]
Patch
Comment 4 Build Bot 2019-04-25 18:37:21 PDT
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
Comment 5 Build Bot 2019-04-25 18:37:23 PDT
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
Comment 6 Eric Carlson 2019-05-02 10:20:23 PDT
Created attachment 368784 [details]
Patch
Comment 7 Build Bot 2019-05-02 11:09:31 PDT
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
Comment 8 Build Bot 2019-05-02 11:09:35 PDT
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
Comment 9 Build Bot 2019-05-02 11:18:18 PDT
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
Comment 10 Build Bot 2019-05-02 11:18:22 PDT
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
Comment 11 Build Bot 2019-05-02 12:13:40 PDT
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
Comment 12 Build Bot 2019-05-02 12:13:42 PDT
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
Comment 13 Build Bot 2019-05-02 12:36:04 PDT
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
Comment 14 Build Bot 2019-05-02 12:36:11 PDT
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
Comment 15 Eric Carlson 2019-05-02 12:50:22 PDT
Created attachment 368811 [details]
Patch
Comment 16 WebKit Commit Bot 2019-05-02 14:43:31 PDT
Comment on attachment 368811 [details]
Patch

Clearing flags on attachment: 368811

Committed r244891: <https://trac.webkit.org/changeset/244891>
Comment 17 WebKit Commit Bot 2019-05-02 14:43:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Darin Adler 2019-05-02 14:46:02 PDT
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?
Comment 19 Eric Carlson 2019-05-03 10:11:34 PDT
Reopening to attach new patch.
Comment 20 Eric Carlson 2019-05-03 10:11:35 PDT
Created attachment 368944 [details]
Address post review comments
Comment 21 WebKit Commit Bot 2019-05-03 10:14:40 PDT
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
Comment 22 Shawn Roberts 2019-05-03 10:17:49 PDT
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]
Comment 23 Eric Carlson 2019-05-03 10:35:01 PDT
Created attachment 368947 [details]
Patch
Comment 24 Eric Carlson 2019-05-03 10:37:29 PDT
(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
Comment 25 WebKit Commit Bot 2019-05-03 10:39:18 PDT
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
Comment 26 Truitt Savell 2019-05-06 09:14:53 PDT
Remarked test as failing for Mojave in: https://trac.webkit.org/changeset/244962/webkit while test is investigated further.