Open
Bug 1344609
Opened 7 years ago
Updated 2 years ago
[webvtt] refact the TimeMarchesOn() for :past/:future
Categories
(Core :: Audio/Video: Playback, enhancement, P3)
Core
Audio/Video: Playback
Tracking
()
NEW
People
(Reporter: bechen, Unassigned)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
https://html.spec.whatwg.org/multipage/embedded-content.html#time-marches-on The rendering for the cue is the last step of TimeMarchesOn. There are some early return during TimeMarchesOn especially the step 7. If we want to support :past :future, we need some modification at TimeMarchesOn.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8845298 [details] Bug 1344609 - Pseudo classes support. https://reviewboard.mozilla.org/r/118480/#review120676 General approach looks fine, but it looks like this is just a partial implementation? Actually setting the pseudo-classes is marked 'todo'. So I don't think this is ready to merge. ::: commit-message-9f246:2 (Diff revision 1) > +Bug 1344609 - Pseudo classes support. r=rillian > + Please include the motivation for the change in the commit message. Even the text from the bug description would be good. ::: dom/media/webvtt/vtt.jsm:411 (Diff revision 1) > // Otherwise just ignore the end tag. > continue; > } > var ts = collectTimeStamp(t.substr(1, t.length - 2)); > + // TODO: verify the ts is valid or not, compare to the cue.startTime endTime? > + // TODO: ensure the all ts are monotonic inscrease? Please open a bug for these issues and give the bug number here if you want to include the TODO text. ::: dom/media/webvtt/vtt.jsm:1002 (Diff revision 1) > + nodeFilterConstants.SHOW_PROCESSING_INSTRUCTION, null, false); > + var targetNode; > + var lastNode = timestampWalker.nextNode(); > + // The cue doesn't have timestamp node inside. > + if (!lastNode) { > + // TODO: Do we still need to apply :past or :future? Same here, please. ::: dom/media/webvtt/vtt.jsm:1045 (Diff revision 1) > + while (textWalker.nextNode()) { > + var node = textWalker.currentNode; > + if (node.nodeType == nodeConstants.TEXT_NODE) { > + // TODO: apply :past > + dump("apply :past\n"); > + dump(node.data + "\n"); Seem like this isn't ready to land.
Attachment #8845298 -
Flags: review?(giles) → review-
Reporter | ||
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8845298 [details] Bug 1344609 - Pseudo classes support. https://reviewboard.mozilla.org/r/118480/#review120784 ::: dom/media/webvtt/vtt.jsm:1036 (Diff revision 1) > + var textWalker = window.document.createTreeWalker(cueDiv, > + nodeFilterConstants.SHOW_TEXT | nodeFilterConstants.SHOW_PROCESSING_INSTRUCTION, Since the TextNode is not domelement, I can not pply pseudo class on it. I need to use a domelement to wrap the textnode if the textnode doesn't have a domeelement parent.
Reporter | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8845298 [details] Bug 1344609 - Pseudo classes support. https://reviewboard.mozilla.org/r/118480/#review121436 ::: dom/media/webvtt/vtt.jsm:1002 (Diff revision 1) > + nodeFilterConstants.SHOW_PROCESSING_INSTRUCTION, null, false); > + var targetNode; > + var lastNode = timestampWalker.nextNode(); > + // The cue doesn't have timestamp node inside. > + if (!lastNode) { > + // TODO: Do we still need to apply :past or :future? https://w3c.github.io/webvtt/#the-past-and-future-pseudo-classes spec says: "there exists a WebVTT Timestamp Object ....", so I think in this case, we don't need to apply :past or :future on the cue.
Reporter | ||
Updated•7 years ago
|
Assignee: bechen → nobody
Updated•7 years ago
|
Priority: -- → P3
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•