New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[web] Fix canvas z-index leaking across repaints when element is reused. #17378
Conversation
canvas.id = kTestId; | ||
|
||
sceneElement.remove(); | ||
// Clear so resources are marked for reuse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move the comment one line down
// If a canvas is the first element we set z-index = -1 to workaround | ||
// blink compositing bug. To make sure this does not leak when reused | ||
// reset z-index. | ||
_canvas.style.removeProperty('z-index'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the effect be the same if you put this into an else
branch of the if (isFirstChildElement)
check below? Just so we have all the z-index related code together in one spot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No since it never reaches that code when you reuse from reuse_pool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm referring to line 127. It does reach it. I don't see any early return
statement. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it. So yes thats a good option as well. But this way we are executing the removeProperty call only if we reuse instead of doing it at startup as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, makes sense.
…ed. (flutter#17378) * Fix z-index leak. Add test for canvas reuse * add regression test * update golden locks * fix analysis errors
Related bug: flutter/flutter#51514
Added test that verifies canvas is reused and zindex is reset when paint order changes (similar to hover/repaint reproduced in related issue).