Closed Bug 1307710 Opened 8 years ago Closed 7 years ago

[webvtt] fix getCueAsHTML.html

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

(Blocks 2 open bugs,
URL
)

Details

Attachments

(5 files)

Bug 1307710 - part1: Return Documentfragment instead of div for convertCueToDOMTree.
59 bytes, text/x-review-board-request
rillian
: review+
smaug
: review+
Details
Bug 1307710 - part2: webvtt cue voice span and language span need annotation.
59 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1307710 - part3: fix ProcessingInstruction node for testcase.
59 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1307710 - part4: Enable getCueAsHTML.html testcase, disable entities.html, tags.html, modify timestamp.html.ini .
59 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1307710 - wpt: Update the getCueAsHTML testcase for testing leading zero of webvtt timestamp object.
59 bytes, text/x-review-board-request
rillian
: review+
Details 
      No description provided.
Assignee: nobody → bechen
I'm going to fix it because it is a VTTCue interface.
URL: https://w3c.github.io/webvtt/#the-vtt...
Priority: P5 → P3
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 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 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 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+
(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 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 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 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+
Keywords: checkin-needed
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
Blocks: 1344604
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: