Fix backdrop-filter with invalid mask image issue [chromium/src : main]
Wangsong Jin (Gerrit)
Wangsong Jin added 6 comments![]( "Open in Gerrit")
Wangsong JinSorry Wangsong but I didn't get to this today, and I am OOO for a couple weeks.
f...@opera.com, could you review this?
No worries! Thanks for looping in Fredrik to review.
HandleInvalidMaskImageWithBackdropFilterruntime feature flag as aWangsong JinMissing space
Marked as resolved.
!layer->GetImage()->ErrorOccurred() &&Wangsong JinI would use `CanRender()` instead here.
Marked as unresolved.
if (!image->ErrorOccurred() && image->IsLoaded()) {Wangsong JinDitto here.
Marked as resolved.
while (layer) {
if (StyleImage* image = layer->GetImage()) {
if (!image->ErrorOccurred() && image->IsLoaded()) {
return false;
}
}
layer = layer->Next();
}Wangsong JinI think we should put this in `FillLayer` instead - with the negation removed (i.e more like `AllImagesAreAvailable`.
Thanks, I've refactored to HasAnyImagesAvailable().
background-color: pink;Wangsong JinCould we pick a background color that is not a tint of red? (Red is the color of failure.)
Done!
Related details
- Fredrik Söderquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
// on SVG content (see CSSMaskPainter), we need to update paintWangsong JinThe code in third_party/blink/renderer/core/paint/css_mask_painter.cc seems to be doing something very similar to what you're doing in this patch (see the "kludge" comment), and the effect is similar too (e.g., the mask clip will be null in FragmentPaintPropertyTreeBuilder::UpdateEffect). Are these doing the same thing? If so, maybe we can share code by basically just doing:
```
if (layer->GetImage()->IsMaskSource()) {
// Since an invalid <mask> reference does not yield a paint property,
// we need to update paint properties when a reference changes.
SetNeedsPaintPropertyUpdate();
}
```And similarly, share code in css_mask_painter.cc?
Wangsong JinThanks!
I don't think the special handling in HasSingleInvalidSVGMaskReferenceMaskLayer() do the same thing.
I found that HasSingleInvalidSVGMaskReferenceMaskLayer() creates special behavior for SVG with local mask resource - when a local reference is invalid, we return std::nullopt and don't apply any masking at all. Not really sure that's by design though.
In my test, the blue rectangle gets an exception (invalid local reference, but still visible), while the green circle remains hidden (invalid external resource).
```
<svg width="200" height="200">
<rect x="120" y="10" width="100" height="100"
fill="blue"
mask="url(#local-invalid-mask)" />
<circle cx="60" cy="150" r="40"
fill="green"
mask="url(remote-invalid-mask)" />
</svg>
```
Link: https://codepen.io/JosephJin0815/pen/ByjeXoLTherefore, this function isn't meant to mask out the content. And regardless of whether the mask image is valid or invalid, the mask bounds themselves should still exist and not be null.
Philip RogersDone
Hi Wangsong, I prototyped the idea I was suggesting for about unifying the svg and css codepaths in https://chromium-review.googlesource.com/c/chromium/src/+/7275621, and it seems to pass tests and matches Firefox and Safari in https://codepen.io/JosephJin0815/pen/ByjeXoL. WDYT of this approach vs the one in your latest patchset? If you want to go with it, feel free to copy the patch and land it (it builds on one of your patches here).
Related details
- Fredrik Söderquist
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Hi Philip, thank you so much for your prototype! Sorry I was not quite understanding your suggestion initially—I was confused about SVG element behavior.
I prefer the approach you mentioned since it's more general, and I've updated the code accordingly.
One question about an edge case: With the current logic, if multiple mask images are specified and all are invalid, we won't mask out the content (maybe we should?). It seems we deliberate do that for SVG elements, but let me know if HTML elements should behave differently.
```
bool HasSingleInvalidMaskLayer(const LayoutObject& object,
const FillLayer& first_layer) {
if (first_layer.Next()) {
return false;
}
...
```
Related details
- Fredrik Söderquist
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 5 comments![]( "Open in Gerrit")
(I'll also be OOO after today)
// on SVG content (see CSSMaskPainter), we need to update paintI think this could work, but just like Wangsong I think this won't work as-is if all mask are invalid. I think it's probably better to keep the SVG quirk separate (it's for backwards compat BTW - maybe we should add a use-counter for it?). So maybe add a "all layers are renderable" helper and return `nullopt` for that?
if (style_image->IsMaskSource()) {
const auto* mask_source = DynamicTo<StyleMaskSourceImage>(style_image);
if (mask_source && mask_source->HasSVGMask()) {These three (type-check, type-cast and null-check) should be merged. The way this is written it type-checks twice and then does an unnecessary null-check.
```suggestion
if (auto* mask_source = DynamicTo<StyleMaskSourceImage>(style_image);
mask_source && mask_source->HasSVGMask()) {
```
return style_image->ErrorOccurred();`CanRender()`
// This is a kludge. The spec[1] says that a non-existent <mask>
// reference should yield an image layer of transparent black.
//
// [1] https://drafts.fxtf.org/css-masking/#the-mask-imageI would move this comment into `HasSingleInvalidMaskLayer()`, because it still applies.
Related details
- Wangsong Jin
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. |
Wangsong Jin (Gerrit)
Wangsong Jin added 4 comments![]( "Open in Gerrit")
// on SVG content (see CSSMaskPainter), we need to update paintThanks for the suggestion! I agree we should match Firefox and Safari behavior, where the element is masked out if all masks are invalid. In this case, combining SVG and other cases into a single path also feels a bit messy. I’ve added FillLayer::HasAnyImagesAvailable() back to handle multiple mask layers.
And I add IsLoaded() alongside with CanRender() to avoid filtering with slow‑loading invalid mask images (Content appears briefly, then disappears).
if (style_image->IsMaskSource()) {
const auto* mask_source = DynamicTo<StyleMaskSourceImage>(style_image);
if (mask_source && mask_source->HasSVGMask()) {These three (type-check, type-cast and null-check) should be merged. The way this is written it type-checks twice and then does an unnecessary null-check.
```suggestion
if (auto* mask_source = DynamicTo<StyleMaskSourceImage>(style_image);
mask_source && mask_source->HasSVGMask()) {
```
Done
return style_image->ErrorOccurred();Wangsong Jin`CanRender()`
Done
// This is a kludge. The spec[1] says that a non-existent <mask>
// reference should yield an image layer of transparent black.
//
// [1] https://drafts.fxtf.org/css-masking/#the-mask-imageI would move this comment into `HasSingleInvalidMaskLayer()`, because it still applies.
- Fredrik Söderquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |