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

Fix TabbedPage title displaying incorrectly #17039

Merged
merged 9 commits into from
Apr 29, 2024
22 changes: 19 additions & 3 deletions src/Controls/src/Core/NavigationPage/NavigationPageToolbar.cs
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,9 @@ void UpdateBackButton()

// Once we have better logic inside core to handle backbutton visiblity this
// code should all go away.
// Windows currently doesn't have logic in core to handle back button visibility
// Windows currently doesn't have logic in core to handle back button visibility
// Android just handles it as part of core which means you get cool animations
// that we don't want to interrupt here.
// that we don't want to interrupt here.
// Once it's all built into core we can remove this code and simplify visibility logic
if (_currentPage.IsSet(NavigationPage.HasBackButtonProperty))
{
Expand Down Expand Up @@ -273,7 +273,23 @@ void OnPropertyChanged(object sender, System.ComponentModel.PropertyChangedEvent

Color GetBarTextColor() => _currentNavigationPage?.BarTextColor;
Color GetIconColor() => (_currentPage != null) ? NavigationPage.GetIconColor(_currentPage) : null;
string GetTitle() => GetTitleView() != null ? String.Empty : _currentPage?.Title;

string GetTitle()
{
if (GetTitleView() != null)
{
return string.Empty;
}

Page target = _currentPage;

if (_currentNavigationPage?.CurrentPage is TabbedPage tabbedPage && !string.IsNullOrEmpty(tabbedPage.Title))
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this needs to check the page type at all.

I'm testing a few variations of pages on Xamarin.Forms, and the Title always seems to just be whatever the current page is on the navigation page.

image

Is there ever a scenario where
_currentNavigationPage.CurrentPage.Title isn't what we want here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think _currentNavigationPage.CurrentPage.Title should be the correct value in all scenarios.

{
target = tabbedPage;
}

return target?.Title;
}

VisualElement GetTitleView()
{
Expand Down
33 changes: 33 additions & 0 deletions src/Controls/tests/Core.UnitTests/ToolbarTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,39 @@ public async Task TitleAndTitleViewAreMutuallyExclusive()
Assert.Equal("Test Title", toolbar.Title);
}

[Fact]
public void ToolbarTitle_UsesTabbedPageChildTitle()
{
var window = new Window();
jsuarezruiz marked this conversation as resolved.
Show resolved Hide resolved
IToolbarElement toolbarElement = window;
var childPage = new ContentPage { Title = "Child Test Title" };
var tabbedPage = new TabbedPage
{
Children = { childPage },
};
window.Page = new NavigationPage(tabbedPage);

var toolbar = (Toolbar)toolbarElement.Toolbar;
Assert.Equal(childPage.Title, toolbar.Title);
}


[Fact]
public void ToolbarTitle_UsesTabbedPageTitleWhenSet()
{
var window = new Window();
IToolbarElement toolbarElement = window;
var tabbedPage = new TabbedPage
{
Title = "Test Title",
Children = { new ContentPage { Title = "Child Test Title" } },
};
window.Page = new NavigationPage(tabbedPage);

var toolbar = (Toolbar)toolbarElement.Toolbar;
Assert.Equal(tabbedPage.Title, toolbar.Title);
}

[Fact]
public async Task InsertPageBeforeRootPageShowsBackButton()
{
Expand Down
37 changes: 37 additions & 0 deletions src/Controls/tests/DeviceTests/Elements/Toolbar/ToolbarTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,43 @@ public async Task ToolbarTitle()
});
}

[Fact(DisplayName = "Toolbar Uses TabbedPage Title When Set")]
public async Task ToolbarTabbedPageTitle()
Copy link
Member

Choose a reason for hiding this comment

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

I think it's alright to remove these. We have tests that validate that Toolbar.Title makes it to the platform. So, I think the unit tests plus those tests cover what we need.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, removed and added more tests validating the title changing tabs etc.

{
SetupBuilder();
var navPage = new NavigationPage(
new TabbedPage
{
Title = "Tabbed Page Title",
Children = { new ContentPage { Title = "Child Page Title" } },
});

await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), (handler) =>
{
string title = GetToolbarTitle(handler);
Assert.Equal("Tabbed Page Title", title);
return Task.CompletedTask;
});
}

[Fact(DisplayName = "Toolbar Uses TabbedPage Child Title")]
public async Task ToolbarTabbedPageChildTitle()
{
SetupBuilder();
var navPage = new NavigationPage(
new TabbedPage
{
Children = { new ContentPage { Title = "Child Page Title" } },
});

await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), (handler) =>
{
string title = GetToolbarTitle(handler);
Assert.Equal("Child Page Title", title);
return Task.CompletedTask;
});
}

[Theory]
[InlineData($"{nameof(FlyoutPage)}WithNavigationPage, {nameof(ContentPage)}, {nameof(FlyoutPage)}WithNavigationPage")]
[InlineData($"{nameof(FlyoutPage)}WithNavigationPage, {nameof(FlyoutPage)}, {nameof(FlyoutPage)}WithNavigationPage")]
Expand Down