[webvtt] HTMLTrackElement should not load resource when its text track is disable
Categories
(Core :: Audio/Video: Playback, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: alwu, Assigned: alwu, NeedInfo)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete)
Attachments
(23 files, 1 obsolete file)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
According to the spec step2 [1], we should not load resource for the track element which text track is disabled
.
[1] https://html.spec.whatwg.org/multipage/media.html#start-the-track-processing-model
Assignee | ||
Comment 1•6 years ago
|
||
According to the spec step2 [1], we should not load resource for the track element which text track is disabled.
[1] https://html.spec.whatwg.org/multipage/media.html#start-the-track-processing-model
Assignee | ||
Comment 2•6 years ago
|
||
According to the spec [1], the user agent must start the track processing model when text track has its text track mode changed.
Assignee | ||
Comment 3•6 years ago
|
||
Showing track's kind in debug log is helpful.
Assignee | ||
Comment 4•6 years ago
|
||
According to spec [1], it doesn't mention that we have to run track selection in stable state, it just said "the user agent must queue a task to run the following steps".
Assignee | ||
Comment 5•6 years ago
|
||
In honor user preferences for automatic text track selection
[1], we would set did-perform-automatic-track-selection
flag to true [2], and then we won't execute automatic track selection anymore [3].
It means that we would only do automatic track selection one time, and then user has to enable newly added track explicitly by changing its mode.
In this test, we have done the automatic track selection when we added the metadata
track to media element's text track list, so we won't run it again, even if the newly added track has default
attribute.
Therefore, we have to enable the caption
track explicitly. In addition, add the missing event
parameter for the function trackAdded()
.
[1] https://html.spec.whatwg.org/multipage/media.html#honor-user-preferences-for-automatic-text-track-selection
[2] https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:did-perform-automatic-track-selection-2
[3] https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:did-perform-automatic-track-selection
Assignee | ||
Comment 6•6 years ago
|
||
According to the spec [1], we should empty track's cue list whenever a track element has its src attribute set, changed, or removed.
[1] https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:attr-track-src
Assignee | ||
Comment 7•6 years ago
|
||
Failed to load
[1] indicates that the text track was enabled, but when the user agent attempted to obtain it, this failed in some way.
According to the spec [2], if fetching fails for any reason (network error, the server returns an error code, CORS fails, etc), or if URL is the empty string, then queue a task to first change the text track readiness state to failed to load and then fire an event named error at the track element.
In addition, when we remove src
attribute from a loading or loaded track element, we should also set it to failed to load
.
[1] https://html.spec.whatwg.org/multipage/media.html#text-track-readiness-state
[2] https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:text-track-failed-to-load
Assignee | ||
Comment 8•6 years ago
|
||
As the src url is the same, we have no need to reload the resource.
Assignee | ||
Comment 9•6 years ago
|
||
There are too many self
used in the lambda, we can just capture this
to remove redudant self
.
Assignee | ||
Comment 10•6 years ago
|
||
To reduce non-neccesary input, we can always capture this
and print the address in the log, and use same log level for all of them.
Assignee | ||
Comment 11•6 years ago
|
||
According to the spec [1], the text track list of cues is initially empty. It is dynamically modified when the referenced file is parsed.
Therefore, after reset the track element's url, the number of cues of the text track should be zero, until we start parsing resource and add cue into the list.
[1]
https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:text-track-list-of-cues
https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:text-track-list-of-cues-3
Assignee | ||
Comment 12•6 years ago
|
||
Assignee | ||
Comment 13•6 years ago
|
||
As now we won't start loading for disable tracks, so we have to enable the track by changing its track mode explicitly or adding default
attribute before the media element running the automatic track selection.
There are some common changes for these tests.
(1) listen track element's load
event
There are some tests listening video's loadedmetadata
, but it's wrong. The loading process of media element and track element are completely non-related.
If you would like to check track element's status, you should wait for track element's load
event.
(2) enable track explictly
If the text track which has default
attribute is added to the media element before the media element starts running automatic track selection [1], then it would be enable by the media element.
Otherwise, you have to enable track explicitly by changing its track mode.
(3) refactor tests
Refactor those tests' structure in order to make them more readable, and add the comment to show what the test purpose is for each test.
[1] https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:text-track-7
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 14•6 years ago
|
||
These elemenets are useless and we can run tests without them.
Assignee | ||
Comment 15•6 years ago
|
||
These tests didn't use region at all, so we have no need to set the pref.
Assignee | ||
Comment 16•6 years ago
|
||
Create test elements in HTML beforehead, which can remove unnecessary JS codes and make test cleaner.
Assignee | ||
Comment 17•6 years ago
|
||
Use named function for callback to reduce the indentation.
Assignee | ||
Comment 18•6 years ago
|
||
This patch do two things in order to trigger loading for track element and wait for correct event to check track's and cues' status after loading finished.
(1) listen track element's load event
There are some tests listening video's loadedmetadata, but it's wrong. The loading process of media element and track element are completely non-related.
If you would like to check track element's status, you should wait for track element's load event.
(2) enable track explictly
If the text track which has default attribute is added to the media element before the media element starts running automatic track selection [1], then it would be enable by the media element.
Otherwise, you have to enable track explicitly by changing its track mode.
[1] https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:text-track-7
Assignee | ||
Comment 19•6 years ago
|
||
Refactor those tests' structure in order to make them more readable, and add the comment to show what the test purpose is for each test.
Updated•6 years ago
|
Assignee | ||
Comment 20•6 years ago
|
||
According to spec [1], if the track URL changes so that it is no longer equal to URL while fetching is ongoing, we need to change the text track readiness state tofailed to load
and dispatch error
.
It didn't mention we have to dispatch error
if track element has finished loading.
Assignee | ||
Comment 21•6 years ago
|
||
This test is used to ensure that we queue 'honor user preferences for automatic text track selection' as a marco task, not a mirco task.
In this test, we would trigger a media event before queuing a text track selection task, and check the text track's mode to know whether the text track selection runs after the task for media event.
Assignee | ||
Comment 22•6 years ago
|
||
The channel might not be created correctly if we pass invaild url (eg. "invalid://url"), we should handle this error.
Assignee | ||
Comment 23•6 years ago
|
||
Sometime wpt runs test even before the document becomes visible, which would delay video.play()
and cause play()
running in wrong order.
Updated•6 years ago
|
Comment 24•6 years ago
|
||
Comment 27•6 years ago
|
||
Backed out 22 changesets for browser-chrome failure at browser_cache.js.
Backout link: https://hg.mozilla.org/integration/autoland/rev/100b92fd6219edddc0c788ea7c04e99b85e9c21a
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247906371&repo=autoland&lineNumber=2967
Assignee | ||
Comment 29•6 years ago
|
||
As now we won't automatically load disabled text track, we have to mark track as default
in order to trigger loading.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 30•6 years ago
|
||
Comment 31•6 years ago
|
||
Comment 33•6 years ago
|
||
Backed out for causing bug 1548454 to permafail
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248075630&repo=autoland&lineNumber=10706
Backout: https://hg.mozilla.org/integration/autoland/rev/6acae89335b04d1d634fe5f6be2637f25e3e81e2
Assignee | ||
Comment 35•6 years ago
|
||
I've already updated the fix in patch17. In order to ensure we won't break any test again, pushing patches to try server before relanding all of them.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=825ad84c20cb0b10b247c6c501e6d6c704420e73
Updated•6 years ago
|
Updated•6 years ago
|
Comment 36•6 years ago
|
||
Comment 38•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/572e5b0bcab9
https://hg.mozilla.org/mozilla-central/rev/6d16370ca0cf
https://hg.mozilla.org/mozilla-central/rev/a5ed9d3975c4
https://hg.mozilla.org/mozilla-central/rev/c90139d2c836
https://hg.mozilla.org/mozilla-central/rev/7a20deaf11bc
https://hg.mozilla.org/mozilla-central/rev/b19effb108a9
https://hg.mozilla.org/mozilla-central/rev/01efc1080b51
https://hg.mozilla.org/mozilla-central/rev/bb4756110ece
https://hg.mozilla.org/mozilla-central/rev/079555c7d41b
https://hg.mozilla.org/mozilla-central/rev/6c49cf7caac3
https://hg.mozilla.org/mozilla-central/rev/4d15400f8a6d
https://hg.mozilla.org/mozilla-central/rev/f4d1f1b6e2c3
https://hg.mozilla.org/mozilla-central/rev/63dd1f7a3ff4
https://hg.mozilla.org/mozilla-central/rev/bef097125232
https://hg.mozilla.org/mozilla-central/rev/0c1ef149fc1d
https://hg.mozilla.org/mozilla-central/rev/8d7f304e91f7
https://hg.mozilla.org/mozilla-central/rev/9b78a97b3387
https://hg.mozilla.org/mozilla-central/rev/6a593a6deb7e
https://hg.mozilla.org/mozilla-central/rev/590e376de09a
https://hg.mozilla.org/mozilla-central/rev/4159b0ad4c58
https://hg.mozilla.org/mozilla-central/rev/39c66f35a9a7
https://hg.mozilla.org/mozilla-central/rev/5a39102e79ad
https://hg.mozilla.org/mozilla-central/rev/e210709d41e9
Assignee | ||
Comment 40•6 years ago
|
||
According to the spec step2 [1], now we won't start loading for the track element with disabled
text track.
[1] https://html.spec.whatwg.org/multipage/media.html#start-the-track-processing-model
Comment 41•6 years ago
|
||
Is this a change that actually needs to be documented? Is the web developer or end user really going to notice this, typically? Feels a lot like an implementation detail, so to speak.
Assignee | ||
Comment 42•6 years ago
|
||
It will be good if we can document it. To let users know that if they want to load a track and do any related stuffs on that track, such as waiting load
event, that track should be hidden
or showing
.
Comment 43•6 years ago
•
|
||
Documentation updates:
- Updated TextTrack.mode for bug 1550633; also added example.
- Updated TextTrack for bug 1550633.
- Submitted BCD PR 4579 with the
TextTrack
updates - Updated Firefox 69 for developers
Assignee | ||
Comment 44•6 years ago
|
||
There is a little inaccuracy for the TextTrack.mode
[1], the mode of the text track, which has default
keyword, would only be automatically changed to showing
or hidden
by executing honor user preferences for automatic text track selection
[2]. That selection would only be done ONCE, so if you add default
on a diabled
track after finished the selection, the track mode would not be changed.
[1] https://wiki.developer.mozilla.org/en-US/docs/Web/API/TextTrack/mode
[2] https://html.spec.whatwg.org/multipage/media.html#honor-user-preferences-for-automatic-text-track-selection
Description
•