Closed Bug 1318542 Opened 8 years ago Closed 7 years ago

[webvtt] support ::cue pseudoelement from document.

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 3 obsolete files)

Bug 1318542 - Make CueStyleBox apply ::cue.
58 bytes, text/x-review-board-request
rillian
: review+
heycam
: review+
Details
Bug 1318542 - Disable testcase 3_track.html, see bug1342063, the testcase un-expected-PASS on OSX 10.10.
59 bytes, text/x-review-board-request
alwu
: review+
Details
Bug 1318542 - Add preference "media.webvtt.pseudo.enabled".
59 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1318542 - Label NODE_IS_NATIVE_ANONYMOUS on the elements from ::cue up to nsVideoFrame.
59 bytes, text/x-review-board-request
heycam
: review+
bzbarsky
: review+
Details
Bug 1318542 - fix testcases due to new pseudo element ::cue.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
This is the first step to fully support ::cue pseudoelement for bug 865395.

In this bug, I want to support the ::cue appears in the document such as
<head>
xxx
<style>
::cue
{
color: red;
}
</style>
</head>
Comment on attachment 8818222 [details]
Bug 1318542 - Make CueStyleBox apply ::cue.

https://reviewboard.mozilla.org/r/98366/#review99012

::: dom/media/webvtt/vtt.jsm:739
(Diff revision 1)
> +    if (isFirefox) {
> +      delete styles.color;
> +    }

The modifications of vtt.jsm are remove the css attributes covered by ::cue, and move them into html.css.

::: layout/generic/nsVideoFrame.cpp:140
(Diff revision 1)
> +    CSSPseudoElementType pseudoType = CSSPseudoElementType::cue;
> +    RefPtr<nsStyleContext> newStyleContext = PresContext()->StyleSet()->
> +      ResolvePseudoElementStyle(mCaptionDiv->AsElement(), pseudoType,
> +        nullptr, mCaptionDiv->AsElement());
> +    const nsStyleColor* styleColor = newStyleContext->StyleColor();
> +    printf_stderr("StyleColor %x\n",styleColor->mColor);
> +    nsString colorStr;
> +    CopyUTF8toUTF16(nsPrintfCString("color: rgba(%d, %d, %d, ",
> +                                        NS_GET_R(styleColor->mColor),
> +                                        NS_GET_G(styleColor->mColor),
> +                                        NS_GET_B(styleColor->mColor)),
> +                                        colorStr);
> +    colorStr.AppendFloat(nsStyleUtil::ColorComponentToFloat(NS_GET_A(styleColor->mColor)));
> +    colorStr.Append(')');
> +    div->SetAttr(kNameSpaceID_None, nsGkAtoms::style, colorStr, false);
> +
> +    if(!aElements.AppendElement(ContentInfo(mCaptionDiv))) {

Through |ResolvePseudoElementStyle| function, we can get the styleContext which compare ::cue in document with ::cue in html.css. And the ::cue in document has higher priority.

Then I pick the css attributes in ::cue(color attribute) from the styleContext, set it to the div.

Howevery, I do not append the styleContext to the div directly because the styleContext doesn't contain the whole css attributes I want. For example the "position, width, height" attributes are mandatary for showing the text, but incorrect in the styleContext. (see the .caption-box in html.css)
(In reply to Benjamin Chen [:bechen] from comment #2)
> 
> Through |ResolvePseudoElementStyle| function, we can get the styleContext
> which compare ::cue in document with ::cue in html.css. And the ::cue in
> document has higher priority.
> 
> Then I pick the css attributes in ::cue(color attribute) from the
> styleContext, set it to the div.
> 
> Howevery, I do not append the styleContext to the div directly because the
> styleContext doesn't contain the whole css attributes I want. For example
> the "position, width, height" attributes are mandatary for showing the text,
> but incorrect in the styleContext. (see the .caption-box in html.css)

Hi Cameron:
Would it work if I don't append the styleContext to the div?
And I'm worry about the ::cue(selector) bug 1321489 if I don't append the styleContext.
Will the styleContext be used for ::cue(selector)?
Flags: needinfo?(cam)
Comment on attachment 8818222 [details]
Bug 1318542 - Make CueStyleBox apply ::cue.

https://reviewboard.mozilla.org/r/98366/#review99360

::: layout/generic/nsVideoFrame.cpp:140
(Diff revision 1)
> +    CSSPseudoElementType pseudoType = CSSPseudoElementType::cue;
> +    RefPtr<nsStyleContext> newStyleContext = PresContext()->StyleSet()->
> +      ResolvePseudoElementStyle(mCaptionDiv->AsElement(), pseudoType,
> +        nullptr, mCaptionDiv->AsElement());
> +    const nsStyleColor* styleColor = newStyleContext->StyleColor();
> +    printf_stderr("StyleColor %x\n",styleColor->mColor);
> +    nsString colorStr;
> +    CopyUTF8toUTF16(nsPrintfCString("color: rgba(%d, %d, %d, ",
> +                                        NS_GET_R(styleColor->mColor),
> +                                        NS_GET_G(styleColor->mColor),
> +                                        NS_GET_B(styleColor->mColor)),
> +                                        colorStr);
> +    colorStr.AppendFloat(nsStyleUtil::ColorComponentToFloat(NS_GET_A(styleColor->mColor)));
> +    colorStr.Append(')');
> +    div->SetAttr(kNameSpaceID_None, nsGkAtoms::style, colorStr, false);
> +
> +    if(!aElements.AppendElement(ContentInfo(mCaptionDiv))) {

The problem with doing this, is that if the ::cue rules in the document's style sheets change, then we won't restyle the div.  This is because the only time we would copy the resolved style to the style="" attribute is here, at the time we create the div.  If script adds a new ::cue rule to a document style sheet, or if an external style sheet with a ::cue rule finishes loading after we've created the video's VTT div container element, then there is no way for that to cause the style="" attribute to be updated.

The style context that we get back from ResolvePseudoElementStyle has "::cue" recorded on it, so that if we ever restyle the document (due to style sheet changes) we know what kinds of styles to resolve for it again.  Without using the style context for the div, we can't know this.

So I think we need to solve the problem that is preventing you from using the style context directly.


> Will the styleContext be used for ::cue(selector)?

For ::cue(selector), I'm thinking that we don't need to explicitly style the descendant elements in CreateAnonymousContent.  (In fact, we can't, since in here we only have the top level div that we create.)  Instead, we allow webvtt.jsm to create the child content of the div later, but then we add special handling for the ::cue(selector) selector.  This will make it behave kind of like

  ::cue > selector

(though that is not valid CSS).  It will mean we have to implement the matching behavior of ::cue(selector) in nsCSSRuleProcessor.cpp.

::: layout/generic/nsVideoFrame.cpp:146
(Diff revision 1)
> +    nsString colorStr;
> +    CopyUTF8toUTF16(nsPrintfCString("color: rgba(%d, %d, %d, ",

FYI you can do this a bit shorter with:

  colorStr.AppendPrintf("color: rgba(%d .....

::: layout/style/res/html.css:774
(Diff revision 1)
>    overflow: hidden;
>  }
>  
> +::cue {
> +  color: rgba(255, 255, 255, 1);
> +  whiteSpace: pre-line;

This should be white-space.
Comment on attachment 8818222 [details]
Bug 1318542 - Make CueStyleBox apply ::cue.

https://reviewboard.mozilla.org/r/98364/#review99694

::: layout/generic/nsVideoFrame.cpp:136
(Diff revision 2)
> -    nsGenericHTMLElement* div = static_cast<nsGenericHTMLElement*>(mCaptionDiv.get());
> -    div->SetClassName(NS_LITERAL_STRING("caption-box"));
> +    nsGenericHTMLElement* div1 = static_cast<nsGenericHTMLElement*>(mCaptionDiv.get());
> +    div1->SetClassName(NS_LITERAL_STRING("caption-box"));
>  
> -    if (!aElements.AppendElement(mCaptionDiv))
> +    nodeInfo = nodeInfoManager->GetNodeInfo(nsGkAtoms::div,
> +                                            nullptr,
> +                                            kNameSpaceID_XHTML,
> +                                            nsIDOMNode::ELEMENT_NODE);
> +    NS_ENSURE_TRUE(nodeInfo, NS_ERROR_OUT_OF_MEMORY);
> +    mPesudoElementCue = NS_NewHTMLDivElement(nodeInfo.forget());
> +    NS_ENSURE_TRUE(mPesudoElementCue, NS_ERROR_OUT_OF_MEMORY);
> +    nsGenericHTMLElement* div2 = static_cast<nsGenericHTMLElement*>(mPesudoElementCue.get());
> +    div2->SetClassName(NS_LITERAL_STRING("caption-box-cue"));
> +
> +    // Associate ::cue pseudo-element to the div2.
> +    CSSPseudoElementType pseudoType = CSSPseudoElementType::cue;
> +    RefPtr<nsStyleContext> newStyleContext = PresContext()->StyleSet()->
> +      ResolvePseudoElementStyle(mCaptionDiv->AsElement(), pseudoType,
> +        nullptr, mPesudoElementCue->AsElement());
> +    const nsStyleColor* styleColor = newStyleContext->StyleColor();
> +    printf_stderr("StyleColor %x\n",styleColor->mColor);
> +    const nsStyleDisplay* styleDis = newStyleContext->StyleDisplay();
> +    printf_stderr("position %d\n",styleDis->mPosition);
> +
> +    div1->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
> +        NS_LITERAL_STRING("display: inline"),false);
> +    ContentInfo root(mCaptionDiv);
> +    ContentInfo cue(mPesudoElementCue, newStyleContext);
> +    root.mChildren.AppendElement(cue);
> +    if(!aElements.AppendElement(root)) {

Patch version 2, at this version, we are trying to use the styleContex get by ResolvePseudoElementStyle and also preserve the "position/width/height" css attributes.

So, under nsVideoFrame, we have 2 divs named div1 and div2. nsVideoFrame is div1's parent and div1 is div2's parent. From html.css we set "position/width/height" to div1 as usual, then append the styleContex to div2.

Here comes 2 issues:
1. If I don't set "display: inline" for div1, the styleContext of div2 is useless.
It is because the div2 is not an AnonymousContent of div1, so nsCSSFrameConstructor::ProcessChildren for div1 won't refer the styleContex at div2.
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#10786 to line 10794

2. Then I found the nsCSSFrameConstructor::BuildInlineChildItems function, if I set inline to div1, the styleContext works for div2.
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#6008
But the styleContext from ResolvePseudoElementStyle seems not inherit the "position/width/height" from div1, the result still incorrect.
Thanks for writing down these issues you found about style contexts not being able to be given for children of anonymous content, unless you make the parent inline.

I had a thought about how we could avoid this problem:

* we have a single <div> to contain the WebVTT content
* we set a style="position: ..." attribute on the <div> to set the position and other "special" properties that we don't want authors to be able to set
* we keep using ResolvePseudoElementStyle with nsGkAtoms::cue
* we add the CSS_PSEUDO_ELEMENT_SUPPORTS_STYLE_ATTRIBUTE flag to the "cue" entry in nsCSSPseudoElementList.h

I did a quick experiment adding style="" attribute support to some other pseduo-element (::-moz-meter-bar) and it seemed to work.  Can you give that a try?
Flags: needinfo?(cam)
See Also: → 1326172
Attachment #8821996 - Flags: review?(cam)
Attachment #8821997 - Flags: review?(cam)
Attachment #8821996 - Flags: review?(cam) → review?(dbaron)
Comment on attachment 8821997 [details]
Bug 1318542 - part2: Add an argument in ResolvePseudoElementStyle and FileRules to refer ::cue in document under NAC. r?

https://reviewboard.mozilla.org/r/101072/#review101966

This looks good, but just one change I think will make this clearer.  Let's use an aFlags argument so that we can pass in more obvious-looking values, like nsStyleSet::ResolveStyleWithReplacement does.  So, add an enum with a single value eForceDocSheets (or maybe better: eDontSkipDocSheets), and make the argument a |uint32_t aFlags|.  r=me with that.

::: layout/style/nsStyleSet.cpp:1157
(Diff revision 2)
>      // We can supply additional document-level sheets that should be walked.
>      mBindingManager->WalkRules(aCollectorFunc,
>                                 static_cast<ElementDependentRuleProcessorData*>(aData),
>                                 &cutOffInheritance);
>    }
> -  if (!skipUserStyles && !cutOffInheritance && // NOTE: different
> +  if (((!skipUserStyles && !cutOffInheritance) || aForceDocSheet) && // NOTE: different

The comment is pointing out that "skipUserStyles" and "cutOffInheritance" are different things.  I'm not sure how valuable that comment is, but to make it less confusing, let's rewrap the line like:

  if (((!skipUserStyles && !cutOffInheritance) ||  // NOTE: different
       aForceDocSheet) &&
      mRuleProcessors[SheetType::Doc])
Attachment #8821997 - Flags: review?(cam) → review+
Comment on attachment 8821996 [details]
Bug 1318542 - part1: Allow JS to create NAC pseudo-elements. r?

https://reviewboard.mozilla.org/r/101070/#review101984

Here are some preliminary comments, although I still need to look further into what makes dynamic reresolution of style work correctly here.

::: dom/base/nsDocument.cpp:5475
(Diff revision 2)
> +  if (aString.IsEmpty()) {
> +    return CSSPseudoElementType::NotPseudo;
> +  }

It's not clear to me why you want a special case for an explicitly-passed empty string.

::: dom/base/nsDocument.cpp:5519
(Diff revision 2)
>      if (rv.Failed()) {
>        return nullptr;
>      }
> +
> +    // Check 'pseudo' and throw an exception if it's not a recognized
> +    // pseduo-element name, or if it's one of the structural pseudo-elements

s/pseduo/pseudo/

::: dom/base/nsDocument.cpp:5526
(Diff revision 2)
> +    if (options.mPseudo.WasPassed()) {
> +      pseudoType = GetPseudoElementType(options.mPseudo.Value(), rv);
> +      if (rv.Failed()) {
> +        return nullptr;
> +      }
> +      if (pseudoType != CSSPseudoElementType::NotPseudo) {

Silently ignoring the request if the result is NotPseudo seems like the wrong thing.  Shouln't it fail, like some of the other errors?

::: layout/base/nsCSSFrameConstructor.cpp:5049
(Diff revision 2)
>  
>    RefPtr<nsStyleContext> result;
>    if (aContent->IsElement()) {
> +    auto pseudoType = CSSPseudoElementType(reinterpret_cast<uintptr_t>(
> +        aContent->GetProperty(nsGkAtoms::pseudoProperty)));
> +    if (pseudoType == CSSPseudoElementType(0)) {

Instead of testing this, please pass the second (nsresult) parameter to nsINode::GetProperty, and check it against NS_PROPTABLE_PROP_NOT_THERE.  (You can then remove the 3-line comment following.)

::: layout/base/nsCSSFrameConstructor.cpp:5070
(Diff revision 2)
> +      Element* parent = aContent->GetParentElement();
> +      while (!parent->IsRootOfNativeAnonymousSubtree()) {
> +        parent = parent->GetParentElement();
> +      }

This seems strange to me, for two reasons:

1.  I would have expected parent to be the closest ancestor *outside* the native-anonymous subtree, not the root of the native-anonymous subtree.

2. I would have expected aParentStyleContext to be the style context of parent.
I also don't understand what makes part 2 work correctly for recomputation of style.
Comment on attachment 8821996 [details]
Bug 1318542 - part1: Allow JS to create NAC pseudo-elements. r?

https://reviewboard.mozilla.org/r/101070/#review101984

> Silently ignoring the request if the result is NotPseudo seems like the wrong thing.  Shouln't it fail, like some of the other errors?

How about check the mPseudo is ::cue or not?

> This seems strange to me, for two reasons:
> 
> 1.  I would have expected parent to be the closest ancestor *outside* the native-anonymous subtree, not the root of the native-anonymous subtree.
> 
> 2. I would have expected aParentStyleContext to be the style context of parent.

1. In my ::cue case, the tree structure is: nsVideoFrame --> div named caption-box(native-anonymous root) --> subtree created by vtt.jsm (::cue inside here), so I would expect the |parent| is the native-anonymous root because I don't think the css rules on nsVideoFrame should affect the ::cue.
2. Do you mean that we should pass |parent| and |parent's styleContex| into ResolvePseudoElementStyle instead |aParentStyleContext|? I'm not quite familiar the code here, :heycam, help please.
Flags: needinfo?(cam)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #17)
> I also don't understand what makes part 2 work correctly for recomputation
> of style.

Because the ::cue is under native-anonymous subtree, so the nsStyleSet::FileRules won't read the ::cue rules appear in document.
(In reply to Benjamin Chen [:bechen] from comment #19)
> (In reply to David Baron :dbaron: ⌚️UTC-8 from comment #17)
> > I also don't understand what makes part 2 work correctly for recomputation
> > of style.
> 
> Because the ::cue is under native-anonymous subtree, so the
> nsStyleSet::FileRules won't read the ::cue rules appear in document.

Oh, I might misunderstand your question.
Comment on attachment 8821996 [details]
Bug 1318542 - part1: Allow JS to create NAC pseudo-elements. r?

https://reviewboard.mozilla.org/r/101070/#review102414

::: layout/base/nsCSSFrameConstructor.cpp:5070
(Diff revision 2)
> +      Element* parent = aContent->GetParentElement();
> +      while (!parent->IsRootOfNativeAnonymousSubtree()) {
> +        parent = parent->GetParentElement();
> +      }

Yes, I think this might be wrong.  We want parent to be the video element (so that video::cue works).

I think David is right that the video element's style should be the parent style.  This would mean for example:

  <style>
  video { outline: 1px solid red; }
  video::cue { outline: inherit; }
  </style>
  <video>...</video>

would result in cues having a red outline, as well as the entire video, but that is consistent with how pseudo-elements are modelled in CSS.
Comment on attachment 8821996 [details]
Bug 1318542 - part1: Allow JS to create NAC pseudo-elements. r?

https://reviewboard.mozilla.org/r/101070/#review101984

I maybe need to move this patch to another bug so I can push updates to it, but I'll reply to these comments here first.

> It's not clear to me why you want a special case for an explicitly-passed empty string.

It was really just for consistency with getComputedStyle's second argument.

> How about check the mPseudo is ::cue or not?

Yeah, I guess we should just throw NS_ERROR_DOM_NOT_SUPPORTED_ERR here too.

> 1. In my ::cue case, the tree structure is: nsVideoFrame --> div named caption-box(native-anonymous root) --> subtree created by vtt.jsm (::cue inside here), so I would expect the |parent| is the native-anonymous root because I don't think the css rules on nsVideoFrame should affect the ::cue.
> 2. Do you mean that we should pass |parent| and |parent's styleContex| into ResolvePseudoElementStyle instead |aParentStyleContext|? I'm not quite familiar the code here, :heycam, help please.

Yes, I think this might be wrong.  We want parent to be the video element (so that video::cue works).

I think David is right that the video element's style should be the parent style.  This would mean for example:

  <style>
  video { outline: 1px solid red; }
  video::cue { outline: inherit; }
  </style>
  <video>...</video>

would result in cues having a red outline, as well as the entire video, but that is consistent with how pseudo-elements are modelled in CSS.
Comment on attachment 8821996 [details]
Bug 1318542 - part1: Allow JS to create NAC pseudo-elements. r?

https://reviewboard.mozilla.org/r/101070/#review102414

> Yes, I think this might be wrong.  We want parent to be the video element (so that video::cue works).
> 
> I think David is right that the video element's style should be the parent style.  This would mean for example:
> 
>   <style>
>   video { outline: 1px solid red; }
>   video::cue { outline: inherit; }
>   </style>
>   <video>...</video>
> 
> would result in cues having a red outline, as well as the entire video, but that is consistent with how pseudo-elements are modelled in CSS.

After I change the code to pass "videoElement" and "its styleContext "into |ResolvePseudoElementStyle|, the "video::cue" works.
But now I have another problem about font-size.
The tree structure is: nsVideoElement --> div named caption-box(anonymous root) --> div named paddedoverlay(created by vtt.jsm) --> cueStyleBox(::cue, created by vtt.jsm) (note that the nodes created by vtt.jsm will keep destroy/create at runtime).
At the original implementation, the font-size is changed according to the videoElement size. During playback, the vtt.jsm creates subtree append to the caption-box, and set font-size to cueStyleBox.
At this bug, we apply ::cue to cueStyleBox, so we must do some change about the font-size. The goal is that if the font-size rule appears in ::cue, of course respect it, otherwise the font-size should depend on the size of videoElement. To achieve that, I set the font-size at the paddedoverlay and "font-size: 1em" at cueStyleBox. But after the code change(use videoElement's styleContex), it doesn't work.
Hi David: do you have any idea about how do we respect the rules in ::cue but also keep the mechanism (dynamic font-size)?
Flags: needinfo?(dbaron)
(In reply to Benjamin Chen [:bechen] from comment #23)
> After I change the code to pass "videoElement" and "its styleContext "into
> |ResolvePseudoElementStyle|, the "video::cue" works.
> But now I have another problem about font-size.
> The tree structure is: nsVideoElement --> div named caption-box(anonymous
> root) --> div named paddedoverlay(created by vtt.jsm) --> cueStyleBox(::cue,
> created by vtt.jsm) (note that the nodes created by vtt.jsm will keep
> destroy/create at runtime).
> At the original implementation, the font-size is changed according to the
> videoElement size. During playback, the vtt.jsm creates subtree append to
> the caption-box, and set font-size to cueStyleBox.
> At this bug, we apply ::cue to cueStyleBox, so we must do some change about
> the font-size. The goal is that if the font-size rule appears in ::cue, of
> course respect it, otherwise the font-size should depend on the size of
> videoElement. To achieve that, I set the font-size at the paddedoverlay and
> "font-size: 1em" at cueStyleBox. But after the code change(use
> videoElement's styleContex), it doesn't work.
> Hi David: do you have any idea about how do we respect the rules in ::cue
> but also keep the mechanism (dynamic font-size)?

So this is the expected result -- video::cue should inherit directly from video, and not from implementation-specific elements that happen to live between them.

If I'm understanding correctly, it seems like the thing that you're doing to change the default font size as a function of the video element's size should specify that size on video::cue, but at a lower precedence in the cascade than an author rule would have, so that an author rule will override it.
Flags: needinfo?(dbaron)
Comment on attachment 8821996 [details]
Bug 1318542 - part1: Allow JS to create NAC pseudo-elements. r?

https://reviewboard.mozilla.org/r/101070/#review103592

So the further thing (one comment on it below) that needs to be investigated more carefully is whether this is determining the parent the same way for recomputation (starting from implementations of nsIFrame::GetParentStyleContext, and particularly where it ends up calling into things like nsFrame::GetCorrectedParent) as it does for the initial pass of computation.

I think there are some assertions to check that, but you likely want to add some more to verify the invariants you're adding, since this is getting tricky.

::: layout/base/nsCSSFrameConstructor.cpp:5070
(Diff revision 2)
> +      Element* parent = aContent->GetParentElement();
> +      while (!parent->IsRootOfNativeAnonymousSubtree()) {
> +        parent = parent->GetParentElement();

One further comment here is that, in addition to this being wrong, I think you want to reuse nsFrame::CorrectStyleParentFrame (or perhaps even one of its callers in nsFrame.cpp) so that we're using the same code for determining the parent in the initial frame construction case and when we recompute style.

It's also worth considering carefully whether interesting things happen if this interacts with other interesting cases, e.g., ::cue { display: table } or ::cue { overflow: scroll }... or maybe similar styles for video, if that's possible.
Attachment #8821996 - Flags: review?(dbaron) → review-
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #24)
> So this is the expected result -- video::cue should inherit directly from
> video, and not from implementation-specific elements that happen to live
> between them.
> 
> If I'm understanding correctly, it seems like the thing that you're doing to
> change the default font size as a function of the video element's size
> should specify that size on video::cue, but at a lower precedence in the
> cascade than an author rule would have, so that an author rule will override
> it.

Yeah, I guess that would give the correct cascading and inheritance behavior.  Otherwise things like

  ::cue { background: inherit; }

are not going to correctly inherit from the <video>.

But where would be the best place to inject that style?  The font-size computations are done by WebVTT.jsm, which makes it easy for the current approach of just manipulating style="" on the parent of the <div> acting as the ::cue.  We could maybe use a <style scoped>, but I really don't want to add a use of that given that it's likely to go away.

What about: we generate a unique class name for the ::cue <div> element, then create an add a sheet to the UA level with nsIDocument::AddAdditionalStyleSheet()?  Since we're in NAC, there's no chance of the rules in this sheet accidentally matching elements in the author's document.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #25)
> It's also worth considering carefully whether interesting things happen if
> this interacts with other interesting cases, e.g., ::cue { display: table }
> or ::cue { overflow: scroll }... or maybe similar styles for video, if
> that's possible.

::cue has a limited set of properties allowed on it, and display and overflow aren't in there, so we're safe from uncommon frame tree structures here.
Flags: needinfo?(cam) → needinfo?(dbaron)
Adding style sheets is slow since we need to restyle everything.  So that's not so great.

It seems like it would be nice to use CSS variables, although we don't have a mechanism for setting a hidden-from-content CSS variable on the <video>.  Maybe we should have one?  Or maybe we could specify the variable's value in a style attribute on the pseudo-element, and also use CSS_PSEUDO_ELEMENT_SUPPORTS_STYLE_ATTRIBUTE?
Flags: needinfo?(dbaron)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #27)
> Adding style sheets is slow since we need to restyle everything.  So that's
> not so great.
> 
> It seems like it would be nice to use CSS variables, although we don't have
> a mechanism for setting a hidden-from-content CSS variable on the <video>. 
> Maybe we should have one?  Or maybe we could specify the variable's value in
> a style attribute on the pseudo-element, and also use
> CSS_PSEUDO_ELEMENT_SUPPORTS_STYLE_ATTRIBUTE?

CSS variable seems works. I declare --cue-font-size in html.css and set it at runtime in vtt.jsm.

In vtt.jsm:
div = window.document.createElement("div", {pseudo: "::cue"});
div.style.setProperty('--cue-font-size', fontSize + "px "); // The fontSize is the value based on <video> size

In html.css:
::cue {
  font-size: var(--cue-font-size);
}
That looks like a good solution!  I was worried that it would still allow the author to interfere by setting --cue-font-size themselves somewhere, but |video { --cue-font-size: ... }| is OK, since the one we set in the style="" attribute will override it, and |::cue { --cue-font-size: ... }| is also OK, since that will be rejected by the CSS parser (as it's not one of the predefined properties we allow in ::cue).
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #25)
> Comment on attachment 8821996 [details]
> Bug 1318542 - part1: Allow JS to create NAC pseudo-elements. r?
> 
> https://reviewboard.mozilla.org/r/101070/#review103592
> 
> So the further thing (one comment on it below) that needs to be investigated
> more carefully is whether this is determining the parent the same way for
> recomputation (starting from implementations of
> nsIFrame::GetParentStyleContext, and particularly where it ends up calling
> into things like nsFrame::GetCorrectedParent) as it does for the initial
> pass of computation.
> 
So here we should do:
nsFrame::CorrectStyleParentFrame()
{
 if (::cue)
  retrun "frame of <video>"
}

To make sure the <video> is the parent of ::cue in style system, right?

> ::: layout/base/nsCSSFrameConstructor.cpp:5070
> (Diff revision 2)
> > +      Element* parent = aContent->GetParentElement();
> > +      while (!parent->IsRootOfNativeAnonymousSubtree()) {
> > +        parent = parent->GetParentElement();
> 
> One further comment here is that, in addition to this being wrong, I think
> you want to reuse nsFrame::CorrectStyleParentFrame (or perhaps even one of
> its callers in nsFrame.cpp) so that we're using the same code for
> determining the parent in the initial frame construction case and when we
> recompute style.
The nsFrame::CorrectStyleParentFrame or GetCorrectedParent needs a nsFrame as input parameter.
Do we have a frame object at the first time we call nsCSSFrameConstructor::ResolveStyleContext? I guess no.

And if we decide to make <video> as ::cue's parent, are the CSS rules of nodes between them useless to ::cue?
Depends on: 1330843
Attachment #8821996 - Attachment is obsolete: true
Attachment #8821997 - Attachment is obsolete: true
Attachment #8818222 - Flags: review?(giles)
Comment on attachment 8822358 [details]
Bug 1318542 - part4: Disable tests.

https://reviewboard.mozilla.org/r/101300/#review105588
Attachment #8822358 - Flags: review?(alwu) → review+
Comment on attachment 8818222 [details]
Bug 1318542 - Make CueStyleBox apply ::cue.

https://reviewboard.mozilla.org/r/98366/#review105750

The change looks fine to me. However, please figure out the pgo test result a difference before landing.

::: dom/media/webvtt/vtt.jsm:761
(Diff revision 5)
>        font: styleOptions.font,
>        whiteSpace: "pre-line",
>        position: "absolute"
>      };
> +    if (isFirefox) {
> +      this.div = window.document.createElement("div", {pseudo: "::cue"});

In the linux64 pgo build, there's an unexpected pass on `processing/model/evil/single_quote.html` which is worrying.
Attachment #8818222 - Flags: review?(giles) → review+
Blake - can you prioritise this?
Flags: needinfo?(bwu)
I expect this could be done in Q1.
Benjamin, 
Let me know if it is not possible. 
Thanks!
Flags: needinfo?(bwu)
Priority: -- → P2
Blocks: 1321488
Benjamin, I just updated the patch in bug 1330843 after Bobby's pseudo-element-related work.  But I took out the actual definition of ::cue in nsCSSPseudoElementList.h.  So can you please adjust your part 3 here (I guess should be part 1 now) to define ::cue using the new CSS_PSEUDO_ELEMENT_IS_JS_CREATED_NAC flag https://reviewboard-hg.mozilla.org/gecko/file/2244d94377ba/layout/style/nsCSSPseudoElements.h#l39 and also just check that the bug 1330843 patch still works for you?
Flags: needinfo?(bechen)
(In reply to Cameron McCormack (:heycam) (away 25 Feb–5 Mar) from comment #37)
> Benjamin, I just updated the patch in bug 1330843 after Bobby's
> pseudo-element-related work.  But I took out the actual definition of ::cue
> in nsCSSPseudoElementList.h.  So can you please adjust your part 3 here (I
> guess should be part 1 now) to define ::cue using the new
> CSS_PSEUDO_ELEMENT_IS_JS_CREATED_NAC flag
> https://reviewboard-hg.mozilla.org/gecko/file/2244d94377ba/layout/style/
> nsCSSPseudoElements.h#l39 and also just check that the bug 1330843 patch
> still works for you?

Thanks, it works.
Flags: needinfo?(bechen)
Attachment #8822358 - Attachment is obsolete: true
Comment on attachment 8818222 [details]
Bug 1318542 - Make CueStyleBox apply ::cue.

https://reviewboard.mozilla.org/r/98366/#review118614

::: dom/media/webvtt/vtt.jsm:491
(Diff revision 7)
>      this.cueDiv = parseContent(window, cue.text, false);
>      var styles = {
>        color: color,
>        backgroundColor: backgroundColor,
> -      position: "relative",
> -      left: 0,
> +      display: "inline",
> +      font: "5vh sans-serif",

spec define font size to 5vh. And since the "this.cueDiv" act as cueStyleBox ::cue and "this.div" will move the "cueDiv" to the right position, we can remove the "position, left ..." attributes on cueDiv.
Comment on attachment 8818222 [details]
Bug 1318542 - Make CueStyleBox apply ::cue.

https://reviewboard.mozilla.org/r/98366/#review119142

The changes to the files under layout/style/ look good to me, but two things:

1. Should we be putting ::cue support behind a pref?  Probably we should.  Unfortunately we don't have a simple way to put a pseudo-element behind a pref.  If you want to add a general pseudo-element pref mechanism, you could make it work like nsCSSPseudoClasses::sPseudoClassEnabled.

2. If you haven't already you should probably send out an Intent to Implement mail.

::: layout/style/res/html.css:784
(Diff revision 7)
> +  font-size: 5vh;
> +  font-family: sans-serif;

You should use "font: 5vh sans-serif" instead of the separate font-size and font-family declarations, otherwise we won't set the other font related properties back to their initial values.
Attachment #8818222 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #44)
> Comment on attachment 8818222 [details]
> Bug 1318542 - part3: Make CueStyleBox apply ::cue.
> 
> https://reviewboard.mozilla.org/r/98366/#review119142
> 
> The changes to the files under layout/style/ look good to me, but two things:
> 
> 1. Should we be putting ::cue support behind a pref?  Probably we should. 
> Unfortunately we don't have a simple way to put a pseudo-element behind a
> pref.  If you want to add a general pseudo-element pref mechanism, you could
> make it work like nsCSSPseudoClasses::sPseudoClassEnabled.
> 
> 2. If you haven't already you should probably send out an Intent to
> Implement mail.
> 

I don't think we need a pref to control the ::cue feature.
Because I feel the ::cue :past :future ::cue-region won't impact other components a lot?
It's really if (a) we think the spec isn't stable enough, and so this is really an experimental feature (and we want the flexibility to change its behaviour as the spec stabilises), or (b) there is some risk of breaking existing content by shipping the feature (and we need a way to turn it off easily).
Comment on attachment 8818222 [details]
Bug 1318542 - Make CueStyleBox apply ::cue.

https://reviewboard.mozilla.org/r/98366/#review119854

::: layout/style/res/html.css:784
(Diff revision 7)
> +/* ::cue default settings */
> +::cue {
> +  color: rgba(255, 255, 255, 1);
> +  white-space: pre-line;
> +  background-color: rgba(0, 0, 0, 0.8);
> +  font-size: 5vh;

The spec says the viewport size for the vh is the video element's size, however during my local tests, the viewport size now is full screen?
Hmm, making vw/vh resolve against the video element's size sounds like it would be quite hard. :(  Do other browsers correctly implement this?
(In reply to Cameron McCormack (:heycam) from comment #48)
> Hmm, making vw/vh resolve against the video element's size sounds like it
> would be quite hard. :(  Do other browsers correctly implement this?

That's why the original code calculate the font size into px in jsm.vtt(I use css variable do the same thing).
Maybe I can use percent on font size, because the ::cue's style parent is video element.
(In reply to Cameron McCormack (:heycam) from comment #48)
> Hmm, making vw/vh resolve against the video element's size sounds like it
> would be quite hard. :(  Do other browsers correctly implement this?

If we can not set the viewport size to video element, that means our ::cue can not support vh.
Chrome browser doesn't support video element as viewport.
(In reply to Benjamin Chen [:bechen] from comment #50)
> If we can not set the viewport size to video element, that means our ::cue
> can not support vh.
> Chrome browser doesn't support video element as viewport.

OK.  I think leaving vw/vh resolve against the window size makes sense.  So if we can calculate the 5vh size to px in webvtt.jsm and pass it in using the CSS variable then let's do that.
Comment on attachment 8846494 [details]
Bug 1318542 - Disable testcase 3_track.html, see bug1342063, the testcase un-expected-PASS on OSX 10.10.

https://reviewboard.mozilla.org/r/119538/#review121492

r+ with adding the reason why we need to disable this test.
Attachment #8846494 - Flags: review?(alwu) → review+
Comment on attachment 8846495 [details]
Bug 1318542 - Add preference "media.webvtt.pseudo.enabled".

https://reviewboard.mozilla.org/r/119540/#review121688

r=me with the issue addressed.

::: dom/media/webvtt/vtt.jsm:474
(Diff revision 1)
>    function CueStyleBox(window, cue, styleOptions) {
>      var isIE8 = (typeof navigator !== "undefined") &&
>        (/MSIE\s8\.0/).test(navigator.userAgent);
>  
> -    var isFirefox = (/firefox/i.test(window.navigator.userAgent));
> +    var isFirefoxSupportPseudo = (/firefox/i.test(window.navigator.userAgent))
> +          && Services.prefs.getBoolPref("media.webvtt.pseudo.enabled");

`getBoolPref` is an expensive operation with e10s, and CueStyleBox is called for every cue. Can you capture the value from an outer scope to avoid looking this up every time?

There's `XPComUtils.defineLazyPreferenceGetter` for constructing a caching method for these things, but I'm not sure where to call it, since you don't have a local state object here. Maybe adding it to /Cue/CueStyleBox.prototype would work?
Attachment #8846495 - Flags: review?(giles) → review+
Comment on attachment 8846495 [details]
Bug 1318542 - Add preference "media.webvtt.pseudo.enabled".

https://reviewboard.mozilla.org/r/119540/#review121688

> `getBoolPref` is an expensive operation with e10s, and CueStyleBox is called for every cue. Can you capture the value from an outer scope to avoid looking this up every time?
> 
> There's `XPComUtils.defineLazyPreferenceGetter` for constructing a caching method for these things, but I'm not sure where to call it, since you don't have a local state object here. Maybe adding it to /Cue/CueStyleBox.prototype would work?

XPCOMUtils.defineLazyPreferenceGetter(StyleBox.prototype, "supportPseudo", ...) works for me.
Comment on attachment 8818222 [details]
Bug 1318542 - Make CueStyleBox apply ::cue.

https://reviewboard.mozilla.org/r/98366/#review122928

::: dom/media/webvtt/vtt.jsm:964
(Diff revision 8)
>        for (var i = 0; i < cues.length; i++) {
>          cue = cues[i];
>  
>          // Compute the intial position and styles of the cue div.
>          styleBox = new CueStyleBox(window, cue, styleOptions);
> +        styleBox.cueDiv.style.setProperty("--cue-font-size", fontSize + "px ");

Nit: don't need the space after the "px".
Attachment #8818222 - Flags: review?(cam) → review+
Latest codebase:
Thread 1 "firefox" received signal SIGSEGV, Segmentation fault.
0x00007fffe90ad1eb in nsContentUtils::GetClosestNonNativeAnonymousAncestor (aElement=<optimized out>)
    at /home/benjamin/hg/mozilla-central/dom/base/nsContentUtils.cpp:9983
9983	  MOZ_ASSERT(aElement->IsNativeAnonymous());
(In reply to Benjamin Chen [:bechen] from comment #59)
> Latest codebase:
> Thread 1 "firefox" received signal SIGSEGV, Segmentation fault.
> 0x00007fffe90ad1eb in nsContentUtils::GetClosestNonNativeAnonymousAncestor
> (aElement=<optimized out>)
>     at /home/benjamin/hg/mozilla-central/dom/base/nsContentUtils.cpp:9983
> 9983	  MOZ_ASSERT(aElement->IsNativeAnonymous());

http://searchfox.org/mozilla-central/source/dom/base/nsINode.h#136

The ::cue doesn't have the bit. Maybe we should change the assert into IsInNativeAnonymousSubtree.
Ah, this is because the <div> implementing the ::cue doesn't have that bit set.  Which is expected, since we created it in JS. :-)  I think it would be fine to change the assertion to check IsInNativeAnonymousSubtree.  (The IsNativeAnonymous check was really just documenting what I thought the situation was.)

If you change this, please also update the comment in nsContentUtils.h to state that the element must be in a NAC subtree.
After address comment 61, assertion solved.
However, the style sheet in document failed to be applied to ::cue. Seems that the pseudo element skip the UserStyle again in nsStyleSet::FileRules.
Not sure what's the different between the patch version 4(last time I test the patch, https://treeherder.mozilla.org/#/jobs?repo=try&revision=653ec1bf40d0) and version 7 of Bug 1330843.
Flags: needinfo?(cam)
Bobby, it looks like my final patch in bug 1330843 didn't quite work for ::cue here.  Here's a summary of the problem:

1. an nsVideoFrame creates a NAC child in its CreateAnonymousContent function, its mCaptionDiv
2. webvtt.jsm uses JS to append some content to the mCaptionDiv; this content IsInNativeAnonymousSubtree(), but not IsNativeAnonymous()
3. one of the JS-created descendants of the mCaptionDiv uses the {pseudo:...} arg to createElement to mark it as a pseudo-element ("the ::cue div")
4. it's the ::cue div's great-grandparent that is the mCaptionDiv
5. the call to GetClosestNonNativeAnonymousAncestor identifies its immediate parent as the originating element, when really it needs to be mCaptionDiv's parent (the video element)

As far as I can tell, there is no need for the ::cue div to inherit its styles from its immediate parent, so we can at least maintain the invariant that the originating element and the inheriting element are the same.

But can we come up with a rule that would allow the video element to be used as the originating element here?  The problem really is just that we happen to use JS to create this anonymous content, and so it really isn't NAC, but it's fulfilling the same role as NAC.  (I guess this confusion between NAC and non-native AC inside a NAC subtree also occurs if we use C++ to append content to actual NAC, since any appended-later content won't be marked as NAC.)  What's the reason we shouldn't just use IsInNativeAnonymousSubtree() rather than IsNativeAnonymous() here?
Flags: needinfo?(cam) → needinfo?(bobbyholley)
(In reply to Cameron McCormack (:heycam) from comment #63)
> What's the reason we shouldn't just use IsInNativeAnonymousSubtree()
> rather than IsNativeAnonymous() here?

Because, in general, XBL-generated content expects normal style semantics, not our weird nac semantics. The <video> control is the whole reason we _can't_ use IsInNativeAnonymousSubtree() for this stuff.

If ::cue is supposed to be a pseudo-element on <video>, but the NAC is supposed to be arbitrarily nested inside of XBL-generated <video> content, I can't think of a sane way to get those semantics. But I'm also about to go to bed...
Flags: needinfo?(bobbyholley)
> but the NAC is supposed to be arbitrarily nested inside of XBL-generated <video> content

That's not what I got from comment 63.

What I got is that this is the "create NAC, then append a kid to it" scenario.  Similar to the editor thing.  The way it works is that the video frame creates the NAC root, then hands it out via GetCaptionOverlay.  TextTrackManager::UpdateCueDisplay then passes it to ProcessCues which is implemented in JS and just adds kids to it.

We should flag everything involved as IsNativeAnonymous(), because the fact that it's created in JS and then appended, rather than directly in CreateAnonymousContent, is totally an implementation detail.

The question is how best to do that.
I had the same thought this morning that the simplest thing would be to just flag them as NAC.

Would it be acceptable to mark the ::cue div (and its other JS-created ancestors under the actual NAC mCaptionDiv) as NAC, but leave all the other descendants as non-native?  The descendants change as the video progresses, and there's no need (from the perspective of styling the ::cue pseudo) to mark them as NAC.

So how about we make nsIWebVTTParserWrapper.processCues return the ::cue div (just so we don't have to go searching for it), and then TextTrackManager::UpdateCueDisplay can call SetFlags(NODE_IS_NATIVE_ANONYMOUS) on it and its ancestors up to |overlay| (the mCaptionDiv), if it doesn't already have that flag set.
(In reply to Cameron McCormack (:heycam) from comment #66)
> I had the same thought this morning that the simplest thing would be to just
> flag them as NAC.
> 
> Would it be acceptable to mark the ::cue div (and its other JS-created
> ancestors under the actual NAC mCaptionDiv) as NAC, but leave all the other
> descendants as non-native?  The descendants change as the video progresses,
> and there's no need (from the perspective of styling the ::cue pseudo) to
> mark them as NAC.
> 
> So how about we make nsIWebVTTParserWrapper.processCues return the ::cue div
> (just so we don't have to go searching for it), and then
> TextTrackManager::UpdateCueDisplay can call
> SetFlags(NODE_IS_NATIVE_ANONYMOUS) on it and its ancestors up to |overlay|
> (the mCaptionDiv), if it doesn't already have that flag set.

SetFlags seems not working.

The NAC root |Overlay| has both NODE_IS_NATIVE_ANONYMOUS and NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE.
So after js append other nodes to it, the descendants of Overlay all have NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE.
Then I add NODE_IS_NATIVE_ANONYMOUS on them, doesn't work. Remove NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE doesn't work either.

/*pseudo code*/
sParserWrapper->ProcessCues(window, jsCues, overlay, controls);
paddedOverlay = overlay->GetFirstChild();
paddedOverlay->SetFlags(NODE_IS_NATIVE_ANONYMOUS);
paddedOverlay->UnsetFlags(NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE);

for (paddedOverlay->GetChildArray ...) {
  // styleBox.div
  children[i]->SetFlags(NODE_IS_NATIVE_ANONYMOUS);
  children[i]->UnsetFlags(NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE);
  // styleBox.cueDiv, ::cue
  children[i]->GetFirstChild()->SetFlags(NODE_IS_NATIVE_ANONYMOUS);
  children[i]->GetFirstChild()->UnsetFlags(NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE);
}
We should leave NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE on there as well.

When you set the NODE_IS_NATIVE_ANONYMOUS flag on the ::cue div and the other elements up to |overlay|, and we later get to here:

http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/layout/base/nsCSSFrameConstructor.cpp#5100-5101

does GetClosestNonNativeAnonymousAncestor return the video element, and if not, why not?
Flags: needinfo?(bechen)
(In reply to Cameron McCormack (:heycam) from comment #68)
> We should leave NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE on there as well.
> 
> When you set the NODE_IS_NATIVE_ANONYMOUS flag on the ::cue div and the
> other elements up to |overlay|, and we later get to here:
> 
> http://searchfox.org/mozilla-central/rev/
> 9af9b1c7946ebef6056d2ee4c368d496395cf1c8/layout/base/nsCSSFrameConstructor.
> cpp#5100-5101
> 
> does GetClosestNonNativeAnonymousAncestor return the video element, and if
> not, why not?

When I set the flag NODE_IS_NATIVE_ANONYMOUS after |ProcessCues|, the ::cue had been bind to the dom tree. The GetClosestNonNativeAnonymousAncestor already called(not get the video element).
Flags: needinfo?(bechen)
OK.  In that case, maybe we can do this during BindToTree, and leverage the fact that we have recorded on the cue box div that it is for a ::cue, and we know that ::cue is CSS_PSEUDO_ELEMENT_IS_JS_CREATED_NAC.

So that added code would do something like:

  if (aDocument) {
    if (GetPseudoElementType() isn't None, and PseudoElementIsJSCreatedNAC(type) is true) {
      assert aParent->IsInNativeAnonymousSubtree()
      for (n = aParent; !n->IsNativeAnonymous(); n = n->GetParent()) {
        n->SetFlags(NODE_IS_NATIVE_ANONYMOUS)
      }
    }
  }

Doing this is probably nicer than just adding a ChromeOnly function that JS can call to set the flags before appending the elements, since it ties it to the CSS_PSEUDO_ELEMENT_IS_JS_CREATED_NAC mechanism.

It's probably not worth doing anything in UnbindFromTree; we never clear the NODE_IS_NATIVE_ANONYMOUS flag for C++-created NAC if it happens to get removed from the document.
How fast is that new check we're proposing adding?  Because BindToTree performance is rather sensitive.  And yes, I know it's already terrible. :(

Also, that setup would violate the invariants bholley was trying to have for the NAC flags, fwiw.  :(
> How fast is that new check we're proposing adding?  Because BindToTree performance is rather sensitive.  And yes, I know it's already terrible. :(

Well, for the common case of an element not annotated as being for a pseudo-element, the check is a lookup of the element's property table.  So that's a bunch of pointer chasing, and searching the proprety table's list...

> Also, that setup would violate the invariants bholley was trying to have for the NAC flags, fwiw.  :(

Which part specifically?
The part where the parent of a NODE_IS_NATIVE_ANONYMOUS node that is not a NAC root is itself NODE_IS_NATIVE_ANONYMOUS, so you don't get weirdness as follows: A is child of B is child of C is child of D, B inherits style from C, but A inherits style from D.
> Also, that setup would violate the invariants bholley was trying to have for the NAC flags, fwiw.  :(

I misread the proposed change.  Since it sets the flag up the tree, it's fine wrt this invariant.
Comment on attachment 8850866 [details]
Bug 1318542 - Label NODE_IS_NATIVE_ANONYMOUS on the elements from ::cue up to nsVideoFrame.

https://reviewboard.mozilla.org/r/123382/#review125784

::: dom/base/Element.cpp:1703
(Diff revision 1)
> +  // Label NODE_IS_NATIVE_ANONYMOUS at elements
> +  // between nsVideoFrame to ::cue.

I think we should make this comment more generic, since it operates on any pseudo-element implemented with this JS-created-NAC flag, and explain why we need to do it.  So how about something like:

// Pseudo-elements implemented by JS must have the NODE_IS_NATIVE_ANONYMOUS
// flag set on them.  For C++-created pseudo-elements, this is done in
// nsCSSFrameConstructor::GetAnonymousContent, but any JS that creates
// pseudos would run after that.  So we set that flag here, when the element
// implementing the pseudo is inserted into the document.  We maintain the
// invariant that any NAC-implemented pseudo-element's anonymous ancestors are
// also flagged as NAC, which the style system relies on.

::: dom/base/Element.cpp:1711
(Diff revision 1)
> +      nsIContent* i = aParent;
> +      while (i && !i->IsNativeAnonymous()) {
> +        i->SetFlags(NODE_IS_NATIVE_ANONYMOUS);
> +        i = i->GetParent();
> +      }

Nit: maybe "parent" instead of "i".

::: dom/base/Element.cpp:1717
(Diff revision 1)
> +      while (i && !i->IsNativeAnonymous()) {
> +        i->SetFlags(NODE_IS_NATIVE_ANONYMOUS);
> +        i = i->GetParent();
> +      }
> +    }
> +  }

Nit: put a blank line after this one.
Attachment #8850866 - Flags: review?(cam) → review+
Though since this is purely in dom/ I might like to get bz to sign off on it.
Flags: needinfo?(bzbarsky)
(In reply to Cameron McCormack (:heycam) from comment #70)
> 
> It's probably not worth doing anything in UnbindFromTree; we never clear the
> NODE_IS_NATIVE_ANONYMOUS flag for C++-created NAC if it happens to get
> removed from the document.

After my local test, we still need to remove the NODE_IS_NATIVE_ANONYMOUS flag during UnbindFromTree.
Because the vtt.jsm now will do something break the NODE_IS_NATIVE_ANONYMOUS path.

ex: nsVideoFrame--> mCaption--> paddedOverlay--> movingBox--> ::cue
One of the behaviour of the vtt.jsm will recreate paddedOverlay and append the "old movingBox which has NODE_IS_NATIVE_ANONYMOUS" to paddedOverlay. That makes the NODE_IS_NATIVE_ANONYMOUS path broken.
Maybe it is a flaw in vtt.jsm, I can change/fix the behaviour later, but I think we still need to remove NODE_IS_NATIVE_ANONYMOUS when UnbindFromTree.
Is this http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/dom/media/webvtt/vtt.jsm#933-939 or something else?  So ::cue notices that movingBox is NAC, and stops there?  In that case, I suggest just changing the loop so that it stops when it finds IsRootOfNativeAnonymousSubtree() instead of IsNativeAnonymous().
(In reply to Cameron McCormack (:heycam) from comment #82)
> Is this
> http://searchfox.org/mozilla-central/rev/
> c48398abd9f0f074c69f2223260939e30e8f99a8/dom/media/webvtt/vtt.jsm#933-939 
Yes
> or something else?  So ::cue notices that movingBox is NAC, and stops there? 
> In that case, I suggest just changing the loop so that it stops when it
> finds IsRootOfNativeAnonymousSubtree() instead of IsNativeAnonymous().

I'll try it.
Comment on attachment 8850866 [details]
Bug 1318542 - Label NODE_IS_NATIVE_ANONYMOUS on the elements from ::cue up to nsVideoFrame.

https://reviewboard.mozilla.org/r/123382/#review125936

::: dom/base/Element.cpp:1712
(Diff revision 1)
> +    if (pseudoType != CSSPseudoElementType::NotPseudo &&
> +        nsCSSPseudoElements::PseudoElementIsJSCreatedNAC(pseudoType)) {
> +      MOZ_ASSERT(aParent->IsInNativeAnonymousSubtree());
> +      SetFlags(NODE_IS_NATIVE_ANONYMOUS);
> +      nsIContent* i = aParent;
> +      while (i && !i->IsNativeAnonymous()) {

This is wrong.  It will set the flag all the way up to the root of the document in the normal case, no?
Attachment #8850866 - Flags: review-
Flags: needinfo?(bzbarsky)
If we are in a native anonymous subtree (per the assertion above), we will have to hit a native anonymous node at some point before hitting the root of the document, won't we?  (Same goes for my suggested change to check for IsNativeAnonymousSubtreeRoot.)
> we will have to hit a native anonymous node at some point before hitting the root of the document, won't we? 

That's not obvious to me, offhand...  For example, document-level native anon content is not flagged IsNativeAnonymous(), iirc.

I guess the common case of video would in fact have an IsNativeAnonymous() node in there, though.  I hope.

If we were checking for IsNativeAnonymousSubtreeRoot(), then I would agree that we would stop before going too far.  At least as long as that IsInNativeAnonymousSubtree() assert holds.  ;)
Comment on attachment 8850866 [details]
Bug 1318542 - Label NODE_IS_NATIVE_ANONYMOUS on the elements from ::cue up to nsVideoFrame.

https://reviewboard.mozilla.org/r/123382/#review126196

::: dom/base/Element.cpp:1703
(Diff revision 2)
> +  // Pseudo-elements implemented by JS must have the
> +  // NODE_IS_NATIVE_ANONYMOUS flag set on them.
> +  // For C++-created pseudo-elements, this is done in
> +  // nsCSSFrameConstructor::GetAnonymousContent, but any JS
> +  // that creates pseudo-elements would run after that.
> +  // So we set that flag here, when the element
> +  // implementing the pseudo is inserted into the document.
> +  // We maintain the invariant that any NAC-implemented pseudo-element's
> +  // anonymous ancestors are also flagged as NAC, which the style
> +  // system relies on.

Nit: this paragraph could probably use more of the 80 columns available to it, and wrap to fewer lines. :-)

::: dom/base/Element.cpp:1719
(Diff revision 2)
> +    CSSPseudoElementType pseudoType = GetPseudoElementType();
> +    if (pseudoType != CSSPseudoElementType::NotPseudo &&
> +        nsCSSPseudoElements::PseudoElementIsJSCreatedNAC(pseudoType)) {
> +      SetFlags(NODE_IS_NATIVE_ANONYMOUS);
> +      nsIContent* parent = aParent;
> +      while (parent && !parent->IsRootOfAnonymousSubtree()) {

This should be "IsRootOfNativeAnonymousSubtree".

::: dom/base/Element.cpp:1723
(Diff revision 2)
> +      nsIContent* parent = aParent;
> +      while (parent && !parent->IsRootOfAnonymousSubtree()) {
> +        MOZ_ASSERT(parent->IsInNativeAnonymousSubtree());
> +        parent->SetFlags(NODE_IS_NATIVE_ANONYMOUS);
> +        parent = parent->GetParent();
> +      }

After the while loop, can you assert that parent is non-null (since we should always find an anonymous subtree root)?
Comment on attachment 8850866 [details]
Bug 1318542 - Label NODE_IS_NATIVE_ANONYMOUS on the elements from ::cue up to nsVideoFrame.

https://reviewboard.mozilla.org/r/123382/#review126204

Looks good.

::: dom/base/Element.cpp:1703
(Diff revision 3)
>                               aCompileEventHandlers);
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
>    }
>  
> +  // Pseudo-elements implemented by JS must have theNODE_IS_NATIVE_ANONYMOUS

Nit: space after "the".
Boris, can you take one more look?
Flags: needinfo?(bzbarsky)
Comment on attachment 8850866 [details]
Bug 1318542 - Label NODE_IS_NATIVE_ANONYMOUS on the elements from ::cue up to nsVideoFrame.

https://reviewboard.mozilla.org/r/123382/#review126312

r=me
Attachment #8850866 - Flags: review+
Flags: needinfo?(bzbarsky)
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57efe450b180
Make CueStyleBox apply ::cue. r=heycam,rillian
https://hg.mozilla.org/integration/autoland/rev/8ca158aca790
Disable testcase 3_track.html, see bug1342063, the testcase un-expected-PASS on OSX 10.10. r=alwu
https://hg.mozilla.org/integration/autoland/rev/14e0332a1b84
Add preference "media.webvtt.pseudo.enabled". r=rillian
https://hg.mozilla.org/integration/autoland/rev/c92af084fbfa
Label NODE_IS_NATIVE_ANONYMOUS on the elements from ::cue up to nsVideoFrame. r=bz,heycam
Keywords: checkin-needed
(In reply to Cameron McCormack (:heycam) from comment #44)
> 2. If you haven't already you should probably send out an Intent to
> Implement mail.

Please don't forget to send out the Intent to Implement mail.  Ideally that would have been sent before we ship the feature, but it looks like the patch that added the pref has the pref set to true.  So that would make it an Intent to Implement and Ship mail.  Thanks!
Flags: needinfo?(bechen)
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec33d5d99da8
Backed out 4 changesets for xpcshell and mochitest failures
Comment on attachment 8853285 [details]
Bug 1318542 - fix testcases due to new pseudo element ::cue.

https://reviewboard.mozilla.org/r/125368/#review127912

The diff for devtools/shared/css/generated/properties-db.js are generated by ./mach devtools-css-db, not sure I should upload it?
Comment on attachment 8853285 [details]
Bug 1318542 - fix testcases due to new pseudo element ::cue.

https://reviewboard.mozilla.org/r/125368/#review127914
Attachment #8853285 - Flags: review?(cam) → review+
I just tried regenerating that file locally and it also included that grayscale change, so that's fine.
Flags: needinfo?(bechen)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2547dbaf2d6
Make CueStyleBox apply ::cue. r=heycam,rillian
https://hg.mozilla.org/integration/autoland/rev/177fd4ad1b10
Disable testcase 3_track.html, see bug1342063, the testcase un-expected-PASS on OSX 10.10. r=alwu
https://hg.mozilla.org/integration/autoland/rev/30f9283c7a33
Add preference "media.webvtt.pseudo.enabled". r=rillian
https://hg.mozilla.org/integration/autoland/rev/d8fc3ce28d3e
Label NODE_IS_NATIVE_ANONYMOUS on the elements from ::cue up to nsVideoFrame. r=bz,heycam
https://hg.mozilla.org/integration/autoland/rev/6c828cc61fa1
fix testcases due to new pseudo element ::cue. r=heycam
Keywords: checkin-needed
Blocks: 1353689
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: