🐛 : FIX : Long labels overflowing in extended navigation rail.

aliasgar4558 commented 1 month ago

It has been observed that while in extended navigation rail, if longer labels are given the it is overflowing. Problem identified in "NavigationRail" widget only in case of extended version of it. This PR includes the fix for the same.

Fixes https://github.com/flutter/flutter/issues/110901.

NO breaking changes.

Pre-launch Checklist

[x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
[x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
[x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
[x] I signed the [CLA].
[x] I listed at least one issue that this PR fixes in the description above.
[x] I updated/added relevant documentation (doc comments with ///).
[x] I added new tests to check the change I am making, or this PR is [test-exempt].
[x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
[x] All existing and new tests are passing.

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

aliasgar4558 commented 1 month ago

Closed by mistake, re-opened the PR.

dkwingsmt commented 1 month ago

When I run it locally, with you additional Expanded, the navigation_rail_test throws tons of errors for multiple tests. And even after I revert your changes (removing the Expanded), your test throws the following assertion:

'package:flutter/src/material/navigation_rail.dart': Failed assertion: line 121 pos 16: '!extended
|| (labelType == null || labelType == NavigationRailLabelType.none)': is not true.

Yet somehow all checks have passed. Have you run it locally? I don't know what's happening.

aliasgar4558 commented 1 month ago

@dkwingsmt - What are the configuration that I need to make to run the tests additionally, as when I tried to run the test, it didn't allowed me to execute.

dkwingsmt commented 1 month ago

What do you mean it doesn't allow you to execute? To run the test you cd into packages/flutter then run

flutter test test/material/navigation_rail_test.dart

aliasgar4558 commented 1 month ago

Sure, will check and update here accordingly

aliasgar4558 commented 1 month ago

@dkwingsmt - is 'dev' channel fine to work on this one? Or shall I verify on master or some other environment?

dkwingsmt commented 1 month ago

Any branch should be OK, since the navigation rail hasn't been changed for a while.

aliasgar4558 commented 1 month ago

I tried with stable channel, but it shows several compile time errors which are related to conflicts into pubspec versions, so thats the purpose of asking if I can try with dev or master instead. Mind me asking, what env did you used to verify the changes?

dkwingsmt commented 1 month ago

I'm using the main channel. Can you try with that?

aliasgar4558 commented 1 month ago

Yeah sure, Allow me some time. Will check out in main channel

aliasgar4558 commented 1 month ago

@dkwingsmt - Do we have to make any changes in pubspec in flutter/packages/flutter' folder, as I am unable to executepub get` there.?

aliasgar4558 commented 1 month ago

@dkwingsmt - You are right, 18 tests are breaking. However, I found one issue with my test, so will update that and commit into this PR.

I don't feel like I shall change any other tests as it might result into something else to break. Please suggest your thoughts!

aliasgar4558 commented 1 month ago

@dkwingsmt - Please check if you can verify now, as I am not getting any test breaks.

Root cause : Expanded widget was the one causing issue as the widget is not bounded by parent. Remedy : Replaced with Flexible

dkwingsmt commented 1 month ago

All tests now passes, but after I revert your changes (removing the Flexible wrap) your test still passes. Can you check it out?

aliasgar4558 commented 1 month ago

@dkwingsmt - sure, I will add more fine grain test which checks for "Flexible" with the longer texts.

dkwingsmt commented 1 month ago

The idea is that the unit test you add should fail before your change and pass after your change, so that it prevents your change from being reverted in the future. One unit test is enough as long as it's on the point.

aliasgar4558 commented 1 month ago

@dkwingsmt - Yes I agree. Sure thing, will update the test so in future if something changes, it can be indicated. Thanks for the insights.

aliasgar4558 commented 1 month ago

@dkwingsmt - Please have a check and let me know anything else we can add to make it better.

dkwingsmt commented 1 month ago

Your test checks if the text can be found within a Flexible. It's not testing the feature, but the implementation. What you really care is that no overflow is reported, instead of whether it's implemented via Flexible or any other widgets.

And in fact, your implementation does not solve the problem. Here's what I got from running your fix (which is the same as the current main branch):

How to test it:

Open examples/api/lib/material/navigation_rail/navigation_rail.1.dart

Change line 73 to a really long label, such as

          destinations: const <NavigationRailDestination>[
            NavigationRailDestination(
              icon: Icon(Icons.favorite_border),
              selectedIcon: Icon(Icons.favorite),
              label: Text('LongLongLongLongLongLongLongLongLongLongLongLongLongLongLongLongLongLongLongLongLongLong'),
            ),

Run it, for example,

flutter run ~/dev/flutter/examples/api/lib/material/navigation_rail/navigation_rail.1.dart -d chrome

Make the window really small

aliasgar4558 commented 1 month ago

@dkwingsmt - I think problem is with how navigation rail is used in UI. It might require width constraints in normal case scenario also. Suggest your insights please!

When I revert this change, then also it is causing issue. Please do have a check and suggest!

dkwingsmt commented 1 month ago

When I revert this change, then also it is causing issue.

That's what I'm saying: this is the issue, and your current change sadly isn't fixing the problem.

Or did I misunderstand something? Can you demonstrate with an app what your change is fixing?

aliasgar4558 commented 1 month ago

yes @dkwingsmt - There are 2 issues basically here. First, what I fixed - that is when long text added, it shall not overflow (wrapped in flexible) & second is NavigationRail without max width constraints is over flowing when size is compressed (what you have faced - in your case, if you wrap navigation rail with SizedBox & give width, then it will not overflow)

for first point, I have made the fix & it will work when navigation rail is available with max width constraint but the second thing that you faced, for that we need to take a change. [We can raise a bug for that for "NavigationRail"]

For 2nd point - I think we can introduce "maxWidth" parameter in NavigationRail which will help to constraint its width. [New change]

aliasgar4558 commented 1 month ago

Allow me some time, I will send the code snippet here - which will demonstrate my fix with an example for you.

aliasgar4558 commented 4 weeks ago

@dkwingsmt any update on this one mate?

dkwingsmt commented 3 weeks ago

@dkwingsmt any update on this one mate?

Hi! I'm still waiting for the snippet you mentioned. Can you demonstrate the scenario that this PR fixes? Such as some comparison screenshots would be even better.

aliasgar4558 commented 3 weeks ago

@dkwingsmt Ohh, Okay sure. I will be sending that across. Thank you

aliasgar4558 commented 3 weeks ago

This is the problem you faced when you shrink the screen size very small.
But the problem is not with the fix that I made, it is because of "NavigationRail"'s width is not constraint & the other view is crashing/overflowing.

Example screenshot when NavigationRail's width is given :

Now below is the screenshot of what I fixed. [Same width given to navigation rail & maxLines specified for longer rail destination title.]

-- So from above screenshots, it seems like NavigationRail shall be provided with width constraints in consumer applications so other views can be rendered correctly.

@dkwingsmt - Please have a check on this one & you can try the same thing out & let me know if anything. (I referred to navigation_rail.1.dart file for demo purpose)

dkwingsmt commented 2 weeks ago

(Sorry I missed your PR last week) Thanks for your screenshot comparison, which are super clear about what was fixed.

Now, my problem with your unit test is that, it tests an implementation detail ("the text must be wrapped within a Flexible widget") instead of the behavior ("the text must wrap / the max-line must be triggered"). The widget might be refactored in the future with same functionality implemented differently, and you want your test to still be valid. My suggestion is that your test can check the height of the widget in this setup and ensure it's below some number that wasn't possible before the PR, which I think is one of the most observable difference.

aliasgar4558 commented 1 week ago

Not a problem for the delay. Thanks for the valuable insights.

However, I didn't got your point to check what widget's height while in test. Can you please let me know more details on it ?

aliasgar4558 commented 1 week ago

@dkwingsmt - Updated test case to check Text widget's height value & compared with the other smaller destination's height (which is a normal one - which can be adjusted in a single line). Hope this is what you are expecting.

dkwingsmt commented 1 week ago

I ran the widget in your test individually but the result is not the desired one as you demonstrated.

Before PR: (with exception)

After PR:

Although they're indeed different, they do not demonstrate the ellipsis fix that you showed earlier.

I found out that it was because you missed the "ellipsis" setting for Text in your test. With the "ellipsis",

Before PR: (with exception)

After PR:

Anyway, since with either cases the before-PR version throws a layout exception, I think we can omit checking anything here.

aliasgar4558 commented 1 week ago

@dkwingsmt - Yes, ellipsis was intentionally not added in tests to demonstrate that text would be wrapped if navigation rail has a width constraint, so it can be determined that Flexible is making some difference.

So, Do you suggest we can exclude the test cases for this fix?

aliasgar4558 commented 1 week ago

I've applied the changes you suggested. @dkwingsmt - Thank you for the valuable insights

bleroux commented 1 week ago

@aliasgar4558 Sorry for the burden, you will have to rebase this PR on the master branch (you can close this one and file a new one if it is easier for you), main branch can not be targeted for PRs on flutter/flutter (the bots won't work with it).

FYI, for the other Flutter repos, main branch is ok, especially for flutter/engine but this is not the case for flutter/flutter.

aliasgar4558 commented 1 week ago

@bleroux - Thanks for the suggestions. It seems good to have those in the PR so will add that.

Also, Just to confirm - instead of main branch, we have to target this PR to master branch. Is that correct?

bleroux commented 1 week ago

Also, Just to confirm - instead of main branch, we have to target this PR to master branch. Is that correct?

Yes, this is required in flutter/flutter. Sorry for the burden.

aliasgar4558 commented 1 week ago

@bleroux - Not problem at all. I am here to learn and contribute & you guys are helping me. So all good. Will do the needful and update PR.

aliasgar4558 commented 1 week ago

@bleroux - I have updated test as per your suggested changes & now PR is targeting to master branch. Please review once you get some time.

aliasgar4558 commented 6 days ago

@dkwingsmt @bleroux - Thank you so much for your valuable insights for this PR. Really Appreciate it 😊

dkwingsmt commented 6 days ago

Thank YOU for the contribution, and continuously addressing our comments!

flutter / flutter

🐛 : FIX : Long labels overflowing in extended navigation rail. #145474

Pre-launch Checklist