Skip to content

Engine Support for Dynamic View Resizing #173610

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

LouiseHsu
Copy link
Contributor

@LouiseHsu LouiseHsu commented Aug 12, 2025

Part of #169147 (iOS) and #149033 (Android). Implementation PRs coming up!

This enables support for dynamic view resizing in non-web embedders. While this PR enables support, it does not implement support.

Related PRs:
dart:ui: flutter/engine#48090
framework: #138648
framework reland: #140918
Web: flutter/engine#50271

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@github-actions github-actions bot added engine flutter/engine repository. See also e: labels. f: scrolling Viewports, list views, slivers, etc. labels Aug 12, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the necessary engine-side support for dynamic view resizing. The changes correctly plumb through view constraints from the embedder to the framework. My review identified a functional issue where new constraint parameters are ignored in hooks.dart, a documentation error in platform_dispatcher.dart, a minor typo, and a C++ style guide violation in shell.h. Addressing these points will improve the correctness and quality of the implementation.

@flutter flutter deleted a comment from gemini-code-assist bot Aug 12, 2025
@LouiseHsu
Copy link
Contributor Author

This PR still needs tests btw.

size?.width ?? physicalSize.width,
size?.height ?? physicalSize.height,
size?.width ?? _viewConfiguration.viewConstraints.maxWidth,
size?.height ?? _viewConfiguration.viewConstraints.maxHeight,
Copy link
Contributor

Choose a reason for hiding this comment

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

In my testing this didn't need to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting. let me try that

Copy link
Contributor

@mboetger mboetger Aug 12, 2025

Choose a reason for hiding this comment

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

I believe because the size is calculated from constraints in view.dart and then passed to the render function here

@@ -1080,7 +1079,6 @@ void Shell::OnPlatformViewSetViewportMetrics(int64_t view_id,

if (metrics.device_pixel_ratio <= 0 || metrics.physical_width <= 0 ||
metrics.physical_height <= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to be updated if we are passing 0 as width or height. Should probably consider the constraints:

if (metrics.device_pixel_ratio <= 0) {
    return;
  }

  if (metrics.physical_width == 0 &&
      !(metrics.min_width_constraint > 0 || metrics.max_width_constraint > 0)) {
    FML_LOG(ERROR) << "Ignoreing invalid viewport metrics width!";
    return;
  }
  if (metrics.physical_height == 0 && !(metrics.min_height_constraint > 0 ||
                                        metrics.max_height_constraint > 0)) {
    FML_LOG(ERROR) << "Ignoreing invalid viewport metrics height!";
    return;
  }

Copy link
Member

@loic-sharma loic-sharma Aug 12, 2025

Choose a reason for hiding this comment

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

Why are we passing 0 as width or height? That's not immediately obvious to me

Copy link
Contributor

Choose a reason for hiding this comment

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

If width/height is just set to WRAP_CONTENT for Android it will be 0. I'm still working on the best way to force it to be a certain initial width/height there had to be at least a minimum height of 1 pixel - so it may not end up being any issue, but initial testing I definitely had to add a check for the min/max constraints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good point.

Comment on lines +1109 to +1129
bool has_given_constraints = (metrics.min_width_constraint != 0.0 ||
metrics.max_width_constraint != 0.0 ||
metrics.min_height_constraint != 0.0 ||
metrics.max_height_constraint != 0.0);

if (has_given_constraints) {
expected_frame_constraints_[view_id] = {
.min_width = metrics.min_width_constraint,
.max_width = metrics.max_width_constraint,
.min_height = metrics.min_height_constraint,
.max_height = metrics.max_height_constraint,

};
} else {
expected_frame_constraints_[view_id] = {
.min_width = metrics.physical_width,
.max_width = metrics.physical_width,
.min_height = metrics.physical_height,
.max_height = metrics.physical_height,
};
}
Copy link
Member

Choose a reason for hiding this comment

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

If the embedder does not support content-sized views, it should pass tight constraints. If so, can we simplify this logic to just use the constraints? (Feel free to push back on this feedback if I'm missing something!)

Suggested change
bool has_given_constraints = (metrics.min_width_constraint != 0.0 ||
metrics.max_width_constraint != 0.0 ||
metrics.min_height_constraint != 0.0 ||
metrics.max_height_constraint != 0.0);
if (has_given_constraints) {
expected_frame_constraints_[view_id] = {
.min_width = metrics.min_width_constraint,
.max_width = metrics.max_width_constraint,
.min_height = metrics.min_height_constraint,
.max_height = metrics.max_height_constraint,
};
} else {
expected_frame_constraints_[view_id] = {
.min_width = metrics.physical_width,
.max_width = metrics.physical_width,
.min_height = metrics.physical_height,
.max_height = metrics.physical_height,
};
}
expected_frame_constraints_[view_id] = {
.min_width = metrics.min_width_constraint,
.max_width = metrics.max_width_constraint,
.min_height = metrics.min_height_constraint,
.max_height = metrics.max_height_constraint,
};

Comment on lines 1080 to 1081
if (metrics.device_pixel_ratio <= 0 || metrics.physical_width <= 0 ||
metrics.physical_height <= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a check that the size is within the constraints?

double max_width = 0.0;
double min_height = 0.0;
double max_height = 0.0;
};
Copy link
Member

Choose a reason for hiding this comment

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

FYI, //shell/platform/common has:

class BoxConstraints {
public:
BoxConstraints() = default;
BoxConstraints(const std::optional<Size>& smallest,
const std::optional<Size>& biggest)
: smallest_(smallest.value_or(Size(0, 0))),
biggest_(
biggest.value_or(Size(std::numeric_limits<double>::infinity(),
std::numeric_limits<double>::infinity()))) {}
BoxConstraints(const BoxConstraints& other) = default;
Size biggest() const { return biggest_; }
Size smallest() const { return smallest_; }
private:
Size smallest_ = Size(0, 0);
Size biggest_ = Size(std::numeric_limits<double>::infinity(),
std::numeric_limits<double>::infinity());
};

I'd consider moving that down to //fml/geometry.h or something

Copy link
Contributor

Choose a reason for hiding this comment

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

If you change the constructor, you'll need to change where it's called - platform_view_android_jni_impl.cc (even though I know you didn't want to change platform code). Otherwise, you'll need to make a new constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine flutter/engine repository. See also e: labels. f: scrolling Viewports, list views, slivers, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants