Closed Bug 1338030 Opened 7 years ago Closed 6 years ago

[webvtt] implement the region functionality.

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files)

Bug 1338030 - parsecontent: refact the parseContent function input argument.
59 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1338030 - regionNodeBox : implementation RegionNodeBox which correspond to "WebVTT region object".
59 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1338030 - regionCueStyleBox: implementation RegionCueStyleBox which correspong to "the children WebVTT region object".
59 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1338030 - region : integrate the RegionNodeBox and RegionCueStyleBox.
59 bytes, text/x-review-board-request
rillian
: review+
Details 
Since the VTTRegion is created by vtt.jsm from a vtt file, we need to implement its functionality: Move the cue into the VTTRegion.

Now we only create a VTTRegion object but do nothing.
Blocks: 1338031
Blocks: 1294833
Assignee: nobody → bechen
Depends on: 1409983
Depends on: 1412180
Comment on attachment 8923736 [details]
Bug 1338030 - parsecontent: refact the parseContent function input argument.

https://reviewboard.mozilla.org/r/194882/#review199930

The region preference doesn't enable yet. So these patches should not impact the current usecase/testcase.

And there should be some other follow bugs such as
1. how to render 2 cues with the same region at the same time?
2. style.transitionProperty = "top" seems not work?
3. I should check every CSS attributes.
4. open the Safari browser to verify our implementation.
5. enable the region preference and pass the wpt tests.
...

Thanks for the review!
Comment on attachment 8923736 [details]
Bug 1338030 - parsecontent: refact the parseContent function input argument.

https://reviewboard.mozilla.org/r/194884/#review200076

Looks cleaner. Thanks!

::: commit-message-bb655:2
(Diff revision 1)
> +Bug 1338030 - parsecontent: refact the parseContent function input argument. r=rillian
> +

Please describe the reason for the change in the commit message.
Attachment #8923736 - Flags: review?(giles) → review+
Comment on attachment 8923737 [details]
Bug 1338030 - regionNodeBox : implementation RegionNodeBox which correspond to "WebVTT region object".

https://reviewboard.mozilla.org/r/194886/#review200650

::: dom/media/webvtt/vtt.jsm:606
(Diff revision 1)
>    CueStyleBox.prototype.constructor = CueStyleBox;
>  
> +  function RegionNodeBox(window, region, container) {
> +    StyleBox.call(this);
> +
> +    var boxLineHeight = container.height * 0.0533 // 0.0533vh ? 5.33vh

Shouldn't this derive from `FONT_SIZE_PERCENT` so the line height matches what we do for cues?
Attachment #8923737 - Flags: review?(giles) → review+
Comment on attachment 8923738 [details]
Bug 1338030 - regionCueStyleBox: implementation RegionCueStyleBox which correspong to "the children WebVTT region object".

https://reviewboard.mozilla.org/r/194888/#review200658

::: dom/media/webvtt/vtt.jsm:650
(Diff revision 1)
> +      unicodeBidi: "plaintext",
> +      width: "auto",
> +      height: "auto",
> +      textAlign: cue.align,
> +    };
> +    // TODO: fix me, LTR and RTL ? using margin replace the "left/right"

Seems like this code could be shared with the non-region cue layout. That can be a follow-up bug though.
Attachment #8923738 - Flags: review?(giles) → review+
Comment on attachment 8923739 [details]
Bug 1338030 - region : integrate the RegionNodeBox and RegionCueStyleBox.

https://reviewboard.mozilla.org/r/194890/#review200690
Attachment #8923739 - Flags: review?(giles) → review+
Comment on attachment 8923737 [details]
Bug 1338030 - regionNodeBox : implementation RegionNodeBox which correspond to "WebVTT region object".

https://reviewboard.mozilla.org/r/194886/#review201768

::: dom/media/webvtt/vtt.jsm:606
(Diff revision 1)
>    CueStyleBox.prototype.constructor = CueStyleBox;
>  
> +  function RegionNodeBox(window, region, container) {
> +    StyleBox.call(this);
> +
> +    var boxLineHeight = container.height * 0.0533 // 0.0533vh ? 5.33vh

No, the font-size of cue is 5vh, and 5.33vh for region lineHeight, (5.33/1.3)vh for region font-size.
Comment on attachment 8923738 [details]
Bug 1338030 - regionCueStyleBox: implementation RegionCueStyleBox which correspong to "the children WebVTT region object".

https://reviewboard.mozilla.org/r/194888/#review200658

> Seems like this code could be shared with the non-region cue layout. That can be a follow-up bug though.

Yes, the code could be shared.
Here I write a TODO because now we set the styles.unicodeBidi = "plaintext", the RTL or LTR writing direction decided by our native(c++) implementation. And also we remove the RTL table at bug 1339355, we don't know the direction in vtt.jsm, so we can not set "left" or "right" here (LTR should set left, RTL should set right). I ask some front-end guys, they said maybe we can use margin to make the regionCueStyle box position/width correct.
Depends on: 1415805
Blocks: 1415821
Blocks: 1415805
No longer depends on: 1415805
Comment on attachment 8923739 [details]
Bug 1338030 - region : integrate the RegionNodeBox and RegionCueStyleBox.

https://reviewboard.mozilla.org/r/194890/#review203256

::: dom/media/webvtt/vtt.jsm:1086
(Diff revision 1)
> +          if (cue.region.scroll == "up" && currentRegionNodeDiv.childElementCount > 0) {
> +            currentRegionNodeDiv.style.transitionProperty = "top";
> +            currentRegionNodeDiv.style.transitionDuration = "0.433s";
> +          }

https://www.w3.org/TR/webvtt1/#introduction-other-features
After read the example 10, I think the transitionProperty should set on the RegionCueStyleBox.cueDiv to scroll up the cue.
Blocks: 1416143
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea98df19ce7b
parsecontent: refact the parseContent function input argument. r=rillian
https://hg.mozilla.org/integration/autoland/rev/4beea83e0858
regionNodeBox : implementation RegionNodeBox which correspond to "WebVTT region object". r=rillian
https://hg.mozilla.org/integration/autoland/rev/ff19950e47d9
regionCueStyleBox: implementation RegionCueStyleBox which correspong to "the children WebVTT region object". r=rillian
https://hg.mozilla.org/integration/autoland/rev/a8075676dcb3
region : integrate the RegionNodeBox and RegionCueStyleBox. r=rillian
Keywords: checkin-needed
Am I correct in understanding that I can describe this bug in documentation revisions as meaning that while we already were creating VTTRegion objects from the .vtt file, we were not actually rendering them, and now we do support that (while behind a flag in this bug, then enabled by default in 59)?

I see no technical details that need explaining (other than the fact that presently we don't have documentation for VTTRegion, which probably won't change because of this bug due to time constraints). Please let me know if I'm mistaken.

Anthony: I'm ni'ing you, since :bechen is not available.
Flags: needinfo?(ajones)
Going with the interpretation from comment 19 above and updated Firefox 59 for developers. Advise if mistaken.

Updated the following page to include info below the compatibility table about this change:

https://developer.mozilla.org/en-US/docs/Web/API/WebVTT_API#Browser_compatibility

Added a note to Firefox 59 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Yes. There may be bugs, but we support Regions now.
Flags: needinfo?(ajones)
You need to log in before you can comment on or make changes to this bug.