Try to simplify our ::first-line implementation

NEW
Assigned to

Status

()

defect
P3
normal
Last year
6 days ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Depends on 1 bug, Blocks 10 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

It's a continuous cause of headache.
Depends on: 1465478
Blocks: 1452214
Depends on: 1465502
Depends on: 1465511
Priority: -- → P3
Full of correctness issues and inconsistencies of course, given it's 4AM+, but wanted some early feedback...

Mats, Boris, Jonathan, what do you think of making the first-line code work this way instead of the way it currently does?

Obviously better abstractions / more resiliency to errors is needed near nsTextFrame / PropertyProvider and nsLineLayout, but I think an approach like this could work.

The issue could be making the first-line style application very patchy /
inconsistent, but I think most of the first-line properties are used during line
layout and painting in nsTextFrame, and that it should be doable to make it not
prone to error.

This obviously needs a lot more work, but I'd be very excited about not having to keep most of the style reparenting code and nsFirstLineFrame around, and it would fix a fair amount of not-pretty issues we have right now with ::first-line making bogus assumptions.

Would you be ok with such an approach assuming I invest time it making it cleaner, less patchy, and pass the existing first-line tests?

Thanks!
Attachment #8982098 - Flags: feedback?(mats)
Attachment #8982098 - Flags: feedback?(jfkthame)
Attachment #8982098 - Flags: feedback?(bzbarsky)
Attachment #8982098 - Attachment description: Rough prototype that makes it work. → Rough prototype that makes 'color' / '-webkit-text-fill-color' work.
So, to elaborate a bit more on the model: nsIFrame::StyleForFirstLine() would return the style as-if it was on the first-line. That's what should be used for line layout and such. Afterwards, callers are responsible of looking at the style or the first-line style, either based on mLineNumber during line layout, or the bit we set there during painting.

I _think_ it can work, and the amount of "is in first-line" information that we'd need to pass around would not be huge. But all of you have way more knowledge about line layout and such to weigh in here.

I'm still trying to make sense of which functions are called when... Basically, anything that interacts with first-line properties before PlaceLine would need to carry the state around, which includes the pref sizing and such I assume.

I suspect the result would be much nicer than what we have now...
This would have the side effect of changing how some things work in presence of, e.g., font-size changes.

For example in this test-case:

<!doctype html>
<style>
  div::first-line {
    color: yellow;
    font-size: 100px;
    background: blue;
  }

  span {
    padding: 1em;
    background: green;
  }
</style>
<div>
  I am a <span>first-line</span>, hoorray!<br>
  And I'm not. boo :(
</div>


Right now the padding of the <span> is computed relative to the 100px font-size. I also only plan to handle inlines, I think, otherwise it gets very out of hand.

But given how Chrome handles this:

<!doctype html>
<style>
  div::first-line {
    color: yellow;
    font-size: 100px;
    background: blue;
  }

  span {
    padding: 1em;
    background: green;
    display: inline-block;
  }
</style>
<div>
  I am a <span>first-line</span>, hoorray!<br>
  And I'm not. boo :(
</div>

I don't think it matters much. We'd handle it better too, I suspect.

I think this is technically regressing some of these complex use-cases, but I think it's the right call.
Edge also doesn't resolve padding correctly in the first case... It seems to support inline-blocks decently though.

This is all so broken... Per discussion with bz I think I want to try this and get the CSSWG to simplify the first-line spec...
Blocks: 1421053, 1422838
Getting rid of ::first-line frames seems like a good idea.
I also think that while we're redesigning this we should make it
work for nested blocks and such that the spec now requires but
that we haven't implemented yet (bug 317081 etc).

> aIsFirstLine ? aFrame->StyleForFirstLine() : *aFrame->Style()

I assume you suggest we do that before accessing any computed value
that applies to ::first-line [1], or could potentially be affected by it,
everywhere?  Hmm, isn't that pretty much all properties ;-)
Even with an accessor that seems a bit tedious and error prone...

I wonder if it would be simpler to do the ComputedStyle reparenting
while setting up the ReflowInput instead?  By passing down an
"inside-first-line" bit recursively as we go...
Ditto for GetMinISize/GetPrefISize.

That way we can leave all consumers of computed values as is and just
assume that Style() was already reparented if needed.

[1] https://drafts.csswg.org/css-pseudo-4/#first-line-styling
Attachment #8982098 - Flags: feedback?(mats)
> I wonder if it would be simpler to do the ComputedStyle reparenting
> while setting up the ReflowInput instead?

What happens on color-only changes?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> What happens on color-only changes?

Hmm, yeah, I guess we'd have to request reflow rather than repaint
for frames that are marked as "inside-first-line".
Probably not much of a perf issue in practice.
Actually, color-only changes can't change any frame's "inside-first-line"
state so repaint should just work.  So I guess I'm not following - can
you elaborate?
(In reply to Mats Palmgren (:mats) from comment #8)
> Actually, color-only changes can't change any frame's "inside-first-line"
> state so repaint should just work.  So I guess I'm not following - can
> you elaborate?

Sure, but that means that we need to keep the current style-system frame-reparenting code, which is part of the issue here...

(In reply to Mats Palmgren (:mats) from comment #5)
> Getting rid of ::first-line frames seems like a good idea.
> I also think that while we're redesigning this we should make it
> work for nested blocks and such that the spec now requires but
> that we haven't implemented yet (bug 317081 etc).

This approach would allow that conceptually, but with all the gotchas already mentioned of course.

> > aIsFirstLine ? aFrame->StyleForFirstLine() : *aFrame->Style()
> 
> I assume you suggest we do that before accessing any computed value
> that applies to ::first-line [1], or could potentially be affected by it,
> everywhere?  Hmm, isn't that pretty much all properties ;-)

Well, only properties that have APPLIES_TO_FIRST_LINE, fwiw.

> Even with an accessor that seems a bit tedious and error prone...

Well, yes, to some extent.

I do plan on adding an accessor to use once the frame knows it's on the first-line... In general I don't think implementing the spec is super-practical, tbh (that's why nobody implements it properly).

I planned to look at first-line styles mostly from line layout / nsTextFrame / nsInlineFrame / etc, and then convince the working group to be sane about ::first-line.

> I wonder if it would be simpler to do the ComputedStyle reparenting
> while setting up the ReflowInput instead?  By passing down an
> "inside-first-line" bit recursively as we go...
> Ditto for GetMinISize/GetPrefISize.

I'm in general very skeptic about setting the frame's style pointer the same way we're doing it. That kind of reparenting is what causes the security issues this work is trying to fix... Reparenting that way means that we need to do style system work afterwards to re-set the styles to the correct thing (instead of just invalidating them as this patch does)... I'll think a bit more about it...
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> I planned to look at first-line styles mostly from line layout / nsTextFrame
> / nsInlineFrame / etc, and then convince the working group to be sane about
> ::first-line.

Of course doing this would be a regression in our ::first-line support.

But given the state of the feature everywhere I'm not really concerned about it.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Sure, but that means that we need to keep the current style-system
> frame-reparenting code, which is part of the issue here...

No, what I suggested doesn't require *frame*-reparenting, or first-line
frames at all.  I was talking about the reparenting that
ReparentComputedStyle does.  So what I suggest is, for a DOM like so:
<div><span>x
we get a frame tree like so:
Block(div)
  Inline(span)
    Text("x")
and a ComputedStyle tree like so:
div
  div::first-line
    span
      ::text

whether the span's ComputedStyle is parented to div or div::first-line
depends on whether it's on the first line or not.

With the above ComputedStyle tree I would expect color changes to just
require a repaint.  The styling would work the same as how it works
for display:contents nodes.
That wouldn't work.  In a frame tree like you have, the span should, per spec, inherit the values of properties allowed on first-line from the first-line, and the rest from the div.

In our impl, it inherits default-inherited stuff from first-line and the rest from the div.

In particular, the idea is to not get into trouble when the span has "float: inherit" or some such.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Well, only properties that have APPLIES_TO_FIRST_LINE, fwiw.

I don't think that's enough, as your "font-size affects padding"
demonstrates.  I suspect that a ::first-line style may indeed
affect the value of most properties on all descendants.
So when accessing the computed value of padding we need to
provide the "inside-first-line" state since internally we
need to at some point resolve '1em' using the correct style.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> In our impl, it inherits default-inherited stuff from first-line and the
> rest from the div.

Could we make "all:inherit" (or something to that effect) the default
on a ::first-line ComputedStyle?
> Could we make "all:inherit" (or something to that effect) the default on a ::first-line ComputedStyle?

How would that help?
Hmm, yeah, "all:inherit" doesn't work.

I still don't see why a "div::first-line" ComputedStyle is any different
from what we're currently doing with a first-line frame from a style
inheritance point of view...  I mean, we could set it up *exactly*
the way nsFirstLineFrame's ComputedStyle is today and it would
indistinguishable from the <span>'s POV, no?

I thought the main problem we wanted to solve here was to get rid
of nsFirstLineFrame (to simplify frame construction and whatnot).
Are there other issues too?
> I mean, we could set it up *exactly* the way nsFirstLineFrame's ComputedStyle is today and it would
> indistinguishable from the <span>'s POV, no?

Sure.  The question is how the span gets its ComputedStyle.

> Are there other issues too?

Yes.  The issue is when and how the ComputedStyles of the things on the first line are updated.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> Sure.  The question is how the span gets its ComputedStyle.

OK, then I think propagating an "is-inside-first-line" state to
ReflowInput / GetMin/PrefISize that calls ReparentComputedStyle
as needed could work.

> > Are there other issues too?
> 
> Yes.  The issue is when and how the ComputedStyles of the things on the
> first line are updated.

If a frame is pushed from the first line to the next, we'll need to
undo the ComputedStyle reparenting of course, but that seems like
an easy problem to solve.

Other than that, I'll leave it to emilio to solve any stylo issues :-p
(In reply to Mats Palmgren (:mats) from comment #11)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> > Sure, but that means that we need to keep the current style-system
> > frame-reparenting code, which is part of the issue here...
> 
> No, what I suggested doesn't require *frame*-reparenting, or first-line
> frames at all.  I was talking about the reparenting that
> ReparentComputedStyle does.

Sure, what I meant is that this does require keeping our walk over the frame tree to reparent styles, which itself is the only thing that keeps alive all the logic from nsIFrame::GetParentComputedStyle / CorrectStyleParentFrame / etc, which is a whole lot of code that has a single caller right now and is prone to getting outdated and bogus.
OK, yeah, that'd be nice, but it remains to seen whether the solution
you suggest is less error prone.  A radical way to solve it would be
to always propagate the ComputedStyle via a parameter everywhere,
and/or as a member on ReflowInput.  Then, only nsBlockFrame needs
to deal with choosing the right one based on the first-line state.
The first patch in that series should probably rename or remove
nsIFrame::Style() to ensure where not accidentally using it anywhere
where we should have used StyleForFirstLine().
(In reply to Mats Palmgren (:mats) from comment #13)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> > Well, only properties that have APPLIES_TO_FIRST_LINE, fwiw.
> 
> I don't think that's enough, as your "font-size affects padding"
> demonstrates.  I suspect that a ::first-line style may indeed
> affect the value of most properties on all descendants.
> So when accessing the computed value of padding we need to
> provide the "inside-first-line" state since internally we
> need to at some point resolve '1em' using the correct style.

Right, I think it's important we don't regress this. I realize there's a sad lack of browser-interop in this area at the moment, but it seems clear to me that our existing behavior in a case like

  data:text/html,<style>div::first-line { font-size:3em;background: silver; }
                        span { padding:0 .25em; background:yellow; color:red; }
                 </style><div>I am a <span>first-line</span>, hooray!<br>And I am <span>not</span>, boo.</div>

is the correct thing to do (from an author-expectations POV), and I'd regard the webkit/blink rendering as a bug.

(I was curious what Edge would do, but it turns out to be even buggier in this area so it's not much use as a guide. See https://codepen.io/anon/pen/wXKmEy for an example. It's perhaps worth noting, though, that in the one case where its rendering of the first-line <span> is at all reasonable -- the middle of the three examples in the codepen -- it does compute the padding based on the first-line's font-size.)
I'm sympathetic to that feeling, but on the other hand I see no intention from other browsers to fix their first-line implementation, and ours gets in the way more often than not...

Personally, I'd rather simplify the spec and our implementation than waiting for longstanding issues in other browsers to be addressed.
I strongly disagree with that sentiment.  The rendering in Chrome
and Edge is clearly just wrong.  It makes no sense whatsoever from
an authoring POV.  We shouldn't standardize obvious bugs just
because the largest market share of UAs hasn't implemented the spec
correctly yet.

Fwiw, I couldn't find a Chromium bug that covers this issue, so I filed
https://bugs.chromium.org/p/chromium/issues/detail?id=849013
Emilio, I'm not going to get to this until after I get back...
Comment on attachment 8982098 [details] [diff] [review]
Rough prototype that makes 'color' / '-webkit-text-fill-color' work.

Yeah, no worries, we've discussed all this in person, so the details of this WIP are not that relevant.

Enjoy your PTO! :)
Attachment #8982098 - Flags: feedback?(bzbarsky)
Blocks: 1469354
Getting this on the layout perf docket given the impact on the testcases in bug 1385658. Emilio says that style attribute sets will get faster since we won't need to clone declaration blocks everywhere.

If this doesn't improve any of our existing style attribute benchmarks we should write a new one, and get that (along with the testcases in bug 1385658) checked into automation.
Summary: Try to simplify our ::first-line implementation. → Try to simplify our ::first-line implementation
Edge removed their inline-block in ::first-line stuff in their latest preview, for the record.
Blocks: 317081
Note: we've got a comment in nsFrameStateBits saying that (until recently) there were 2 unused bits, and to please check in here before using them.

Bug 1465474 used one of them (bit #44):
https://hg.mozilla.org/mozilla-central/rev/3adb1922d88d#l5.12
...and I believe bug 1159042 "part 5" is going to use to use the other one (bit #45).

Is that OK?  It looks like there's nothing about-to-land here, so if we were saving those bits for use in this bug (I'm guessing), then perhaps we can create / free up some other state bits by the time this is landable?
Flags: needinfo?(emilio)
Yeah, that's fine.

Too bad I spent a bit of time removing state bits just to have some free for this bug in bug 1465502 and such... :P

Oh well, as you've noticed I'm not actively working on this right now so it sounds fine :)
Flags: needinfo?(emilio)
(In reply to Daniel Holbert [:dholbert] from comment #29)
> Bug 1465474 used one of them (bit #44):

Just FTR, that was actually bug 1421105.
Blocks: 1509095
Blocks: 1513282
Blocks: 1523134
Blocks: 1564945
You need to log in before you can comment on or make changes to this bug.