Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[skwasm] Fix platform view placement. #53845

Merged

Conversation

eyebrowsoffire
Copy link
Contributor

Previously, each platform view contained styling/placement information from the entire stack of the layer builder. This caused issues when using addRetained, since it would contain stale styling/placement information from its old parent layers. I have changed it so that platform views only contain local styling information, and that styling is combined with the parent only when the layers are merged.

Previously, each platform view contained styling/placement information
from the entire stack of the layer builder. This caused issues when using
`addRetained`, since it would contain stale styling/placement information
from its old parent layers. I have changed it so that platform views only
contain local styling information, and that styling is combined with the
parent only when the layers are merged.
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Jul 12, 2024
@eyebrowsoffire
Copy link
Contributor Author

Note, I suspect this change will fix flutter/flutter#146539, flutter/flutter#149909, and flutter/flutter#150502

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

final ui.Offset? offset = _styling!.position.offset;
final double logicalLeft = (offset?.dx ?? 0) / devicePixelRatio;
final double logicalTop = (offset?.dy ?? 0) / devicePixelRatio;
final PlatformViewPosition position = PlatformViewPosition.combine(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unable to comment on the internals of this class, so commenting here. Just a nit that we should probably put transform first and offset second in the order of fields, arguments, and various expressions that involve both because transform is applied first and offset is applied second. For example, if you have a transform layer and you do addPlatformView(offset), you first apply the transform and then the offset. Unless I'm misunderstanding what these fields do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an "either-or" class, where either the offset or the transform are specified but never both. If there is an offset and a transform that are combined, they just become a transform only (the offset is applied properly to the transform). As such, there is no ordering implied here within this class.


final ui.Offset offset = position.offset ?? ui.Offset.zero;
final double logicalLeft = offset.dx / devicePixelRatio;
final double logicalTop = offset.dy / devicePixelRatio;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you, but local variable types are now optional. It might be good to take advantage of it in individual PRs so we don't have to do a huge type rewrite later.

@eyebrowsoffire eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 12, 2024
@auto-submit auto-submit bot merged commit b555f8f into flutter:main Jul 12, 2024
29 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 12, 2024
…151692)

flutter/engine@1554a0d...b555f8f

2024-07-12 [email protected] [skwasm] Fix platform view placement. (flutter/engine#53845)

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],[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
@Rexios80
Copy link
Member

This fixes the rendering of google_maps_flutter_web compiled to WASM. Thank you for this!

Rexios80 pushed a commit to Rexios80/flutter_engine that referenced this pull request Aug 29, 2024
Previously, each platform view contained styling/placement information from the entire stack of the layer builder. This caused issues when using `addRetained`, since it would contain stale styling/placement information from its old parent layers. I have changed it so that platform views only contain local styling information, and that styling is combined with the parent only when the layers are merged.
auto-submit bot pushed a commit that referenced this pull request Sep 11, 2024
@Rexios80 Rexios80 added the cp: stable cherry pick to the stable release candidate branch label Sep 13, 2024
@flutteractionsbot
Copy link

Failed to create CP due to merge conflicts.
You will need to create the PR manually. See the cherrypick wiki for more info.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App cp: stable cherry pick to the stable release candidate branch platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants