Fix/fullscreen pop ups alt

PR Checklist

[x] PR title descriptive (can be used in release notes)

Description

Fixes various issues with fullscreen popups triggered from the template action:

Fixes an issue where a child pop-up on a full screen pop-up closes only on the second attempt (#2195)
Allows navigating back to close a fullscreen pop-up (previously this would navigate to the page in history preceding the template that launched the pop-up. This makes sense to me as a default but can be discussed, perhaps this should be exposed as a parameter that authors can set.
Fixes an issue where navigating back whilst a non-fullscreen popup is open would not dismiss the popup (the navigation would occur, but the popup would still display over the new screen)
The URL query params should now match the popup state. Previously, a fullscreen popup would not add associated query params to the URL. This should allow better direct linking to fullscreen popups.

Currently, this new implementation of fullscreen pop-ups can be triggered using the parameter fullscreen_alt passed to the pop_up action, e.g. click | pop_up: a_template_name | fullscreen_alt: true. This is in order to preserve existing functionality for deployments that already use the fullscreen property. I would welcome suggestions of how we might go about making the migration to using the new logic, or else supporting both options with a nicer syntax.

There are still some edge cases that cause navigation to behave unexpectedly, see videos below. It's a shame that this implementation hasn't fully fixed all pop-up functionality, but I think we should discuss how much of a priority this is and whether the improvements made by this PR make it worth merging, or whether we want to abandon this attempt to patch up our current system and invest time into more of an overhaul.

Testing

As well as the functionality of the debug_fullscreen_pop_up_1 series of templates (see video below for an example), we should test various combinations of existing pop-up functionality to ensure all of our use cases are still handled correctly.

Dev notes

We currently have two distinct methods triggered by the pop_up action:

If not fullscreen, the pop_up action calls templateNavService.handlePopupAction()
If fullscreen, the pop_up action calls templateService.runStandaloneTemplate() The runStandaloneTemplate() method is used elsewhere in the code, so we don't want to replace it, but it makes sense to me for the pop_up template action to use the handlePopUp() method with a fullscreen param. This PR therefore tries to incorporate some of the functionality of the former method into the latter, whilst fixing some of the issues with the former.

Working on the template nav service was a headache. There is lots of complicated logic and handling of various specific cases, so it's difficult to make changes without risking breaking something. I've tried to confine all of the changes to only apply when the new fullscreen option is passed to minimise unintended knock-ons. The exception is when it comes to dismissing popups: now the popup component itself handles dismissing on back and, if relevant, calls the templateNavService.dismissPopup() to handle its dismissal, which in turn now handles updating the query params.

Alternative approach

I am wary of overhauling the whole popup system, as it seems to handle a lot of edge cases that are difficult to understand. However, if we were to start from scratch, it may be sensible to consider the following.

We have three different ways of tracking popups

Query params
The TemplateNavService.openPopupsByName object
Browser history state
- the history must occasionally be manipulated to preserve expected nav behaviour It would seem sensible to keep these three in sync. Perhaps we could use signals to observe TemplateNavService.openPopupsByName and update the query params accordingly to keep the two in sync. To properly handle launching popups within popups (one of the major motivations for this PR), we would likely need the query params to store values for parent pop ups as well as children, essentially maintaining a history of popups to handle cases of navigation and refresh, so this could get complicated fast. Possibly, we would want to use a queue system for closing popups to properly handle the case of closing multiple popups at once (e.g. when navigating back from a fullscreen parent with a non-fullscreen child).

Git Issues

Closes #2319 (previous, buggy implementation) Closes #2195

Screenshots/Videos

debug_fullscreen_pop_up_1

Fullscreen pop-up and child working as expected

https://github.com/IDEMSInternational/parenting-app-ui/assets/64838927/1121c122-76e7-4a11-a97f-c0093170d363

Nav issue

There is an issue where closing a popup that is the child of a fullscreen popup causes the browser history to be in a state such that navigating back from the top-level parent that launched the fullscreen pop-up goes back to the open fullscreen pop-up, rather than the previous page

https://github.com/IDEMSInternational/parenting-app-ui/assets/64838927/57a83e0b-cab2-45e2-a13d-3c0e86e4b15f

I'm going to hold off on a full code review until after functional testing as likely any issues picked up there will lead to more iterations. Also just to confirm, with the nav issue you've identified is that something you are planning to address as part of the PR or not?

Good question. I think I wanted to get a PR made because I was going round in circles trying to fix the last few issues. But on reflection, the nav bug is pretty significant and means we wouldn't want to merge this implementation as-is (confirmed on a call with @esmeetewinkel). So I'll take another look at this with fresh eyes and see if I can iron out the issues. I'll remove the review requests for now and add you both back when it's ready.

@jfmcquade I noticed some strange behaviour on this branch, a template entered through a alt full screen pop-up behaves differently from how templates behave when entered through a go_to action.

Go to https://plh-kids.web.app/template/module_overview Video demonstrates the desired behaviour: the third screen rendered shows the "Learn" category header and the introduction text from the learn_1 article

go_to.webm

Alt fullscreen https://plh-kids.web.app/template/module_overview_alt Video demonstrates the strange behaviour: the third screen still shows the previous category header ("Intro") rather than "Learn", and the introduction text of the learn_2 article rather than learn_1.

alt fullscreen.webm

@jfmcquade I noticed some strange behaviour on this branch, a template entered through a alt full screen pop-up behaves differently from how templates behave when entered through a go_to action.

Thanks, @esmeetewinkel. It's very helpful to have these two parallel sheets to debug the different implementations.

I believe the issue is to do with emitting the "completed" event from within the pop-up and handling it on the parent template. We have quite a complex system for handling navigation with the go_to action and the actions that should be carried out on the parent template on return from the child. I hadn't realised that this would need to be replicated for the case of pop-ups, but I believe it would be in order to allow fullscreen pop-ups to be used in ways like this.

At this point it feels like extending our implementation of fullscreen pop-ups, as included in this PR, and using this new implementation for this immediate use case is not the best idea at this moment in time. As mentioned in this PR description, there are so many specific cases handled in the code in terms of navigation, pop-ups and passing data between templates, that trying to patch up a certain aspect without having knock-ons elsewhere is difficult.

It might make sense to wait to overhaul pop-up functionality more thoroughly as part of a new template rendering system. It would also be possible, within the current template rendering system, to try and rebuild the nav and pop-up systems in a way that integrates the two more closely, as opposed to patching up just the pop-up system as I have been trying to do so far with this PR. Essentially it's just a question of time and priorities, I'm sure I could get there eventually, but working on this code so far has been slow.

@chrismclarke, I'd be interested to hear if you have any thoughts about this. I appreciate that you may not have an informed opinion without engaging with the changes included in this PR, which may not be worth your time to do if we're going to abandon this attempt to rethink fullscreen popups along these lines. But let me know if you have any thoughts about whether the current nav and popup system is worth extending and improving upon or if it would be better to wait for a more thorough overhaul.

In any case, I think we should consider a different solution to our immediate use case of displaying these article steppers. For example, we could return to the idea of a template being able to specify certain visual changes while that template is displayed, e.g. that the app footer should be hidden and that the "back" button should display as a cross icon. As far as I can tell from the example article stepper template, the navigation does seem to work as expected using the go_to action, and so it's only these visual changes that would be required in order for the pages to match the figma designs, does that seem right, @esmeetewinkel?

@chrismclarke, I'd be interested to hear if you have any thoughts about this. I appreciate that you may not have an informed opinion without engaging with the changes included in this PR, which may not be worth your time to do if we're going to abandon this attempt to rethink fullscreen popups along these lines. But let me know if you have any thoughts about whether the current nav and popup system is worth extending and improving upon or if it would be better to wait for a more thorough overhaul.

It needs an overhaul, but in order to do that we need improved understanding and testing capability for the existing cases. I think your work on this PR has definitely helped to flesh out some of the issues and improve the example sheets, but really we need more work on the testing infrastructure before trying to push larger scale changes (i.e. adding spec tests that can interact with the UI components and routing structures).

In any case, I think we should consider a different solution to our immediate use case of displaying these article steppers. For example, we could return to the idea of a template being able to specify certain visual changes while that template is displayed, e.g. that the app footer should be hidden and that the "back" button should display as a cross icon. As far as I can tell from the example article stepper template, the navigation does seem to work as expected using the go_to action, and so it's only these visual changes that would be required in order for the pages to match the figma designs, does that seem right, @esmeetewinkel?

I think herein might lie the heart of the issue - what exactly are we trying to achieve right now? Was this PR designed to addressed issue #2195 (as referenced), or a different issue related to pending ui/figma updates? I'd say trying to broadly 'improve the system' and possibly address both at the same time has a bit too much scope for unintended knock-ons, but would be keen to know more about the exact issues we are trying to address.

I think herein might lie the heart of the issue - what exactly are we trying to achieve right now? Was this PR designed to addressed issue #2195 (as referenced), or a different issue related to pending ui/figma updates? I'd say trying to broadly 'improve the system' and possibly address both at the same time has a bit too much scope for unintended knock-ons, but would be keen to know more about the exact issues we are trying to address.

Good point.

The impetus for this PR was the requested UI updates for some "module stepper" pages as shown in these figma designs (the sequence of screens starting with the linked one) (@esmeetewinkel is that the most up-to-date version?). The thought was that fullscreen pop-ups might be the best solution to the proposed UI experience of having a fullscreen window that can display a number of pages of content in sequence, whilst allowing the user to "close" the window and return to the page that launched it.

We knew that the current implementation of fullscreen pop-ups was inadequate, due to #2195 but also some other undocumented issues that we weren't too sure of the nature of (at least that fullscreen pop-ups don't add query params to the URL and so user visits to them can't be tracked (is that right, @esmeetewinkel?)). So the work of this PR was always a bit exploratory in that I wanted to better understand the current implementation and get an idea of what would be required to flesh out a new version of fullscreen pop-ups that used the nav service's pop-up and query param features. This was successful in that I did get a better idea of these things, but the actual proposed changes have too many knock-ons for this to be merged.

So I think the lesson for me is that I spent too long on trying to fix the additional problems once I'd moved away from the core use case.

It needs an overhaul, but in order to do that we need improved understanding and testing capability for the existing cases. I think your work on this PR has definitely helped to flesh out some of the issues and improve the example sheets, but really we need more work on the testing infrastructure before trying to push larger scale changes (i.e. adding spec tests that can interact with the UI components and routing structures).

This is a helpful framing, you're right that starting with tests would be a good idea for working on this functionality. I'm assuming these would need to be E2E tests? In which case, a prerequisite would be building (or fixing/updating?) an E2E testing infrastructure?

The impetus for this PR was the requested UI updates for some "module stepper" pages as shown in these figma designs (the sequence of screens starting with the linked one) (@esmeetewinkel is that the most up-to-date version?). The thought was that fullscreen pop-ups might be the best solution to the proposed UI experience of having a fullscreen window that can display a number of pages of content in sequence, whilst allowing the user to "close" the window and return to the page that launched it.

Ah, I see. I think the full-screen popup was always more designed for a single page, which was then iterated on in a somewhat hacky way to add support for things like navigation actions, popups from popups etc., without any testing infrastructure so yeah building on top of it is particularly tricky.

This is a helpful framing, you're right that starting with tests would be a good idea for working on this functionality. I'm assuming these would need to be E2E tests? In which case, a prerequisite would be building (or fixing/updating?) an E2E testing infrastructure?

Ideally we'd use whatever testing structure does the best job at covering the test cases at hand. I.e. use spec tests to cover things like launching popup, popup within popup, action emit etc., and then e2e tests for actions across multiple nav pages (might even still be possible to cover some of that behaviour from spec tests) - so not to say we would fully need to fix the E2E system to start work on covering core use cases.

As for the issue of how best to achieve what we need in the Figmas, might be worth discussing on the next call as I can imagaine a few potential options depending on desired functionality, e.g

Using named router outlets
Using service to track nav state
Creating template to use within existing full screen popup that can handle child pages

As for the issue of how best to achieve what we need in the Figmas, might be worth discussing on the next call as I can imagaine a few potential options depending on desired functionality, e.g

Using named router outlets

Using service to track nav state

Creating template to use within existing full screen popup that can handle child pages

Agreed. I've opened #2339, will discuss scheduling a call on mattermost

IDEMSInternational / open-app-builder