Benjamin Chen [:bechen] Assignee
	Description • 7 years ago

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.

	Benjamin Chen [:bechen] Assignee
	Comment 5 • 7 years ago
mozreview-review

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!

	Ralph Giles (:rillian)
	Comment 6 • 7 years ago
mozreview-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.

	Ralph Giles (:rillian)
	Comment 7 • 7 years ago
mozreview-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?

	Ralph Giles (:rillian)
	Comment 8 • 7 years ago
mozreview-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.

	Ralph Giles (:rillian)
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8923739 [details]
Bug 1338030 - region : integrate the RegionNodeBox and RegionCueStyleBox.

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

	Benjamin Chen [:bechen] Assignee
	Comment 10 • 7 years ago
mozreview-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.

	Benjamin Chen [:bechen] Assignee
	Comment 11 • 7 years ago
mozreview-review-reply

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.

	Benjamin Chen [:bechen] Assignee
	Comment 12 • 7 years ago
mozreview-review

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.

	Pulsebot
	Comment 17 • 7 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ea98df19ce7b
https://hg.mozilla.org/mozilla-central/rev/4beea83e0858
https://hg.mozilla.org/mozilla-central/rev/ff19950e47d9
https://hg.mozilla.org/mozilla-central/rev/a8075676dcb3

	Eric Shepherd [:sheppy]
	Comment 19 • 6 years ago

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.

	Eric Shepherd [:sheppy]
	Comment 20 • 6 years ago

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.

	Ralph Giles (:rillian)
	Comment 21 • 6 years ago

Yes. There may be bugs, but we support Regions now.