1544455 - [webvtt] cue class spans get rendered as block elements instead of inline elements

Status: VERIFIED FIXED

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

Description

[Dennis Schubert [:denschub]]

This was reported via a webcompat report about YouTube subtitles on fennec.

Go to the testcase under https://stuff.schub.io/mozilla/testcases/webc-29281/ and skip 45 seconds into the video. In Firefox, all words are rendered as block elements, thus being displayed as a vertical list, while in Chrome, the subtitles are displayed correctly.

The WebVTT here is using a lot of timed cues,

00:00:32.529 --> 00:00:34.890 align:start position:0%
<c.colorCCCCCC>this isn't a real photograph but</c><c.colorE5E5E5> a
computer<00:00:32.890><c> graphic</c><00:00:33.340><c> rendering</c></c><c.colorCCCCCC><00:00:33.550><c> an</c><00:00:34.000><c> artistic</c></c>

and it seems like we render a new block element for each of those, although they probably should be rendered inline. This may very well be a duplicate of one of the bugs linked in bug 865395, but I'll leave this decision to people more experienced in this subject. :)

Comment 1

[John Lin [:jhlin][:jolin]]

Alastor, could you please help take a look at this one? Thanks!

Comment 2

[Alastor Wu [:alwu]]

Will check this bug after finishing other higher priority vtt bugs.

Comment 3

[Firefox Bug Husbandry Bot]

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Comment 4

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating whiteboard priority tags to program flags.

Comment 5

[Alastor Wu [:alwu]]

According to the spec [1], when cue's text alignment is start, we should return either line-left or line-right depending on the cue text's base direction. However, we didn't implement that, so we would return center [2], which results in incorrect calculation for cue box's size [3] and also causes those <c> element placing on different lines.

[1] https://www.w3.org/TR/webvtt1/#cue-computed-position-alignment
[2] https://searchfox.org/mozilla-central/rev/fe7dbedf223c0fc4b37d5bd72293438dfbca6cec/dom/media/TextTrackCue.cpp#209
[3] https://searchfox.org/mozilla-central/rev/fe7dbedf223c0fc4b37d5bd72293438dfbca6cec/dom/media/webvtt/vtt.jsm#645-655

Comment 6

[Alastor Wu [:alwu]]

First, the spec didn't have a definition about what the base direction of cue text is, there is only a definition about the base direction for each line, [1] which is to check the first character to see if it's LTR or RTL. Because of lacking of definition, I would assume that the base direction of cue text is the base direction of the first line in the cue text.

And then, now the problem is that how do we know the base direction of cue text in TextTrackCue?

I was looking for if there is any function I can use to require a text direction by feeding an input string, but I couldn't find it. So there are several possible solutions.

1. Implement a fucntion to return the base direction for the input string and then TextTrackCue can return the correct value based on the spec.
2. Use some way to get the frame in TextTrackCue and get the base direction from the frame
   • the problem is that we have to append DIV element to text track cue early before doing any calculation. Now we would only assign the DIV to TextTrackCue when we finish the calculation [2]. This method would need to assign the DIV to TextTrackCue before any calcution, which is quite hacky.
3. When calling computedPositionAlign [3], we won't return either line-left or line-right in step3 and 4 [4], and instead to return a special value and then let vtt.jsm to handle this value to check the base direction via a Chrome-API method.

I prefer the first method, but I don't know if it's possible.

[1] https://www.w3.org/TR/webvtt1/#webvtt-cue-text-alignment
[2] https://searchfox.org/mozilla-central/rev/fe7dbedf223c0fc4b37d5bd72293438dfbca6cec/dom/media/webvtt/vtt.jsm#1221
[3] https://searchfox.org/mozilla-central/rev/fe7dbedf223c0fc4b37d5bd72293438dfbca6cec/dom/media/webvtt/vtt.jsm#645
[4] https://www.w3.org/TR/webvtt1/#cue-computed-position-alignment

---

Hi, heycam,
May I have your opinion here?
Thank you!

Comment 7

[Alastor Wu [:alwu]]

Attachment: Bug 1544455 - part1 : check the base direction of cue text in 'ComputedPositionAlign()'.

According to the spec [1], when text alignment is start or end, we have to check the base direction of the cue text in order to decide the position alignment.

As the spec only defines what the base direction is for each line [2], it didn't clearly define what the base direction is for the cue text.

Therefore, we would use the first line's base direction as the result for the base direction of the cue text if cue text contains multiple lines.

[1] https://www.w3.org/TR/webvtt1/#cue-computed-position-alignment
[2] https://www.w3.org/TR/webvtt1/#webvtt-cue-text-alignment

Comment 8

[Alastor Wu [:alwu]]

Attachment: Bug 1544455 - part2 : modify test 'test_webvtt_positionalign.html'.

Add new test cases for alignment start and end for both LTR and RTL text.

Comment 9

[Alastor Wu [:alwu]]

Attachment: Bug 1544455 - part3 : consider the result of computed position alignment to return correct computed position.

In the spec [1], it only considers text's alignment when computing the position. However, the text alignment start and end can make the same result of what left and right make, depending on what direction the text uses.

For example, setting text alignment left is equal to start for the LTR text. Therefore, we could check the result of ComputedPositionAlign, which would return correctly direction automatically accoding to the text's base direction.

[1] https://www.w3.org/TR/webvtt1/#cue-computed-position

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c84d2f6f5130
part1 : check the base direction of cue text in 'ComputedPositionAlign()'. r=heycam
https://hg.mozilla.org/integration/autoland/rev/0e1b6cb00cd8
part2 : modify test 'test_webvtt_positionalign.html'. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e9b8793567dd
part3 : consider the result of computed position alignment to return correct computed position. r=heycam

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c84d2f6f5130
https://hg.mozilla.org/mozilla-central/rev/0e1b6cb00cd8
https://hg.mozilla.org/mozilla-central/rev/e9b8793567dd

Comment 12

[Alastor Wu [:alwu]]

Comment on attachment 9072710 [details]
Bug 1544455 - part1 : check the base direction of cue text in 'ComputedPositionAlign()'.

Beta/Release Uplift Approval Request

• User impact if declined: A sentense of subtitle would be broke into multiple words and showing each word indepedently on new line, instead of showing all of them in one line. It really interferes people reading subtitle. It affects major video website, such like youtube.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. go to https://alastor0325.github.io/htmltests/webvtt/align-start-and-position-zero.html

2. to see if subtitles are showing correctly

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): These patches only affect those subtitles which have been set position alignment explicitly as start or end, so it won't affect all subtitles. In addition, we have also added an automation test for it.
• String changes made/needed:

Comment 13

[Hani Yacoub, Desktop QA]

I confirm that the subtitle in "https://alastor0325.github.io/htmltests/webvtt/align-start-and-position-zero.html" website is displayed correctly on the latest Firefox Nightly 70.0a1 (2019-07-09) on Windows 10 x64, Mac OS X 10.15 and on Ubuntu 18.04 x64.

But when I accessed the example in comment 1 (https://stuff.schub.io/mozilla/testcases/webc-29281/) the subtitle is displayed different comparing to Chrome and here I'm referring to how the subtitle is highlighted depending on the progress of the video.
I'm not sure about the expected behavior here. Could you please clarify it to me?
Thanks.

Comment 14

[Alastor Wu [:alwu]]

That is something we haven't supported yet, :past and :future pseudo class which can be used to set different CSS styles on cues depending on the playback time.

Comment 15

[Hani Yacoub, Desktop QA]

Got it! Thanks for the response.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9072710 [details]
Bug 1544455 - part1 : check the base direction of cue text in 'ComputedPositionAlign()'.

Fixes a bug causing subtitle sentences to be broken up, causing readability problems. Approved for 69.0b4.

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2bd470454f72
https://hg.mozilla.org/releases/mozilla-beta/rev/66e1967c3c5f
https://hg.mozilla.org/releases/mozilla-beta/rev/66c851b17882

Comment 18

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 69.0b4 on Windows 10 x64, Mac OS X 10.15, Mac OS X 10.14 and on Ubuntu 18.04 x64.