1557882 - [webvtt] the cue text with different base direction should align to different side of the cue box

Status: RESOLVED FIXED

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

Description

[Alastor Wu [:alwu]]

STR.

1. go to https://alastor0325.github.io/htmltests/webvtt/different-base-direction.html

Expected.
2. Hello! is align to the left side of the video, and שלום! is align to the right side of the video.

Actual.
2. both line are aligned to the left side of the video.

Note, this issue didn't happen on Firefox Release 67, but I can reproduce it on the latest Nightly

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1ed43c15c0ea97e68338cc1800a2ba9739bc7c32&tochange=7626a0df2e262a4afd6220a0dec6bef8d6790706

Regressed by: Bug 1488673

Comment 2

[Alastor Wu [:alwu]]

Hi, Jonathan,

I heard that you might be the right persion to consult for this issue.

In our VTT implementaion, I created a div element and appended a text node with Hello! (line-break) שלום! on it. Because of line-break, the layout would generate two line boxes for each line. One line for Hello! and another line for שלום!. I found that the second line, which contains RTL texts didn't align to the correct position, as I set the alignment to start, it should be aligned to the right side of the box.
(In addition, here is the frame tree I dumped from --layoutdebug [1]).

For the comparison, I created a HTML markup [2] to simulate the same situation, and I found that this reference file can work well as I expect on Chrome, but not on Firefox. Therefore, I wonder if it's a layout bug which didn't set the writing direction well when generate the new line box.

Thank you.

[1] https://pastebin.com/4fyLRD9G
[2] https://alastor0325.github.io/htmltests/webvtt/different-base-direction-ref.html

Comment 3

[Jonathan Kew [:jfkthame]]

Yes, it looks like this is a bug in how unicode-bidi:plaintext behaves. I made a reduced testcase at https://codepen.io/anon/pen/RzwQPd. Note that here, the bug occurs on the first of the three examples, but not on the other two.

The issue only occurs on the initial reflow of the document; if something triggers a new reflow (e.g. try zooming the page in the browser with Ctrl-+, or use the style editor to tweak the width property of the enclosing div), then the Hebrew line jumps to its expected position.

A key factor appears to be the fact that the forced line-break is inside a <span> rather than at the top level of the block that's using unicode-bidi:plaintext. Without that span, or with the <br> lifted out of it (as in my second and third examples) I was unable to reproduce the problem.

So the "regression" probably occurred because the previous styling didn't depend on unicode-bidi:plaintext in the same way, or else something about the previous CSS/JS was causing an additional reflow that effectively masked the bug.

There's clearly a core layout bug here, so we should fix that; but in the meantime (as it may not be an immediate fix -- bidi resolution is a complex web of code!) you may want to consider some kind of workaround.

As an interim fix, you can probably work around this (at some minor perf cost) by doing something that triggers an extra reflow of the .video element. For example, if I toggle the unicode-bidi property to initial and then back to plaintext after the document loads, this fixes the bad display: https://codepen.io/anon/pen/agbYbM.

Comment 4

[Jonathan Kew [:jfkthame]]

I filed bug 1558431 for the core layout issue here, rather than morphing this bug to that component, as I figure you may want to take a workaround in the front end here while waiting for the real fix.

Comment 5

[Alastor Wu [:alwu]]

Attachment: Bug 1557882 - part1 : delay setting 'unicode-bidi' until finishing the position calculation.

Because of bug1558431, now the text alignment for bidi won't take effect on the initial reflow of the document, so we have to delay setting 'unicode-bidi' in order to trigger reflow again.

It would make the bidi text showing correctly after reflow.

Comment 6

[Alastor Wu [:alwu]]

Attachment: Bug 1557882 - part2 : add wpt 'start_alignment.html'.

Add a new wpt 'start_alignment.html' to ensure that two cues would have different text alignment.

In addition, add line:0 in vtt file to put cue on the top in order to reduce the complexity of using CSS to markup the test, because if we don't specifiy the postion for cue, those two cues won't be put the bottom of the video, instead they would be move upward one line above the bottom according to the spec.

The reason is that the base moving unit for adjusting cue box's position is the Bsize of the first line box, it would require extra moving if one cue contains two different cues which line boxes are not the same height.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50efff6cae91
part1 : delay setting 'unicode-bidi' until finishing the position calculation. r=heycam
https://hg.mozilla.org/integration/autoland/rev/65393203d1c5
part2 : add wpt 'start_alignment.html'. r=heycam

Comment 8

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17324 for changes under testing/web-platform/tests

Comment 9

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/50efff6cae91
https://hg.mozilla.org/mozilla-central/rev/65393203d1c5

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Does the user impact of this bug warrant uplift consideration or can this ride the trains?

Comment 13

[Alastor Wu [:alwu]]

As this is a rare case which require VTT file contains two different base direction in one cue, and put them on different lines, I think we can just let the fix riding the train.