Bug 175031 - Old subtitle track is not deleted on 'src' attribute change event
Summary: Old subtitle track is not deleted on 'src' attribute change event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL: https://rednoize.su/vtt-bug/index2.html
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-01 09:40 PDT by Kirill Ovchinnikov
Modified: 2017-08-23 09:50 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.79 KB, patch)
2017-08-04 02:38 PDT, Kirill Ovchinnikov
no flags Details | Formatted Diff | Diff
Patch (5.58 KB, patch)
2017-08-04 03:26 PDT, Kirill Ovchinnikov
no flags Details | Formatted Diff | Diff
Patch (7.15 KB, patch)
2017-08-09 06:53 PDT, Kirill Ovchinnikov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kirill Ovchinnikov 2017-08-01 09:40:26 PDT
Test page: https://rednoize.su/vtt-bug/index2.html

1. Start video playing and enable subtitles
2. Ensure that subtitle track is displayed (after 0:00:05 time position)
3. Click on "Next subtitle track" - track URL will be changed
4. You will see the old subtitle track and the new subtitle track in the same time.
5. Click on "Next subtitle track" one more time - the first track URL will be set again.
6. You will see one more clone of the first subtitle track

I cannot find anything in the standards about it. As I can see, it is not prohibited to change 'src' attribute of the <track> element, and I logically suppose that on 'src' change event the old subtitle track should be deleted from the media and hidden from the screen.

Tested on Midori 0.5.11.
Chromium has the same problem, Firefox handles it well.
Comment 1 Kirill Ovchinnikov 2017-08-01 14:34:56 PDT
"Whenever a track element has its src attribute set, changed, or removed, the user agent must immediately empty the element's text track's text track list of cues. (This also causes the algorithm above to stop adding cues from the resource being obtained using the previously given URL, if any.)"
https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:text-track-list-of-cues-5
Comment 2 Radar WebKit Bug Importer 2017-08-01 17:25:57 PDT
<rdar://problem/33666796>
Comment 3 Kirill Ovchinnikov 2017-08-04 02:38:03 PDT
Created attachment 317229 [details]
Patch
Comment 4 Build Bot 2017-08-04 02:39:40 PDT
Attachment 317229 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Kirill Ovchinnikov 2017-08-04 03:26:15 PDT
Created attachment 317231 [details]
Patch
Comment 6 Eric Carlson 2017-08-04 10:07:10 PDT
Comment on attachment 317231 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317231&action=review

Thank you for the patch! Marking r- for now because of the manual test.

> ManualTests/media-track-src-change.html:18
> +    <title>HTML5 Video Subtitle refresh bug test</title>
> +    <script>
> +        var vtts = ["https://svn.webkit.org/repository/webkit/trunk/LayoutTests/media/track/captions-webvtt/captions-long.vtt", 
> +                    "https://svn.webkit.org/repository/webkit/trunk/LayoutTests/media/track/captions-webvtt/captions.vtt"];
> +        var actualSub = 0;
> +        document.addEventListener("DOMContentLoaded", function () {
> +            document.getElementById('vid').play();
> +            document.getElementById('nextSub').addEventListener('click', function () {
> +                actualSub++;
> +                var subtitlesSrc = vtts[actualSub % vtts.length];
> +                document.getElementById('vid').children[0].setAttribute("src", subtitlesSrc);
> +            }, false);
> +        });

Manual tests are not run frequently, so they are generally used for changes that can only be verified manually. This would be much better as a regular layout test, so it will be run automatically every time someone submits a patch.

Because this bug results in a track with both the old and new cues, a test can check the number of cues before and after changing .src. Existing vtt tests are in LayoutTests/media/track/. Something like track-text-track-cue-list.html could be modified to test your change if you wish.
Comment 7 Kirill Ovchinnikov 2017-08-09 06:53:03 PDT
Created attachment 317697 [details]
Patch
Comment 8 Kirill Ovchinnikov 2017-08-09 06:54:03 PDT
Thank you for your feedback and advice!
I prepared a new patch with layout auto-test.
Comment 9 Eric Carlson 2017-08-09 09:12:39 PDT
Comment on attachment 317697 [details]
Patch

Thank you for the fix! 

If you don't have commit privileges, make the bug cq? and any committer can have the commit bot do it.
Comment 10 WebKit Commit Bot 2017-08-09 12:13:55 PDT
Comment on attachment 317697 [details]
Patch

Clearing flags on attachment: 317697

Committed r220472: <http://trac.webkit.org/changeset/220472>
Comment 11 WebKit Commit Bot 2017-08-09 12:13:56 PDT
All reviewed patches have been landed.  Closing bug.