[webvtt] cue display wasn't updated correctly when there are multiple cues with overlapping timestamp

VERIFIED FIXED in Firefox 69

Status

()

defect
P2
normal
VERIFIED FIXED
2 months ago
Last month

People

(Reporter: alwu, Assigned: alwu)

Tracking

(Blocks 1 bug)

unspecified
mozilla69
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox69 verified)

Details

Attachments

(3 attachments, 5 obsolete attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

STR.

  1. goto https://alastor0325.github.io/htmltests/webvtt/cues_time_overlap.html

Expected.
2. "Second cue" should disappear after 2s

Actual.
2. "Second cue" is still appearing, it disappears with "First cue".

We can actually let processCue() to handle rendering cues or cleaning displayed cues, no need to use another way to clear the cue.

The advantages is to make the code cleaner and easier to read, now we just need to know JS side would handle all rendering stuffs for us. We don't need to have different behavior when there is no showing cue.

The way we clear displayed cues are intuitive, we would remove all child nodes under the overlay, which are used to display cues.

Attachment #9064931 - Attachment description: Bug 1551385 - part1 : let 'processCue()' handle cleaning cues div. → Bug 1551385 - part2 : recompute display state when display cues amount is different from the one we rendered last time.

We can actually let processCue() to handle rendering cues or cleaning displayed cues, no need to use another way to clear the cue.

The advantages is to make the code cleaner and easier to read, now we just need to know JS side would handle all rendering stuffs for us. We don't need to have different behavior when there is no showing cue.

The way we clear displayed cues are intuitive, we would remove all child nodes under the overlay, which are used to display cues.

Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/883151b42d88
part1 : let 'processCue()' handle cleaning cues div. r=heycam
https://hg.mozilla.org/integration/autoland/rev/fb20dcf3c072
part2 : recompute display state when display cues amount is different from the one we rendered last time. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4268e661e741
part3 : add test 'test_webvtt_overlapping_time.html' and 'vtt_overlapping_time.html'. r=heycam

That is a known issue (bug1546133), I will add fuzzy for this test on Windows.

Flags: needinfo?(alwu)

From the try result [1], although we got some orange on wpt8, but all of them are related with service work, which are impossible to be affected by my changes.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba57ed9eb351b1ce92cc1aa31c1f5c70b8596df2&selectedJob=248100189

Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d3159b7f0a9c
part1 : let 'processCue()' handle cleaning cues div. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3ca6e66532a6
part2 : recompute display state when display cues amount is different from the one we rendered last time. r=heycam
https://hg.mozilla.org/integration/autoland/rev/08cd8019be34
part3 : add test 'test_webvtt_overlapping_time.html' and 'vtt_overlapping_time.html'. r=heycam

Comment on attachment 9064932 [details]
Bug 1551385 - part1 : let 'processCue()' handle cleaning cues div.

Beta/Release Uplift Approval Request

  • User impact if declined: When there are some cues having overlapping timestamp, the cue with earlier end time won't disappear correctly when the time reaches its end time.
  • 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: See comment0.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): What my change did is to force us to recompute cue display when the number of active cues changes, so it just trigger another display update, we didn't add new functionality in this bug. In addition, we have automation tests for this issue, it would be safe to catch any unexpected regressions.
  • String changes made/needed: none
Attachment #9064932 - Flags: approval-mozilla-beta?
Attachment #9064931 - Flags: approval-mozilla-beta?
Attachment #9064933 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Regressions: 1555197

I managed to reproduce the issue using an older version of Nightly (2019-05-13) on Windows 10 x64.
I retested everything using latest Nightly 69.0a1 on Windows 10 x64, Ubuntu 18.04 x64 and macOS 10.13. The bug is not reproducing anymore.

I did notice on the other hand that the button "Picture-in-Picture" is not displayed anymore. Is that a known issue or it's something expected? Or is it a regression caused by this fix?

Flags: needinfo?(alwu)

Is this a regression? If so when was it introduced?

I found the regression:
last good: 23-05-2019
first bad: 24-05-2019
But mozregression couldn't find a pushlog, but maybe this one will help: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=840b7106d8ae3158aeba8268d2ac0b40c3682bcb&tochange=f58ae8ec64c812739509de09659327bf7ea33494

I can log a separate bug for this issue.

No, my change is impossible to affect the video control. That must be caused by other bug.

Flags: needinfo?(alwu)

(In reply to Oana Botisan, Desktop Release QA from comment #21)

I did notice on the other hand that the button "Picture-in-Picture" is not displayed anymore. Is that a known issue or it's something expected? Or is it a regression caused by this fix?

Look like Bug 1547795.

According to comment 21 and comment 25 I will mark this issue as verified fixed.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

(In reply to Julien Cristau [:jcristau] from comment #22)

Is this a regression? If so when was it introduced?

Flags: needinfo?(alwu)

This issue is an old existing bug, it's not a regression.

Flags: needinfo?(alwu)

Comment on attachment 9064931 [details]
Bug 1551385 - part2 : recompute display state when display cues amount is different from the one we rendered last time.

In that case I'd prefer to let this ride the trains to 69.

Attachment #9064931 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9064932 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9064931 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
Attachment #9064933 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.