[webvtt] refact the TimeMarchesOn() for :past/:future

NEW
Unassigned

Status

()

Core
Audio/Video: Playback
P3
normal
9 months ago
6 months ago

People

(Reporter: bechen, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
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

9 months 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

9 months 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)

Updated

8 months ago
Blocks: 1346700
(Reporter)

Comment 4

8 months 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

8 months ago
Assignee: bechen → nobody
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.