CommunityToolkit / WindowsCommunityToolkit

The Windows Community Toolkit is a collection of helpers, extensions, and custom controls. It simplifies and demonstrates common developer tasks building .NET apps with UWP and the Windows App SDK / WinUI 3 for Windows 10 and Windows 11. The toolkit is part of the .NET Foundation.
https://docs.microsoft.com/windows/communitytoolkit/
Other
5.9k stars 1.37k forks source link

How can I handle MasterDetail Back with NavigationView BackButton? #2475

Closed mvegaca closed 6 years ago

mvegaca commented 6 years ago

Handle MasterDetail Back with NavigationView BackButton

Current behavior

In Windows Template Studio we are creating MasterDetail pages using the Windows Community Toolkit Control in different project types (Blank, Navigation Pane, Pivot).

In Navigation Pane, we are making different Proof of concepts to integrate the WinUI NavigationView instead of SDK NavigationView and also move the go back navigation from Back System Button to NavigationView Back Button.

We would like to know hoy to specift to MasterDetail control to stop using the Syste Navigation Button to use only the NavigationView button.

You can read the original Issue and get the PoCApp.

image

Windows 10 Build Number:

App min and target version:

nmetulev commented 6 years ago

@skendrot might be able to help here

touseefbsb commented 6 years ago

seems @skendrot is busy, can someone else have a look at it? @nmetulev ?

skendrot commented 6 years ago

Hey, was on vacation, thanks for the additional ping. I don't think we currently handle this, but it is something we've been talking about doing. We had talked about a way to just turn off showing/hiding/using the system back button. But I could also see that maybe we could integrate with the new navigation view as well. What do you think about the following:

skendrot commented 6 years ago

We could also have an enum to control the back button System, NavigationView, Off

nmetulev commented 6 years ago

I like having the enum to control the behavior, that seems like the most developer friendly way to handle it. Having a way to turn it off completely and let the developer handle back navigation also makes sense for future scenarios that we can't support immediately

touseefbsb commented 6 years ago

how about having an enum for this and also turning off system back button by default and then developers can turn it on if they want to? Because the new back button navigation within NavigationView is being encouraged in the docs going forward.

nmetulev commented 6 years ago

IMO, that would create a behavior change for existing users unless we can detect the new navigation view. @skendrot, what do you think?

touseefbsb commented 6 years ago

yeah that is a fair point, keeping existing users in check is also very important, I agree.

skendrot commented 6 years ago

That was mentioned in the first comment. Keep things as they are, but see if we have a new NavigationView as a visual parent. If so, we should just use that for the back button instead of the system back button. I think we should still provide a way to turn it off completely if a developer wants to control everything themself

touseefbsb commented 6 years ago

yes @skendrot options are always flexible and always good to have specially when making a major transition. So how should this move forward now?

skendrot commented 6 years ago

Whatever option is decided, I'll get that into the control. We need to decide, are we going to

  1. Use the standard back button and "light-up" if the new navigation view is in place AND offer a way to turn off the back button completely
  2. Offer a new enum option that allows the developer to pick between standard back button, navigation view back button, and off

Vote 👍 for 1 and 🎉 for 2

nmetulev commented 6 years ago

I'd combine these two and use the enum to control how the back button lights up by providing three values:

mdtauk commented 6 years ago

I would suggest, rather than Automatic, Legacy and Off, you provide enum options that are clear in what they will do. For example

BackButtonBehaviour:

DisplayInline would be default, following the guidance of displaying a standard styled back button within the control. This could also work with the B button on the gamepad, or the backspace key on a keyboard when the control has focus.

UseExternal would allow you to set in code, a back button control you place yourself, the shell control (in the title bar for standard windows, or in the lower bar for a Sets window) or from another control. I guess you could make it possible to provide a control name in XAML which would trigger and override the back button pressed event for that control.

BackDisabled would disable back navigation entirely.

Devs and projects like Template 10, could then create a page or template page with the settings in place that allow them to control the back button usage.

JustinXinLiu commented 6 years ago

Agree with @mdtauk!

touseefbsb commented 6 years ago

@mdtauk yes definitely this suggestion makes more sense and seems more powerful.

nmetulev commented 6 years ago

I'm completely ok with creating a back button inside of the control, which matches with the current back button guidance.

However, we should also provide automatic handling for the NavigationView back button as well. I don't think it makes sense to disable back navigation as well, that should be something the user should handle

Here is my updated suggestion

Enum: BackButtonHandling

skendrot commented 6 years ago

Instead of Legacy how about System or SystemAppViewBackButton or AppView or something else

skendrot commented 6 years ago

Also, to maintain backward compatibility, and not have a breaking change, shouldn't the default be the legacy option. Then in the next major version we can change the default to be auto

nmetulev commented 6 years ago

I like System better than legacy, that's much better

I'm not particularly pationate about the default value either way since next version is a major update (5.0). I'm ok with keeping it default to System and then changing the default to Automatic in 6.0.

touseefbsb commented 6 years ago

and there can be a notice as a warning that System back button will be deprecated in a future release ( 6.0 ) something like that? or will the system back button always be an option, even after the sets are released (probably in april 2019 release )?

nmetulev commented 6 years ago

We probably won't deprecate System back button until it's deprecated from the platform.

mdtauk commented 6 years ago

Even with Sets, the system back button is not being deprecated. It will just place it into a bar below the tabs with a back button on it. But if Sets is there, you should absolutely switch to an in app button

image

nmetulev commented 6 years ago

Right, I should clarify: We should only deprecate the option from the enum if it will also be deprecated from the platform (I don't believe there are any plans to do so)

touseefbsb commented 6 years ago

so apparently there is a whole toolbar just for 1 backbutton ( in case of sets ) ? or will there be more platform controls on it?

mdtauk commented 6 years ago

so apparently there is a whole toolbar just for 1 backbutton ( in case of sets ) ? or will there be more platform controls on it?

I don't know of any additional controls the shell would add. But it is because this is a far from elegant solution for Back Navigation - that the guidance is to handle it in app.

If you think about it, the back button is scoped within a Sets Tab. It wouldn't appear alongside tabs, and so can't appear in the Titlebar.

touseefbsb commented 6 years ago

yeah it just looks too much wasted UI space there, just 1 button there.

mdtauk commented 6 years ago

@touseefbsb As of right now, that is what the standard behaviour will be if you choose to let the Shell handle the Back Button with a Sets window. As Sets is still not included in Windows builds, this may change, but better to enable the control/app shell to handle drawing a back button, and move away from having the Titlebar handle it.

touseefbsb commented 6 years ago

I agree, that is why I also think moving away from titlebar stuff and not even extending the app into title bar at all will help the app being used properly with sets.

mdtauk commented 6 years ago

Sets may not even make it to the 19H1 release, so for the time being, it is important for apps to look good and extend into titlebars when possible. Devs can also choose not to allow their app to work with Sets, so having the flexibility with the control, and altering/overriding the default when running in a Sets tab, makes sense. At least it does to me lol

touseefbsb commented 6 years ago

@mdtauk if devs opt their apps out of sets, does that mean their app will not be opened into sets?

mdtauk commented 6 years ago

@touseefbsb That is how I understand it. When opened from another Tab it will appear in its own window, wont be able to be stored along with other tabs either.

skendrot commented 6 years ago

Small complication to support the new NavigationView. The BackRequested event handler does not contain a way to cancel the event or mark that it has been handled. Without this ability the MasterDetailsView would handle moving from Details to Master views and then the developer would handle going back in state as well. We can support IFF a frame is being used as well to aid navigation.

nmetulev commented 6 years ago

@mvegaca, do you use a frame to host the control? What if instead trying to handle back on the NavView, we handle back on a Frame (that should handle scenarios outside of the NavView too)?

skendrot commented 6 years ago

We already handle frame navigation so that's already built in. We just can't rely on using the new NavigationView unless there is a frame also

nmetulev commented 6 years ago

So if WTS is using a Frame for their navigation inside of the NavView, it should already work?

skendrot commented 6 years ago

Yes, with unwanted extras. The MasterDetailsView will enable the system back button, which WTS doesn't want. But, if they handle the NavigationView back button by navigating in a frame, then the MasterDetailsView will catch that event, and if it is the collapsed Details state, will mark it as cancel and move back to the master view.

sibille commented 6 years ago

We (WTS) are using a frame inside the NavView. The navview backbutton is already working, but an extra system back button is showing. To handle back navigation we do Frame.GoBack, but our problem is the system back button shown before going back. So, I'm not sure if we're fine. Is there a way to test this?

nmetulev commented 6 years ago

Yeah, that's the issue @skendrot's PR is fixing (#2561). It would be amazing if you can test it as well.

mvegaca commented 6 years ago

Hi @nmetulev I'm trying to test the new BackButtonBehaviorProperty in a git repository clone of @skendrot but I can't compile the app targeting directly to the library. Can you approve the PR to test this changes in MyGet CI packages?

Thanks

skendrot commented 6 years ago

@mvegaca I had the same problem when trying to test. I could not reference my local copy of the assemblies. We need to figure out what is going on here because it makes testing issues a lot more difficult

nmetulev commented 6 years ago

What are the issues you are having with testing the assemblies?

mvegaca commented 6 years ago

We've added a PoC app in the WCT solution, remove nuget package reference and reference directly to the projects. When I compile the project I got the following error in all XAML files:

C:\dev\skendrot\UWPCommunityToolkit\Issue2475PoC\Issue2475PoC.csproj : XamlCompiler error WMC1013: XAML files 'App.xaml' and 'App.xaml' have the same project path 'App.xaml'.  Each file must have a unique project path.

This also happens if I add a empty new UWP App1 to solution.

XAML files 'App.xaml' and 'App.xaml' have the same project path 'App.xaml'.  Each file must have a unique project path.
App1    C:\dev\skendrot\UWPCommunityToolkit\App1\App1.csproj        

Thank you @nmetulev ;)

nmetulev commented 6 years ago

I recommend building the nugets locally and referencing those. You can do it by using the build\build.ps1 script which should generate the nuges in the bin folder.

skendrot commented 6 years ago

@mvegaca Same issues I was seeing

nmetulev commented 6 years ago

The projects can't be referenced directly unfortunatly as they depend on the solution configuration. For development, I tend to link the source files directly in new project as that seems to work much better and faster that developing in the sample app. For testing, I always build the nugets locally and reference those

mvegaca commented 6 years ago

We've tested it locally and it works fine 😄 You can merge the PR if all it's ok for you.

Thank you all guys

image

nmetulev commented 6 years ago

PR merged

mvegaca commented 6 years ago

Hi @nmetulev

I've tried to use it in pre-release version 5.0.0-preview.gb86cb1c4cb but the property BackButtonBehavior is not available.

When do you think that this fix will be available on the pre-release version and will be available on the stable version?

Thanks

nmetulev commented 6 years ago

Yeah, it will be available in the 5.0.0 release next week. It's also available on pre-release on MyGet: https://dotnet.myget.org/gallery/uwpcommunitytoolkit