Skip to content

Conversation

@TahaTesser
Copy link
Member

fixes TabBar with isScrollable set to true is broken

This regressed due to a tiny mistake in #146293

Code sample

expand to view the code sample
import 'dart:ui';

import 'package:flutter/material.dart';

/// Flutter code sample for [TabBar].

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

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

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

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

  @override
  Widget build(BuildContext context) {
    final List<Tab> tabs =
        List<Tab>.generate(20, (int index) => Tab(text: 'Tab $index'));

    return ScrollConfiguration(
      behavior: ScrollConfiguration.of(context)
          .copyWith(dragDevices: <PointerDeviceKind>{
        PointerDeviceKind.touch,
        PointerDeviceKind.mouse,
      }),
      child: DefaultTabController(
        length: tabs.length,
        child: Scaffold(
          appBar: AppBar(
            title: const Text('TabBar Sample'),
            bottom: TabBar(
              isScrollable: true,
              tabs: tabs,
              tabAlignment: TabAlignment.start,
            ),
          ),
          body: TabBarView(
            children: <Widget>[
              for (int i = 0; i < tabs.length; i++)
                Center(
                  child: Text('Page $i'),
                ),
            ],
          ),
        ),
      ),
    );
  }
}

Before

before.mov

After

after.mov

Pre-launch Checklist

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 Jun 11, 2024
@TahaTesser TahaTesser marked this pull request as ready for review June 11, 2024 14:15
@TahaTesser TahaTesser requested review from Piinks and goderbauer June 11, 2024 14:15
@PerLycke
Copy link

Even though this fix works to some extent, it doesn't fix "TabBar with isScrollable set to true is broken" as with the changes proposed in this PR the TabBar jumps after swiping to Tab 3 and back to Tab 2:

Screen.Recording.2024-06-13.at.10.27.41.mov

@TahaTesser TahaTesser marked this pull request as draft June 13, 2024 08:41
@TahaTesser
Copy link
Member Author

@PerLycke
Thank you so much for testing the PR code. I'm out of office today, I'll address your issue first thing tomorrow.

@TahaTesser TahaTesser force-pushed the fix_scrollable_tab_bar_jitter branch from 0d1c142 to 1a31012 Compare June 14, 2024 10:34
@TahaTesser
Copy link
Member Author

@PerLycke
I just updated the fix and added a new test for the jitter at the start of the scrollable TabBar.

Could you please try it and confirm it works as expected?

@TahaTesser TahaTesser marked this pull request as ready for review June 14, 2024 11:30
@PerLycke
Copy link

@TahaTesser I can confirm it now works as expected 👍

@TahaTesser TahaTesser changed the title Fix scrollable TabBar jitters in the middle position Fix scrollable TabBar jittering Jun 17, 2024
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

Tricky, good catch. LGTM 👍

FYI @nate-thegrate

Comment on lines -1556 to -1557
< 0 => lerpDouble(middlePosition, leadingPosition, index - value)!,
_ => lerpDouble(middlePosition, trailingPosition, value - index)!,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching my mistake here!

(And thanks @justinmc for letting me know!)

Comment on lines 1551 to 1557
final double offset = switch ((value, index)) {
(_, _) when value == index - 1.0 => leadingPosition ?? middlePosition,
(_, _) when value == index + 1.0 => trailingPosition ?? middlePosition,
(_, _) when value == index => middlePosition,
(_, _) when value < index => leadingPosition == null ? middlePosition : lerpDouble(middlePosition, leadingPosition, index - value)!,
(_, _) => trailingPosition == null ? middlePosition : lerpDouble(middlePosition, trailingPosition, value - index)!,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a bit weird to pass a record into a switch expression and then not use it.

Suggested change
final double offset = switch ((value, index)) {
(_, _) when value == index - 1.0 => leadingPosition ?? middlePosition,
(_, _) when value == index + 1.0 => trailingPosition ?? middlePosition,
(_, _) when value == index => middlePosition,
(_, _) when value < index => leadingPosition == null ? middlePosition : lerpDouble(middlePosition, leadingPosition, index - value)!,
(_, _) => trailingPosition == null ? middlePosition : lerpDouble(middlePosition, trailingPosition, value - index)!,
};
final double offset = switch (value - index) {
-1.0 => leadingPosition ?? middlePosition,
1.0 => trailingPosition ?? middlePosition,
0 => middlePosition,
< 0 => leadingPosition == null ? middlePosition : lerpDouble(middlePosition, leadingPosition, index - value)!,
_ => trailingPosition == null ? middlePosition : lerpDouble(middlePosition, trailingPosition, value - index)!,
};

I believe the logic is identical, based on a local test run.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

This is better, thanks!

Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

I really appreciate the effort put into correcting a previous error I made.

If we want to just merge right away, that's good with me 👍

@TahaTesser TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 18, 2024
@auto-submit auto-submit bot merged commit a9f554a into flutter:master Jun 18, 2024
@TahaTesser TahaTesser deleted the fix_scrollable_tab_bar_jitter branch June 18, 2024 08:31
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 20, 2024
Manual roll requested by [email protected]

flutter/flutter@ccf3abe...6c06abb

2024-06-18 [email protected] Add test for engine artifact framework permissions (flutter/flutter#148786)
2024-06-18 [email protected] Add test for icon_button.3.dart (flutter/flutter#149988)
2024-06-18 [email protected] Roll Flutter Engine from 78fdd06af541 to 74f42ca3544c (6 revisions) (flutter/flutter#150421)
2024-06-18 [email protected] Fix transparent `dividerColor` breaks `TabBar.tabAlignment` (flutter/flutter#150350)
2024-06-18 [email protected] Fix scrollable `TabBar` jittering (flutter/flutter#150041)
2024-06-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland 3: [CupertinoActionSheet] Match colors to native (#150386)" (flutter/flutter#150413)
2024-06-18 [email protected] Extend the Windows web_tool_tests_1_2 shard timeout to 45 minutes (flutter/flutter#150393)
2024-06-18 [email protected] Roll Flutter Engine from 1c4e5e230ecb to 78fdd06af541 (3 revisions) (flutter/flutter#150403)
2024-06-18 [email protected] Roll Flutter Engine from a4f266f7eb1a to 1c4e5e230ecb (8 revisions) (flutter/flutter#150399)
2024-06-18 [email protected] Rename doc file to use standard hyphens (flutter/flutter#150314)
2024-06-17 [email protected] Fix typo in `SliverLayoutDimensions.hashCode` where not all properties are used in the hash code. (flutter/flutter#150306)
2024-06-17 [email protected] Fix doc comment references to 'this' (flutter/flutter#150379)
2024-06-17 [email protected] Add 'fail-fast' argument to flutter test (flutter/flutter#149587)
2024-06-17 [email protected] Update matchesGoldenFile documentation reference to goldenFileComparator (flutter/flutter#150343)
2024-06-17 [email protected] Reland 3: [CupertinoActionSheet] Match colors to native (flutter/flutter#150386)
2024-06-17 [email protected] [a11y] Add semantics: button to bottom navigation bar items and dropdown menu items (flutter/flutter#149375)
2024-06-17 [email protected] Reland "sliverGridDelegate mainAxisExtent add assert (#148470)"  (flutter/flutter#149720)
2024-06-17 [email protected] `ScaffoldBackgroundColor` should default to `ColorScheme.surface` (flutter/flutter#149772)
2024-06-17 [email protected] Reland TreeSliver  (flutter/flutter#149839)
2024-06-17 [email protected] Reland: [CupertinoActionSheet] Add sliding tap gesture (flutter/flutter#150219)
2024-06-17 [email protected] Roll Flutter Engine from 5989f0215fed to a4f266f7eb1a (1 revision) (flutter/flutter#150377)

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] 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
@nate-thegrate nate-thegrate mentioned this pull request Oct 14, 2024
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TabBar with isScrollable set to true is broken

4 participants