Closed Bug 1296556 Opened 4 years ago Closed 4 years ago

stylo: Consistent inheritance and change hint processing inside pseudo-elements

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(4 files)

Just after pushing bug 1292618, I realized we don't handle well style changes generated only from inherited properties, like if only the `color` property changes.

This is (if I've diagnosed it well) because we inherit using Servo_InheritComputedValues directly, which means that our parent style context doesn't compute the nsStyleColor struct. In normal traversals we stick the change hint into the parent element in order to take it into account, but inside pseudo-elements this doesn't work because pseudo-element children aren't traversed by Servo.

Some possible solutions:

 * Creating an API that allows inheriting directly from a nsStyleContext (computing the inherited structs). Seems doable.

 * Touching inherited structs each time we inherit styles from Servo explicitly. Seems easy to do, and might work just fine. Involves a few FFI calls, though that would also be needed in the previous case. We only pay a few atomic increments more.

 * Adding some sort of markers to the style context so it knows it has to compute the inherited style structs in CalcStyleDifference.
Bobby asked for more explanation here so there it goes:

So when a change hint is computed on a pseudo-element, we stick that change in the change of the parent, which works just fine... Except in the case of inherited styles.

It's basically the same problem that we have for text nodes in Servo, only that we can't stick it in the parent while traversing, because we don't traverse it.

Basically, the content tree looks something like that:

 <p>
  - <_moz_generatedcontent_before>
     - Text node.
  - Text node.

And Servo only traverses <p> and the second text node.

The strategy of sticking the change of the PE into the parent works fine when Gecko's layout need the changed properties to paint the parent frame, e.g., background, ..., but not when there are property changes like color, because the computed change hint is 0 in those cases (because the style struct isn't in the cache of the PE's style context).

For example, if there's a color-only change in the pseudo-element style, that should generate a nsChangeHint_RepaintFrame. But since that struct isn't in the cache (it has only been accessed on the child text node), we skip that check entirely, computing effectively a nsChangeHint(0).

That's effectively the same problem we've had for change hints of text nodes, that we've solved temporarily computing change hints for them and sticking them in the parent. This bug is a good opportunity to stop doing that too.
Emilio and I just chatted about this on IRC.

As mentioned above, this is really just an instance of a more general problem. Change hint processing happens at the Element level, which generally works fine because text nodes never have explicit style. The one wrinkle is that if an inherited style is accessed on the child text node but never on the element itself, CalcStyleDifference on the element might incorrectly conclude that the style struct had never been accessed by layout and thus that no change hint is necessary for that struct.

I think we should solve this by having the getters for inherited structs on text node style contexts forward to those of the parent style context, which will mark the 'observed' bit appropriately. We can then stop calling CalcStyleDifference on text nodes entirely.
That sounds reasonable to me.  The reason why this works in Gecko is that when we look up the struct on the text node's style struct, it will ask the rule node for the struct, which determines that the struct can be inherited directly from the parent, which requests the struct from the parent, which caches the struct on the parent.
Comment on attachment 8783239 [details]
Bug 1296556: Reach the parent to compute inherited style structs.

https://reviewboard.mozilla.org/r/73144/#review71164

::: layout/style/ServoBindingList.h:87
(Diff revision 1)
> +SERVO_BINDING_FUNC(Servo_ComputedValuesMatchNoRules, bool,
> +                   ServoComputedValuesBorrowed computed_values)

The naming of this will need to change after bug 1296173.

::: layout/style/nsStyleContext.h:657
(Diff revision 1)
> +         * This allows having the parent style struct cache correctly   \
> +         * initialized to handle restyle hints, that is, prevents us    \
> +         * having to compute the style difference for nodes that match  \
> +         * no rules.                                                    \

What does this have to do with restyle hints? Do you mean change hints?

It seems like we should describe the caching short-circuit mechanism in CalcStyleDifference in more detail here.

::: layout/style/nsStyleContext.h:662
(Diff revision 1)
> +         * Note that adding the inherit bit is ok, because the struct   \
> +         * pointer returned by the parent and the child is owned by     \
> +         * Servo.                                                       \

Well, it's owned by the StyleContextSource, in this case the parent's StyleContextSource. So this could leave us with a dangling pointer if we reparented style contexts and didn't update the pointers directly. Though I suppose it wouldn't be as long as the assertion here actually holds.

I guess this issue will go away when heycam lands bug 1188721. Still probably worth explaining a bit more in the comment.
Comment on attachment 8783241 [details]
Bug 1296556: Don't store change hints for non-elements.

https://reviewboard.mozilla.org/r/73148/#review71166

::: layout/style/nsStyleContext.h:537
(Diff revision 1)
>    // This is not the case right now, since the changes of childs of frames that
>    // go through frame construction are not consumed.
>    void StoreChangeHint(nsChangeHint aHint)
>    {
>      MOZ_ASSERT(!IsShared());
> -    mStoredChangeHint |= aHint;
> +    mStoredChangeHint = aHint;

Can we assert mConsumedChangeHint now to prevent us storing multiple change hints?
Also, what value do we get by hitting the parent for any node which matched no rules, as opposed to text nodes specifically? IIUC this is only valuable because it enables us to skip change hint computation on text nodes. Isn't this just going to cause us to generate unnecessary change hints for ancestor elements who experience style changes that don't affect layout?
Flags: needinfo?(ealvarez)
Hmm, good question. I assumed that we wanted to mimic Gecko's behavior here, though you're right for elements that match no rules it seems we can do better.

That being said, we can't reach the content node from the source, so we can record it some other way, via a bit for example, but then we need to know in Servo_InheritComputedValues whether we're inheriting a text node or not, and I'm not sure if the complexity is worth it.

Sincerely, I don't think there are too many elements that match no rules at all (maybe custom elements and such?) that this extra complexity could be worth it.
Flags: needinfo?(ealvarez)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Hmm, good question. I assumed that we wanted to mimic Gecko's behavior here,
> though you're right for elements that match no rules it seems we can do
> better.

Does Gecko trigger style struct resolution on the parent style context in the no rules case? That would mean we're not regression anything with this, but we should take whatever wins we could get.

> That being said, we can't reach the content node from the source, so we can
> record it some other way, via a bit for example, but then we need to know in
> Servo_InheritComputedValues whether we're inheriting a text node or not, and
> I'm not sure if the complexity is worth it.

Can't we just test the mozText anon box on the style context? The machinery to detect matches-no-rules seems like more complexity, unless we need it for something else.

> Sincerely, I don't think there are too many elements that match no rules at
> all (maybe custom elements and such?) that this extra complexity could be
> worth it.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> Can't we just test the mozText anon box on the style context? The machinery
> to detect matches-no-rules seems like more complexity, unless we need it for
> something else.

Oh, crap, totally forgot about the mPseudoTag in the style context. You're right, updating this ASAP.
Comment on attachment 8783239 [details]
Bug 1296556: Reach the parent to compute inherited style structs.

https://reviewboard.mozilla.org/r/73144/#review71164

> The naming of this will need to change after bug 1296173.

No FFI call anymore.

> What does this have to do with restyle hints? Do you mean change hints?
> 
> It seems like we should describe the caching short-circuit mechanism in CalcStyleDifference in more detail here.

Yep, I tried to describe it a bit more.

> Well, it's owned by the StyleContextSource, in this case the parent's StyleContextSource. So this could leave us with a dangling pointer if we reparented style contexts and didn't update the pointers directly. Though I suppose it wouldn't be as long as the assertion here actually holds.
> 
> I guess this issue will go away when heycam lands bug 1188721. Still probably worth explaining a bit more in the comment.

I commented on why it's fine if the assertion doesn't hold too (for our case).
Bobby requested me to keep track of this Servo patch in this bug, just in case it's needed for other stuff, since it could be useful.
Comment on attachment 8783239 [details]
Bug 1296556: Reach the parent to compute inherited style structs.

https://reviewboard.mozilla.org/r/73144/#review72602

::: layout/style/nsStyleContext.h:654
(Diff revision 2)
> -          Servo_GetStyle##name_(mSource.AsServoComputedValues());       \
> +         * Reach the parent to grab the inherited style struct if       \
> +         * we're sure we don't match any rules (e.g., if we're a text   \

Since we're now making this work only for text frames, can you update the comment so it doesn't mention nodes matching no rules?

::: layout/style/nsStyleContext.h:658
(Diff revision 2)
> +         * This allows having the parent style struct cache correctly   \
> +         * initialized to compute change hints, and so, prevents us     \
> +         * having to compute the style difference for nodes that match  \
> +         * no rules. CalcStyleDifference skips some style structs if    \

It took me a bit to grok this first sentence.  How about this:

This causes the parent element's style context to cache any inherited structs we request for a text node, which means we don't have to compute change hints for the text node, as handling the change on the parent element is sufficient.

::: layout/style/nsStyleContext.h:668
(Diff revision 2)
> +         * Note that adding the inherit bit is ok, because the struct   \
> +         * pointer returned by the parent and the child is owned by     \
> +         * Servo. This is fine if the pointers are the same (as it      \
> +         * should, read below), because both style context sources will \
> +         * hold it.                                                     \

I wonder if, instead of caching on this style context the struct that we got from the parent, and warning if they are not the same, that we just do:

  mParent->Style##name_();
  newData = Servo_GetStyle##name_(...);

Then if we happened to get different structs, we won't run into any ownership issues.

::: layout/style/nsStyleContext.h:674
(Diff revision 2)
> +         * In the case of a mishandled frame, we could end up with the  \
> +         * pointer to and old parent style, but that's fine too, since  \
> +         * the parent style context will remain alive until we reframe, \
> +         * in which case we'll discard both style contexts.             \

What's a mishandled frame in this context?  Is it the anonymous table wrappers you mention in the next paragraph?

::: layout/style/nsStyleContext.h:679
(Diff revision 2)
> +         * Also, note that the assertion below should be true, except   \
> +         * for some frames we still don't handle correctly, like        \
> +         * anonymous table wrappers, in which case the pointers will    \
> +         * differ.                                                      \

Do the pointers differ but we're guaranteed to have the same values in them?  I don't quite understand this situation.

::: layout/style/nsStyleContext.h:684
(Diff revision 2)
> +         * That means we're not going to restyle correctly text frames  \
> +         * of anonymous table wrappers, for example. It's kind of       \
> +         * embarrassing, but I think it's not worth it to add more      \
> +         * logic here unconditionally, given that's going to be fixed.  \

What would be the solution here?  Loop up caching structs on style context until we find a non-anonymous box style context?

::: layout/style/nsStyleContext.h:693
(Diff revision 2)
> +         * TODO(emilio): Maybe look for mozOtherNonElement too? That's  \
> +         * used by ::first-line and placeholder frames, but it seems we \
> +         * should be safe here (assuming we reconstruct the style       \
> +         * contexts correctly for those frames), since the actual frame \
> +         * used for layout is the text or out of flow one.              \

Do I remember correctly that dbaron mentioned the actual style of placeholder frames doesn't matter at all?
Comment on attachment 8783240 [details]
Bug 1296556: Recreate style contexts inside generated content.

https://reviewboard.mozilla.org/r/73146/#review72604

In the commit message:

> I wonder if we could just reparent them and remove the cache with the following
> patches... It *should* work.

Which cache and following patches are you referring to?  Reparenting style context is basically just re-resolving a style context without running selector matching.  But ResolveStyleForText already doesn't run selector matching, so I don't think there's any need to try reparenting here.

r=me with that message removed or clarified, and looping over content.

::: layout/base/ServoRestyleManager.cpp:180
(Diff revision 2)
> +            for (nsIFrame* frame : pseudoFrame->PrincipalChildList()) {
> +              if (frame->GetContent()->IsNodeOfType(nsINode::eTEXT)) {
> +                RefPtr<nsStyleContext> childContext =
> +                  aStyleSet->ResolveStyleForText(frame->GetContent(),
> +                                                 pseudoContext);
> +                frame->SetStyleContext(childContext);
> +              }
> +            }

Is pseudoFrame->GetContent() the <_moz_generated_content_before/after> element?  If so, can we just iterate its children rather than go through the frames?  That would let us consistently work on the DOM tree rather than sometimes use the frame tree.
Comment on attachment 8783240 [details]
Bug 1296556: Recreate style contexts inside generated content.

https://reviewboard.mozilla.org/r/73146/#review72606
Attachment #8783240 - Flags: review?(cam) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Sincerely, I don't think there are too many elements that match no rules at
> all (maybe custom elements and such?) that this extra complexity could be
> worth it.

I think <span> elements match no rules, so it might not be that uncommon.
Comment on attachment 8783241 [details]
Bug 1296556: Don't store change hints for non-elements.

https://reviewboard.mozilla.org/r/73148/#review72608

r=me with that.

::: layout/style/nsStyleContext.h:538
(Diff revision 2)
>    // This is not the case right now, since the changes of childs of frames that
>    // go through frame construction are not consumed.
>    void StoreChangeHint(nsChangeHint aHint)
>    {
>      MOZ_ASSERT(!IsShared());
> -    mStoredChangeHint |= aHint;
> +    mStoredChangeHint = aHint;

Can we restore the assertions that check whether we only store one nsChangeHint value before consumign it?
Attachment #8783241 - Flags: review?(cam) → review+
Comment on attachment 8783239 [details]
Bug 1296556: Reach the parent to compute inherited style structs.

https://reviewboard.mozilla.org/r/73144/#review72688

::: layout/style/nsStyleContext.h:668
(Diff revision 2)
> +         * Note that adding the inherit bit is ok, because the struct   \
> +         * pointer returned by the parent and the child is owned by     \
> +         * Servo. This is fine if the pointers are the same (as it      \
> +         * should, read below), because both style context sources will \
> +         * hold it.                                                     \

Well, I don't think we run into any ownership issue in general, because the style context is kept alive by the parent, and to kill it, you need to kill the owner frame, which if I'm not wrong also implies taking down this one, right?
Comment on attachment 8783239 [details]
Bug 1296556: Reach the parent to compute inherited style structs.

https://reviewboard.mozilla.org/r/73144/#review72688

> Well, I don't think we run into any ownership issue in general, because the style context is kept alive by the parent, and to kill it, you need to kill the owner frame, which if I'm not wrong also implies taking down this one, right?

Yes, I think you're right that we're safe here.  The child style context holds a strong reference to its parent too.
Comment on attachment 8783239 [details]
Bug 1296556: Reach the parent to compute inherited style structs.

https://reviewboard.mozilla.org/r/73144/#review73886

r=me with the other comments addressed.
Attachment #8783239 - Flags: review?(cam) → review+
Comment on attachment 8783241 [details]
Bug 1296556: Don't store change hints for non-elements.

https://reviewboard.mozilla.org/r/73148/#review74236

::: layout/style/nsStyleContext.h:538
(Diff revision 2)
>    // This is not the case right now, since the changes of childs of frames that
>    // go through frame construction are not consumed.
>    void StoreChangeHint(nsChangeHint aHint)
>    {
>      MOZ_ASSERT(!IsShared());
> -    mStoredChangeHint |= aHint;
> +    mStoredChangeHint = aHint;

No, we can't because we don't consume it when reframing. If we stop generating change hints after ReconstructFrame is generated, we can re-add that assertion.
Comment on attachment 8783239 [details]
Bug 1296556: Reach the parent to compute inherited style structs.

https://reviewboard.mozilla.org/r/73144/#review72602

> Since we're now making this work only for text frames, can you update the comment so it doesn't mention nodes matching no rules?

Sure, good catch :)

> It took me a bit to grok this first sentence.  How about this:
> 
> This causes the parent element's style context to cache any inherited structs we request for a text node, which means we don't have to compute change hints for the text node, as handling the change on the parent element is sufficient.

Of course, I suck at English ;-)

> I wonder if, instead of caching on this style context the struct that we got from the parent, and warning if they are not the same, that we just do:
> 
>   mParent->Style##name_();
>   newData = Servo_GetStyle##name_(...);
> 
> Then if we happened to get different structs, we won't run into any ownership issues.

That will of course also work, but I don't know if we want to do it. Basically, I think the ownership thing is tricky, but not problematic in any way, since it's only for text nodes. I could be wrong, but the parent style context is holded alive by the parent frame, and taking down the parent implies taking down this frame too, right?

> What's a mishandled frame in this context?  Is it the anonymous table wrappers you mention in the next paragraph?

Yeah, it's basically a frame we've missed to update with the style context.

> Do the pointers differ but we're guaranteed to have the same values in them?  I don't quite understand this situation.

No, the only part when the assertion doesn't hold is when we've missed to update a parent frame, that's what I'm trying to write, maybe that way is clearer?

> What would be the solution here?  Loop up caching structs on style context until we find a non-anonymous box style context?

The solution would be properly update the style context of anonymous boxes, which we don't as of right now.

> Do I remember correctly that dbaron mentioned the actual style of placeholder frames doesn't matter at all?

Yep, this was the reason I asked that question in the meeting, but this comment is from before that.
(Some of the comments above were old and hadn't been published, please ignore them)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1bb4fec09264
Reach the parent to compute inherited style structs. r=heycam
https://hg.mozilla.org/integration/autoland/rev/774608c17570
Recreate style contexts inside generated content. r=heycam
https://hg.mozilla.org/integration/autoland/rev/54394c32a106
Don't store change hints for non-elements. r=heycam
You need to log in before you can comment on or make changes to this bug.