Closed Bug 1307710 Opened 8 years ago Closed 8 years ago

[webvtt] fix getCueAsHTML.html

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

(Blocks 2 open bugs, )

Details

Attachments

(5 files)

No description provided.
Assignee: nobody → bechen
I'm going to fix it because it is a VTTCue interface.
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

Created:
Updated:
Size: