Closed Bug 1330843 Opened 7 years ago Closed 7 years ago

Allow JS to create NAC pseudo-elements.

Categories

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

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

Tracking

()

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

People

(Reporter: bechen, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

Bug 1330843 - Allow JS to create NAC pseudo-elements.
59 bytes, text/x-review-board-request
bholley
: review+
Details
reduced display-contents-xbl.xhtml
1.22 KB, application/xhtml+xml
Details
stack
7.74 KB, text/plain
Details
Fwiw, manually recreating the frames after 2seconds works (L1 is blue)
1.46 KB, text/html
Details 
Fork from bug 1318542 comment 22.
Attachment #8826436 - Attachment is obsolete: true
Attachment #8826437 - Attachment is obsolete: true
Blocks: 1331045
Comment on attachment 8826448 [details]
Bug 1330843 - Allow JS to create NAC pseudo-elements.

https://reviewboard.mozilla.org/r/104406/#review105402

::: layout/base/nsCSSFrameConstructor.cpp:5062
(Diff revision 3)
> +      MOZ_ASSERT(aContent->GetParentElement() &&
> +                 aContent->GetParentElement()->IsInNativeAnonymousSubtree(),
> +                 "only use nsGkAtoms::pseudoProperty for elements in native "
> +                 "anonymous subtrees");
> +      Element* parentElement = aContent->AsElement();
> +      for (;;) {
> +        bool isRoot = parentElement->IsRootOfNativeAnonymousSubtree();
> +        parentElement = parentElement->GetParentElement();
> +        MOZ_ASSERT(parentElement);
> +        if (isRoot) {
> +          break;
> +        }
> +      }

See the discussion in bug 1331047 comment 3 on whether we should inherit from the parent or from the host.
I'm adding more usage of this stuff, so I wanted to contain the casting. Feel
free to roll this into part 1 if that's easier.
Attachment #8826793 - Flags: review?(cam)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> Comment on attachment 8826448 [details]
> Bug 1330843 - Part 1: Allow JS to create NAC pseudo-elements.
> 
> https://reviewboard.mozilla.org/r/104406/#review105402
> 
> ::: layout/base/nsCSSFrameConstructor.cpp:5062
> (Diff revision 3)
> > +      MOZ_ASSERT(aContent->GetParentElement() &&
> > +                 aContent->GetParentElement()->IsInNativeAnonymousSubtree(),
> > +                 "only use nsGkAtoms::pseudoProperty for elements in native "
> > +                 "anonymous subtrees");
> > +      Element* parentElement = aContent->AsElement();
> > +      for (;;) {
> > +        bool isRoot = parentElement->IsRootOfNativeAnonymousSubtree();
> > +        parentElement = parentElement->GetParentElement();
> > +        MOZ_ASSERT(parentElement);
> > +        if (isRoot) {
> > +          break;
> > +        }
> > +      }
> 
> See the discussion in bug 1331047 comment 3 on whether we should inherit
> from the parent or from the host.

And unfortunately, in my local testing, this "walk-us-out-of-the-nac-subtree" approach doesn't work when frames are being constructed for the first time. nsCSSFrameConstructor::ConstructFrameFromItemInternal calls ProcessChildren() _before_ invoking SetPrimaryFrame() on the content. So while the frame has been constructed (it's an argument to ProcessChildren()), it isn't yet reachable from the element.

So, _if_ we decide in bug 1331047 that we want to do the walking-out-of-the-nac-subtree thing, then we'll need some other implementation strategy here.
Depends on: 1331322
No longer blocks: 1331045
Comment on attachment 8826793 [details] [diff] [review]
Use an accessor on Element for getting/setting the pseudo. v1

Most of this patch already landed; I'll rebase these patches on what's there now.
Attachment #8826793 - Flags: review?(cam)
Attachment #8826449 - Attachment is obsolete: true
Attachment #8826793 - Attachment is obsolete: true
Comment on attachment 8826448 [details]
Bug 1330843 - Allow JS to create NAC pseudo-elements.

https://reviewboard.mozilla.org/r/104406/#review117188

::: layout/base/nsCSSFrameConstructor.cpp:5059
(Diff revision 4)
> +      if (!aOriginatingElementOrNull) {
> +        // For pseudo-implementing NAC created by JS using the ChromeOnly
> +        // document.createElement(..., { pseudo: ... }) API, we find the
> +        // originating element by lookup the tree until we reach out of the NAC
> +        // subtree.
> +        MOZ_ASSERT(nsCSSPseudoElements::PseudoElementIsJSCreatedNAC(pseudoType));
> +        aOriginatingElementOrNull =
> +          nsContentUtils::GetParentOfNativeAnonymousSubtree(aContent->AsElement());
> +      }
> +      MOZ_ASSERT(aOriginatingElementOrNull);

Hm, this seems wrong. The whole idea was that we use the first non-NAC ancestor (which is different than the root of the NAC subtree) as both the style parent and the originating element for pseudos (and any other NAC).

::: layout/style/nsCSSPseudoElements.h:105
(Diff revision 4)
> +  static bool PseudoElementIsJSCreatedNAC(Type aType)
> +  {
> +    return PseudoElementHasFlags(aType, CSS_PSEUDO_ELEMENT_IS_JS_CREATED_NAC);
> +  }
> +

Why do we need this accessor? Shouldn't we just be able to treat all NAC-backed pseudos the same?

::: layout/style/nsComputedDOMStyle.cpp:866
(Diff revision 4)
> +        mContent->AsElement()->GetPseudoElementType() ==
> +          CSSPseudoElementType::NotPseudo) {

Why do we need this?
Attachment #8826448 - Flags: review?(bobbyholley) → review-
Comment on attachment 8826448 [details]
Bug 1330843 - Allow JS to create NAC pseudo-elements.

https://reviewboard.mozilla.org/r/104406/#review117188

> Why do we need this accessor? Shouldn't we just be able to treat all NAC-backed pseudos the same?

One thing is that I want to restrict the document.createElement(..., { pseudo: ... }) API to work only with the pseudos that we decide.  For the ElementForStyleContext change in GeckoRestyleManager.cpp, yes I think we can replace the PseudoElementIsJSCreatedNAC() call with frameElement->IsNativeAnonymous() here.  (Perhaps instead of the assertion in nsCSSFrameConstructor::ResolveStyleContext, we should we be relying less on the aOriginatingElementOrNull argument and instead looking up to find the originating element, but if so I'd rather defer making that change.)

> Why do we need this?

We hit the assertion in here otherwise, if we call getComputedStyle(aJSImplementedNACElement), as opposed to passing in getComputedStyle(originatingElement, "::blah").  Content script can't do that, but for ::cue, webvtt.jsm could in theory.  Maybe the condition would be better written as `!mContent->IsNativeAnonymous()` instead?
(In reply to Cameron McCormack (:heycam) from comment #15)
> Comment on attachment 8826448 [details]
> Bug 1330843 - Allow JS to create NAC pseudo-elements.
> 
> https://reviewboard.mozilla.org/r/104406/#review117188
> 
> > Why do we need this accessor? Shouldn't we just be able to treat all NAC-backed pseudos the same?
> 
> One thing is that I want to restrict the document.createElement(..., {
> pseudo: ... }) API to work only with the pseudos that we decide.  For the
> ElementForStyleContext change in GeckoRestyleManager.cpp, yes I think we can
> replace the PseudoElementIsJSCreatedNAC() call with
> frameElement->IsNativeAnonymous() here.  (Perhaps instead of the assertion
> in nsCSSFrameConstructor::ResolveStyleContext, we should we be relying less
> on the aOriginatingElementOrNull argument and instead looking up to find the
> originating element, but if so I'd rather defer making that change.)

I see. Yeah I'm fine with using this accessor on the WebIDL entry point, so long as we don't use it in the actual layout code.

> 
> > Why do we need this?
> 
> We hit the assertion in here otherwise, if we call
> getComputedStyle(aJSImplementedNACElement), as opposed to passing in
> getComputedStyle(originatingElement, "::blah").  Content script can't do
> that, but for ::cue, webvtt.jsm could in theory.  Maybe the condition would
> be better written as `!mContent->IsNativeAnonymous()` instead?

That seems reasonable, I think. Please document it copiously in the comments.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16)
> I see. Yeah I'm fine with using this accessor on the WebIDL entry point, so
> long as we don't use it in the actual layout code.

Using IsNativeAnonymous() for the check in ElementForStyleContext caused a couple of test failures (display:contents and ::before/::after related), which I'll look into:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=485fac60d16b0a481b1fa4faf686995fb9827838
Hi Mats, I'm trying to debug an issue with my patches here.  One of the test failures I'm getting is layout/reftests/css-display/display-contents-xbl.xhtml.  I'm unsure what the correct behaviour is, and in reducing that file down to the relevant sub-test, and running it without my patches, I'm still unsure. :-)  See the attached display-contents-xbl.xhtml.

When I run that as a reftest, the L1 text is bold and red.  The original reftest (which dynamically adds the -moz-binding to #hostL) expects it to be bold and blue.  My guess is that blue is what we want, since the <b> should inherit from the <xhtml:span class="a">.  Is that right?  If so, I guess we have some issues picking the correct parent to inherit from when restyling the display:contents <b>.
Flags: needinfo?(mats)
Attached file stack — 
Right, L1 should be blue.

I debugged this a bit and it seems the first <xhtml:span class="a">
element is resolving its style wrong.

The stack here shows the frame construction for the <div> when its
-moz-binding load event fires.  For the <xhtml:span> we call
styleSet->ResolveStyleFor with an aParentStyleContext that has
blue color, but the resulting style context still says the color
is red, although its display correctly is 'contents'.  I don't
understand how that's possible.

(I suspect the original reftest wallpapers this bug because some later
element causes a WipeContainingBlock or something so that <body>
recreates its frames.  If I wrap each of the "host" <div>s in a new
<div> then L1 becomes red here too.)
Flags: needinfo?(mats)
Just waiting 2 seconds, without toggling 'display', fails though (L1 is red).
Thanks for looking at that.  Just so I'm not blocked here, I'm going to split out the failing subtests into a separate test, mark them as expected fail, and file a bug for them.
(In reply to Cameron McCormack (:heycam) from comment #21)
> Thanks for looking at that.  Just so I'm not blocked here, I'm going to
> split out the failing subtests into a separate test, mark them as expected
> fail, and file a bug for them.

Turns out I don't need to do this, with the fix I've added for ::before/::after.  (The incorrect handling of these must have exposed that underlying bug.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c81b0800038da63017b9f38f3b4db38574638a65
Filed bug 1345809 for that bug anyway.
Comment on attachment 8826448 [details]
Bug 1330843 - Allow JS to create NAC pseudo-elements.

https://reviewboard.mozilla.org/r/104406/#review120576

r=me with those things addressed. I don't really understand the GeckoRestyleManager change, but I don't know that code in general, so it's probably worth discussing the correct solution with someone who does.

::: dom/base/nsDocument.cpp:5674
(Diff revision 5)
> +  CSSPseudoElementType pseudoType = CSSPseudoElementType::NotPseudo;
>    if (aOptions.IsElementCreationOptions()) {
> +    const ElementCreationOptions& options =
> +      aOptions.GetAsElementCreationOptions();
>      // Throw NotFoundError if 'is' is not-null and definition is null
>      is = CheckCustomElementName(aOptions.GetAsElementCreationOptions(),

Nit: use |options| here.

::: dom/base/nsDocument.cpp:5684
(Diff revision 5)
> +      if (rv.Failed() ||
> +          !nsCSSPseudoElements::PseudoElementIsJSCreatedNAC(pseudoType)) {

As noted below, we need to handle NotPseudo here, because GetPseudoElementType can return it, and PseudoElementIsJSCreatedNAC can't handle it.

::: dom/webidl/Document.webidl:21
(Diff revision 5)
> +  [ChromeOnly]
> +  DOMString pseudo;

Doesn't this need to be [Func="IsChromeOrXBL"]? I thought the whole point of this was to expose this to video controls.

I'm not 100% confident in how the conditional stuff works on dictionary members, so please get signoff from bz on the webidl change (making clear that it would be a security problem if untrusted content could ever set |pseudo|).

::: layout/base/GeckoRestyleManager.cpp:736
(Diff revision 5)
> +  Element* frameElement = aFrame->GetContent()->AsElement();
> +  if (frameElement->IsNativeAnonymous() &&
> +      aPseudoType != CSSPseudoElementType::before &&
> +      aPseudoType != CSSPseudoElementType::after) {
> +    // NAC-implemented pseudos use the closest non-NAC element as their
> +    // element to inherit from.
> +    Element* originatingElement =
> +      nsContentUtils::GetClosestNonNativeAnonymousAncestor(frameElement);
> +    if (originatingElement) {
> +      return originatingElement;
> +    }
> +  }

Hm. I don't really know what this function does, nor understand why we're exempting ::before and ::after here. If this only applies to JS-implemented NAC, then shouldn't we test for that? If not, does that mean that m-c is incorrect right now with respect to other NAC parentage?

::: layout/base/nsCSSFrameConstructor.cpp:5067
(Diff revision 5)
> +        // For pseudo-implementing NAC created by JS using the ChromeOnly
> +        // document.createElement(..., { pseudo: ... }) API, we find the
> +        // originating element by lookup the tree until we find a non-NAC
> +        // ancestor.

Please expand the comment to note that these are the correct semantics for other C++-generated NAC as well, but we just don't need to do that work here because the caller passes in the originating element in that case.

::: layout/style/nsCSSPseudoElements.h:105
(Diff revision 5)
> +  static bool PseudoElementIsJSCreatedNAC(Type aType)
> +  {
> +    return PseudoElementHasFlags(aType, CSS_PSEUDO_ELEMENT_IS_JS_CREATED_NAC);
> +  }

Hm, PseudoElementHasFlags can't handle NotPseudo IIUC. So we either need to fix that, or make this function handle NotPseudo, or make the callers handle NotPseudo.
Attachment #8826448 - Flags: review?(bobbyholley) → review+
<bholley>>bz: does [ChromeOnly] work right with dictionaries?
<bholley>>bz: i.e. if a dictionary member is annotated [ChromeOnly], can we rely on the fact that content can never pass a value for that property to an API that accepts the dict?
<bz>>~bholley: the way it works is that we will treat the property as having teh value "undefined" on the object unless nsContentUtils::ThreadsafeIsSystemCaller(cx) at the point when we're converting from a JS value to the dictionary struct
<bz>>~s/teh/the/
<bholley>>bz: hm
<bholley>>bz: with xrays, does that apply to the caller or the callee? 
<bz>>~bholley: With Xrays that applies to the caller. 
<bholley>>bz: heycam|away is adding a dictionary member to ElementCreationOptions that we want to be able to pass to document.createElement from XBL but not from content. Does that mean that setting [Func="IsChromeOrXBL"] on the dictionary member is the right way to do this? 
<bz>>~bholley: yes


So looks like we're good there.
FWIW the code that wants to create the pseudo is running in a jsm, and I don't think it's considered XBL.  [ChromeOnly] seemed to work for it, so if that is more restrictive than [Func="IsChromeOrXBL"], should we just use it instead?
Comment on attachment 8826448 [details]
Bug 1330843 - Allow JS to create NAC pseudo-elements.

https://reviewboard.mozilla.org/r/104406/#review120576

> Hm. I don't really know what this function does, nor understand why we're exempting ::before and ::after here. If this only applies to JS-implemented NAC, then shouldn't we test for that? If not, does that mean that m-c is incorrect right now with respect to other NAC parentage?

This function is used to find the originating element, given a frame whose style context is for a pseudo, or returns the element itself, if the frame's style context is not for a pseudo.

I think my earlier patch did check for JS-implemented NAC here, but your review comments said it should be possible to apply this to pseudo-implementing NAC in general, so I made it more general.  I didn't look into why, but I guess we are returning something wrong for ::before/::after generated content.  (It normally takes the code path at the end of the function, just returning the parent element.)

I'd be happy to move back to an explicit "is aPseudoType a JS-implemented NAC one" check here, and since that's a less invasive change.  (And I'd like to minimize the time I spend on this bug, ideally.)
ni? for comment 27 and comment 28 questions.
Flags: needinfo?(bobbyholley)
(In reply to Cameron McCormack (:heycam) from comment #27)
> FWIW the code that wants to create the pseudo is running in a jsm, and I
> don't think it's considered XBL.  [ChromeOnly] seemed to work for it, so if
> that is more restrictive than [Func="IsChromeOrXBL"], should we just use it
> instead?

Oh, I thought this was for video controls. In that case [ChromeOnly] certainly works fine.

(In reply to Cameron McCormack (:heycam) from comment #28)
> I think my earlier patch did check for JS-implemented NAC here, but your
> review comments said it should be possible to apply this to
> pseudo-implementing NAC in general, so I made it more general.  I didn't
> look into why, but I guess we are returning something wrong for
> ::before/::after generated content.  (It normally takes the code path at the
> end of the function, just returning the parent element.)

In what situation do we end up with generated before/after content that is parented to a native-anonymous element? That seems like the crux of the issue here, since I wouldn't think that could happen, and it appears to be happening (otherwise the results would be the same). If this indeed can happen, it's a potential source of bugs when we switch over to stylo for styling these things (since we're going to always be looking at the first non-nac ancestor).

> 
> I'd be happy to move back to an explicit "is aPseudoType a JS-implemented
> NAC one" check here, and since that's a less invasive change.  (And I'd like
> to minimize the time I spend on this bug, ideally.)

I still think this is a bug, but I also agree I'd rather not rathole too much on it. Please run a quick test to figure out what the situation above is. If the issue doesn't look easily fixable, I'm ok with filing a followup bug and referencing that in the comment to explain why we're only checking JS-implemented NAC for now. Depending on the nature of what the above experiment reveals, the followup may or may not block stylo.
Flags: needinfo?(bobbyholley)
Depends on: 1346623
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf88b5709261
Allow JS to create NAC pseudo-elements. r=bholley
https://hg.mozilla.org/mozilla-central/rev/cf88b5709261
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: