[webvtt] fix getCueAsHTML.html

RESOLVED FIXED in Firefox 54

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bechen, Assigned: bechen)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(URL)

Attachments

(5 attachments)

Comment hidden (empty)
Priority: -- → P5
(Assignee)

Updated

2 years ago
Assignee: nobody → bechen
(Assignee)

Comment 1

2 years ago
I'm going to fix it because it is a VTTCue interface.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: P5 → P3

Comment 10

2 years ago
mozreview-review
Comment on attachment 8840326 [details]
Bug 1307710 - part1: Return Documentfragment instead of div for convertCueToDOMTree.

https://reviewboard.mozilla.org/r/114832/#review116562

Looks reasonable to me, but I'd like someone with more dom experience to look at it was well. Smaug, do you mind?

There's an unexpected web platform test pass which needs updating before this can land. `webvtt/webvtt-file-format-parsing/webvtt-cue-text-parsing-rules/tests/entities.html`
Attachment #8840326 - Flags: review?(giles) → review+
Attachment #8840326 - Flags: review?(bugs)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8840327 [details]
Bug 1307710 - part2: webvtt cue voice span and language span need annotation.

https://reviewboard.mozilla.org/r/114834/#review116568
Attachment #8840327 - Flags: review?(giles) → review+

Comment 12

2 years ago
mozreview-review
Comment on attachment 8840328 [details]
Bug 1307710 - part3: fix ProcessingInstruction node for testcase.

https://reviewboard.mozilla.org/r/114836/#review116570

::: dom/media/webvtt/vtt.jsm:375
(Diff revision 2)
>          }
>          var ts = collectTimeStamp(t.substr(1, t.length - 2));
>          var node;
>          if (ts) {
>            // Timestamps are lead nodes as well.
> -          node = window.document.createProcessingInstruction("timestamp", ts);
> +          node = window.document.createProcessingInstruction("timestamp", t.substr(1, t.length - 2));

Rather than repeating `t.subst()` here I think it would be better to store it in another variable before line 371 so we can use it here and also in `var ts = collectTimeStamp(text_timestamp)`.

Is this change specific enough? The spec says:

> ProcessingInstruction node whose target is "timestamp" and whose data is a WebVTT timestamp representing the value of the WebVTT Timestamp Object, with all optional components included, with one leading zero if the hours component is less than ten, and with no leading zeros otherwise.

I don't think we enforce the leading zero detail when we parse, so we could still propagate a sloppy timestamp from the input file. It would be good the update the test to verify this case as well.

I think you might need to add a `normalizedTimeStamp(ts)` function you can use here instead. That will also remove the need to keep the `t.subst()` output in a shared temporary.
Attachment #8840328 - Flags: review?(giles) → review-

Comment 13

2 years ago
mozreview-review
Comment on attachment 8840329 [details]
Bug 1307710 - part4: Enable getCueAsHTML.html testcase, disable entities.html, tags.html, modify timestamp.html.ini .

https://reviewboard.mozilla.org/r/114838/#review116572
Attachment #8840329 - Flags: review?(giles) → review+
(Assignee)

Comment 14

2 years ago
(In reply to Ralph Giles (:rillian) | needinfo me from comment #10)

> There's an unexpected web platform test pass which needs updating before
> this can land.
> `webvtt/webvtt-file-format-parsing/webvtt-cue-text-parsing-rules/tests/
> entities.html`

I hit crash at webvtt/webvtt-file-format-parsing/webvtt-cue-text-parsing-rules/tests/tags.html and timestamp.html, so I will close these testcase first, the file some follow up bugs.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8840326 [details]
Bug 1307710 - part1: Return Documentfragment instead of div for convertCueToDOMTree.

https://reviewboard.mozilla.org/r/114832/#review116704

::: dom/media/webvtt/nsIWebVTTParserWrapper.idl:15
(Diff revision 2)
>  interface nsIVariant;
>  
>  /**
>   * Interface for a wrapper of a JS WebVTT parser (vtt.js).
>   */
> -[scriptable, uuid(8dfe016e-1701-4618-9f5e-9a6154e853f0)]
> +[scriptable, uuid(cc61f640-b3c0-435f-8447-e671eabe95d3)]

FWIW, uuid changes aren't needed anymore. Doesn't cause harm though.

::: dom/media/webvtt/vtt.jsm:726
(Diff revision 2)
>      StyleBox.call(this);
>      this.cue = cue;
>  
>      // Parse our cue's text into a DOM tree rooted at 'cueDiv'. This div will
>      // have inline positioning and will function as the cue background box.
> -    this.cueDiv = parseContent(window, cue.text);
> +    var containerFrag = parseContent(window, cue.text);

This stuff feels odd. Why to parse first to a document fragment and then append fragment to a div.
That creates a useless temporary documentfragment object.

But r+ for the .idl and c++
Attachment #8840326 - Flags: review?(bugs) → review+

Comment 19

2 years ago
mozreview-review
Comment on attachment 8840328 [details]
Bug 1307710 - part3: fix ProcessingInstruction node for testcase.

https://reviewboard.mozilla.org/r/114836/#review116834

::: dom/media/webvtt/vtt.jsm:356
(Diff revision 3)
>        return element;
>      }
>  
> +    // https://w3c.github.io/webvtt/#webvtt-timestamp-object
> +    function normalizedTimeStamp(inputString) {
> +      return inputString.indexOf(":") == 1 ? "0" + inputString : inputString;

This works with the parsing rules we have in collectTimeStamp(). I think generating the string directly from the parse ts value would be more robust against future code changes, but this should be correct.
Attachment #8840328 - Flags: review?(giles) → review+

Comment 20

2 years ago
mozreview-review
Comment on attachment 8840808 [details]
Bug 1307710 - wpt: Update the getCueAsHTML testcase for testing leading zero of webvtt timestamp object.

https://reviewboard.mozilla.org/r/115260/#review116928

Sorry for the delay, I wanted to make sure parsers did indeed handle one-character hours. LGTM.

I tried a brief test on Firefox, Safari and Chrome and got the same number of parsed cues for each. https://people-mozilla.org/~rgiles/2017/vttcue.html
Attachment #8840808 - Flags: review?(giles) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 29

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d67ed0a83794
part1: Return Documentfragment instead of div for convertCueToDOMTree. r=rillian,smaug
https://hg.mozilla.org/integration/autoland/rev/e99bfc26ca21
part2: webvtt cue voice span and language span need annotation. r=rillian
https://hg.mozilla.org/integration/autoland/rev/73b6ba6b82a0
part3: fix ProcessingInstruction node for testcase. r=rillian
https://hg.mozilla.org/integration/autoland/rev/a4f7f1dd00bd
wpt: Update the getCueAsHTML testcase for testing leading zero of webvtt timestamp object. r=rillian
https://hg.mozilla.org/integration/autoland/rev/8e673e61fb8e
part4: Enable getCueAsHTML.html testcase, disable entities.html, tags.html, modify timestamp.html.ini .r=rillian
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Blocks: 1344604
You need to log in before you can comment on or make changes to this bug.