Open Bug 1465474 Opened 6 years ago Updated 1 year ago

Try to simplify our ::first-line implementation

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

People

(Reporter: emilio, Assigned: emilio)

References

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

Details

Attachments

(1 file)

Rough prototype that makes 'color' / '-webkit-text-fill-color' work.
31.98 KB, patch
emilio
: feedback?
jfkthame
Details | Diff | Splinter Review
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
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.
Blocks: 1385658, layout-perf
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
Blocks: 1625187
Severity: normal → S3
See Also: → 1809675
See Also: → 1841128
No longer blocks: 1452214, 1469354, 1513282
See Also: → 1452214, 1469354, 1513282
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: