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

NavigationRail isExtended:true destination label highlight has the wrong width #117126

Closed
HansMuller opened this issue Dec 15, 2022 · 8 comments · Fixed by #117320
Closed

NavigationRail isExtended:true destination label highlight has the wrong width #117126

HansMuller opened this issue Dec 15, 2022 · 8 comments · Fixed by #117320
Assignees
Labels
c: regression It was better in the past than it is now f: material design flutter/packages/flutter/material repository. found in release: 3.7 Found to occur in 3.7 framework flutter/packages/flutter repository. See also f: labels. has reproducible steps The issue has been confirmed reproducible and is ready to work on

Comments

@HansMuller
Copy link
Contributor

HansMuller commented Dec 15, 2022

The destination label highlight for a M3 NavigationRail with isExtended:true has the wrong width or the wrong origin.

This is a regression for useMaterial3:true. It's possible (I haven't checked) that it was introduced in #116108.

Here's an example that demonstrates the problem: https://dartpad.dev/?id=272d62da895a9e17c38c7f562a6d3d5f&channel=master

Screen Shot 2022-12-14 at 6 52 49 PM

@HansMuller
Copy link
Contributor Author

CC @darrenaustin
A fix for this may need to be cherry-picked

@exaby73 exaby73 added in triage Presently being triaged by the triage team c: regression It was better in the past than it is now framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. has reproducible steps The issue has been confirmed reproducible and is ready to work on found in release: 3.7 Found to occur in 3.7 and removed in triage Presently being triaged by the triage team labels Dec 15, 2022
@TahaTesser
Copy link
Member

TahaTesser commented Dec 15, 2022

Apologies, I'm working on this. I'm also accounting for custom padding from destinations which also messes up highlight position.

Maybe get rid of the _IndicatorInkWell class and move InkResponse to the indicator itself and wrap a custom render box with expands hitTest area of the destination.
Something that chip.dart has:
https://github.com/flutter/flutter/blob/dbc9306380d8a72273b478b8fcc934a6014f946d/packages/flutter/lib/src/material/chip.dart#LL1332C17-L1332C17

This doesn't work without refactoring a whole lot of code.

A better getRectCallback is needed that use the exact position of the NavigationIndicator.

@Piinks
Copy link
Contributor

Piinks commented Dec 16, 2022

I haven't been able to reproduce this yet. I wonder if there is more than one bug here. Going to the dartpad link above does not look like it is actually using material 3.

Screen Shot 2022-12-16 at 11 58 24 AM

@TahaTesser
Copy link
Member

This should be reproducible on the beta channel. The current stable doesn't have this

@Piinks
Copy link
Contributor

Piinks commented Dec 16, 2022

Yeah I have it running on master, and for some reason dartpad wont let me change the channel using the button at the bottom

@TahaTesser
Copy link
Member

TahaTesser commented Dec 19, 2022

I've attempted to fix this issue: #117320 and added regression tests.

Unrelated to this regression, after the holidays, I want to streamline the NavigationRail class, it's not as clean and flexible as the new NavigationBar.

  1. It doesn't have a smarter destination rendering. Everything is rendered in a column with lots of SizedBox widgets.
  2. minWidth and minExtendedWidth behave weirdly.
  3. It could also fix: [Proposal] Find a way to reuse code fron NavigationDestination for widgets other than NavigationBar  #104813 NavigationRailDestination in selected mode doesn't shift in M3 #116298

@Piinks
Copy link
Contributor

Piinks commented Dec 19, 2022

Thanks @TahaTesser!

@github-actions
Copy link

github-actions bot commented Mar 4, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: regression It was better in the past than it is now f: material design flutter/packages/flutter/material repository. found in release: 3.7 Found to occur in 3.7 framework flutter/packages/flutter repository. See also f: labels. has reproducible steps The issue has been confirmed reproducible and is ready to work on
Projects
Status: Done (PR merged)
Development

Successfully merging a pull request may close this issue.

4 participants