1305732 - [webvtt] cue box should be restricted placing inside the video rendering area

Status: VERIFIED FIXED

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

Description

[John]

Attachment: WebVTT pics.docx

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160922113459

Steps to reproduce:

Using http://ronallo.com/demos/webvtt-cue-settings/
WebVTT file matched to video with left-aligned subtitles
Position Cue Setting changed to 8
Videos are played.

00:00.000 --> 00:33.000 line:13 position:8% align:left size:100%
This is a placeholder caption.



Actual results:

Using http://ronallo.com/demos/webvtt-cue-settings/
WebVTT file matched to video with left-aligned subtitles
Position Cue Setting changed to 8
Videos are played.
Subtitles are shown half cutoff due to them being off screen on the left side

00:00.000 --> 00:33.000 line:13 position:8% align:left size:100%
This is a placeholder caption.

This is present in Branches 48-51


Expected results:

WebVTT file matched to video with left-aligned subtitles inside of file. 
Videos are played.
Subtitles should display normally according to WebVTT alignment

Comment 1

[Blake Wu [:bwu][:blakewu]]

Ni Benjamin to get his attention.

Comment 2

[John]

My apologies, the stated versions that this is present in is actually versions 49-51, it is not currently present in version 48.

Comment 3

[Blake Wu [:bwu][:blakewu]]

Benjamin,
I know this is more related to what alwu used to work on, but I am afraid he has too many things on his plate recently. 
Can you help on this one?

Comment 4

[Blake Wu [:bwu][:blakewu]]

Just discussing with alwu offline, this one is easy to him. No sweat. :-)

Comment 5

[John]

Let me know if you guys would like more test cases, info, etc. I believe this bug also affects all right aligned VTT subtitles as well.

Comment 6

[Alastor Wu [:alwu]]

It might be solved by bug1277437, because now we didn't follow the exactly steps defined in spec to decide the value for the position of the cue box.

In this case, the step 7-2 in "apply WebVTT cue settings" mentions that we should adjust x-position and size to place the cue within the title-safe area.

Comment 7

[Alastor Wu [:alwu]]

Link for "apply WebVTT cue settings" : https://w3c.github.io/webvtt/#apply-webvtt-cue-settings

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Blake - can you assign a priority to this?

Comment 9

[Alastor Wu [:alwu]]

Still can reproduce this issue, will take a look.

Comment 10

[Alastor Wu [:alwu]]

Attachment: Bug 1305732 - part1 : use correct size for calculating position.

According to the spec [1], we have calculated the size in step 7.3 and then we should use that for calculating x-position and y-position, instead of using cue's size.

[1] https://w3c.github.io/webvtt/#apply-webvtt-cue-settings

Comment 11

[Alastor Wu [:alwu]]

Attachment: Bug 1305732 - part2 : use correct font size to adjust line position.

According to the spec [1], step 10.2 said that the step is the height or width of the line box, which means the font size.

We should use font size directly, instead of using text box's width or height, because the width or height of the box would be changed when the text is wrapped to different line.

For example, if text is wrapped to two line, the height or width of the box would become 2 times of font size.

[1] https://w3c.github.io/webvtt/#apply-webvtt-cue-settings

Comment 12

[Alastor Wu [:alwu]]

Attachment: Bug 1305732 - hide cue box when its width or height is zero.

If the cue box's height or width is zero, which means that the cue box has no display area, so we should hide it.

Comment 13

[Alastor Wu [:alwu]]

According to the spec, the cue box's size will be changed dynamically depending on the position of the cue box. However, I found a text align issue, which is the texts are always aligning at the left side of the cue box.

For more clearly to see what the problem is, in [1], I fill the background cue box with blue, and the background of text box is black.

In [2], because the text-align is center, the text would be wrapped to multiple lines and keep the align. In [3], when the box became smaller which was not width enough to put some of words within the box, you can see those texts became aligning to the left side of the cue box.

However, when the cue box was moving to the right side and was in the same situation, you can see the those texts were still aligning to the left side of the cue box [4], which made the text exceed the right side of the video rendering area.

I'm wondering if there is any way to make those texts aligning to the right side of the cue box in this situation, not the left side.

There is another example [5], when the text's width exceeds the width of the parent div, it was not right align anymore.

--

Hi, Daniel,

I hear that you might be the right person to ask, may you give me any suggestion for this issue?

Thank you very much!

[1] https://drive.google.com/open?id=15haagTdHHyqf1-_fDjs8SUsTXEjK_rA4
[2] https://drive.google.com/open?id=1e_kHPRRMHBK5rx68sww_mpZQkxJ8-hbS
[3] https://drive.google.com/open?id=1s8vG-Dt6wKbhp1SHU_paKPamm5hc7BDO
[4] https://drive.google.com/open?id=12lHKisI0zT0WwC0ANihx8qVhuhshdj54
[5] https://codepen.io/alwu/pen/MxvvoQ

Comment 15

[Daniel Holbert [:dholbert]]

I don't know enough about our VTT positioning styling/markup to offer feedback on what's going on there, but at least for your codepen ([5] in previous comment): here's a possibly-clearer testcase:
https://jsfiddle.net/dholbert/u3fv0tLp/

And what you're seeing there is "safe overflow" behavior. Various things in CSS (including text-align) default to "safe" overflow -- i.e. overflowing off of the writing-mode's "end" side regardless of alignment -- to make it more likely that any overflowing+clipped content will be reachable with a scrollbar. (Browsers' scrollbars only let you scroll to see stuff that runs off the "end" side of a scrollport -- not for stuff that runs off the "start" side of a scrollport.)

I believe https://bugzilla.mozilla.org/show_bug.cgi?id=1388949 is filed on adding support for this "unsafe" text-align behavior.

In the meantime, I'm not sure what the best way is to trigger this behavior, but I have several ideas:
(1) Add an inline-block wrapper around the text, and give the outer container direction: rtl to right-align the inline-block: https://jsfiddle.net/dholbert/yrztwn4x/

(2) Or, similarly, add a "float:right" wrapper around the text: https://jsfiddle.net/dholbert/yvkmu5as/

Note that in both cases, the beginning of the long "Cccc...." string is not visible & is impossible for the user to scroll to see, which is the problem that "safe overflow" is intending to solve. So, it's worth considering how you want to handle that, if you do go down this route.

Comment 16

[Alastor Wu [:alwu]]

I found that simply setting the direction: rtl; in the outer cue box doesn't work, it seems that the unicode-bidi: plaintext; setting in the outer box interfere the direction somehow. So I have to find a way to fix this problem.

But before that, after discussed with :heycam, we thought this issue might be a spec bug as well, because the spec didn't mention how should we handle this situation when overflow happens. So now my plan is to file a spec bug and then to modify the spec to find a proper way to handle this issue.

Comment 17

[Alastor Wu [:alwu]]

Filed a spec issue [1].

[1] https://github.com/w3c/webvtt/issues/459

Comment 18

[Alastor Wu [:alwu]]

Attachment: Bug 1305732 - part1 : don't show the box with zero width or height.

Setting cue's position would also affect cue box's size which would be determined by cue's position alignment as well. If the cue box's width or height is zero, it means that this box should not be display on the screen and we should clear cue's display state as well.

Comment 19

[Alastor Wu [:alwu]]

Attachment: Bug 1305732 - part5 : set CSS properties directly on '::cue'.

According to spec [1], setting overflow-wrap: break word can avoid cue over the boundary of video rendering area.

Here are two examples [2] which are showing the difference between not having break word and having break-word respectively.

Although we have set overflow-wrap: break-word in the cue box, it seems that the inner pseudo backgroung box didn't inherit that attribute, so we have to set it explicitly.

[1] https://www.w3.org/TR/webvtt1/#applying-css-properties

[2]
Without break-word : https://jsfiddle.net/qzm5ujta/2/
With break-word : https://jsfiddle.net/zn312h6o/

Comment 20

[Alastor Wu [:alwu]]

Attachment: Bug 1305732 - part6 : modify wpts.

According to Ahem font page [1], the font should be a multiple of 5px, otherwise baseline alignment may be rendered inconsistently.

It indeed happen on the try server and I found the different rendering result among the different platforms, which seems easily to happen when we break Ahem to multiple lines.

As we change the font size, so we also have to modify the reference files in order to generate the line break.

[1] https://web-platform-tests.org/writing-tests/ahem.html

Comment 21

[Alastor Wu [:alwu]]

Attachment: Bug 1305732 - part2 : list all properties set on the root node in one place to make them clear.

In order to clearly see what properties are applied on the root node, create a function to list all of them together.

Comment 22

[Alastor Wu [:alwu]]

Attachment: Bug 1305732 - part3 : remove redundent properties setting on non-pseudo background node.

If the background node is not a pseudo element, these properties would actually be inherited, so we have no need to set them again.

Comment 23

[Alastor Wu [:alwu]]

Attachment: Bug 1305732 - part4 : background box should be a inline node.

According to the spec [1], the background box should be display:inline.

Therefore, we should create a <span>, which already is an inline element, for the background box, not a <div>, which is a block element.

[1] https://www.w3.org/TR/webvtt1/#webvtt-cue-background-box

Comment 24

[Alastor Wu [:alwu]]

Attachment: Bug 1305732 - part7 : remove duplicated declaration.

We have declared supportPseudo, remove the duplicated declaration.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d98dbb0d47b
part1 : don't show the box with zero width or height. r=heycam
https://hg.mozilla.org/integration/autoland/rev/26e86a6e1f4e
part2 : list all properties set on the root node in one place to make them clear. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b982264a086e
part3 : remove redundent properties setting on non-pseudo background node. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b38ba22de6db
part4 : background box should be a inline node. r=heycam
https://hg.mozilla.org/integration/autoland/rev/809f38c8859a
part5 : set CSS properties directly on '::cue'. r=heycam,emilio
https://hg.mozilla.org/integration/autoland/rev/81c49925fdb4
part6 : modify wpts. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4e9bdebf6639
part7 : remove duplicated declaration. r=heycam

Comment 26

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/6d98dbb0d47b
https://hg.mozilla.org/mozilla-central/rev/26e86a6e1f4e
https://hg.mozilla.org/mozilla-central/rev/b982264a086e
https://hg.mozilla.org/mozilla-central/rev/b38ba22de6db
https://hg.mozilla.org/mozilla-central/rev/809f38c8859a
https://hg.mozilla.org/mozilla-central/rev/81c49925fdb4
https://hg.mozilla.org/mozilla-central/rev/4e9bdebf6639

Comment 27

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

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

Comment 28

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

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/17713
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/HA9B7LqZRKqOqd3nUdIyCA)

Comment 29

[Alastor Wu [:alwu]]

Hi, James,

I wonder if the upstream check would rebuild the firefox based on the patches I landed or it's just executing the latest Nightly?
After I checked those falilures, it seems the nightly running on upstream didn't be applied my changes.

---

Ah, I found that the Nightly running on upstream is this one [1] which is based on the version of July 8th, but my changes was applied in central on July 9th [2].

Could you help me retrigger the checking again to see if the failure still exists?

Thank you.

[1] https://hg.mozilla.org/mozilla-central/log?rev=50fec259d5a66e44a087253fa5f9f805d32e8ef3
[2] https://hg.mozilla.org/mozilla-central/rev/4e9bdebf6639

Comment 30

[Alastor Wu [:alwu]]

Comment on attachment 9073624 [details]
Bug 1305732 - part1 : don't show the box with zero width or height.

Beta/Release Uplift Approval Request

• User impact if declined: When user set subtitles, they can define the postion where subtitles should be showed. However, subtitles would only be allowed to be showed within the video rendering area. If we don't have these patches, users would see subtitles being painted outside the video or even are not able to see subtitles, when subtitle's length exceeds the length of the subtitle's box.
• 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 ronallo.com/demos/webvtt-cue-settings

2. randomly drag the bar of "position cue setting" to see if subtitles are painted outside the video rendering area

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change won't affect all subtitles, only affect those one which has been set explicit position by users and length exceeds the length of the subtitle's box. In addition, we also added automation tests to avoid regression.
• String changes made/needed: no

Comment 31

[Timea Cernea [:tbabos][inactive]]

Reproduced on affected latest Firefox Beta 69.0b3 (64-bit) and Firefox Release 68.0
Verified - Fixed on latest Nightly 70.0a1 (Build ID: 20190709153742) on Windows 10, Mac OS 10.14 and Ubuntu 16.04.

Adding affected and verified flags, waiting for fix on Beta.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9073624 [details]
Bug 1305732 - part1 : don't show the box with zero width or height.

Fixes a bug which can cause subtitles to not appear on videos. Approved for 69.0b4.

Comment 33

[James Graham [:jgraham]]

(In reply to Alastor Wu [:alwu] from comment #29)

Hi, James,

I wonder if the upstream check would rebuild the firefox based on the patches I landed or it's just executing the latest Nightly?
After I checked those falilures, it seems the nightly running on upstream didn't be applied my changes.

Yeah it's just latest nightly. In general the sync isn't nearly clever enough to know how to block upstream CI until the relevant build becomes avaialble, so there can be cases like this where the stability checks are misleading.

Ah, I found that the Nightly running on upstream is this one [1] which is based on the version of July 8th, but my changes was applied in central on July 9th [2].

Could you help me retrigger the checking again to see if the failure still exists?

In general the procedure is just to close and reopen the relevant PR. If you don't have the necessary permissions to do that let me know and we can add you to the relevant GH team. But in this case I've done it for you :)

Comment 34

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ebd13e20a872
https://hg.mozilla.org/releases/mozilla-beta/rev/bfa35429419f
https://hg.mozilla.org/releases/mozilla-beta/rev/0e4e5645b822
https://hg.mozilla.org/releases/mozilla-beta/rev/cb42f7e76d0f
https://hg.mozilla.org/releases/mozilla-beta/rev/8c74a8452ad5
https://hg.mozilla.org/releases/mozilla-beta/rev/b399656312da
https://hg.mozilla.org/releases/mozilla-beta/rev/58db79efd8d4

Comment 35

[Alastor Wu [:alwu]]

(In reply to James Graham [:jgraham] from comment #33)

In general the procedure is just to close and reopen the relevant PR. If you don't have the necessary permissions to do that let me know and we can add you to the relevant GH team. But in this case I've done it for you :)

Yes, if it's possible, I would like to have that permission.
Thank you very much!

Comment 36

[Timea Cernea [:tbabos][inactive]]

Verified - Fixed on latest Beta 69.0b4 on Windows 10, Mac OS 10.14 and Ubuntu 16.04.
Marking this is Verified - Fixed.

Comment 37

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

Upstream PR merged

Comment 38

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.