The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla22

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jesse Silver, Assigned: Jesse Silver)

Tracking

(Blocks: 1 bug)

Trunk
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121128204232
Spin-off from bug 629350 to implement nsVideoFrame changes for webvtt captions.
Assignee: nobody → jasilver1
Blocks: 629350
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
(Assignee)

Comment 2

4 years ago
Created attachment 708628 [details] [diff] [review]
Work in progress for the layout portion of the patch

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

Comment 3

4 years ago
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.
Attachment #708628 - Flags: review?(cpearce)
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?
Attachment #708628 - Flags: review?(roc)
Attachment #708628 - Flags: review?(cpearce)
Attachment #708628 - Flags: review-
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.
(Assignee)

Comment 6

4 years ago
Created attachment 716496 [details] [diff] [review]
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.
Attachment #708628 - Attachment is obsolete: true
Attachment #708628 - Flags: review?(roc)
Attachment #716496 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #716496 - Flags: review? → review?(roc)
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.
(Assignee)

Comment 8

4 years ago
Created attachment 717161 [details] [diff] [review]
v3 of the Caption Div patch

Removed the .captionDiv class, changed the css to be for video > div instead.
Attachment #716496 - Attachment is obsolete: true
Attachment #716496 - Flags: review?(roc)
Attachment #717161 - Flags: review?(roc)
Attachment #717161 - Flags: review?(roc) → review+
Jesse, please rebase this against current m-c. I'll do a try server push and then we can land.
(Assignee)

Comment 10

4 years ago
Created attachment 718166 [details] [diff] [review]
Updating to the current m-c.
Attachment #717161 - Attachment is obsolete: true
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
Attachment #718166 - Flags: feedback+
(Assignee)

Comment 12

4 years ago
Created attachment 718209 [details] [diff] [review]
Added in author/commit info
Attachment #718166 - Attachment is obsolete: true
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.
(Assignee)

Comment 14

4 years ago
Created attachment 718622 [details] [diff] [review]
Removed trailing whitespace
Attachment #718209 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
Created attachment 718627 [details] [diff] [review]
Changed commit message slightly
Attachment #718622 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=4e3ade8ac100
(Assignee)

Comment 17

4 years ago
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.
Attachment #718627 - Flags: review?(roc)
Attachment #718627 - Flags: review?(roc) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/41458ff35ced

Thanks for the patch!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/41458ff35ced
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
>+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!
Depends on: 875305
You need to log in before you can comment on or make changes to this bug.