Fix SVG text selection colors [chromium/src : main]
Stephen Chenney (Gerrit)
Stephen Chenney voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Chenney
| Commit-Queue | +1 |
1 comment
This is a mess, but the best solution I could come up with. Note the rebaselines are due to (a) no longer drawing shadows twice when both stroke and fill are used (b) not applying opacity to shadows, matching the regular text path and (c) keeping originating properties when ::selection does not define them. All this matches the HTML behavior, and is not specified for SVG at all.
It's not ready to land because I need more tests.
Related details
- Fredrik Söderquist
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
This is a mess, but the best solution I could come up with. Note the rebaselines are due to (a) no longer drawing shadows twice when both stroke and fill are used (b) not applying opacity to shadows, matching the regular text path and (c) keeping originating properties when ::selection does not define them. All this matches the HTML behavior, and is not specified for SVG at all.
It's not ready to land because I need more tests.
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney added 1 comment![]( "Open in Gerrit")
Ready for review. I've tried to make it more like HTML selection to the extent possible, with the belief that it better represents what authors would like the behavior to be.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 15 comments![]( "Open in Gerrit")
Note the SVG spec is silent on how to treat ::selection applied to SVG text. This CL treats SVG text the same as HTML text to the extent possible. https://svgwg.org/svg2-draft/text.html#TextSelectionLine break
default_value: "SVGPaint::CreateInitialCurrentColor()",This name is misleading, because while the `color` field happens to have that value, the general state of the paint is dictated by the `type` field - which is "none". Given that, does marking this paint have any use?
paint.SetColor(ConvertStyleColor(state, *local_value));The `SVGPaint` will never have the initial flag set here, which makes this unnecessary. Keeping the setting of flag to the special `Create...()` functions should also aid with that (maybe it should be documented).
const UnzoomedLength* length_override = nullptr);`stroke_width_override`?
if (length_override) {
stroke_data.SetThickness(
ValueForLength(*length_override, viewport_resolver));
} else {
stroke_data.SetThickness(
ValueForLength(style.StrokeWidth(), viewport_resolver));
}I think I would prefer this as:
```
const UnzoomedLength& effective_stroke_width =
length_override ? *length_override : style.StrokeWidth();
stroke_data.SetThickness(ValueForLength(effective_stroke_width, ...));
```
const ShadowList* selection_shadows =
GetTextShadows(selection_style, layout_object);
if (selection_shadows) {
const ShadowList* text_shadows = GetTextShadows(style, layout_object);
if (text_shadows) {
for (auto& shadow : text_shadows->Shadows()) {
merged_shadow_list.push_back(shadow);
}
}
for (auto& shadow : selection_shadows->Shadows()) {
merged_shadow_list.push_back(shadow);
}
}Why do this here and not when it's needed? This seems to just give `SelectionStyleScope` additional responsibilities.
!pseudo_selection_style->FillPaint().IsInitial();Should context paints be allowed? Paint-servers?
// If the ::selection has fill: none or stroke: none we need to respect that,
// and not fall back to the originating element.
if (have_selection_fill &&
!SVGObjectPainter::HasFill(*pseudo_selection_style, context_paints)) {
have_fill = have_selection_fill = false;
}I think this could be handled in a more logical way by just trying to apply the paint (if it is non-initial).
// If the selection stroke is not visible we still do not want to draw it,
// but we do want to fall back to the originating stroke.This seems to be making the assumption that a `stroke-width` won't be set to 0 in a selection style? Given that the initial stroke width is 1, if the selection style contains a stroke width of 0, it seems we should just apply that? Maybe this means that we should be tracking if stroke-width is initial too though?
have_selection_fill = false; // Use the originating fill instead.Given that we've handled `none` already, this would only happen for missing/broken paint-servers - I don't see why we'd drop back to originating style in that case (given the restrictions in the spec one might argue that we should just directly fall-back if a paint-server is used).
// Note that ::selection does not allow the SVG stroke properties, so
// use the originating element style. Except for stroke-width.Given how the spec frames this restriction, we should probably just apply all the stroke properties from the selection style (i.e defaults for everything else). There's not really any analog to HTML here because there we only have stroke width.
return paint_order == kPaintOrderFillStrokeMarkers ||
paint_order == kPaintOrderMarkersFillStroke ||
paint_order == kPaintOrderFillMarkersStroke ||
paint_order == kPaintOrderNormal;```suggestion
return PaintOrderArray(paint_order, PaintOrderArray::Type::kNoMarkers)[0] == PT_FILL;
```
Can probably just do that at the single call-site though.
StyleColor color;I think I'd rather just keep this ordered as it was than introduce some semi-hiding. See also note elsewhere.
SVGPaint result = SVGPaint(Color::kBlack);```suggestion
SVGPaint result(Color::kBlack);
```
SVGPaint result = SVGPaint();```suggestion
SVGPaint result;
```
Related details
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney added 16 comments![]( "Open in Gerrit")
Thanks for taking on this complex review.
Note the SVG spec is silent on how to treat ::selection applied to SVG text. This CL treats SVG text the same as HTML text to the extent possible. https://svgwg.org/svg2-draft/text.html#TextSelectionStephen ChenneyLine break
Done
default_value: "SVGPaint::CreateInitialCurrentColor()",This name is misleading, because while the `color` field happens to have that value, the general state of the paint is dictated by the `type` field - which is "none". Given that, does marking this paint have any use?
Right. Initially I left this as it was and considered using an IsNone check instead of IsInitial. The problem then would be that the code can't distinguish "Set by author to none" vs "was initially none". In the former we should respect the author and not stroke the text, but in the latter we should fall back to the originating element value.
So I can rename to CreateInitial to avoid confusion. What do you think?
paint.SetColor(ConvertStyleColor(state, *local_value));The `SVGPaint` will never have the initial flag set here, which makes this unnecessary. Keeping the setting of flag to the special `Create...()` functions should also aid with that (maybe it should be documented).
No sure what you mean here. As I understand it the code right here is the case when some style sets the color, so removing initial is exactly what I want. But I'm not very confident in my understanding.
const UnzoomedLength* length_override = nullptr);Stephen Chenney`stroke_width_override`?
Done
if (length_override) {
stroke_data.SetThickness(
ValueForLength(*length_override, viewport_resolver));
} else {
stroke_data.SetThickness(
ValueForLength(style.StrokeWidth(), viewport_resolver));
}I think I would prefer this as:
```
const UnzoomedLength& effective_stroke_width =
length_override ? *length_override : style.StrokeWidth();
stroke_data.SetThickness(ValueForLength(effective_stroke_width, ...));
```
Done
const ShadowList* selection_shadows =
GetTextShadows(selection_style, layout_object);
if (selection_shadows) {
const ShadowList* text_shadows = GetTextShadows(style, layout_object);
if (text_shadows) {
for (auto& shadow : text_shadows->Shadows()) {
merged_shadow_list.push_back(shadow);
}
}
for (auto& shadow : selection_shadows->Shadows()) {
merged_shadow_list.push_back(shadow);
}
}Why do this here and not when it's needed? This seems to just give `SelectionStyleScope` additional responsibilities.
Yes, I agree it can be moved out.
!pseudo_selection_style->FillPaint().IsInitial();Should context paints be allowed? Paint-servers?
According to the CSS pseudo spec, any valid CSS "stroke" or "fill" is allowed, so yes. I think that allows context_paint but not paint servers. I'm not testing it, however because it seems super unlikely in practice and I have no reason to doubt it would work.
// If the ::selection has fill: none or stroke: none we need to respect that,
// and not fall back to the originating element.
if (have_selection_fill &&
!SVGObjectPainter::HasFill(*pseudo_selection_style, context_paints)) {
have_fill = have_selection_fill = false;
}I think this could be handled in a more logical way by just trying to apply the paint (if it is non-initial).
I added this because tests were failing when the selection style set stroke and fill to none. Without this it painted the originating style rather than the one expected. I'd rather leave it as is.
// If the selection stroke is not visible we still do not want to draw it,
// but we do want to fall back to the originating stroke.This seems to be making the assumption that a `stroke-width` won't be set to 0 in a selection style? Given that the initial stroke width is 1, if the selection style contains a stroke width of 0, it seems we should just apply that? Maybe this means that we should be tracking if stroke-width is initial too though?
This one I was somewhat undecided on. If the author sets a non-visible stroke on the selection I suppose that is still saying something about the stroke, though I think it would likely be an error. Rethinking it, I do believe we should respect a 0 width.
I don't think it's worth tracking initial value on stroke width. It seems super confusing to have the stroke color come from one place and the width from another.
have_selection_fill = false; // Use the originating fill instead.Given that we've handled `none` already, this would only happen for missing/broken paint-servers - I don't see why we'd drop back to originating style in that case (given the restrictions in the spec one might argue that we should just directly fall-back if a paint-server is used).
Good point. I'll change it here and below.
// Note that ::selection does not allow the SVG stroke properties, so
// use the originating element style. Except for stroke-width.Given how the spec frames this restriction, we should probably just apply all the stroke properties from the selection style (i.e defaults for everything else). There's not really any analog to HTML here because there we only have stroke width.
Yeah, let's do that. Means I don't need the stroke width override either.
return paint_order == kPaintOrderFillStrokeMarkers ||
paint_order == kPaintOrderMarkersFillStroke ||
paint_order == kPaintOrderFillMarkersStroke ||
paint_order == kPaintOrderNormal;```suggestion
return PaintOrderArray(paint_order, PaintOrderArray::Type::kNoMarkers)[0] == PT_FILL;
```
Can probably just do that at the single call-site though.
Done
StyleColor color;I think I'd rather just keep this ordered as it was than introduce some semi-hiding. See also note elsewhere.
Done
SVGPaint result = SVGPaint(Color::kBlack);Stephen Chenney```suggestion
SVGPaint result(Color::kBlack);
```
Done
SVGPaint result = SVGPaint();Stephen Chenney```suggestion
SVGPaint result;
```
Done
Related details
- Fredrik Söderquist
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney added 1 comment![]( "Open in Gerrit")
Related details
- Fredrik Söderquist
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
HighlightStyleUtils methods for resolving colors, but this fails becauseI had a look at moving the setup out of `TextPainter`-land (which looks doable) - I think that would be a step towards being able to do this (but there would probably be quite a few steps after that...).
default_value: "SVGPaint::CreateInitialCurrentColor()",Stephen ChenneyThis name is misleading, because while the `color` field happens to have that value, the general state of the paint is dictated by the `type` field - which is "none". Given that, does marking this paint have any use?
Right. Initially I left this as it was and considered using an IsNone check instead of IsInitial. The problem then would be that the code can't distinguish "Set by author to none" vs "was initially none". In the former we should respect the author and not stroke the text, but in the latter we should fall back to the originating element value.
So I can rename to CreateInitial to avoid confusion. What do you think?
Yes, just `CreateInitial()` should work I think - being an analog to the regular constructor but with the "is initial" flag set.
paint.SetColor(ConvertStyleColor(state, *local_value));Stephen ChenneyThe `SVGPaint` will never have the initial flag set here, which makes this unnecessary. Keeping the setting of flag to the special `Create...()` functions should also aid with that (maybe it should be documented).
No sure what you mean here. As I understand it the code right here is the case when some style sets the color, so removing initial is exactly what I want. But I'm not very confident in my understanding.
The `SVGPaint` object here (from line 2861) is one which has the default state of the is initial flag - i.e it is already false - and it should always be false here. So this is always a false -> false transition. It seems that what needs isolation is rather the setting of the flag to true - which is what we have the `CreateInitial...()` helpers for.
!pseudo_selection_style->FillPaint().IsInitial();Stephen ChenneyShould context paints be allowed? Paint-servers?
According to the CSS pseudo spec, any valid CSS "stroke" or "fill" is allowed, so yes. I think that allows context_paint but not paint servers. I'm not testing it, however because it seems super unlikely in practice and I have no reason to doubt it would work.
Well, a context paint could be a paint-server, so we get a form of transitive-ness here. Not allowing paint-servers directly while allowing them indirectly I feel is a bit of a weird situation.
// If the ::selection has fill: none or stroke: none we need to respect that,
// and not fall back to the originating element.
if (have_selection_fill &&
!SVGObjectPainter::HasFill(*pseudo_selection_style, context_paints)) {
have_fill = have_selection_fill = false;
}Stephen ChenneyI think this could be handled in a more logical way by just trying to apply the paint (if it is non-initial).
I added this because tests were failing when the selection style set stroke and fill to none. Without this it painted the originating style rather than the one expected. I'd rather leave it as is.
```
if (pseudo_selection_style &&
!pseudo_selection_style->FillPaint().IsInitial()) {
if (SVGObjectPainter::HasFill(*pseudo_selection_style, ...)) {
...apply paint...
}
} else {
if (SVGObjectPainter::HasFill(style, ...)) {
...apply paint...
}
}
```
which would lend itself to just picking one style or the other:
```
const ComputedStyle& fill_style_to_use =
condition_for_using_selection_style ? selection_style : style;
if (SVGObjectPainter::HasFill(fill_style_to_use, ...)) {
...apply paint...
}
```
This should allow eliminating the "flag soup". I don't think `PreparePaint()` returning false should trigger a fallback.
bool fill_paints_first = PaintOrderArray(style.PaintOrder())[0] == PT_FILL;You want to pass `PaintOrderArray::Type::kNoMarkers` as the second argument here to ignore the markers keyword.
<meta charset="utf-8" />We don't need these in regular HTML. Ditto below in the second `<link>`.
<meta name="assert" content="Verify that ::selection with only fill specificed uses selection fill and originating stroke.">typo
<meta name="assert" content="Verify that ::selection with only stroke specificed uses selection stroke and originating fill.">typo
fill: lightgreen;Should this be `stroke` to match the name?
<meta name="assert" content="Verify correct painting of SVG shadows with filled">...filled text
?
<meta name="assert" content="Verify correct painting of SVG shadows with filled">...filled and stroke text (with non-default paint-order)
?
Related details
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney added 6 comments![]( "Open in Gerrit")
I'll iterate again from here and let you know when it's ready.
default_value: "SVGPaint::CreateInitialCurrentColor()",Stephen ChenneyThis name is misleading, because while the `color` field happens to have that value, the general state of the paint is dictated by the `type` field - which is "none". Given that, does marking this paint have any use?
Fredrik SöderquistRight. Initially I left this as it was and considered using an IsNone check instead of IsInitial. The problem then would be that the code can't distinguish "Set by author to none" vs "was initially none". In the former we should respect the author and not stroke the text, but in the latter we should fall back to the originating element value.
So I can rename to CreateInitial to avoid confusion. What do you think?
Yes, just `CreateInitial()` should work I think - being an analog to the regular constructor but with the "is initial" flag set.
Done
paint.SetColor(ConvertStyleColor(state, *local_value));Stephen ChenneyThe `SVGPaint` will never have the initial flag set here, which makes this unnecessary. Keeping the setting of flag to the special `Create...()` functions should also aid with that (maybe it should be documented).
Fredrik SöderquistNo sure what you mean here. As I understand it the code right here is the case when some style sets the color, so removing initial is exactly what I want. But I'm not very confident in my understanding.
The `SVGPaint` object here (from line 2861) is one which has the default state of the is initial flag - i.e it is already false - and it should always be false here. So this is always a false -> false transition. It seems that what needs isolation is rather the setting of the flag to true - which is what we have the `CreateInitial...()` helpers for.
I get it now. I would tend to leave the code here as it is just to demonstrate good practice for setting the color. What do you think?
!pseudo_selection_style->FillPaint().IsInitial();Stephen ChenneyShould context paints be allowed? Paint-servers?
Fredrik SöderquistAccording to the CSS pseudo spec, any valid CSS "stroke" or "fill" is allowed, so yes. I think that allows context_paint but not paint servers. I'm not testing it, however because it seems super unlikely in practice and I have no reason to doubt it would work.
Well, a context paint could be a paint-server, so we get a form of transitive-ness here. Not allowing paint-servers directly while allowing them indirectly I feel is a bit of a weird situation.
Yeah, it is weird. Typical CSS not understanding all the situations, I suspect. I guess I'll add some tests and verify the behavior, and then we can figure out how to proceed.
// If the ::selection has fill: none or stroke: none we need to respect that,
// and not fall back to the originating element.
if (have_selection_fill &&
!SVGObjectPainter::HasFill(*pseudo_selection_style, context_paints)) {
have_fill = have_selection_fill = false;
}Stephen ChenneyI think this could be handled in a more logical way by just trying to apply the paint (if it is non-initial).
Fredrik SöderquistI added this because tests were failing when the selection style set stroke and fill to none. Without this it painted the originating style rather than the one expected. I'd rather leave it as is.
```
if (pseudo_selection_style &&
!pseudo_selection_style->FillPaint().IsInitial()) {
if (SVGObjectPainter::HasFill(*pseudo_selection_style, ...)) {
...apply paint...
}
} else {
if (SVGObjectPainter::HasFill(style, ...)) {
...apply paint...
}
}
```
which would lend itself to just picking one style or the other:
```
const ComputedStyle& fill_style_to_use =
condition_for_using_selection_style ? selection_style : style;
if (SVGObjectPainter::HasFill(fill_style_to_use, ...)) {
...apply paint...
}
```
This should allow eliminating the "flag soup". I don't think `PreparePaint()` returning false should trigger a fallback.
Yes. Now that I have moved the shadow code out I think that is viable. I tried it initially and it didn't work for some reason, but I'm guessing it will now.
bool fill_paints_first = PaintOrderArray(style.PaintOrder())[0] == PT_FILL;You want to pass `PaintOrderArray::Type::kNoMarkers` as the second argument here to ignore the markers keyword.
- Fredrik Söderquist
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
paint.SetColor(ConvertStyleColor(state, *local_value));Stephen ChenneyThe `SVGPaint` will never have the initial flag set here, which makes this unnecessary. Keeping the setting of flag to the special `Create...()` functions should also aid with that (maybe it should be documented).
Fredrik SöderquistNo sure what you mean here. As I understand it the code right here is the case when some style sets the color, so removing initial is exactly what I want. But I'm not very confident in my understanding.
Stephen ChenneyThe `SVGPaint` object here (from line 2861) is one which has the default state of the is initial flag - i.e it is already false - and it should always be false here. So this is always a false -> false transition. It seems that what needs isolation is rather the setting of the flag to true - which is what we have the `CreateInitial...()` helpers for.
I get it now. I would tend to leave the code here as it is just to demonstrate good practice for setting the color. What do you think?
Well, setting the color outside of this function would not be "good practice" 😄 (that's why we have a constructor for that case), so that's a bit of a misnomer... It would be better if the object was immutable - which is also why I'd prefer to not have setters.
Related details
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney voted and added 12 comments![]( "Open in Gerrit")
Votes added by Stephen Chenney
| Commit-Queue | +1 |
12 comments
default_value: "SVGPaint::CreateInitialCurrentColor()",Stephen ChenneyThis name is misleading, because while the `color` field happens to have that value, the general state of the paint is dictated by the `type` field - which is "none". Given that, does marking this paint have any use?
Right. Initially I left this as it was and considered using an IsNone check instead of IsInitial. The problem then would be that the code can't distinguish "Set by author to none" vs "was initially none". In the former we should respect the author and not stroke the text, but in the latter we should fall back to the originating element value.
Fredrik SöderquistSo I can rename to CreateInitial to avoid confusion. What do you think?
Stephen ChenneyYes, just `CreateInitial()` should work I think - being an analog to the regular constructor but with the "is initial" flag set.
Done
paint.SetColor(ConvertStyleColor(state, *local_value));Stephen ChenneyThe `SVGPaint` will never have the initial flag set here, which makes this unnecessary. Keeping the setting of flag to the special `Create...()` functions should also aid with that (maybe it should be documented).
Fredrik SöderquistNo sure what you mean here. As I understand it the code right here is the case when some style sets the color, so removing initial is exactly what I want. But I'm not very confident in my understanding.
Stephen ChenneyThe `SVGPaint` object here (from line 2861) is one which has the default state of the is initial flag - i.e it is already false - and it should always be false here. So this is always a false -> false transition. It seems that what needs isolation is rather the setting of the flag to true - which is what we have the `CreateInitial...()` helpers for.
OK. I've removed the SVGPaint::SetColor method and reverted this back to paint.color = ...
!pseudo_selection_style->FillPaint().IsInitial();Stephen ChenneyShould context paints be allowed? Paint-servers?
Fredrik SöderquistAccording to the CSS pseudo spec, any valid CSS "stroke" or "fill" is allowed, so yes. I think that allows context_paint but not paint servers. I'm not testing it, however because it seems super unlikely in practice and I have no reason to doubt it would work.
Stephen ChenneyWell, a context paint could be a paint-server, so we get a form of transitive-ness here. Not allowing paint-servers directly while allowing them indirectly I feel is a bit of a weird situation.
OK, I looked some more at the spec. There's a note at https://drafts.fxtf.org/fill-stroke-3/#svg-paint saying that more spec work needs to be done to incorporate context-fill and context-stroke, so the MDN page is a bit misleading. I don't want to write tests for things that are not in a spec yet. Are you OK landing as it is now?
// If the ::selection has fill: none or stroke: none we need to respect that,
// and not fall back to the originating element.
if (have_selection_fill &&
!SVGObjectPainter::HasFill(*pseudo_selection_style, context_paints)) {
have_fill = have_selection_fill = false;
}Stephen ChenneyI think this could be handled in a more logical way by just trying to apply the paint (if it is non-initial).
Fredrik SöderquistI added this because tests were failing when the selection style set stroke and fill to none. Without this it painted the originating style rather than the one expected. I'd rather leave it as is.
Stephen Chenney```
if (pseudo_selection_style &&
!pseudo_selection_style->FillPaint().IsInitial()) {
if (SVGObjectPainter::HasFill(*pseudo_selection_style, ...)) {
...apply paint...
}
} else {
if (SVGObjectPainter::HasFill(style, ...)) {
...apply paint...
}
}
```
which would lend itself to just picking one style or the other:
```
const ComputedStyle& fill_style_to_use =
condition_for_using_selection_style ? selection_style : style;
if (SVGObjectPainter::HasFill(fill_style_to_use, ...)) {
...apply paint...
}
```
This should allow eliminating the "flag soup". I don't think `PreparePaint()` returning false should trigger a fallback.
Done
bool fill_paints_first = PaintOrderArray(style.PaintOrder())[0] == PT_FILL;Stephen ChenneyYou want to pass `PaintOrderArray::Type::kNoMarkers` as the second argument here to ignore the markers keyword.
Done
We don't need these in regular HTML. Ditto below in the second `<link>`.
Done. But we do need the /fonts/ahem.css to avoid presubmit errors.
<meta name="assert" content="Verify that ::selection with only fill specificed uses selection fill and originating stroke.">Stephen Chenneytypo
Done
<meta name="assert" content="Verify that ::selection with only stroke specificed uses selection stroke and originating fill.">Stephen Chenneytypo
Done
Should this be `stroke` to match the name?
Done
<meta name="assert" content="Verify correct painting of SVG shadows with filled">Stephen Chenney...filled text
?
Done
<meta name="assert" content="Verify correct painting of SVG shadows with filled">...filled and stroke text (with non-default paint-order)
?
- Fredrik Söderquist
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist voted and added 3 comments![]( "Open in Gerrit")
Votes added by Fredrik Söderquist
| Code-Review | +1 |
3 comments
static bool HasStroke(const ComputedStyle&, const SvgContextPaints*);This is no longer used?
!pseudo_selection_style->FillPaint().IsInitial();Stephen ChenneyShould context paints be allowed? Paint-servers?
Fredrik SöderquistAccording to the CSS pseudo spec, any valid CSS "stroke" or "fill" is allowed, so yes. I think that allows context_paint but not paint servers. I'm not testing it, however because it seems super unlikely in practice and I have no reason to doubt it would work.
Stephen ChenneyWell, a context paint could be a paint-server, so we get a form of transitive-ness here. Not allowing paint-servers directly while allowing them indirectly I feel is a bit of a weird situation.
OK, I looked some more at the spec. There's a note at https://drafts.fxtf.org/fill-stroke-3/#svg-paint saying that more spec work needs to be done to incorporate context-fill and context-stroke, so the MDN page is a bit misleading. I don't want to write tests for things that are not in a spec yet. Are you OK landing as it is now?
I guess that basically says that a `fill-color: context-stroke` would work, but (probably only) pick up the color. I guess the status quo is the closest we get to that without adding additional code.
Related details
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stephen Chenney
| Commit-Queue | +1 |
2 comments
Thanks again. Looks like I need another +1. I think there's an error in the Gerrit vote copy system but I'm yet to prove it with a truly trivial change.
static bool HasStroke(const ComputedStyle&, const SvgContextPaints*);This is no longer used?
- Fredrik Söderquist
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist voted Code-Review+1![]( "Open in Gerrit")
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/54671.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix SVG text selection colors
When selecting SVG text, the current behavior relies on the selection
styling having all of the color information, which it does not always
have. Colors not specified in the ::selection should use the
non-selected colors, as happens for HTML content.
The ideal solution would be for SVG selection to use the
HighlightStyleUtils methods for resolving colors, but this fails because
the initial value for the CSS fill property is black (rather than
currentColor) so the color will never resolve to currentColor leading to
the fallback color being used. While the spec has been updated to use
currentColor for the initial fill value, that change appears to break
the world because all existing SVG content assumes the initial value for
fill is black.
So keep track of whether SVGPaint is using the initial value for its
colors and if so ignore the ::selection colors when painting SVG
selection.
Also refactor shadow selection to correctly layer selection shadows
according to spec and to avoid double painting shadows when both fill
and stroke are given.
Note the SVG spec is silent on how to treat ::selection applied to SVG
text. This CL treats SVG text the same as HTML text to the extent
possible. https://svgwg.org/svg2-draft/text.html#TextSelection
- M third_party/blink/renderer/core/css/css_properties.json5
- M third_party/blink/renderer/core/paint/text_painter.cc
- M third_party/blink/renderer/core/style/svg_paint.cc
- M third_party/blink/renderer/core/style/svg_paint.h
- A third_party/blink/web_tests/external/wpt/css/css-pseudo/reference/svg-text-selection-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-pseudo/reference/svg-text-selection-shadow-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-pseudo/svg-text-selection-fill-only.html
- A third_party/blink/web_tests/external/wpt/css/css-pseudo/svg-text-selection-shadow.html
- A third_party/blink/web_tests/external/wpt/css/css-pseudo/svg-text-selection-stroke-only.html
- A third_party/blink/web_tests/external/wpt/css/css-pseudo/svg-text-selection-transparent-background.html
- A third_party/blink/web_tests/external/wpt/svg/painting/reftests/text-shadow-01.html
- A third_party/blink/web_tests/external/wpt/svg/painting/reftests/text-shadow-02.html
- A third_party/blink/web_tests/external/wpt/svg/painting/reftests/text-shadow-03.html
- A third_party/blink/web_tests/external/wpt/svg/painting/reftests/text-shadow-ref.html
- M third_party/blink/web_tests/platform/linux/svg/css/text-shadow-multiple-expected.png
- M third_party/blink/web_tests/platform/linux/svg/text/selection-background-color-expected.png
- M third_party/blink/web_tests/platform/mac/svg/css/text-shadow-multiple-expected.png
- M third_party/blink/web_tests/platform/mac/svg/text/selection-background-color-expected.png
- A third_party/blink/web_tests/platform/win10/svg/css/text-shadow-multiple-expected.png
- A third_party/blink/web_tests/platform/win10/svg/text/selection-background-color-expected.png
- A third_party/blink/web_tests/platform/win11-arm64/svg/css/text-shadow-multiple-expected.png
- A third_party/blink/web_tests/platform/win11-arm64/svg/text/selection-background-color-expected.png
Code-Review: +1 by Fredrik Söderquist
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/54671
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |