[Android] wire up the correct toolbar to SetSupportActionBar

[PureWeen]

Description of Change

I finished up this PR knowing it would be slightly breaking but then realized it causes the back button on Android to operate differently than iOS which maybe was intentional?

is the purpose of this code

Xamarin.Forms/Xamarin.Forms.Platform.Android/AppCompat/FormsAppCompatActivity.cs

Lines 160 to 170 in 78385f9

	AToolbar bar;
	if (ToolbarResource != 0)
	{
	bar = LayoutInflater.Inflate(ToolbarResource, null).JavaCast<AToolbar>();
	if (bar == null)
	throw new InvalidOperationException("ToolbarResource must be set to a Android.Support.V7.Widget.Toolbar");
	}
	else
	bar = new AToolbar(this);
	SetSupportActionBar(bar);

To make it so that the software back button doesn't fire the OnBackButtonPressed of the contentpage on Android?

The Visible toolbar is created on the NavigationRenderer

Xamarin.Forms/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs

Lines 744 to 759 in ceb2537

	void SetupToolbar()
	{
	Context context = Context;
	var activity = (FormsAppCompatActivity)context;
	AToolbar bar;
	if (FormsAppCompatActivity.ToolbarResource != 0)
	bar = activity.LayoutInflater.Inflate(FormsAppCompatActivity.ToolbarResource, null).JavaCast<AToolbar>();
	else
	bar = new AToolbar(context);
	bar.SetNavigationOnClickListener(this);
	AddView(bar);
	_toolbar = bar;
	}

So SetSupportActionBar(bar); isn't using the right toolbar which means this code never runs

Xamarin.Forms/Xamarin.Forms.Platform.Android/AppCompat/FormsAppCompatActivity.cs

Line 69 in 78385f9

public override bool OnOptionsItemSelected(IMenuItem item)

I came across this while investigating
#4827

I'll fill out the rest if this PR seems valid

Issues Resolved

• fix OnOptionsItemSelected() not called after a configuration/orientation change. #4827

Possibly fixes
#2118
Need to research a bit further

Platforms Affected

• Android

Behavioral/Visual Changes

Clicking the software backbutton on Android will now fire the OnBackButtonPressed on Android

Also now on startup we're only inflating the toolbar view once opposed to twice

Testing Procedure

Run the included Issue

PR Checklist

• Has automated tests
• Rebased on top of the target branch at time of PR
• Changes adhere to coding standard

[PureWeen]

Outside of intentionally setting the SupportActionBar to the wrong instance of the Toolbar and thus breaking software back button events. I'm not quite sure what the purpose of this code is

[hartez]

I doubt that the purpose of setting up the toolbar outside of the NativationPageRenderer was to mess with the back button. That's a lot of effort to go to just to stop the back button from working.

So what you are saying is that your change fixes the issue, but it also causes the back button to behave differently? Can you elaborate on how it's different (and how it compares to iOS?)

[hartez]

Need more info.

[PureWeen]

@hartez so right now the OnBackButtonPressed override on Page only executes for the hardware button on Android and on UWP. If you click the software BackButton (from a navigation page) on iOS or android that method doesn't fire

This PR fixes it on Android but it doesn't on iOS. I know there was a lot of talk around iOS and OnBackButtonPressed but I think that was mainly around not tying swipe to OnBackButtonPressed

If this PR seems valid then we should probably mark this one DNM and I should also make it so OnBackButtonPressed fires for the software button on iOS

Or Merge this one so it gets applied and then schedule an iOS fix for the near future

[hartez]

@hartez so right now the OnBackButtonPressed override on Page only executes for the hardware button on Android and on UWP. If you click the software BackButton (from a navigation page) on iOS or android that method doesn't fire

Yes. And that's the correct behavior, at least according to the documentation. OnBackButtonPressed only applies to the hardware button, and only on platforms which have one (i.e., not iOS):
https://docs.microsoft.com/en-us/dotnet/api/xamarin.forms.navigationpage.onbackbuttonpressed?view=xamarin-forms

This PR fixes it on Android but it doesn't on iOS. I know there was a lot of talk around iOS and OnBackButtonPressed but I think that was mainly around not tying swipe to OnBackButtonPressed

So this PR makes the hardware back button on Android work for NavigationPage after the app resumes?

If this PR seems valid then we should probably mark this one DNM and I should also make it so OnBackButtonPressed fires for the software button on iOS

Or Merge this one so it gets applied and then schedule an iOS fix for the near future

If this fixes the after-resume-hardware-back-button on Android, we should get it merged and have the iOS discussion (if any) elsewhere.

Also, why is this marked with the breaking label?

[PureWeen]

To summarize this a bit first

• None of this has anything to do with the hardware back button
• right now we call SetSupportActionBar with a ToolBar that never sees the light of day. The ToolBar that's created in the NavigationRenderer is what gets renderered but this never gets passed to SetSupportActionBar
• We have code in our base class that looks like it was put there so that the Software Back Button would call OnBackButtonPressed https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Platform.Android/FormsApplicationActivity.cs#L64 as is this code is never called and if it was removed from the current core code there would be no behavior change
• The issue that prompted this fix OnOptionsItemSelected() not called after a configuration/orientation change. #4827 was the user using a popular workaround to get the OnBackButtonPressed firing for the software button but that workaround fails if the NavigationRenderer is recreated during an orientation change or a resume that has to recreate the NAvigationRenderer because then NAvigationRenderer renderers a new ToolBar that doesn't get passed to SetSupportActionBar
• Another way to fix OnOptionsItemSelected() not called after a configuration/orientation change. #4827 would be to add an api to NavigationPage telling it to call that method for a software OnBackButtonPressed https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Platform.Android/FormsApplicationActivity.cs#L64 then we won't break default behavior but will provide something lots of people want :-)

Yes. And that's the correct behavior, at least according to the documentation. OnBackButtonPressed only applies to the hardware button, and only on platforms which have one (i.e., not iOS):
https://docs.microsoft.com/en-us/dotnet/api/xamarin.forms.navigationpage.onbackbuttonpressed?view=xamarin-forms

I'm just curious if that documentation was written because it's always been a bug or the intention initially was for it to work like this?

So this PR makes the hardware back button on Android work for NavigationPage after the app resumes?

No it makes the software back button work after the app resumes or the orientation changes. Really anything that causes the NavigationRenderer to recreate itself. The hardware back button always stays working but the original issues is about how they are trying to make the Software back button work #4827. The workaround in that issue is found a number of places where people suggest how to wire up the OnBackButtonPressed to the Software button. But that workaround breaks whenever the NavigationRenderer is recreated

Since our NavigationRenderer creates a new support bar on Orientation change then the code they have wired up stops working as our current NavigationRenderer doesn't call SetSupportActionBar all we do in our code is set the action bar to some dummy ActionBar that never sees the light of day

Also, why is this marked with the breaking label?
Because it makes it so the software back button now fires OnBackButtonPressed

If we want to honor the original documentation and just never enable the SoftwareBackButton then we should just close #1944, #1413, and #4827 and say it's by design

Though it seems like adding an API to enable the Software Back Button to fire OnBackButtonPressed on all three platforms would be super helpful

[hartez]

I'm just curious if that documentation was written because it's always been a bug or the intention initially was for it to work like this?

AFAIK, it has always worked this way. And that makes sense - the hardware back button is not the same thing as "navigating back". The hardware back button also serves functions like dismissing keyboards, nav drawers, and other stuff. The "back button" provided by the NavigationPage is a distinct UI concept from any hardware buttons provided on Android or UWP.

But looking at the comments in #1944 and #1413, it's clear that the current API (and years of workaround cruft) is causing a bunch of confusion. We need to fix it, but not as part of this PR.

[hartez]

Looks good, just needs minor changes.

[PureWeen]

the hardware back button is not the same thing as "navigating back". The hardware back button also serves functions like dismissing keyboards, nav drawers, and other stuff. The "back button" provided by the NavigationPage is a distinct UI concept from any hardware buttons provided on Android or UWP.

Only thing is that UWP calls OnBackButtonPressed when using the navigate back button.

But yea we should probably provide a specific OnNavigatingBack or similar that ties the navigation concept more directly to intent. As is people keep overriding OnBackButtonPressed and then some permutation of what they test stops the navigation so they think they're good to go.

Maybe even adding a context args to OnBackButtonPressed would help so they know exactly what action is happening they want to respond to.

[PureWeen]

@hartez so I deleted this from our code since it's only purpose (AFAICT) is to call BackPressed when the user clicks the software back arrow. Maybe there's a use case I'm forgetting about though? This allows users to just override OnOptionsItemSelected like I do here https://github.com/xamarin/Xamarin.Forms/pull/4893/files#diff-48c5e139c569153d4219aa3bbb433ceaR104 and will thus resolve #4827

[PureWeen]

@hartez @kingces95 I added DNM for now
Hold off on reviewing. I need to test a few more things first

[PureWeen]

build --uitests

[PureWeen]

alright @hartez @kingces95 ready for review

[kingces95]

Can do a review now. But do we wait for tests to pass?

[PureWeen]

@kingces95 just wait for now

I ran into some additional issues with this PR that will need to be addressed first which is probably why the tests are failing

[pauldiston]

Having upgraded from XF 3.3 to XF 4.0 yesterday I am experiencing a number of issues, one being OnOptionsItemSelected isn't called any longer, which I presume is a result of the incorrect toolbar being wired up, hence this PR.
For clarification, this comment "samhouts removed this from Closed in vNext (4.1.0) 9 days ago", does this mean that this issue will be resolved in XF 4.1?
I have tried the setting of the toolbar "manually" from (https://forums.xamarin.com/discussion/67854/android-menu-and-back-button-not-working) but I don't seem to be able to get this working. If I have to wait until XF 4.1 for this to be resolved then so be it, as I am already waiting for issues to be resolved with the CarouselView which is planned for XF 4.1 as well.

[pauldiston]

@samhouts Would you be able to provide any information regarding the question I had about the comment?

[samhouts]

@pauldiston This was not merged, so it will not be resolved yet. There was a workaround described in #4827 (comment) that may help you.

@PureWeen fyi

[pauldiston]

@samhouts Having parked this issue back in June 2019, I am back again with the same issue in version 4.5.0.430. Having tried a work around I found here (https://theconfuzedsourcecode.wordpress.com/2017/03/02/formsappcompatactivity-is-not-calling-onoptionsitemselected-xamarin-android/) I am now running into this issue (#2118).
There must be a way of handling the OnOptionsItemSelected without having all these problems.

[pauldiston]

@samhouts After further investigation, is seems that everytime I want to set the toolbar items for a page I need to call SetSupportActionBar, doing it in the OnCreate is just not enough. Time to add my SetToolbar method call to each page that has toolbar items.