[skwasm] Combine offset and transform properly. #53967
Conversation
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
| // are applied properly. | ||
| sb.pushOffset(50, 50); | ||
| sb.pushTransform(Matrix4.rotationZ(0.1).toFloat64()); | ||
| sb.pushOffset(25, 25); |
There was a problem hiding this comment.
Maybe make a separate copy of the test? The previous scenario with a transform that's not sandwiched between two offsets seems like an interesting case to test too.
| style.width = '${logicalWidth}px'; | ||
| style.height = '${logicalHeight}px'; | ||
| style.left = '${logicalLeft}px'; | ||
| style.top = '${logicalTop}px'; |
There was a problem hiding this comment.
I believe top/left properties invalidate layout when updated, but transform only invalidates painting and therefore is much more efficient. Can we use transform: translate(x, y) instead of top/left?
There was a problem hiding this comment.
Ah, I didn't know that. Good idea.
| style.width = '${_bounds!.width}px'; | ||
| style.height = '${_bounds!.height}px'; | ||
| style.left = '0px'; | ||
| style.top = '0px'; |
There was a problem hiding this comment.
I think top/left are no-ops when transform is present. No need to set them (unless this is unsetting the previously set values, in which case maybe clear them so the browser doesn't need to parse anything).
There was a problem hiding this comment.
Yes, I was doing this to unset any previously set values. But if we're always using a transform in all cases as per your other suggestion, we'll never be setting these at all, so I can clean this up.
…ther issues with the unit test that was readded.
|
Golden file changes are available for triage from new commit, Click here to view. |
harryterkelsen
left a comment
harryterkelsen
left a comment
There was a problem hiding this comment.
LGTM
Depending on how complex the remaining fixes for transforms and clipping in platform views are, consider generalizing and reusing some code in embedded_views.dart for laying out and clipping platform views.
eyebrowsoffire
added
the
autosubmit
…152004) flutter/engine@766f7be...b65c93e 2024-07-18 [email protected] [skwasm] Combine offset and transform properly. (flutter/engine#53967) 2024-07-18 [email protected] Gracefully fail when `gn desc` returns no targets. (flutter/engine#53999) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#152004) flutter/engine@766f7be...b65c93e 2024-07-18 [email protected] [skwasm] Combine offset and transform properly. (flutter/engine#53967) 2024-07-18 [email protected] Gracefully fail when `gn desc` returns no targets. (flutter/engine#53999) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#152004) flutter/engine@766f7be...b65c93e 2024-07-18 [email protected] [skwasm] Combine offset and transform properly. (flutter/engine#53967) 2024-07-18 [email protected] Gracefully fail when `gn desc` returns no targets. (flutter/engine#53999) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
Our treatment of the interaction between offset and transform was incorrect. Modified our platform view unit tests to cover more cases of nested offsets and transforms.