Skip to content

Conversation

@chingjun
Copy link
Contributor

Reverts #138648

This caused the app to be stuck in the splash screen on certain phone models.

Context: b/316244317

@chingjun chingjun requested a review from goderbauer December 14, 2023 19:09
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Dec 14, 2023
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@chingjun chingjun added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2023
@auto-submit auto-submit bot merged commit d24c01b into master Dec 14, 2023
@auto-submit auto-submit bot deleted the revert-138648-dynamically-sized-views branch December 14, 2023 19:42
@jiahaog
Copy link
Member

jiahaog commented Dec 14, 2023

To add more context, the affected devices were able to work around the issue by changing the launch mode of the app (to use split screen or a "windowed" mode).

A wild guess of what is happening could be:

  • These devices may be running in a non-standard "full screen" mode by default
  • Dynamic view sizing #138648 causes the framework to incorrectly infer that the size of the view is zero for these devices
  • The engine doesn't produce a frame
  • On Android, the OS only takes down the splash screen when the preDrawListener returns true
  • The embedding is set up to only return true for the preDrawListener after the Flutter first frame
  • Since there are no frames, the splash screen doesn't get taken down.

@goderbauer
Copy link
Member

causes the framework to incorrectly infer that the size of the view is zero for these devices.

This is the one that baffles me. The PR is not really changing how the size is determined for environments where only one size is possible (and even with this change the Android embedding only allows one size). Anyways, looking forward to diving into this once we have the device.

@jiahaog Did this only reproduce with a specific app on those devices or is any Flutter app affected, i.e. can you repro this with the counter app?

@jiahaog
Copy link
Member

jiahaog commented Dec 15, 2023

Thanks to some folks on the affected team, they were able to reproduce it on a Redmi Note 5 pro with the internal minimal app (go/flutter-minimal), which is pretty much the counter app. After launch, It freezes on a white screen (presumably the splash screen for this app) and never shows the counter.

I don't know if it matters, but the minSdkVersion is 23 and the targetSdkVersion 34 for that app.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 15, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Dec 15, 2023
flutter/flutter@a51e33a...2407f69

2023-12-15 [email protected] Move package:web dependency to dev dependency (flutter/flutter#139696)
2023-12-15 [email protected] Roll Flutter Engine from 9524a185b055 to 986a6fe198dc (1 revision) (flutter/flutter#140221)
2023-12-15 [email protected] Roll Packages from 1151191 to 3f2e16b (9 revisions) (flutter/flutter#140218)
2023-12-15 [email protected] Roll Flutter Engine from 7a50221733c2 to 9524a185b055 (1 revision) (flutter/flutter#140217)
2023-12-15 [email protected] Roll Flutter Engine from 767223f7a4f8 to 7a50221733c2 (1 revision) (flutter/flutter#140216)
2023-12-15 [email protected] Roll Flutter Engine from 91f65eea0c11 to 767223f7a4f8 (1 revision) (flutter/flutter#140210)
2023-12-15 [email protected] Roll Flutter Engine from a47da28c9a62 to 91f65eea0c11 (1 revision) (flutter/flutter#140207)
2023-12-15 [email protected] Roll Flutter Engine from cde1a596432d to a47da28c9a62 (1 revision) (flutter/flutter#140204)
2023-12-15 [email protected] Roll Flutter Engine from 46ff5c08a905 to cde1a596432d (1 revision) (flutter/flutter#140200)
2023-12-15 [email protected] Roll Flutter Engine from a17bb0a63b7e to 46ff5c08a905 (1 revision) (flutter/flutter#140198)
2023-12-15 [email protected] Roll Flutter Engine from 4cb3ba7a85f6 to a17bb0a63b7e (1 revision) (flutter/flutter#140196)
2023-12-15 [email protected] Roll Flutter Engine from 0e7248d43251 to 4cb3ba7a85f6 (14 revisions) (flutter/flutter#140195)
2023-12-15 [email protected] Increase versions of leak tracker libraries. (flutter/flutter#140018)
2023-12-15 [email protected] Set compile test iOS app target version to not embed Swift runtime (flutter/flutter#140188)
2023-12-15 [email protected] Cupertino text clear label (flutter/flutter#129727)
2023-12-15 [email protected] [github actions] use token from real user flutter mirror bot  (flutter/flutter#140191)
2023-12-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 0e7248d43251 to 0b0fab821536 (4 revisions)" (flutter/flutter#140194)
2023-12-14 [email protected] Roll Flutter Engine from 0e7248d43251 to 0b0fab821536 (4 revisions) (flutter/flutter#140180)
2023-12-14 [email protected] feat: Add onTapAlwaysCalled in TextFormField (flutter/flutter#140089)
2023-12-14 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 3.1.3 to 4.0.0 (flutter/flutter#140177)
2023-12-14 [email protected] Roll Flutter Engine from 2140942444ea to 0e7248d43251 (2 revisions) (flutter/flutter#140176)
2023-12-14 [email protected] fix reorderable_list drop animation (flutter/flutter#139362)
2023-12-14 [email protected] Roll Flutter Engine from 997d3dfa1e74 to 2140942444ea (4 revisions) (flutter/flutter#140171)
2023-12-14 [email protected] Fix BottomNavigationBarItem label overflow (flutter/flutter#120206)
2023-12-14 [email protected] Roll Flutter Engine from a565cea256c7 to 997d3dfa1e74 (2 revisions) (flutter/flutter#140170)
2023-12-14 [email protected] Revert "Dynamic view sizing" (flutter/flutter#140165)
2023-12-14 [email protected] ð��¨: fix cupertionActionSheet design (flutter/flutter#134345)
2023-12-14 [email protected] Make improvements to existing new issue templates  (flutter/flutter#140142)
2023-12-14 [email protected] Roll Flutter Engine from caf33276468b to a565cea256c7 (1 revision) (flutter/flutter#140163)
2023-12-14 [email protected] Expand and update a few release.yml categories (flutter/flutter#140120)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
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 Packages: 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
goderbauer added a commit to goderbauer/flutter that referenced this pull request Jan 9, 2024
goderbauer added a commit that referenced this pull request Jan 9, 2024
This reverts commit
d24c01b.

The original change was reverted because it caused some apps to get
stuck on the splash screen on some phones.

An investigation determined that this was due to a rounding error.
Example: The device reports a physical size of 1008.0 x 2198.0 with a
dpr of 1.912500023841858. Flutter would translate that to a logical size
of 527.0588169589221 x 1149.2810314243163 and use that as the input for
its layout algorithm. Since the constraints here are tight, the layout
algorithm would determine that the resulting logical size of the root
render object must be 527.0588169589221 x 1149.2810314243163.
Translating this back to physical pixels by applying the dpr resulted in
a physical size of 1007.9999999999999 x 2198.0 for the frame. Android
now rejected that frame because it didn't match the expected size of
1008.0 x 2198.0 and since no frame had been rendered would never take
down the splash screen.

Prior to dynamically sized views, this wasn't an issue because we would
hard-code the frame size to whatever the requested size was.

Changes in this PR over the original PR:

* The issue has been fixed now by constraining the calculated physical
size to the input physical constraints which makes sure that we always
end up with a size that is acceptable to the operating system.
* The `ViewConfiguration` was refactored to use the slightly more
convenient `BoxConstraints` over the `ViewConstraints` to represent
constraints. Both essentially represent the same thing, but
`BoxConstraints` are more powerful and we avoid a couple of translations
between the two by translating the` ViewConstraints` from the
`FlutterView` to `BoxConstraints` directly when the `ViewConfiguration`
is created.

All changes over the original PR are contained in the second commit of
this PR.

Fixes b/316813075
Part of #134501.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants