1242599 - textTracks are not added when track element appended to video

Status: RESOLVED FIXED

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

Description

[Paul Sayre]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36

Steps to reproduce:

I loaded a page with a <video> element. I added a <track> element as a child of the <video> element.

Test case: http://jsfiddle.net/jxLf4cnv/1/


Actual results:

video.textTracks.length === 0 after appending a new <track> element to the <video>


Expected results:

The <track> should show up in video.textTracks

Comment 1

[Paul Sayre]

This may be similar to another bug I recently reported: Bug 1242594 - textTracks lost when video removed from DOM

Comment 2

[Chris Peterson [:cpeterson]]

P2

Comment 3

[Benjamin Chen [:bechen]]

line 1: video.appendChild(track);
line 2: added.innerHTML = video.textTracks.length;

Looks like the HTMLTrackElement::LoadResource is an asynchronous call in HTMLTrackElement::BindToTree.
So line 2 will get 0 if HTMLTrackElement::LoadResource is not finished.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 1242599 - Create TextTrack before LoadResource(). r=rillian

Review commit: https://reviewboard.mozilla.org/r/52710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52710/

Comment 5

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 1242599 - testcase. r=rillian

Review commit: https://reviewboard.mozilla.org/r/52712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52712/

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8752695 [details]
MozReview Request: Bug 1242599 - Create TextTrack before LoadResource(). r=rillian

https://reviewboard.mozilla.org/r/52710/#review49810

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8752696 [details]
MozReview Request: Bug 1242599 - testcase. r=rillian

https://reviewboard.mozilla.org/r/52712/#review49812

::: dom/media/test/test_bug1242594.html:34
(Diff revision 1)
>  
>  document.getElementById("content").appendChild(video);
>  video.appendChild(trackElement);
>  
> -video.addEventListener("loadedmetadata", function run_tests() {
> +// Bug 1242599, access video.textTracks.length immediately after
> +// the track element bind into the media element.

Removing the event trigger makes me a little nervous that this will be racy.

I recall vaguely that we changed things so we don't need to wait for loadedmetadata.Is this correct behaviour by the spec?

::: dom/media/test/test_bug1242594.html:35
(Diff revision 1)
>  document.getElementById("content").appendChild(video);
>  video.appendChild(trackElement);
>  
> -video.addEventListener("loadedmetadata", function run_tests() {
> +// Bug 1242599, access video.textTracks.length immediately after
> +// the track element bind into the media element.
> -  is(video.textTracks.length, 1, "Video should have one TextTrack.");
> +is(video.textTracks.length, 1, "Video should have one TextTrack.");

Should be 'element binds'. Third-person singular.

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8752696 [details]
MozReview Request: Bug 1242599 - testcase. r=rillian

https://reviewboard.mozilla.org/r/52712/#review49814

Comment 9

[Ralph Giles (:rillian)]

(In reply to Paul Sayre from comment #0)

> Test case: http://jsfiddle.net/jxLf4cnv/1/

BTW, I am astounded by your testcase accessing elements by id without going through querySelector or getElementById. How does that work?

Comment 10

[Ralph Giles (:rillian)]

bz pointed me at bug 622491 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=11960

https://twitter.com/bz_moz/status/732332291842772993

Comment 11

[Paul Sayre]

Ralph, in the code where I discovered the issue, I don't actually know where the node comes from, so I was mostly lazy in the test case.

Comment 12

[Ralph Giles (:rillian)]

Ok, thanks. Seems to be IE 'feature'.

Comment 13

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/52712/#review49812

> Removing the event trigger makes me a little nervous that this will be racy.
> 
> I recall vaguely that we changed things so we don't need to wait for loadedmetadata.Is this correct behaviour by the spec?

The spec only says the loadedmetadata fired after "the text tracks are ready". I don't see any relation between "loadedmetadata" and "video.textTracks".

Comment 14

[Benjamin Chen [:bechen]]

Comment on attachment 8752695 [details]
MozReview Request: Bug 1242599 - Create TextTrack before LoadResource(). r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52710/diff/1-2/

Comment 15

[Benjamin Chen [:bechen]]

Comment on attachment 8752696 [details]
MozReview Request: Bug 1242599 - testcase. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52712/diff/1-2/

Comment 16

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 1242599 - [webplatformtest] Enable /html/semantics/embedded-content/media-elements/track/track-element/track-api-texttracks.html. r=rillian

Review commit: https://reviewboard.mozilla.org/r/57150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57150/

Comment 17

[Benjamin Chen [:bechen]]

Comment on attachment 8752695 [details]
MozReview Request: Bug 1242599 - Create TextTrack before LoadResource(). r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52710/diff/2-3/

Comment 18

[Benjamin Chen [:bechen]]

Comment on attachment 8752696 [details]
MozReview Request: Bug 1242599 - testcase. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52712/diff/2-3/

Comment 19

[Ralph Giles (:rillian)]

Comment on attachment 8759060 [details]
MozReview Request: Bug 1242599 - [webplatformtest] Enable /html/semantics/embedded-content/media-elements/track/track-element/track-api-texttracks.html. r=rillian

https://reviewboard.mozilla.org/r/57150/#review54052

Hooray for passing more tests!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9dd7a6b157b
Create TextTrack before LoadResource(). r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3cf4d5824e
testcase. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/318fae85147f
[webplatformtest] Enable /html/semantics/embedded-content/media-elements/track/track-element/track-api-texttracks.html. r=rillian

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9dd7a6b157b
https://hg.mozilla.org/mozilla-central/rev/9d3cf4d5824e
https://hg.mozilla.org/mozilla-central/rev/318fae85147f