833388 - [webvtt] Add captions div to nsVideoFrame for webvtt subtitle display

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Silver]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121128204232

Comment 1

[David Humphrey (:humph)]

Spin-off from bug 629350 to implement nsVideoFrame changes for webvtt captions.

Comment 2

[Jesse Silver]

Attachment: Work in progress for the layout portion of the patch

This is simply the patch from #629350 split up into just the layout changes.

Comment 3

[Jesse Silver]

Comment on attachment 708628 [details] [diff] [review]
Work in progress for the layout portion of the patch

This is my initial patch for this that was simply from Ralph's original larger WebVTT patch. I would like to get this reviewed so I know how to move forward on this bug.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 708628 [details] [diff] [review]
Work in progress for the layout portion of the patch

Review of attachment 708628 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this! This patch is mostly there, I'm not granting review because we shouldn't mix unrelated build system changes in with behaviour changes, plus a few minor changes/questions.

I'd also like Roc to take a look at this, he's more qualified to review layout/generic/ changes than I.

::: configure.in
@@ +4245,5 @@
>  MOZ_OMX_PLUGIN=
>  MOZ_VP8=
>  MOZ_VP8_ERROR_CONCEALMENT=
>  MOZ_VP8_ENCODER=
> +MOZ_WEBVTT=1

This isn't used in this patch, this change, and the change in Makefile.in should be in another patch (possibly with other changes) and it should be reviewed by one of the build peers, like :khuey or :ted.

::: layout/generic/nsVideoFrame.cpp
@@ +114,5 @@
>    NS_TrustedNewXULElement(getter_AddRefs(mVideoControls), nodeInfo.forget());
>    if (!aElements.AppendElement(mVideoControls))
>      return NS_ERROR_OUT_OF_MEMORY;
>  
> +  // Set up the caption overlay div for showing any TextTrack data

Based on the definition of the <track> element's @kind attribute, it seems subtitles and captions should only be shown on <video> elements:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#attr-track-kind

Indeed, the spec authors seem to be saying here that if you want subtitles on an audio element, you should use a video element instead:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=20898

So assuming we only need the caption div for subtitles and captions, I think we should only create it inside the if(HasVideoElement()) block.

(We use these video controls for audio too, the controls are written in JavaScript and figure out if they are controlling a video or audio mode and behave appropriately).

@@ +439,4 @@
>    for (nsIFrame *child = mFrames.FirstChild();
>         child;
>         child = child->GetNextSibling()) {
> +    if (child->GetContent() != mPosterImage || ShouldDisplayPoster()) {

Hmmm, why did you remove the boxFrame condition here?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 708628 [details] [diff] [review]
Work in progress for the layout portion of the patch

Review of attachment 708628 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsVideoFrame.cpp
@@ +120,5 @@
> +                                          nullptr,
> +                                          kNameSpaceID_XHTML,
> +                                          nsIDOMNode::ELEMENT_NODE);
> +  NS_ENSURE_TRUE(nodeInfo, NS_ERROR_OUT_OF_MEMORY);
> +  element = NS_NewHTMLDivElement(nodeInfo.forget());

Don't reuse the 'element' declaration. Use a new Element* declaratoin or just directly assign to mCaptionDiv.

@@ +135,5 @@
> +    return NS_ERROR_OUT_OF_MEMORY;
> +
> +  // add a test string so we can test the div
> +  nsCOMPtr<nsIDOMHTMLElement> div = do_QueryInterface(mCaptionDiv);
> +  rv = div->SetInnerHTML(NS_LITERAL_STRING(""));

Don't do this. This doesn't do anything anyway.

@@ +335,5 @@
> +                                       availableSize,
> +                                       aMetrics.width,
> +                                       aMetrics.height);
> +      kidReflowState.SetComputedWidth(aReflowState.ComputedWidth());
> +      kidReflowState.SetComputedHeight(aReflowState.ComputedHeight());

You need to subtract mCaptionDiv's computed border+padding here --- you need to pass in mCaptionDiv's content-box size to SetComputedWidth/SetComputedHeight.

@@ +439,4 @@
>    for (nsIFrame *child = mFrames.FirstChild();
>         child;
>         child = child->GetNextSibling()) {
> +    if (child->GetContent() != mPosterImage || ShouldDisplayPoster()) {

I think that's OK. Basically this code says that we display all child content unconditionally except for the poster image.

Comment 6

[Jesse Silver]

Attachment: v2 of the Caption Div patch

Made changes to the patch based on reviews.

"You need to subtract mCaptionDiv's computed border+padding here --- you need to pass in mCaptionDiv's content-box size to SetComputedWidth/SetComputedHeight."

I'm a bit unsure about this one. I took a guess based on other places I saw SetComputedWidth() being used. I would like to know the proper way this should be done.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 716496 [details] [diff] [review]
v2 of the Caption Div patch

Review of attachment 716496 [details] [diff] [review]:
-----------------------------------------------------------------

Otherwise looks good.

::: layout/generic/nsVideoFrame.cpp
@@ +113,5 @@
> +    // hang a class name on the div for default styling
> +    nsresult rv = mCaptionDiv->SetAttr(kNameSpaceID_None,
> +                                       nsGkAtoms::_class,
> +                                       NS_LITERAL_STRING("captionDiv"),
> +                                       true);

Actually I don't think we need this class. There's only one anonymous DIV in a <video> so you can just style video > div.

Comment 8

[Jesse Silver]

Attachment: v3 of the Caption Div patch

Removed the .captionDiv class, changed the css to be for video > div instead.

Comment 9

[Ralph Giles (:rillian)]

Jesse, please rebase this against current m-c. I'll do a try server push and then we can land.

Comment 10

[Jesse Silver]

Attachment: Updating to the current m-c.

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 718166 [details] [diff] [review]
Updating to the current m-c.

Thanks, the rebase looks good to me.

The next step is to generate a proper git diff with a commit message and author information suitable for check-in. This should include the name and email address you want in the commit history. Maintainers often do this for casual contributors, but it's good practice to learn to do so when submitting patches.

Create at git branch (or hg patch queue) with just this change on it, then do a 'git commit' or 'git rebase -i' and squash your changes, then write a commit message describing the bug, change, review approval and motivation. Standard style is something like:

  Bug 833388 - Add captions div to nsVideoFrame - r=roc
  [blank line]
  <description of what changed and why>

Please upload the result (output from git show or git format-patch) as a new attachment for final review.

Pushed this one to try as https://tbpl.mozilla.org/?tree=Try&rev=a3717d34b250

Comment 12

[Jesse Silver]

Attachment: Added in author/commit info

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 718209 [details] [diff] [review]
Added in author/commit info

Review of attachment 718209 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Commit message looks good. You should probably strip the changed-files section to avoid hg importing that as part of the commit message.

Please remove the trailing whitespace. You can use 'git diff --check' to catch problems like this. You also have inconsistent line endings in your patch, with windows eol on all the new additions, but unix on the context. You should fix that, probably by configuring your editor to use whatever line ending convention msysgit expects. Sorry for not noticing earlier.

::: layout/generic/nsVideoFrame.cpp
@@ +113,5 @@
> +
> +    if (!aElements.AppendElement(mCaptionDiv))
> +      return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  

Please remove this trailing whitespace.

@@ +325,5 @@
> +                                       aMetrics.height);
> +      nsSize size(aReflowState.ComputedWidth(), aReflowState.ComputedHeight());
> +      size.width -= kidReflowState.mComputedBorderPadding.LeftRight();
> +      size.height -= kidReflowState.mComputedBorderPadding.TopBottom();
> +      

Here too.

@@ +328,5 @@
> +      size.height -= kidReflowState.mComputedBorderPadding.TopBottom();
> +      
> +      kidReflowState.SetComputedWidth(std::max(size.width, 0));
> +      kidReflowState.SetComputedHeight(std::max(size.height, 0));
> +      

And here.

Comment 14

[Jesse Silver]

Attachment: Removed trailing whitespace

Comment 15

[Jesse Silver]

Attachment: Changed commit message slightly

Comment 16

[Ralph Giles (:rillian)]

https://tbpl.mozilla.org/?tree=Try&rev=4e3ade8ac100

Comment 17

[Jesse Silver]

Comment on attachment 718627 [details] [diff] [review]
Changed commit message slightly

Review of attachment 718627 [details] [diff] [review]:
-----------------------------------------------------------------

There were changes made because of the rebase, so it needs to be looked over one more time.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41458ff35ced

Thanks for the patch!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41458ff35ced

Comment 20

[Boris Zbarsky [:bzbarsky]]

>+video > div {
>+video > div p {

Why are we adding slow rules like this (which will require nontrivial extra work for every div and p) to html.css?  See https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS

Please get this fixed!