Ensure system back event is passed to child page once - #21246
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates how PageNavigationSystemBackButtonPressedEvent is routed and handled across Avalonia Page types to prevent recursive event forwarding (and resulting StackOverflow) when pages are hosted inside PageNavigationHost.
Changes:
- Changed
PageNavigationSystemBackButtonPressedEventrouting strategy fromBubbletoDirectand removed the global forwarding class handler onPage. - Added per-page class handlers to host pages (e.g.,
TabbedPage,CarouselPage,DrawerPage) to perform page-specific back behavior and then forward toCurrentPageonce. - Added a
ContentPagehandler to invokeOnSystemBackButtonPressed()without forwarding.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Avalonia.Controls/Page/TabbedPage.cs | Adds a class handler to handle/forward system back to CurrentPage. |
| src/Avalonia.Controls/Page/Page.cs | Switches system-back routed event to Direct and removes the base class forwarding handler. |
| src/Avalonia.Controls/Page/DrawerPage.cs | Updates system-back handler to early-exit when handled, handle drawer behavior, then forward to CurrentPage. |
| src/Avalonia.Controls/Page/ContentPage.cs | Adds a system-back handler that invokes OnSystemBackButtonPressed() only. |
| src/Avalonia.Controls/Page/CarouselPage.cs | Adds a class handler to handle/forward system back to CurrentPage. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
Added tests |
…nSystemBackButtonPressed
|
You can test this PR using the following package version. |
jsuarezruiz
left a comment
There was a problem hiding this comment.
The fix direction looks good overall, especially changing the forwarded child raises to use fresh RoutedEventArgs.
One quesiton: the base Page class handler added back in the last commit now runs alongside the concrete class handlers. Since Direct routed events still invoke all matching class handlers for the sender type, a ContentPage, TabbedPage, CarouselPage, or DrawerPage matches both Page and its concrete handler. So, when OnSystemBackButtonPressed() returns false (default/common path) the Handled guard does not stop a second call, so, would observe two invocations? Can you verify that behavior?
|
Yes. It would be called multiple times |
|
You can test this PR using the following package version. |
| { | ||
| PageNavigationSystemBackButtonPressedEvent.AddClassHandler<Page>((page, args) => | ||
| AffectsMeasure<Page>(SafeAreaPaddingProperty); | ||
| PageNavigationSystemBackButtonPressedEvent.AddClassHandler<Page>((sender, eventArgs) => |
There was a problem hiding this comment.
This removes the generic CurrentPage forwarding behavior from Page and replaces it with per-host forwarding in the built-in page containers. That fixes the immediate recursion problem, but it also changes the contract for custom Page subclasses that use the public CurrentPage property to host an active child.
Those custom hosts will now silently stop propagating system back requests unless they know to register their own class handler. Is that intended? If not, we may need a shared non-recursive forwarding helper or another way to preserve the old base behavior for custom page hosts.
There was a problem hiding this comment.
This is intended. Forwarding the event would most like result in another recursion issue. Requiring the dev to handle the event if their custom page class has unique back behavior would prevent that.
| RoutedEvent.Register<Page, RoutedEventArgs>( | ||
| nameof(PageNavigationSystemBackButtonPressed), | ||
| RoutingStrategies.Bubble); | ||
| RoutingStrategies.Direct); |
There was a problem hiding this comment.
Changing the back event to Direct removes the old base Page forwarding path, but NavigationPage still only handles modals and stack popping. In the normal non-modal path, the active CurrentPage no longer gets OnSystemBackButtonPressed() / PageNavigationSystemBackButtonPressed before the navigation stack reacts.
There was a problem hiding this comment.
Having the CurrentPage get the event when it's not modal makes it have some modal behavior. Since if they handle the event, the page won't be popped. That would make it modal, won't it.
|
You can test this PR using the following package version. |
|
Found one modal-ordering issue: I tested moving modal handling before
|
|
You can test this PR using the following package version. |
* ensure system back event is passed to child page once * fix formatting in DrawerPage * add tests * add back default page handler for system back event, but only check OnSystemBackButtonPressed * removed `OnSystemBackButtonPressed` checks in derived page classes * nav page - send back event to current page before modal pages. * Fix modal-first system back routing in NavigationPage --------- Co-authored-by: Javier Suárez Ruiz <javiersuarezruiz@hotmail.com>
What does the pull request do?
This PR changes the routing strategy for
PageNavigationSystemBackButtonPressedEvent, and updates handlers for all current page types. Pages that host other pages will generate an event for that child page and raise it. This fixes a StackOverflow that occurs on pages hosted in aPageNavigationHost.What is the current behavior?
When Back system buttons is pressed,
PageNavigationHostgenerates aPageNavigationSystemBackButtonPressedEventand raises it on the child page. The globalPagehandler for this event receives it and raises it again on the current page's child page. BecausePageNavigationSystemBackButtonPressedEventrouting strategy isBubble, this event is raised on the parent again if the child doesn't handle it. The globalPagehandler grabs this and raises it again on the child. This causes a closed loop and ends up in a StackOverflow.What is the updated/expected behavior with this PR?
The global
Pagehandler forPageNavigationSystemBackButtonPressedEventis removed.ContentPage, which is the only non-hostingPagewill not forward this event toCurrentPage, since it has no logic to set that property. Other pages that host pages will check if they have special behavior when back is requested, like closing the drawer inDrawerPage, before raising the event on theirCurrentPage.How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues