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

[framework] re-rasterize when window size or insets changes #113647

Merged
merged 13 commits into from Oct 24, 2022

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Oct 18, 2022

When navigating to a new page, if the keyboard is dismissed partially through the navigation, the page view will be too small. Track the widow size and re-rasterize if it changes during the transition.

b/250815292

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Oct 18, 2022
// If the screen size changes during the transition, perhaps due to
// a keyboard dismissal, then ensure that contents are re-rasterized once.
final MediaQueryData data = MediaQuery.of(context);
if (mediaQueryData != null && data.size != mediaQueryData!.size) {
Copy link
Contributor

@kevmoo kevmoo Oct 18, 2022

Choose a reason for hiding this comment

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

Might could inline the MediaQuery.of call so in the case where medaQueryData IS null, you avoid the instantiation?

Copy link
Member Author

@jonahwilliams jonahwilliams Oct 19, 2022

Choose a reason for hiding this comment

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

Its really quite difficult to live without a media query, so we'll almost always want to create one and then cache it. Which I of course forgot to do...

@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 19, 2022

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.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #113647 at sha 4feaf46

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Oct 19, 2022
@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 19, 2022

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #113647 at sha a2c7fcc

@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 19, 2022

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #113647 at sha 1f8b935

@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 19, 2022

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #113647 at sha b9663e6

// If the screen size changes during the transition, perhaps due to
// a keyboard dismissal, then ensure that contents are re-rasterized once.
final MediaQueryData? data = MediaQuery.maybeOf(context);
if (mediaQueryData?.size != data?.size) {
Copy link
Member

@dnfield dnfield Oct 21, 2022

Choose a reason for hiding this comment

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

nit: if we only care about size, just cache the size?

Copy link
Member

@dnfield dnfield Oct 21, 2022

Choose a reason for hiding this comment

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

Also, I should probably know this because I worked on it at one point, but isn't the IME "just" changing the insets/padding values and not the size?

I tried to come up with an IME based reproduction for the internal bug that could be used in open source and failed, but I hadn't spent very long on it. If you've confirmed this fixes the internal bug that's cool, but if it hasn't been confirmed internally it may be worth looking into and figuring out if we're being affected by insets/padding rather than size.

Copy link
Member

@dnfield dnfield Oct 21, 2022

Choose a reason for hiding this comment

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

See https://api.flutter.dev/flutter/widgets/MediaQueryData-class.html - I'm pretty sure it's not the size that'll get impacted here, but the viewInsets.

Copy link
Member Author

@jonahwilliams jonahwilliams Oct 21, 2022

Choose a reason for hiding this comment

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

Good idea, converting to draft for now. I think we should probably handle any of size and/or view insets. But particularly we care about the render object size, so maybe I should just check there instead

Copy link
Member

@dnfield dnfield Oct 21, 2022

Choose a reason for hiding this comment

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

Alternatively, we could just clear whenever anything on the media query changes, which runs a small risk of changing for things not visually impacting the page size.

@jonahwilliams jonahwilliams marked this pull request as draft Oct 21, 2022
@jonahwilliams
Copy link
Member Author

jonahwilliams commented Oct 24, 2022

Yeah, thanks @dnfield it was view insets

Repro app:

// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/material.dart';
import 'package:flutter/scheduler.dart';

void main() {
  timeDilation = 10;
  runApp(MaterialApp(
    home: Example(),
    routes: {
      '/b': (context) => B(),
    },
  ));
}

class Example extends StatefulWidget {
  const Example({super.key});

  @override
  State<Example> createState() => _ExampleState();
}

class _ExampleState extends State<Example> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Container(
        color: Colors.red,
        child: Center(
          child: TextButton(
            child: Text('Forward'),
            onPressed: () {
              Navigator.of(context).pushNamed('/b');
            },
          ),
        ),
      ),
    );
  }
}

class B extends StatelessWidget {
  const B({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Container(
        color: Colors.blue,
        child: Column(
          children: [
            TextField(),
            TextButton(
              child: Text('BACK', style: TextStyle(color: Colors.black),),
              onPressed: () {
                Navigator.of(context).pop();
              },
            )
          ],
        ),
      ),
    );
  }
}

Before:
ezgif-3-356a6d471c

After:
ezgif-3-82cf119d91

@jonahwilliams jonahwilliams changed the title [framework] re-rasterize when window size changes [framework] re-rasterize when window size or insets changes Oct 24, 2022
@jonahwilliams jonahwilliams requested a review from dnfield Oct 24, 2022
@jonahwilliams jonahwilliams marked this pull request as ready for review Oct 24, 2022
@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 24, 2022
@auto-submit auto-submit bot merged commit a7fe536 into flutter:master Oct 24, 2022
65 checks passed
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 f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants