Fix backdrop-filter with invalid mask image issue [chromium/src : main]
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Hi Claire and Olga, could you help to take a look at this change when you have time? Thanks!
Related details
- Claire Chambers
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Claire Chambers (Gerrit)
Claire Chambers added 3 comments![]( "Open in Gerrit")
// UV rect used to indicate a failed mask image.This comment is somewhat misleading as it implies any mask image with this rect is failed, which of course you don't mean since this rect is also the most common uv rect. Probably a better comment would be something like.
```suggestion
// Default mask UV rect that is applied when a mask image cannot be loaded. Used to signal Viz that this layer still requires masking.
```
// Failed mask should always hide content per CSS Masking specDo we know which part of the spec specifically? Maybe the section should be referenced directly.
rpdq_params.has_failed_mask = true;I think this can be avoided. Simply pass the invalid id directly to the mask shader constructor, then add a defensive return to `GetOrCreateSkShader` that returns your transparent shader. Not only would this remove the extra bool from RDPQ params but it would also avoid code duplication between `PrepareColorOrCanvasForRPDQ` and `PreparePaintOrCanvasForRPDQ`.
Related details
- Olga Gerchikov
- 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 3 comments![]( "Open in Gerrit")
This comment is somewhat misleading as it implies any mask image with this rect is failed, which of course you don't mean since this rect is also the most common uv rect. Probably a better comment would be something like.
```suggestion
// Default mask UV rect that is applied when a mask image cannot be loaded. Used to signal Viz that this layer still requires masking.
```
Done
// Failed mask should always hide content per CSS Masking specDo we know which part of the spec specifically? Maybe the section should be referenced directly.
Added.
I think this can be avoided. Simply pass the invalid id directly to the mask shader constructor, then add a defensive return to `GetOrCreateSkShader` that returns your transparent shader. Not only would this remove the extra bool from RDPQ params but it would also avoid code duplication between `PrepareColorOrCanvasForRPDQ` and `PreparePaintOrCanvasForRPDQ`.
- Claire Chambers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Claire Chambers (Gerrit)
Claire Chambers voted Code-Review+1![]( "Open in Gerrit")
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Hi, could you please take a look when you get a chance? Thanks!
Related details
- Michael Ludwig
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Hi Kyle, could you help to take a look when you have chance? Thank you!
Related details
- Kyle Charbonneau
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyle Charbonneau (Gerrit)
Kyle Charbonneau added 2 comments![]( "Open in Gerrit")
if (mask_layer && mask_resource_id == viz::kInvalidResourceId) {Shouldn't we only do this if the mask layer fails to load eg after L605 where we tried to get the resource id for it?
// If the mask resource is invalid (e.g., the mask image failed to load),In general I don't think components/viz/service is the right place to handle this. `cc` is capable of creating a 1x1 resource to use as the mask image if loading the resource failed. There is no need to leak details about how CSS spec does things into the display compositor.
Ideally blink would handle the implementation here but I'm less sure about how that would work.
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 2 comments![]( "Open in Gerrit")
if (mask_layer && mask_resource_id == viz::kInvalidResourceId) {Shouldn't we only do this if the mask layer fails to load eg after L605 where we tried to get the resource id for it?
Updated.
// If the mask resource is invalid (e.g., the mask image failed to load),In general I don't think components/viz/service is the right place to handle this. `cc` is capable of creating a 1x1 resource to use as the mask image if loading the resource failed. There is no need to leak details about how CSS spec does things into the display compositor.
Ideally blink would handle the implementation here but I'm less sure about how that would work.
Thank you for the suggestion. Creating a 1x1 fallback resource was my initial intuitive approach as well, but after exploring it, I found it adds quite a lot complexity.
I attempted to implement LayerTreeImpl::GetOrCreateFallbackMaskResource() to create a proper 1x1 transparent resource. While the approach is technically achievable, it requires substantial additional work:
- Access to SharedImageInterface (currently not available in LayerTreeImpl)
- Actual GPU texture creation with pixel data upload
- Proper resource lifecycle management (creation, sync, cleanup)
- Potentially a new ResourceSource type specifically for fallback resources. I'm not seeing we've created this fallback somewhere.
I'd appreciate your thoughts on the trade-off: how strictly is it to keep CSS spec details out of Viz? Since this is only a minimal null check that naturally extends existing mask-handling logic, could it be viewed as a more pragmatic choice?
Related details
- Kyle Charbonneau
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. |
Kyle Charbonneau (Gerrit)
Kyle Charbonneau added 1 comment![]( "Open in Gerrit")
// If the mask resource is invalid (e.g., the mask image failed to load),Wangsong JinIn general I don't think components/viz/service is the right place to handle this. `cc` is capable of creating a 1x1 resource to use as the mask image if loading the resource failed. There is no need to leak details about how CSS spec does things into the display compositor.
Ideally blink would handle the implementation here but I'm less sure about how that would work.
Thank you for the suggestion. Creating a 1x1 fallback resource was my initial intuitive approach as well, but after exploring it, I found it adds quite a lot complexity.
I attempted to implement LayerTreeImpl::GetOrCreateFallbackMaskResource() to create a proper 1x1 transparent resource. While the approach is technically achievable, it requires substantial additional work:
- Access to SharedImageInterface (currently not available in LayerTreeImpl)
- Actual GPU texture creation with pixel data upload
- Proper resource lifecycle management (creation, sync, cleanup)
- Potentially a new ResourceSource type specifically for fallback resources. I'm not seeing we've created this fallback somewhere.
I'd appreciate your thoughts on the trade-off: how strictly is it to keep CSS spec details out of Viz? Since this is only a minimal null check that naturally extends existing mask-handling logic, could it be viewed as a more pragmatic choice?
I attempted to implement LayerTreeImpl::GetOrCreateFallbackMaskResource() to create a proper 1x1 transparent resource. While the approach is technically achievable, it requires substantial additional work:
I can imagine it's complex to add a new path. We already have code needed to do the right thing at higher levels which could potentially be reused.
The mask layer already has access to all the machinery needed to create shared images and manage shared image lifetime for valid mask images. Reusing that is one option although I'm not positive it's the right layer to solve this at.
I took a closer look at the example in the linked bug. The div with `bad-mask` isn't visible already today, do you know what makes that work? IIUC setting mask to transparent black is effectively hiding the layer. Why doesn't the same mechanism work with `bf-blur bad-mask` case?
I also noticed the following in the spec:
A value of none counts as a transparent black image layer.
I think that means `mask-image: none` should behave similar to `mask-image: url(no-such-image.png)`? It doesn't work, either with or without the backdrop filter. A more holistic solution here might also fix that. I'm adding pdr@ as a reviewer here since this gets blink/cc boundary.
Related details
- Philip Rogers
- 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. |
Kyle Charbonneau (Gerrit)
Kyle Charbonneau added 1 comment![]( "Open in Gerrit")
// If the mask resource is invalid (e.g., the mask image failed to load),Wangsong JinIn general I don't think components/viz/service is the right place to handle this. `cc` is capable of creating a 1x1 resource to use as the mask image if loading the resource failed. There is no need to leak details about how CSS spec does things into the display compositor.
Ideally blink would handle the implementation here but I'm less sure about how that would work.
Kyle CharbonneauThank you for the suggestion. Creating a 1x1 fallback resource was my initial intuitive approach as well, but after exploring it, I found it adds quite a lot complexity.
I attempted to implement LayerTreeImpl::GetOrCreateFallbackMaskResource() to create a proper 1x1 transparent resource. While the approach is technically achievable, it requires substantial additional work:
- Access to SharedImageInterface (currently not available in LayerTreeImpl)
- Actual GPU texture creation with pixel data upload
- Proper resource lifecycle management (creation, sync, cleanup)
- Potentially a new ResourceSource type specifically for fallback resources. I'm not seeing we've created this fallback somewhere.
I'd appreciate your thoughts on the trade-off: how strictly is it to keep CSS spec details out of Viz? Since this is only a minimal null check that naturally extends existing mask-handling logic, could it be viewed as a more pragmatic choice?
I attempted to implement LayerTreeImpl::GetOrCreateFallbackMaskResource() to create a proper 1x1 transparent resource. While the approach is technically achievable, it requires substantial additional work:
I can imagine it's complex to add a new path. We already have code needed to do the right thing at higher levels which could potentially be reused.
The mask layer already has access to all the machinery needed to create shared images and manage shared image lifetime for valid mask images. Reusing that is one option although I'm not positive it's the right layer to solve this at.
I took a closer look at the example in the linked bug. The div with `bad-mask` isn't visible already today, do you know what makes that work? IIUC setting mask to transparent black is effectively hiding the layer. Why doesn't the same mechanism work with `bf-blur bad-mask` case?
I also noticed the following in the spec:
A value of none counts as a transparent black image layer.
I think that means `mask-image: none` should behave similar to `mask-image: url(no-such-image.png)`? It doesn't work, either with or without the backdrop filter. A more holistic solution here might also fix that. I'm adding pdr@ as a reviewer here since this gets blink/cc boundary.
I'd appreciate your thoughts on the trade-off: how strictly is it to keep CSS spec details out of Viz? Since this is only a minimal null check that naturally extends existing mask-handling logic, could it be viewed as a more pragmatic choice?
We can definitely land a fix in viz if it's the pragmatic solution. If the only options were create a new fallback image creation/tracking mechanism in cc or land a small special case in viz then landing special case in viz seems reasonable.
I'm not sure those are the only options though. Something is already hiding the layer correctly for missing mask image so why doesn't that work more generally?
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
Wangsong, could we special-case the bad mask in the painting code, such as InlineBoxFragmentPainter::PaintMask? Alternatively, would FragmentPaintPropertyTreeBuilder::UpdateEffect be a place we could special-case the bad mask and insert a clip?
Related details
- Wangsong Jin
Wangsong Jin (Gerrit)
Message from Wangsong Jin![]( "Open in Gerrit")
- Claire Chambers
- Kyle Charbonneau
- Michael Ludwig
- Philip Rogers
- 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)
Message from Wangsong Jin![]( "Open in Gerrit")
Set Ready For Review
Related details
- Claire Chambers
- Kyle Charbonneau
- Michael Ludwig
- Philip Rogers
- 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 1 comment![]( "Open in Gerrit")
I took a closer look at the example in the linked bug. The div with bad-mask isn't visible already today, do you know what makes that work? IIUC setting mask to transparent black is effectively hiding the layer. Why doesn't the same mechanism work with bf-blur bad-mask case?
They follow fairly different code paths. Plain masks apply SkBlendMode::kDstIn without checking the mask resource validity—the blend mode naturally handles null resources correctly. Backdrop-filter masks use SkShaderMaskFilter, which explicitly checks for a valid resource before applying the mask. It omits handling the invalid mask case.
I think that means mask-image: none should behave similar to mask-image: url(no-such-image.png)
I was also confused about mask-image: none in the spec. Both Safari and Firefox treat it as if no mask is involved, and in Chromium, we also treat none as no mask style. I convinced myself to: while the spec says both create a transparent black image layer, mask-image: none means there's an image layer present but no actual masking operation is performed. Modifying the behavior of mask: none would likely cause undesired compatibility issues with existing cases. The specification may need clarification to better document the intended behavior.
FragmentPaintPropertyTreeBuilder::UpdateEffect be a place we could special-case the bad mask and insert a clip?
Thanks for your suggestion! It could work. I updated with two options.
1. FragmentPaintPropertyTreeBuilder::UpdateEffect():
Checks if all mask layers are invalid, then sets opacity=0 to hide the element. We call NeedsPaintPropertyUpdate() in LayoutBox::ImageChanged() because we cannot determine if an image is invalid until it finishes loading. This has a minor issue: during initial load, the content may briefly flash before the invalid mask is detected and hidden. This is more noticeable than the normal case where content loads first, then the mask image loads and is applied. I'm not sure if this is acceptable or how to safely eliminate the flash without affecting valid mask image cases. [PatchSet 21](https://chromium-review.googlesource.com/c/chromium/src/+/7004946/21)
2. Skip creating the render pass quad to hide content. [PatchSet 22](https://chromium-review.googlesource.com/c/chromium/src/+/7004946/22)
Combined with our origin option in Viz ([PatchSet 12](https://chromium-review.googlesource.com/c/chromium/src/+/7004946/12)), which one do you prefer?
Related details
- Philip Rogers
Kyle Charbonneau (Gerrit)
Kyle Charbonneau added 1 comment![]( "Open in Gerrit")
I was also confused about mask-image: none in the spec. Both Safari and Firefox treat it as if no mask is involved, and in Chromium, we also treat none as no mask style. I convinced myself to: while the spec says both create a transparent black image layer, mask-image: none means there's an image layer present but no actual masking operation is performed. Modifying the behavior of mask: none would likely cause undesired compatibility issues with existing cases. The specification may need clarification to better document the intended behavior.
Ah if no browser is treating mask-image: none as transparent black then making that change is probably a bad idea. I am not a reliably resource on web spec related things and there is probably a reason why it works the way it currently does.
Thanks for your suggestion! It could work. I updated with two options.
IMO handling this in blink is ideal from a layering perspective since it's implementing CSS spec. I have no expertise/opinion if the code in PS21 is correct but I think pdr@ will.
Related details
- Philip Rogers
- Wangsong Jin
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
I agree with Kyle that the PS21 approach is better in terms of layering.
I'd like to know more about the web compat risk here. Can you determine the following?
chromium, mask-image: none: [behavior]
chromium, mask-image: loading image: [behavior] (you may be able to use https://httpbin.org/delay/10 to test this. Or, devtools may work)
chromium, mask-image: invalid: [behavior]
chromium, mask-image: none + backdrop-filter: [behavior]
chromium, mask-image: loading image + backdrop-filter: [behavior]
chromium, mask-image: invalid + backdrop-filter: [behavior]
Similarly for firefox and safari.
Related details
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Sure, here’s the comparison:
1. chromium, mask-image: none:
[Aligned] No mask apply. Content remains VISIBLE.
2. chromium, mask-image: loading image
[Aligned] Content remains INVISIBLE all the time.
3. chromium, mask-image: invalid
[Aligned] Fully masked. Content remains INVISIBLE.
4. chromium, mask-image: none + backdrop-filter
[Aligned] No mask apply. Content remains VISIBLE.
5. chromium, mask-image: loading image + backdrop-filter
[NOT Aligned] Same for Firefox + Safari, Content remains INVISIBLE all the time.
Chromium with the PS21 fix: The content stays VISIBLE initially and DISAPPEARS once the image finishes loading.
Chromium without fix: Content remains VISIBLE all the time.
6. chromium, mask-image: invalid + backdrop-filter
[Aligned] (with PS21 fix) Fully masked. Content remains INVISIBLE.
Also, I tested image loading for a valid mask-image case by throttling network traffic in Chromium and Firefox. Initially, nothing is painted; once the mask image finishes loading, the page displays the content with the mask effect applied.
Test example: https://codepen.io/JosephJin0815/pen/ByjeXoL
Related details
- Philip Rogers
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
Thanks! I think we should try to match the non-backdrop-filter code, where both invalid and loading mask images fully mask the content. PS21 has a lot of bespoke code, and introduces an undesirable difference in the loading case. I think we should be able to re-use however things work for the non-backdrop-filter case.
I dumped the paint chunks with `--vmodule=paint_artifact_compositor=3` and it looks like the PaintChunk for the mask has drawscontent=0 prior to the mask loading. I think the best solution is likely somewhere in third_party/blink/renderer/platform/graphics/compositing/, where we convert from PaintChunks to cc::Layers. Maybe we shouldn't be creating a layer at all for the empty mask, or something like that. [this doc](https://docs.google.com/document/d/1vgQY11pxRQUDAufxSsc2xKyQCKGPftZ5wZnjY2El4w8/preview?tab=t.0) has some tools for dumping state in this area.
Related details
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wangsong Jin
| Commit-Queue | +1 |
1 comment
Thanks for your suggestion! Updated in PS24.
I traverse the pending layers to detect invalid mask images by checking if any
paint chunks draw content. When a backdrop-filter mask is empty, I hide its
parent layer (the content with backdrop-filter) by setting opacity=0 on the
effect node.
I initially tried skipping the layer entirely, but it crashes in
tree_synchronizer.cc. It seems because the compositor expects all layers with property tree indices to exist in the layer list. In general, I feel it's not easy/safe to skip the layer. But let me know if you think that's not true.
Related details
- Philip Rogers
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 1 comment![]( "Open in Gerrit")
Hi Philip, kindly follow up on this. Does the current approach seem fine to you? Looking forward to your thoughts!
Related details
- Philip Rogers
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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
Sorry for the delay.
The current code is quite expensive, and it seems sloppy to create a layer that we later hide with opacity = 0. Can we avoid creating the layer in the first place?
Related details
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
Thanks for your feedback! I've switched to checking mask image availability upfront in compositing_reason_finder.cc to skip creating the backdrop-filter layer entirely when masks are unavailable, avoiding the layer creation.
Thanks for your feedback! I've switched to checking mask image in compositing_reason_finder.cc to skip creating the backdrop-filter layer entirely when masks are unavailable. To avoid the flickering issue where content briefly appears then disappears when invalid mask images finish loading, I've also skipped layer creation when no mask images are available, and trigger layer creation when there is a mask image successfully loads.
Related details
- Philip Rogers
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. |
Philip Rogers (Gerrit)
Philip Rogers added 3 comments![]( "Open in Gerrit")
if (layer->GetImage()->IsMaskSource()) {Would this work if we just did the following?
```
if (layer->GetImage()->IsMaskSource()) {
if (IsSVGChild()) {
// Since an invalid <mask> reference does not yield a paint property
// on SVG content (see CSSMaskPainter), we need to update paint
// properties when such a reference changes.
SetNeedsPaintPropertyUpdate();
} else if (!StyleRef().BackdropFilter().IsEmpty()) {
// May need to re-evaluate backdrop-filter compositing decision.
SetNeedsPaintPropertyUpdate();
}
}
```
Basically, I am wondering if you are trying to avoid calling `SetNeedsPaintPropertyUpdate();` for correctness? I don't think that will work because some other code may end up calling `SetNeedsPaintPropertyUpdate();`. If you are only doing this for efficiency, then I am fine with the code you have.
// on SVG content (see CSSMaskPainter), we need to update paintThe 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?
mask-image: url("invalid://nonexistent");Can you add another test (maybe just in this file) where there's one valid mask and one non-existent mask on the same element? I'm unsure if HasNoMaskImagesAvailable will work for that scenario.
Related details
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 4 comments![]( "Open in Gerrit")
// If the mask resource is invalid (e.g., the mask image failed to load),Done
Would this work if we just did the following?
```
if (layer->GetImage()->IsMaskSource()) {
if (IsSVGChild()) {
// Since an invalid <mask> reference does not yield a paint property
// on SVG content (see CSSMaskPainter), we need to update paint
// properties when such a reference changes.
SetNeedsPaintPropertyUpdate();
} else if (!StyleRef().BackdropFilter().IsEmpty()) {
// May need to re-evaluate backdrop-filter compositing decision.
SetNeedsPaintPropertyUpdate();
}
}
```Basically, I am wondering if you are trying to avoid calling `SetNeedsPaintPropertyUpdate();` for correctness? I don't think that will work because some other code may end up calling `SetNeedsPaintPropertyUpdate();`. If you are only doing this for efficiency, then I am fine with the code you have.
Hi, sorry for the delay, I was out last week.
> If you are only doing this for efficiency
Yeah, for efficiency. Updating paint properties when invalid masks load is just unnecessary.
// on SVG content (see CSSMaskPainter), we need to update paintThe 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?
Thanks!
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/ByjeXoL
Therefore, 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.
Can you add another test (maybe just in this file) where there's one valid mask and one non-existent mask on the same element? I'm unsure if HasNoMaskImagesAvailable will work for that scenario.
Done. The spec doesn't explicitly cover this scenario, but testing in Firefox shows that when at least one valid mask layer exists, it's applied and the content is displayed. Our change aligns with this.
Related details
- Philip Rogers
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 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?
Thanks!
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.
Done
Related details
- Philip Rogers
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")
Sorry Wangsong but I didn't get to this today, and I am OOO for a couple weeks.
f...@opera.com, could you review this?
Related details
- Fredrik Söderquist
- 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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 4 comments![]( "Open in Gerrit")
!layer->GetImage()->ErrorOccurred() &&I would use `CanRender()` instead here.
if (!image->ErrorOccurred() && image->IsLoaded()) {Ditto here.
while (layer) {
if (StyleImage* image = layer->GetImage()) {
if (!image->ErrorOccurred() && image->IsLoaded()) {
return false;
}
}
layer = layer->Next();
}I think we should put this in `FillLayer` instead - with the negation removed (i.e more like `AllImagesAreAvailable`.
background-color: pink;Could we pick a background color that is not a tint of red? (Red is the color of failure.)
Related details
- Wangsong Jin
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
HandleInvalidMaskImageWithBackdropFilterruntime feature flag as aMissing space
Wangsong Jin (Gerrit)
Wangsong Jin added 6 comments![]( "Open in Gerrit")
Sorry 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.
I 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();
}I think we should put this in `FillLayer` instead - with the negation removed (i.e more like `AllImagesAreAvailable`.
Thanks, I've refactored to HasAnyImagesAvailable().
Could we pick a background color that is not a tint of red? (Red is the color of failure.)
- 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")
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. |