Closed Bug 1242599 Opened 5 years ago Closed 4 years ago

textTracks are not added when track element appended to video

Categories

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

46 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: psayre, Assigned: bechen)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files)

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
This may be similar to another bug I recently reported: Bug 1242594 - textTracks lost when video removed from DOM
Component: Untriaged → Audio/Video
Product: Firefox → Core
Blocks: webvtt
P2
Priority: -- → P2
Component: Audio/Video → Audio/Video: Playback
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.
Assignee: nobody → bechen
See Also: → 1206304
Attachment #8752695 - Flags: review?(giles) → review+
Comment on attachment 8752695 [details]
MozReview Request: Bug 1242599 - Create TextTrack before LoadResource(). r=rillian

https://reviewboard.mozilla.org/r/52710/#review49810
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.
Attachment #8752696 - Flags: review?(giles)
(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?
Flags: needinfo?(psayre)
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.
Ok, thanks. Seems to be IE 'feature'.
Flags: needinfo?(psayre)
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 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 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 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 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 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!
Attachment #8759060 - Flags: review?(giles) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c9dd7a6b157b
https://hg.mozilla.org/mozilla-central/rev/9d3cf4d5824e
https://hg.mozilla.org/mozilla-central/rev/318fae85147f
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
See Also: → 1278151
Duplicate of this bug: 1206304
Duplicate of this bug: 1205644
Depends on: 1284601
You need to log in before you can comment on or make changes to this bug.