Skip to content

Conversation

@TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Nov 17, 2023

fixes Invisible SliverAppBar title in Material 3 light theme
fixes FlexibleSpaceBar title is misaligned without the leading widget

Description

  • Fixes title color the FlexibleSpaceBar widget for Material 3
  • Fixes title alignment for the FlexibleSpaceBar widget when leading widget isn't present
  • Updates FlexibleSpaceBar widget tests for Material 3

Code sample

expand to view the code sample
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

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

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Example(),
    );
  }
}

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

  @override
  Widget build(BuildContext context) {
    return const Scaffold(
      body: SafeArea(
          child: CustomScrollView(
        slivers: <Widget>[
          SliverAppBar(
            flexibleSpace: FlexibleSpaceBar(
              title: ColoredBox(
                color: Color(0xff00ff00),
                child: Text('SliverAppBar'),
              ),
            ),
          ),
        ],
      )),
    );
  }
}

Before

before

After

after

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.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Nov 17, 2023
@TahaTesser TahaTesser marked this pull request as ready for review November 17, 2023 13:12
@TahaTesser TahaTesser requested a review from Piinks November 17, 2023 13:12
@flutter-dashboard

This comment was marked as resolved.

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Nov 17, 2023
@flutter-dashboard

This comment was marked as resolved.

@TahaTesser

This comment was marked as resolved.

@flutter-dashboard
Copy link

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 #138611 at sha 4dc61a5

@TahaTesser
Copy link
Member Author

Pushing an empty cmmit fixed this issue #138611 (comment).
Thanks @bleroux for the tip.

@TahaTesser
Copy link
Member Author

If landed, this might be worth CP. #132573 (comment)

@Piinks
Copy link
Contributor

Piinks commented Nov 29, 2023

If landed, this might be worth CP. #132573 (comment)

I have not finished reviewing this yet, but currently I don't think it's a good candidate for CP. It is a large change and fixes multiple issues instead of just one. If there is one issue in particular that needs a CP fix, we should separate it out. If all three issues are CP worthy, they should be broken out individually. 👍

@TahaTesser
Copy link
Member Author

That makes sense

Comment on lines +1259 to +1262
bool hasLeading = leading != null;
if (leading == null && automaticallyImplyLeading) {
hasLeading = (scaffold?.hasDrawer ?? false) || (parentRoute?.impliesAppBarDismissal ?? false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I've probably spent too much time seeing how this could be reworked. I've tripped over it though every time I've read it. 😆

Suggested change
bool hasLeading = leading != null;
if (leading == null && automaticallyImplyLeading) {
hasLeading = (scaffold?.hasDrawer ?? false) || (parentRoute?.impliesAppBarDismissal ?? false);
}
bool hasLeading = leading != null || automaticallyImplyLeading;
if (!hasLeading) {
hasLeading = (scaffold?.hasDrawer ?? false) || (parentRoute?.impliesAppBarDismissal ?? false);
}

Copy link
Member Author

@TahaTesser TahaTesser Nov 30, 2023

Choose a reason for hiding this comment

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

Apologies, this is bit complicated but thanks to your effort with this suggestion, I've spotted a big issue with center alignment when leading widget is provided and spent few hours trying figure out what's going wrong here.

To make sure ALL the possible combinations are as expected. I created an ultimate code sample and added all configurations of FlexibleSpaceBar.title.

As you can see leading widgets mis-aligns the title, this was good for a long title fix #132573, it's not good for a regular title.

Screenshot 2023-11-30 at 15 50 15

Unfortunately, this isn't possible to fix with with just padding as it will move the title position.

I've an idea to do something similar to _ExpandedTitleWithPadding for medium and large title.

// A widget that bottom-start aligns its child (the expanded title widget), and
// insets the child according to the specified padding.
//
// This widget gives the child an infinite max height constraint, and will also
// attempt to vertically limit the child's bounding box (not including the
// padding) to within the y range [0, maxExtent], to make sure the child is
// visible when the AppBar is fully expanded.
class _ExpandedTitleWithPadding extends SingleChildRenderObjectWidget {

I'll figure out this for good and update the PR soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

@Piinks
Copy link
Contributor

Piinks commented Nov 30, 2023

I have not finished reviewing this yet, but currently I don't think it's a good candidate for CP. It is a large change and fixes multiple issues instead of just one. If there is one issue in particular that needs a CP fix, we should separate it out. If all three issues are CP worthy, they should be broken out individually.

I definitely misspoke here. I was collecting up all my PRs for review today and mixed this up with another - the code size is from all of the excellent tests.
I think this would be ok to CP. Let me know what you think of my nitty nit and it should be good to go. 😜

@TahaTesser TahaTesser closed this Dec 6, 2023
@TahaTesser TahaTesser deleted the fix_flexibleSpaceBar branch December 6, 2023 11:16
@TahaTesser
Copy link
Member Author

TahaTesser commented Dec 6, 2023

Closing to do some native testing, dig deeper into how Android itself handles this (I've this done this for some other widgets, it may also require updating docs and more testing than this PR has) and take feedback from Discord into account.

auto-submit bot pushed a commit that referenced this pull request Jan 12, 2024
fixes [Invisible SliverAppBar title in Material 3 light theme](#138296) 
fixes [`FlexibleSpaceBar` title is misaligned without the leading widget](#138608)

Previous attempt #138611

--- 

### Description

 - fixes the `FlexibleSpaceBar` centered title position when there is a leading widget.
 - fixes the `FlexibleSpaceBar` title color for Material 3.
 - Added documentation when using a long `FlexibleSpaceBar` title and update its test.
 - Improved documentation of default title padding.

### Code sample

### Code sample

<details>
<summary>expand to view the code sample</summary> 

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

void main() => runApp(const MyApp());

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

  @OverRide
  Widget build(BuildContext context) {
    return const MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Example(),
    );
  }
}

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

  @OverRide
  Widget build(BuildContext context) {
    return const Scaffold(
      body: SafeArea(
          child: CustomScrollView(
        slivers: <Widget>[
          SliverAppBar(
            leading: Icon(Icons.favorite_rounded),
            flexibleSpace: FlexibleSpaceBar(
              title: ColoredBox(
                color: Color(0xffff0000),
                child: Text('SliverAppBar'),
              ),
            ),
          ),
        ],
      )),
    );
  }
}
```

</details>

###  Before

![Screenshot 2024-01-03 at 18 02 25](https://github.com/flutter/flutter/assets/48603081/92ae1062-c78f-4005-8e28-85af617acd60)

### After

![Screenshot 2024-01-03 at 18 02 16](https://github.com/flutter/flutter/assets/48603081/2ef97108-9b50-44f7-a303-018ff1b28db6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

FlexibleSpaceBar title is misaligned without the leading widget Invisible SliverAppBar title in Material 3 light theme

2 participants