Closed Bug 1460382 Opened 6 years ago Closed 6 years ago

NODE_IS_NATIVE_ANONYMOUS looks redundant to me

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

Bug 1460382: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
I don't get the difference between it and IS_IN_NATIVE_ANONYMOUS_SUBTREE.

It only has two places where that flag is set, and one is very patchy for JS-created NAC.

We always set this flag on all the subtree of NAC, so it should be equivalent to IsInNativeAnonymousSubtree for those except for the weird canvas case, but arguably the line at:

  https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#4193

Should be moved inside the not-canvas condition.

Anyway. So the remaining differences are callers to SetIsNativeAnonymousRoot() which don't set the NODE_IS_ANONYMOUS_CONTENT flag. I think that's only editor's manual NAC, which should be fine to change I think.

Am I missing something?
Hmm, I guess this is not true for generated content image elements, unfortunately...

Looks like this setup still ought to be cleaned up. It looks to me that if we have any element under the ::-moz pseudo-elements those wouldn't inherit as expected (they'd inherit from the originating element instead).

Cam, wdyt about doing the weird "inherit from the closest non-NAC ancestor" only for pseudo-elements, and remove this bit, using IsInNativeAnonymousSubtree() instead pretty everywhere else?
Flags: needinfo?(cam)
pretty much*
Let me give that a try...
Assignee: nobody → emilio
Priority: -- → P3
I'm on PTO, but we added this recently because they weren't equivalent. You should be able to dig up the reasoning in the bug, or ask bz. I think cam wasn't involved.
Hmm... Yeah, I think the case where it may not be equivalent is when NAC loads XBL bindings and what not... I _think_ my proposal in comment 1 could avoid the problems this caused. But let me check.
Yeah, I believe one case was xbl generated content in nac subtrees. That has the subtree bit set but not the self bit, since we don't want the nac style inheritance rules to apply there.
(In reply to Bobby Holley (On Leave Until June 11th) from comment #6)
> Yeah, I believe one case was xbl generated content in nac subtrees. That has
> the subtree bit set but not the self bit, since we don't want the nac style
> inheritance rules to apply there.

I think that's fixable inheriting from the closest anonymous root parent, instead of the closest non-anonymous ancestor. Trying that atm.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> (In reply to Bobby Holley (On Leave Until June 11th) from comment #6)
> > Yeah, I believe one case was xbl generated content in nac subtrees. That has
> > the subtree bit set but not the self bit, since we don't want the nac style
> > inheritance rules to apply there.
> 
> I think that's fixable inheriting from the closest anonymous root parent,
> instead of the closest non-anonymous ancestor. Trying that atm.

But we want non-root xbl content to inheritance from the flattened tree parent. I don't see how we can preserve that without differentiating "being in a nac subtree" from "is nac itself".
(In reply to Bobby Holley (On Leave Until June 11th) from comment #8)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> > (In reply to Bobby Holley (On Leave Until June 11th) from comment #6)
> > > Yeah, I believe one case was xbl generated content in nac subtrees. That has
> > > the subtree bit set but not the self bit, since we don't want the nac style
> > > inheritance rules to apply there.
> > 
> > I think that's fixable inheriting from the closest anonymous root parent,
> > instead of the closest non-anonymous ancestor. Trying that atm.
> 
> But we want non-root xbl content to inheritance from the flattened tree
> parent. I don't see how we can preserve that without differentiating "being
> in a nac subtree" from "is nac itself".

I plan to apply the new rule only to pseudo-elements (which is the only place where it matters and it's observable). Can you think of any reason where that wouldn't work?

Here's the try run fwiw: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00921e97b5d92459d9e8430c660c1b8d6d26cf87
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> (In reply to Bobby Holley (On Leave Until June 11th) from comment #8)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> > > (In reply to Bobby Holley (On Leave Until June 11th) from comment #6)
> > > > Yeah, I believe one case was xbl generated content in nac subtrees. That has
> > > > the subtree bit set but not the self bit, since we don't want the nac style
> > > > inheritance rules to apply there.
> > > 
> > > I think that's fixable inheriting from the closest anonymous root parent,
> > > instead of the closest non-anonymous ancestor. Trying that atm.
> > 
> > But we want non-root xbl content to inheritance from the flattened tree
> > parent. I don't see how we can preserve that without differentiating "being
> > in a nac subtree" from "is nac itself".
> 
> I plan to apply the new rule only to pseudo-elements (which is the only
> place where it matters and it's observable). Can you think of any reason
> where that wouldn't work?

If somebody happens to use a pseudo element in a nac xbl binding?
> 
> Here's the try run fwiw:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=00921e97b5d92459d9e8430c660c1b8d6d26cf87

I don't expect our test suite to catch something like this. And whether or now we currently have anything in the product that exercises this behivor, we designed this setup (recently) to make sense and do the right thing in the edge cases. So unless there's a compelling architectural reason this needs to change beyond general simplification, I don't see why we should make the setup less sane.

Note that my opinion here would change if we got to a world where we could assert that everything is nac subtrees was itself nac. But we're not there yet.
(In reply to Bobby Holley (On Leave Until June 11th) from comment #10)
> > I plan to apply the new rule only to pseudo-elements (which is the only
> > place where it matters and it's observable). Can you think of any reason
> > where that wouldn't work?
> 
> If somebody happens to use a pseudo element in a nac xbl binding?

Pseudo-elements need to be NAC themselves, even js-created ones. So they would do the right thing. Indeed, this happens already, with the <input type="range">s in the videocontrols bindings, for example.

::before and ::after also do the right thing, since they're NAC roots themselves, and thus they inherit from its parent.

> I don't expect our test suite to catch something like this. And whether or
> now we currently have anything in the product that exercises this behivor,
> we designed this setup (recently) to make sense and do the right thing in
> the edge cases. So unless there's a compelling architectural reason this
> needs to change beyond general simplification, I don't see why we should
> make the setup less sane.

So, I'm not sure I get that. How does the current behavior make sense if we start stashing other NAC elements inside pseudo-elements, for example?

Also, how is the setup I'm proposing less sane in edge cases than the current setup?

> Note that my opinion here would change if we got to a world where we could
> assert that everything is nac subtrees was itself nac. But we're not there
> yet.

I agree that'd be the extra-nice thing to do :)
Also, fwiw, our test-suit does definitely catch this if you get it wrong. A previous attempt of mine with the wrong parent style for the ::-moz-range-track pseudos does definitely complain:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f33c53d3538617cec8f8e970c056366327dc332

Anyway, https://treeherder.mozilla.org/#/jobs?repo=try&revision=72ceebb7a603f235debc031c6217aebaa111134d is the last attempt with a small fix for the first-line bits.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> (In reply to Bobby Holley (On Leave Until June 11th) from comment #10)
> > > I plan to apply the new rule only to pseudo-elements (which is the only
> > > place where it matters and it's observable). Can you think of any reason
> > > where that wouldn't work?
> > 
> > If somebody happens to use a pseudo element in a nac xbl binding?
> 
> Pseudo-elements need to be NAC themselves, even js-created ones. So they
> would do the right thing. Indeed, this happens already, with the <input
> type="range">s in the videocontrols bindings, for example.

Actually this is kind-of not true, that is, JS-created NAC are not anonymous roots by default. But that means that we're already following this nasty inheritance rules in <video> since bug 1318542, because of the ::cue bits (see part 3 of that bug).

This setup makes it work correctly and allows removing that hack.
Comment on attachment 8974752 [details]
Bug 1460382: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents.

r?cam too since he is more familiar with the JS-created pseudo setup.
Flags: needinfo?(cam)
Attachment #8974752 - Flags: review?(cam)
(In reply to Bobby Holley (On Leave Until June 11th) from comment #4)
> I'm on PTO, but we added this recently because they weren't equivalent. You
> should be able to dig up the reasoning in the bug, or ask bz. I think cam
> wasn't involved.

Which bug was this?
(In reply to Cameron McCormack (:heycam) from comment #17)
> (In reply to Bobby Holley (On Leave Until June 11th) from comment #4)
> > I'm on PTO, but we added this recently because they weren't equivalent. You
> > should be able to dig up the reasoning in the bug, or ask bz. I think cam
> > wasn't involved.
> 
> Which bug was this?

This was bug 1331322.
OK, I just read through that bug while trying to page this stuff in again...

Just so I'm absolutely clear, can you summarize exactly what the current behavior is for all cases (regular NAC, editor-created manual NAC, element-backed pseudo NAC such as ::before or ::placeholder, pseudo NAC within NAC created for an XBL binding), and what the new behaviour is with the patch?

Also, I'm pretty sure that you've written this patch because of the content:url() work you're doing at the moment, so can you also explain how that influences your patch here?
Flags: needinfo?(emilio)
(In reply to Cameron McCormack (:heycam) from comment #19)
> Just so I'm absolutely clear, can you summarize exactly what the current
> behavior is for all cases (regular NAC, editor-created manual NAC,
> element-backed pseudo NAC such as ::before or ::placeholder, pseudo NAC
> within NAC created for an XBL binding), and what the new behaviour is with
> the patch?

So, right now:

 (1) For regular NAC, we set the NODE_IS_ANONYMOUS bit in nsCSSFrameConstructor, which means that all regular NAC inherits from the originating element instead of its parent right now. This patch changes it to inherit regularly, except for pseudos.
 (2) For editor-created NAC, we don't set that bit, so they inherit from their normal parent (though I believe editor only creates one level of NAC, so it should not be an issue). My patch wouldn't change this in any case.
 (3) For element-backed pseudo NAC, that falls into (1) right now. With this patch they would still inherit from the originating element as the only special case.
 (4) For XBL inside NAC, we don't set the NODE_IS_ANONYMOUS bit there, so regular inheritance rules apply. Except when we have a JS-created pseudo, in which case we go up the tree and do set the bit from BindToTree, which causes the behavior to switch. That happens right now when you insert ::cue into the tree, and I think that's the main broken bit why this patch is worth landing (apart from regular cleanup).
 (5) For pseudo NAC created inside XBL, (e.g., something in an XBL binding for NAC doing ::before / ::after stuff), they inherit from the originating element, because we don't set the NODE_IS_ANONYMOUS bits in the XBL content (except when we do, see above). My patch keeps that working by looking at the NAC subtree's root parent, instead of just at the closest non-NAC ancestor.
 (6) For JS-created NAC (::cue), we do set the NODE_IS_ANONYMOUS bit on it and all its ancestors. That's pretty broken.

> Also, I'm pretty sure that you've written this patch because of the
> content:url() work you're doing at the moment, so can you also explain how
> that influences your patch here?

It doesn't influence it in any way, I was just going through how generated content worked, and tried to make sense of why the setup was like it is, and failed initially, then realized that this setup could be simplified / improved / was broken for ::cue and such.
Flags: needinfo?(emilio)
(Assuming you mean NODE_IS_NATIVE_ANONYMOUS here.)

(In reply to Emilio Cobos Álvarez [:emilio] from comment #20)
>  (1) For regular NAC, we set the NODE_IS_ANONYMOUS bit in
> nsCSSFrameConstructor, which means that all regular NAC inherits from the
> originating element instead of its parent right now. This patch changes it
> to inherit regularly, except for pseudos.

OK.  I think I prefer this, even though it's not what was settled on in bug 1331322.  To me it makes a lot more sense to inherit "normally", except for when we need to inherit from a particular ancestor, which at the moment is just pseudo-elements and which currently is always the originating element.  It means we don't need to perform contortions if we want to create NAC subtrees that is more than just one element high and where we want to rely on style inheritance in that subtreee.

>  (2) For editor-created NAC, we don't set that bit, so they inherit from
> their normal parent (though I believe editor only creates one level of NAC,
> so it should not be an issue). My patch wouldn't change this in any case.
>  (3) For element-backed pseudo NAC, that falls into (1) right now. With this
> patch they would still inherit from the originating element as the only
> special case.
>  (4) For XBL inside NAC, we don't set the NODE_IS_ANONYMOUS bit there, so
> regular inheritance rules apply. Except when we have a JS-created pseudo, in
> which case we go up the tree and do set the bit from BindToTree, which
> causes the behavior to switch. That happens right now when you insert ::cue
> into the tree, and I think that's the main broken bit why this patch is
> worth landing (apart from regular cleanup).
>  (5) For pseudo NAC created inside XBL, (e.g., something in an XBL binding
> for NAC doing ::before / ::after stuff), they inherit from the originating
> element, because we don't set the NODE_IS_ANONYMOUS bits in the XBL content
> (except when we do, see above). My patch keeps that working by looking at
> the NAC subtree's root parent, instead of just at the closest non-NAC
> ancestor.

Where the originating element here is the element created by the XBL binding, not the element that has the binding, right?

If that's the case, wouldn't it make a ::placeholder for an <input type=number> inherit from the <input> in the binding's anonymous content rather than the <input type=number>?  Or is that treated differently from ::before / ::after?

>  (6) For JS-created NAC (::cue), we do set the NODE_IS_ANONYMOUS bit on it
> and all its ancestors. That's pretty broken.

Does your patch have any effect on ::cue?  The behaviour we should have, but I'm not sure we do, is that ::cue inherits from the <video>.  It might have been that the hack resulting in bug 1460454 needed the incorrect inherit-from-parent behavior, but if we can somehow resolve that issue too, that would be nice.

Where does this bit setting happen, incidentally?
Flags: needinfo?(emilio)
(In reply to Cameron McCormack (:heycam) from comment #21)
> (Assuming you mean NODE_IS_NATIVE_ANONYMOUS here.)
> 
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #20)
> >  (1) For regular NAC, we set the NODE_IS_ANONYMOUS bit in
> > nsCSSFrameConstructor, which means that all regular NAC inherits from the
> > originating element instead of its parent right now. This patch changes it
> > to inherit regularly, except for pseudos.
> 
> OK.  I think I prefer this, even though it's not what was settled on in bug
> 1331322.  To me it makes a lot more sense to inherit "normally", except for
> when we need to inherit from a particular ancestor, which at the moment is
> just pseudo-elements and which currently is always the originating element. 
> It means we don't need to perform contortions if we want to create NAC
> subtrees that is more than just one element high and where we want to rely
> on style inheritance in that subtreee.
> 
> >  (2) For editor-created NAC, we don't set that bit, so they inherit from
> > their normal parent (though I believe editor only creates one level of NAC,
> > so it should not be an issue). My patch wouldn't change this in any case.
> >  (3) For element-backed pseudo NAC, that falls into (1) right now. With this
> > patch they would still inherit from the originating element as the only
> > special case.
> >  (4) For XBL inside NAC, we don't set the NODE_IS_ANONYMOUS bit there, so
> > regular inheritance rules apply. Except when we have a JS-created pseudo, in
> > which case we go up the tree and do set the bit from BindToTree, which
> > causes the behavior to switch. That happens right now when you insert ::cue
> > into the tree, and I think that's the main broken bit why this patch is
> > worth landing (apart from regular cleanup).
> >  (5) For pseudo NAC created inside XBL, (e.g., something in an XBL binding
> > for NAC doing ::before / ::after stuff), they inherit from the originating
> > element, because we don't set the NODE_IS_ANONYMOUS bits in the XBL content
> > (except when we do, see above). My patch keeps that working by looking at
> > the NAC subtree's root parent, instead of just at the closest non-NAC
> > ancestor.
> 
> Where the originating element here is the element created by the XBL
> binding, not the element that has the binding, right?

No, it's the element that has the binding, since elements created by XBL anon content aren't native anonymous content, just regular anon content.

> If that's the case, wouldn't it make a ::placeholder for an <input
> type=number> inherit from the <input> in the binding's anonymous content
> rather than the <input type=number>?  Or is that treated differently from
> ::before / ::after?

There's no XBL anon content involved in <input type="number"> whatsoever.

However, now you mention it, <input type="number">s pseudos are really the pseudos of the inner <input> that the number control frame creates... My patch would break that indeed, making them inherit / match against the anonymous input, and thus not match document rules... sigh, tests are lacking for this.

I special-cased this, given if buttons or other elements created by the combobox control frame ever end up having other pseudos, the current behavior would be bizarre / unexpected, I'd say. I think we should make nsNumberControlFrame inherit from nsTextControlFrame instead of just creating an anonymous <input>. I'll file a followup to see whether it's doable.

> >  (6) For JS-created NAC (::cue), we do set the NODE_IS_ANONYMOUS bit on it
> > and all its ancestors. That's pretty broken.
> 
> Does your patch have any effect on ::cue?  The behaviour we should have, but
> I'm not sure we do, is that ::cue inherits from the <video>.  It might have
> been that the hack resulting in bug 1460454 needed the incorrect
> inherit-from-parent behavior, but if we can somehow resolve that issue too,
> that would be nice.

This is the behavior we have now, but in the process we alter the inheritance behavior of all the parent chain in:

  https://searchfox.org/mozilla-central/rev/0e80033a10ca4f9940c083a33d175c99ab3568e5/dom/base/Element.cpp#1783

My patch would preserve that behavior, without that hack.
Flags: needinfo?(emilio)
Oh, apparently we did have a test for that, which my patch somehow didn't regress when I originally wrote that patch[1], but started regressing after bug 1461299. There's something really fishy here, I'll take a closer look.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72ceebb7a603f235debc031c6217aebaa111134d
(In reply to Emilio Cobos Álvarez [:emilio] from comment #24)
> Oh, apparently we did have a test for that, which my patch somehow didn't
> regress when I originally wrote that patch[1], but started regressing after
> bug 1461299.

(that bug number range doesn't even make sense I think...)
Ok, I built the revision from my try run and it definitely breaks input[type="number"]::placeholder, which makes me a bit more comfortable with the current patch...
(In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> > >  (5) For pseudo NAC created inside XBL, (e.g., something in an XBL binding
> > > for NAC doing ::before / ::after stuff), they inherit from the originating
> > > element, because we don't set the NODE_IS_ANONYMOUS bits in the XBL content
> > > (except when we do, see above). My patch keeps that working by looking at
> > > the NAC subtree's root parent, instead of just at the closest non-NAC
> > > ancestor.
> > 
> > Where the originating element here is the element created by the XBL
> > binding, not the element that has the binding, right?
> 
> No, it's the element that has the binding, since elements created by XBL
> anon content aren't native anonymous content, just regular anon content.

I'm confused then.  If we have:

  <something>  (which has an XBL binding)
    |
    +-- <div>  (root of the binding's anonymous content)
          |
          +-- <_moz_generated_content_before>  (::before for the <div>)

isn't the <div> the NAC subtree root parent, and isn't that indeed what we want the ::before to inherit from?

> > If that's the case, wouldn't it make a ::placeholder for an <input
> > type=number> inherit from the <input> in the binding's anonymous content
> > rather than the <input type=number>?  Or is that treated differently from
> > ::before / ::after?
> 
> There's no XBL anon content involved in <input type="number"> whatsoever.

My mistake, I was confusing <input type="number"> with how a XUL <textbox type="number"> is implemented.

I'm not even sure if ::placeholder works on <textbox type="number"> -- does it?  And where does it inherit from?

> However, now you mention it, <input type="number">s pseudos are really the
> pseudos of the inner <input> that the number control frame creates... My
> patch would break that indeed, making them inherit / match against the
> anonymous input, and thus not match document rules... sigh, tests are
> lacking for this.
> 
> I special-cased this, given if buttons or other elements created by the
> combobox control frame ever end up having other pseudos, the current
> behavior would be bizarre / unexpected, I'd say. I think we should make
> nsNumberControlFrame inherit from nsTextControlFrame instead of just
> creating an anonymous <input>. I'll file a followup to see whether it's
> doable.

Is <input type="number"> the only thing that creates NAC and wants to basically export the NAC's pseudo-element as its own like this?  If it's a reasonable thing to do, and maybe other NAC implementations of things would want to do it too (are there any XBL-implemented elements that will become NAC as part of de-XBL-ification that might affected?), it might be better to use a flag somewhere to indicate this rather than hard coding it for ::-moz-number.

I know it shouldn't be visible to the author, but I feel a bit uncomfortable having this special case for all pseudo-elements of ::-moz-number rather than just for ::placeholder.  Maybe I'm over thinking this, but it seems like the <input type="number"> implementation should explicitly opt in to this originating element fixup just for the exported pseudo-elements it wants to support.

WDYT?
Flags: needinfo?(emilio)
IIRC, our motivation for doing the special nac inheritance unconditionally and not just for pseudo elements is that we didn't want there to be an unexpected change in inheritance behavior when somebody decided to make a piece of nac addressable via a pseudo. Number inputs are one example of a nac subtree where lots of stuff is addressable via pseudos for internal reasons, but we could certainly end up with other similar things.
(In reply to Cameron McCormack (:heycam) from comment #27)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> > > >  (5) For pseudo NAC created inside XBL, (e.g., something in an XBL binding
> > > > for NAC doing ::before / ::after stuff), they inherit from the originating
> > > > element, because we don't set the NODE_IS_ANONYMOUS bits in the XBL content
> > > > (except when we do, see above). My patch keeps that working by looking at
> > > > the NAC subtree's root parent, instead of just at the closest non-NAC
> > > > ancestor.
> > > 
> > > Where the originating element here is the element created by the XBL
> > > binding, not the element that has the binding, right?
> > 
> > No, it's the element that has the binding, since elements created by XBL
> > anon content aren't native anonymous content, just regular anon content.
> 
> I'm confused then.  If we have:
> 
>   <something>  (which has an XBL binding)
>     |
>     +-- <div>  (root of the binding's anonymous content)
>           |
>           +-- <_moz_generated_content_before>  (::before for the <div>)
> 
> isn't the <div> the NAC subtree root parent, and isn't that indeed what we
> want the ::before to inherit from?

Yes, sorry, I misread and was thinking about JS-pseudos in XBL. The <_moz_generated_content_before> is the NAC root, so the <div> is the generated element.

> > > If that's the case, wouldn't it make a ::placeholder for an <input
> > > type=number> inherit from the <input> in the binding's anonymous content
> > > rather than the <input type=number>?  Or is that treated differently from
> > > ::before / ::after?
> > 
> > There's no XBL anon content involved in <input type="number"> whatsoever.
> 
> My mistake, I was confusing <input type="number"> with how a XUL <textbox
> type="number"> is implemented.
> 
> I'm not even sure if ::placeholder works on <textbox type="number"> -- does
> it?  And where does it inherit from?

Doubt so.

> > However, now you mention it, <input type="number">s pseudos are really the
> > pseudos of the inner <input> that the number control frame creates... My
> > patch would break that indeed, making them inherit / match against the
> > anonymous input, and thus not match document rules... sigh, tests are
> > lacking for this.
> > 
> > I special-cased this, given if buttons or other elements created by the
> > combobox control frame ever end up having other pseudos, the current
> > behavior would be bizarre / unexpected, I'd say. I think we should make
> > nsNumberControlFrame inherit from nsTextControlFrame instead of just
> > creating an anonymous <input>. I'll file a followup to see whether it's
> > doable.
> 
> Is <input type="number"> the only thing that creates NAC and wants to
> basically export the NAC's pseudo-element as its own like this?  If it's a
> reasonable thing to do, and maybe other NAC implementations of things would
> want to do it too (are there any XBL-implemented elements that will become
> NAC as part of de-XBL-ification that might affected?), it might be better to
> use a flag somewhere to indicate this rather than hard coding it for
> ::-moz-number.

Yes, ::-moz-number-text is the only thing affected for now... I'd rather not make it a flag unless needed, but I don't care much either way :)

> I know it shouldn't be visible to the author, but I feel a bit uncomfortable
> having this special case for all pseudo-elements of ::-moz-number rather
> than just for ::placeholder.  Maybe I'm over thinking this, but it seems
> like the <input type="number"> implementation should explicitly opt in to
> this originating element fixup just for the exported pseudo-elements it
> wants to support.

Perhaps! But again I'd rather not make it overgeneric unless there's a use case for it...

(In reply to Bobby Holley (On Leave Until June 11th) from comment #28)
> IIRC, our motivation for doing the special nac inheritance unconditionally
> and not just for pseudo elements is that we didn't want there to be an
> unexpected change in inheritance behavior when somebody decided to make a
> piece of nac addressable via a pseudo. Number inputs are one example of a
> nac subtree where lots of stuff is addressable via pseudos for internal
> reasons, but we could certainly end up with other similar things.

That makes sense... I wonder how much the inheritance behavior matters in practice btw, I'm pretty sure other browsers don't have such a weird setup for inheritance, and they only mungle the originating element.

Is this something we could consider to change? (splitting originating element vs. element we're inheriting from?). That would remove this issue altogether.
Flags: needinfo?(emilio)
Comment on attachment 8974752 [details]
Bug 1460382: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents.

https://reviewboard.mozilla.org/r/243140/#review251910

I don't think you need review from me here if Cameron is reviewing...
Attachment #8974752 - Flags: review?(bzbarsky)
See Also: → 1463511
We decided to go ahead with this for now, since the weird inheritance already has the potential to break some stuff like the ComputedStyle::HasTextDecorationLines() optimizations that are done in layout.

On the long run we may want to remove support for this weird inheritance, but that requires at least to merge nsNumberControlFrame and nsTextControlFrame, which may be non-trivial...
Comment on attachment 8974752 [details]
Bug 1460382: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents.

https://reviewboard.mozilla.org/r/243140/#review253204

::: commit-message-7bdd0:1
(Diff revision 4)
> +Bug 1460382: Remove NODE_IS_NATIVE_ANONYMOUS. r?heycam
> +
> +Instead of inheriting from the closest non-NAC ancestor, we inherit from the
> +closest native anonymous root's parent, only if we're a pseudo-element.

Mentioning the removal of the node flag kind of buries the lede here, which is that we are changing how NAC element inheritance works.  So how about saying that in the first line, then in the detailed part of the commit message say that this allows us to remove the node flag.

I suggest a slight tweak to this wording here too for clarity, so maybe:


Bug 1460382: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents. r?heycam

Currently, NAC always inherits from the closest non-NAC ancestor element,
regardless of whether it is for an element-backed pseudo or not.  This patch
changes the inheritance so that for element-backed pseudos, we inherit from
the closest native anonymous root's parent, and for other NAC we inherit
from the parent.

This prevents the following two issues and allows us to remove the
NODE_IS_NATIVE_ANONYMOUS flag:

...

::: commit-message-7bdd0:8
(Diff revision 4)
> +Instead of inheriting from the closest non-NAC ancestor, we inherit from the
> +closest native anonymous root's parent, only if we're a pseudo-element.
> +
> +That prevents the two issues that caused this flag to be created:
> +
> + * Following weird NAC inheritance rules in XBL bindings bound to NAC.

Can you go into more detail about what the weird NAC inheritance rules are here?

::: commit-message-7bdd0:10
(Diff revision 4)
> +
> +That prevents the two issues that caused this flag to be created:
> +
> + * Following weird NAC inheritance rules in XBL bindings bound to NAC.
> +
> +   - This is no longer a problem since we only apply the rule only if we're

One too many "only"s.

::: commit-message-7bdd0:11
(Diff revision 4)
> +That prevents the two issues that caused this flag to be created:
> +
> + * Following weird NAC inheritance rules in XBL bindings bound to NAC.
> +
> +   - This is no longer a problem since we only apply the rule only if we're
> +     a pseudo-element, and all pseudo-elements hold from NAC.

I don't know what "hold from NAC" means.  Reword?

::: commit-message-7bdd0:16
(Diff revision 4)
> +     a pseudo-element, and all pseudo-elements hold from NAC.
> +
> + * Inheriting from the wrong thing if we're a nested NAC subtree.
> +
> +   - We no longer look past our NAC subtree.
> +

You should mention the hard coded special case for <input type="number">.

::: layout/generic/nsFrame.cpp
(Diff revision 4)
> -  // We need to take special care not to disrupt the style inheritance of frames
> -  // whose content is NAC but who implement a pseudo (like an anonymous
> -  // box, or a non-NAC-backed pseudo like ::first-line) that does not match the
> -  // one that the NAC implements, if any.

Is this part of the comment still relevant?  We still have the check on the element's pseudo type.

::: layout/generic/nsFrame.cpp:9806
(Diff revision 4)
> -  Element* element =
> +    Element* element =
> -    content && content->IsElement() ? content->AsElement() : nullptr;
> -  if (element && element->IsNativeAnonymous() && !element->IsNativeScrollbarContent() &&
> +      aFrame->GetContent()->IsElement()
> +        ? aFrame->GetContent()->AsElement() : nullptr;

Wondering why we don't have a GetAsElement() method...

::: layout/style/res/forms.css
(Diff revision 4)
> -  text-decoration-color: inherit;
> -  text-decoration-style: inherit;

I guess these were unnecessary since bug 1039488, and would have been better in a separate commit (or bug).

::: layout/style/res/forms.css:1059
(Diff revision 4)
>    /* This pseudo-element is also an 'input' element (nested inside and
>     * distinct from the <input type=number> element) so we need to prevent the
>     * explicit setting of 'text-align' by the general CSS rule for 'input'
>     * above. We want to inherit its value from its <input type=number>
>     * ancestor, not have that general CSS rule reset it.
>     */
>    text-align: inherit;
> +  text-decoration: inherit;

Is it true that the general input rule earlier in this file will match the ::-moz-number-text <input> here?  If so, why do we need text-decoration:inherit repeated here?  If not, why do we need text-align:inherit?

::: layout/style/res/forms.css:1068
(Diff revision 4)
>     * ancestor, not have that general CSS rule reset it.
>     */
>    text-align: inherit;
> +  text-decoration: inherit;
> +  ime-mode: inherit;
> +  resize: inherit;

I don't think resize has any effect on an <input> element, only on <textarea>s.  So I don't think we need this.

::: servo/components/style/gecko/wrapper.rs:569
(Diff revision 4)
>      }
>  }
>  
>  impl<'le> GeckoElement<'le> {
>      #[inline]
> +    fn closest_anon_subtree_root_parent(self) -> Option<Self> {

GeckoElement is Copy, but for consistency with the other functions defined here let's make this function take &self.

::: servo/components/style/gecko/wrapper.rs:1333
(Diff revision 4)
>      #[inline]
>      fn is_native_anonymous(&self) -> bool {
> -        use gecko_bindings::structs::NODE_IS_NATIVE_ANONYMOUS;
> +        self.is_in_native_anonymous_subtree()
> -        self.flags() & (NODE_IS_NATIVE_ANONYMOUS as u32) != 0
>      }

I think we should remove this function and have the callers use is_in_native_anonymous_subtree() instead, so that it doesn't look like there are still two separate concepts.
Comment on attachment 8974752 [details]
Bug 1460382: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents.

https://reviewboard.mozilla.org/r/243140/#review253204

> Mentioning the removal of the node flag kind of buries the lede here, which is that we are changing how NAC element inheritance works.  So how about saying that in the first line, then in the detailed part of the commit message say that this allows us to remove the node flag.
> 
> I suggest a slight tweak to this wording here too for clarity, so maybe:
> 
> 
> Bug 1460382: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents. r?heycam
> 
> Currently, NAC always inherits from the closest non-NAC ancestor element,
> regardless of whether it is for an element-backed pseudo or not.  This patch
> changes the inheritance so that for element-backed pseudos, we inherit from
> the closest native anonymous root's parent, and for other NAC we inherit
> from the parent.
> 
> This prevents the following two issues and allows us to remove the
> NODE_IS_NATIVE_ANONYMOUS flag:
> 
> ...

Much nicer :)

> Is it true that the general input rule earlier in this file will match the ::-moz-number-text <input> here?  If so, why do we need text-decoration:inherit repeated here?  If not, why do we need text-align:inherit?

No, it will match the anon div, which, unless the `<input>` explicitly inherits it, won't be affected otherwise, that's why we explicitly need to inherit it here.

> I don't think resize has any effect on an <input> element, only on <textarea>s.  So I don't think we need this.

True, will remove.
Comment on attachment 8974752 [details]
Bug 1460382: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents.

https://reviewboard.mozilla.org/r/243140/#review253308

::: layout/style/res/forms.css:1059
(Diff revision 4)
>    /* This pseudo-element is also an 'input' element (nested inside and
>     * distinct from the <input type=number> element) so we need to prevent the
>     * explicit setting of 'text-align' by the general CSS rule for 'input'
>     * above. We want to inherit its value from its <input type=number>
>     * ancestor, not have that general CSS rule reset it.
>     */
>    text-align: inherit;
> +  text-decoration: inherit;

No, the general input rule matches the `.preview-div`, not the `<input>`, so we still need this.

::: layout/style/res/forms.css:1068
(Diff revision 4)
>     * ancestor, not have that general CSS rule reset it.
>     */
>    text-align: inherit;
> +  text-decoration: inherit;
> +  ime-mode: inherit;
> +  resize: inherit;

Ah alright, will remove.
Comment on attachment 8974752 [details]
Bug 1460382: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents.

https://reviewboard.mozilla.org/r/243140/#review253926

::: layout/style/res/forms.css:1059
(Diff revision 4)
>    /* This pseudo-element is also an 'input' element (nested inside and
>     * distinct from the <input type=number> element) so we need to prevent the
>     * explicit setting of 'text-align' by the general CSS rule for 'input'
>     * above. We want to inherit its value from its <input type=number>
>     * ancestor, not have that general CSS rule reset it.
>     */
>    text-align: inherit;
> +  text-decoration: inherit;

Thanks, I see I was mistakenly looking at the `input > .anonymous-div` rule rather than the `input` rule when thinking about the `text-decoration` declaration there.
Comment on attachment 8974752 [details]
Bug 1460382: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents.

https://reviewboard.mozilla.org/r/243140/#review253928
Attachment #8974752 - Flags: review?(cam) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb045179d4f
Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents. r=heycam
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11242 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/bbb045179d4f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: 1474561
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: