Skip to content
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

[ci] Roll Flutter to f842ed91 #4513

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

stuartmorgan
Copy link
Contributor

Rolls Flutter master to f842ed9 manually.

The switch to Material 3 by default broke the expectations of the dynamic_layouts package; this updates them to handle the values reported starting with that switch.

@github-actions github-actions bot added the p: dynamic_layouts The dynamic_layouts package label Jul 18, 2023
@stuartmorgan
Copy link
Contributor Author

@Piinks I bisected to the M3 switch commit, so I'm sure that's the cause. I'm assuming that means these changes are expected, but if that's not the case I'd like to spin that out into an issue for follow-up so that I can get the roller unblocked.

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

This LGTM - thank you for fixing it!

@@ -23,10 +22,21 @@ void main() {
expect(find.text('Index 3'), findsOneWidget);
expect(find.text('Index 4'), findsOneWidget);

// Material 3 changes the expected layout positioning.
final bool usesMaterial3 = (app.theme ?? ThemeData.light()).useMaterial3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I double checked. The slight change in offset is expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, thanks for verifying!

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 18, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 18, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 18, 2023

auto label is removed for flutter/packages, pr: 4513, due to - The status or check suite Linux_web web_dart_unit_test_shard_1 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@stuartmorgan
Copy link
Contributor Author

Ugh, looks like web has slightly different offsets. I'll adjust accordingly for web...

@Piinks
Copy link
Contributor

Piinks commented Jul 18, 2023

Oh man. @HansMuller is there meant to be such discrepancies with Material 3 across platforms? I would expect the pixels to be the same when building the same across platforms. It was the same when this was tested in M2. The disparity here is coming from the text, is there a different visual density across platforms for Material 3?

@stuartmorgan
Copy link
Contributor Author

It's exactly half a pixel different, so maybe it's a physical pixel alignment thing?

@stuartmorgan
Copy link
Contributor Author

stuartmorgan commented Jul 18, 2023

I pushed a version that does half-pixel adjustment for web. The other option I can see is that I could do within-half-a-pixel checks on all platforms instead.

@stuartmorgan stuartmorgan changed the title [ci] Roll Flutter to f842ed916514879fe6898b2a5a4053c63c3308fe [ci] Roll Flutter to f842ed91 Jul 18, 2023
});
}

double _getExpectedYOffset(double nonWeb) {
return kIsWeb ? nonWeb - 0.5 : nonWeb;
Copy link
Member

Choose a reason for hiding this comment

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

Yikes! Is this a "web is rounding wrong" bug?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm maybe @yjbanov would know?

Copy link
Member

Choose a reason for hiding this comment

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

I brought this up to the team chat as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

I vaguely recall we had a special-case for an off-by-one issue in text for Firefox, but not generally. @mdebbar might know more.

@stuartmorgan
Copy link
Contributor Author

I'm going to go ahead and autosubmit this as-is since our roller is many days behind and I want to unblock it; we can definitely iterate on how exactly we handle the web diff (and investigate the framework if that's a concern) after the fact.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 18, 2023
@auto-submit auto-submit bot merged commit 7f07f66 into flutter:main Jul 18, 2023
72 checks passed
@ditman
Copy link
Member

ditman commented Jul 18, 2023

@stuartmorgan ACK, we'll revisit this adjustment if we end up doing any changes that affect the vertical spacing in the web later.

@stuartmorgan stuartmorgan deleted the roll-2023-07-18-first branch July 18, 2023 21:49
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 19, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 19, 2023
flutter/packages@3e8b813...209db21

2023-07-19 reidbaker@google.com [tooling] mark infra steps as infra steps in yaml files (flutter/packages#4473)
2023-07-19 engine-flutter-autoroll@skia.org Manual roll Flutter from f842ed9 to 6f09064 (11 revisions) (flutter/packages#4518)
2023-07-19 stuartmorgan@google.com [ci] Enable new sharding (flutter/packages#4515)
2023-07-19 stuartmorgan@google.com [flutter_markdown] Change the way tests get screen size (flutter/packages#4514)
2023-07-18 stuartmorgan@google.com [ci] Roll Flutter to f842ed9 (flutter/packages#4513)
2023-07-18 stuartmorgan@google.com [image_picker] Update Android example (flutter/packages#4504)
2023-07-18 43054281+camsim99@users.noreply.github.com [camerax] Fixes relistening to `onStreamedFrameAvailable`'s stream behavior (flutter/packages#4511)
2023-07-18 43054281+camsim99@users.noreply.github.com [various] Deletes deprecated splash screen meta-data element (flutter/packages#4501)

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-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
flutter/packages@3e8b813...209db21

2023-07-19 reidbaker@google.com [tooling] mark infra steps as infra steps in yaml files (flutter/packages#4473)
2023-07-19 engine-flutter-autoroll@skia.org Manual roll Flutter from f842ed9 to 6f09064 (11 revisions) (flutter/packages#4518)
2023-07-19 stuartmorgan@google.com [ci] Enable new sharding (flutter/packages#4515)
2023-07-19 stuartmorgan@google.com [flutter_markdown] Change the way tests get screen size (flutter/packages#4514)
2023-07-18 stuartmorgan@google.com [ci] Roll Flutter to f842ed9 (flutter/packages#4513)
2023-07-18 stuartmorgan@google.com [image_picker] Update Android example (flutter/packages#4504)
2023-07-18 43054281+camsim99@users.noreply.github.com [camerax] Fixes relistening to `onStreamedFrameAvailable`'s stream behavior (flutter/packages#4511)
2023-07-18 43054281+camsim99@users.noreply.github.com [various] Deletes deprecated splash screen meta-data element (flutter/packages#4501)

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-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@Piinks Piinks mentioned this pull request Aug 11, 2023
11 tasks
auto-submit bot pushed a commit that referenced this pull request Aug 11, 2023
This refactors a test for the example of a wrapped layout in dynamic_tests. This test had become brittle to small text changes because it would check the precise layout offset of the text.

It was patched in these cases in #4513 and #4677 to address tiny text variations across platforms and material 2/3 defaults. This refactor changes the test to check the layout offset of the parent Container of the text, which should not have these subtle variations.

Fixes flutter/flutter#132321
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: dynamic_layouts The dynamic_layouts package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants