Implement text-decoration-inset painting in Blink [chromium/src : main]
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
const gfx::PointF start_point = gfx::PointF(local_origin_) +
gfx::Vector2dF(line_left_adjust, line_offset);
DecorationGeometry geometry = DecorationGeometry::Make(
style,
gfx::RectF(start_point,
gfx::SizeF(inset_width, decoration.resolved_thickness)),I think it would make more sense if this did:
```
gfx::RectF decoration_rect(...);
ApplyInsets(decoration.applied_text_decoration, ..., decoration_rect);
```
Then it could trivially return and do nothing if the `*-inset` property is a no-op (which should be the common case). (It also avoids reducing the readability of this function.)
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
Helmut JanuschkaI think the overall approach is good. I have a few requests:
- Please split into two CLs. One to add the style code and the flag (to the point of creating AppliedTextDecoration data on the style), and then another for the painting portions. Then a style expert could review the style changes.
- Please add someone specializing in inline layout to the painting CL to review the code related to fragment matching/searching. I do not trust myself to see issues there. Maybe tkent@, maybe ikilpatrick@, maybe koji@.
- Please file a bug for ink overflow and implement it in a separate CL. When the insets are negative and extend beyond the text width the overflow will need to account for that. You'll also need tests that add/remove decorations to verify that the repaint is correct. There are existing tests for custom highlights that do this, IIRC.
done, split into two CLs: crrev.com/c/7993418 is the style/flag part, this CL is now painting only. for ink overflow this CL only does conservative bounds (negative insets expand, positive ones never shrink), filed crbug.com/539095434 for the exact overflow + repaint test work, crrev.com/c/8148940 chained on top of this one (also makes text-decoration-inset-028/030 pass). ptal
Stephen Chenney@sche...@chromium.org anything i can do one my end?
do you have anyone in mind who could otherwise do the review?
Helmut JanuschkaTake a look at the first comment from Patchset 14. I think this should be split in 2 CLs, and I listed a couple of people who could review the text fragment logic.
Someone on the style team could review the first CL and I can do the second one.
Done
const gfx::PointF start_point = gfx::PointF(local_origin_) +
gfx::Vector2dF(line_left_adjust, line_offset);
DecorationGeometry geometry = DecorationGeometry::Make(
style,
gfx::RectF(start_point,
gfx::SizeF(inset_width, decoration.resolved_thickness)),I think it would make more sense if this did:
```
gfx::RectF decoration_rect(...);
ApplyInsets(decoration.applied_text_decoration, ..., decoration_rect);
```
Then it could trivially return and do nothing if the `*-inset` property is a no-op (which should be the common case). (It also avoids reducing the readability of this function.)
- Fredrik Söderquist
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 12 comments![]( "Open in Gerrit")
void TextDecorationInfo::ApplyDecorationInsets(
const ResolvedDecoration& decoration,
gfx::RectF& decoration_rect) const {So now it looks quite obvious that this is invariant "per-index", so this could be resolved during `ResolveDecorationAt()` and then just (trivially) applied when computing a specific line.
if (!applied_text_decoration) {
return;
}Should always be non-null.
if (inset.GetStart().IsZero() && inset.GetEnd().IsZero()) {It'd probably be pertinent to check this case first, since it's expect to be true most of the time.
InlineCursor cursor = inline_context_->CursorForDescendantsOfLine();
bool found_item = cursor && cursor.TryMoveTo(text_item);
if (!found_item && cursor) {
found_item = TryMoveToMatchingTextFragment(cursor, text_item,
IsStrictTextFragmentMatch);
}
if (!found_item && cursor) {
// Fallback for cases where the strict fragment identity diverges across
// cursor roots. Prefer node+offset matching over giving up entirely.
found_item = TryMoveToMatchingTextFragment(cursor, text_item,
IsLooseTextFragmentMatch);
}The `TextFragmentPainter` should have an `InlineCursor` already - could we just use that one instead of trying to re-create it? It might make sense to resolve all of this at that level so that we don't have to recompute it several times.
if (inline_context_ &&
NeedsNeighborFragmentsForDecorationInset(style, decoration_override)) {
InlineCursor cursor = inline_context_->CursorForDescendantsOfLine();
bool found_item = cursor && cursor.TryMoveTo(text_item);
if (!found_item && cursor) {
found_item = TryMoveToMatchingTextFragment(cursor, text_item,
IsStrictTextFragmentMatch);
}
if (!found_item && cursor) {
// Fallback for cases where the strict fragment identity diverges across
// cursor roots. Prefer node+offset matching over giving up entirely.
found_item = TryMoveToMatchingTextFragment(cursor, text_item,
IsLooseTextFragmentMatch);
}
if (found_item) {
// Use a cursor rooted at the full containing block so that
// MoveTo{Previous,Next}InlineLeafOnLine() is line-scoped.
InlineCursor line_cursor = cursor;
line_cursor.ExpandRootToContainingBlock();
if (line_cursor.TryMoveTo(*cursor.CurrentItem())) {
InlineCursor previous_cursor = line_cursor;
previous_cursor.MoveToPreviousInlineLeafOnLine();
if (previous_cursor.CurrentItem() &&
previous_cursor.CurrentItem()->IsText() &&
!previous_cursor.CurrentItem()->IsLineBreak()) {
previous_fragment_on_line = previous_cursor.CurrentItem();
}
InlineCursor next_cursor = line_cursor;
next_cursor.MoveToNextInlineLeafOnLine();
if (next_cursor.CurrentItem() && next_cursor.CurrentItem()->IsText() &&
!next_cursor.CurrentItem()->IsLineBreak()) {
next_fragment_on_line = next_cursor.CurrentItem();
}
}
}
}This looks like it could be broken out into a separate function and maybe return a struct with all the relevant data bits (that then an be forwarded to the `TextDecorationInfo` constructor). Passing a struct would also make the constructor a bit less unwieldy (it's already quite parameter-heavy).
body { margin: 0; background: white; }This is unnecessary.
svg { display: block; }...and so is this.
<svg width="420" height="100" viewBox="0 0 420 100">And this. (If it differed from width/height it might be relevant.)
<style>
body { margin: 0; background: white; }
svg { display: block; }
text {
font: 42px/1 monospace;
fill: black;
text-decoration-line: underline;
text-decoration-color: black;
text-decoration-thickness: 3px;
text-decoration-inset: -12px;
}
</style>
<svg width="420" height="100" viewBox="0 0 420 100">Ditto here.
This test seems unrelated to this feature. It's also not particularly interesting (there should be a bug for this somewhere).
Related details
- Helmut Januschka
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |