Canvas regression #17738
Canvas regression #17738
Conversation
| String width = '${rect.width}px'; | ||
| String height = '${rect.height}px'; | ||
| while (parentSurface != null) { | ||
| html.HtmlElement parentElement = parentSurface.rootElement; |
There was a problem hiding this comment.
The surface is placed in the DOM after it is updated. When the surface is moved to another parent, this will get a wrong DOM element.
To fix this you can schedule the update after the update, similar to how canvas painting is scheduled into the _paintQueue.
There was a problem hiding this comment.
Actually, I think I know the proper place for the logic that figures out the nearest clip: recomputeTransformAndClip. It is invoked as part of the preroll to figure out clip changes even for addRetained. This way, the backdrop filter doesn't need to walk up the parent chain at all. Instead, it will be given nearest clip, so it can react appropriately when the clip changes.
There was a problem hiding this comment.
Done. Reading localClipRect from PersistedContainerSurface instead.
| String height = '${rect.height}px'; | ||
| while (parentSurface != null) { | ||
| html.HtmlElement parentElement = parentSurface.rootElement; | ||
| if (parentElement.tagName == 'flt-clip' || parentElement.tagName == 'FLT-CLIP') { |
There was a problem hiding this comment.
Relying on element tag name seems error-prone as we can have other clipping elements with different tag names. In fact, I think this already missed <flt-clippath>.
Instead, let's add a bool isClipping getter to PersistedContainerSurface that returns false by default. Clips can override it and return true instead.
| @@ -65,14 +65,41 @@ class PersistedBackdropFilter extends PersistedContainerSurface | |||
| _invertedTransform = Matrix4.inverted(_transform); | |||
| _previousTransform = _transform; | |||
| } | |||
| final ui.Rect rect = transformLTRB(_invertedTransform, 0, 0, | |||
| ui.window.physicalSize.width, ui.window.physicalSize.height); | |||
| // https://api.flutter.dev/flutter/widgets/BackdropFilter-class.html | |||
There was a problem hiding this comment.
For performance reasons the apply() is not called when the surface is added using addRetained. You also need to override retain() to schedule this logic.
| @@ -0,0 +1,106 @@ | |||
| // Copyright 2013 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
The implementation is sensitive to the surface lifecycle. We should have tests that move the surface around (both as retained and as updated) and check that the style is applied correctly.
There was a problem hiding this comment.
Added test that moves clipRect.
| void _checkForUpdatedAncestorClipElement() { | ||
| // If parent clip element has moved, adjust bounds. | ||
| PersistedContainerSurface parentSurface = parent; | ||
| html.HtmlElement parentElement = null; |
| // If parent clip element has moved, adjust bounds. | ||
| PersistedContainerSurface parentSurface = parent; | ||
| html.HtmlElement parentElement = null; | ||
| while (parentSurface != null) { |
There was a problem hiding this comment.
This looks like it will be an infinite loop unless the immediate parent is a clip. I think there should be parentSurface = parentSurface.parent somewhere inside the loop. Would also be good to add a test that checks that we can climb up the parent chain.
Fixes: flutter/flutter#54871
Fixes: flutter/flutter#55193