Both the MediaViewer and Kartographer set up a route for empty paths, which then conflict with each other as the first one to match from the registry causes the router to stop matching:

MediaViewer matches /^[^/]*$/ and Kartographer tries to match /^$/.

cc @Jdlrobson if he has any insights on the correct use of the router here.

Note the reverse of this bug also exists, but you have to close the image using navigation rather than using the [x] button:

• Open a map first, then close it
• Open an image
• Press back to close the image

Observe nothing happens.

cc @Jdlrobson if he has any insights on the correct use of the router here.

We have a few places where we set what's essentially a handler for an empty hash change. Typically the pattern is:

router.addRoute( '', /* some handler */ );
router.checkRoute();

MediaViewer matches /^[^/]*$/ and Kartographer tries to match /^$/.

The problem seems to be that addRoute calls OO.Registry.prototype.register meaning adding a route removes any existing handlers. Either intentionally or unintentionally, since these use different strings, they create two registery items.

However for an empty hash - both match, however OORouter.checkRoute will exit on the first one it finds.

I suspect the correct behaviour here would be the following:

• Add a new method addDefaultRoute that registers handlers that run on an empty hash. We will allow more than one.
• Update addRoute so that if a RegExp is adding that matches an empty string or "#/" we throw an error asking it to call addDefaultRoute.

Note other use cases of this pattern:

• https://gerrit.wikimedia.org/g/mediawiki/extensions/NearbyPages/+/c7a0cbb5bd8fffdbf072502d4481ea7b7d9b3d98/resources/ext.nearby.scripts/App.vue#259

Possibly related: T361612

Change #1079637 had a related patch set uploaded (by Simon04; author: Simon04):

[mediawiki/core@master] mediawiki.router: allow multiple handlers for empty paths

https://gerrit.wikimedia.org/r/1079637

I thought about this too, and considered the default route approach, but looking at the pattern MediaViewer uses (/^[^/]*$/) which is intended to catch any non-application path, I think what we really either an extra handler for when a route is "exited", or require users to handle this themselves by listening to route events.

The solution implemented in Kartographer is almost correct:

router.on( 'route', closeIfNotMapRoute );
router.route( '', closeIfNotMapRoute );

this second line is actually unnecessary, as navigating to the empty has will trigger the route event already and call closeIfNotMapRoute.

The only reason it doesn't work full is because it relies on router.navigate( '' ); to close the dialog, but this call doesn't trigger a route event.

Change #1079989 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/Kartographer@master] Fix route handling when closing

https://gerrit.wikimedia.org/r/1079989

The only reason it doesn't work full is because it relies on router.navigate( '' ); to close the dialog, but this call doesn't trigger a route event.

I think when navigate/navigateTo is called externally, it should trigger a route event so that listeners to that event can assume it is fired whenever the route changes, as the name suggests.

This would mean that all applications could implement their close handlers the way Kartographer does: by listening to route events and filtering to anything that doesn't match a known route in that application.

Change #1080061 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/MultimediaViewer@master] Routing: Don't register a path for closing MMV

https://gerrit.wikimedia.org/r/1080061

@Esanders how urgent is fixing this? I will need some time refamiliarizing myself with the code and thinking through this to be able to determine which of the two paths forward presented here are preferable.

It's not a priority to the editing team, although I had noticed this issue several times and was about to report it before I noticed it had been reported. Perhaps this is because maps are becoming more common - and the state it leaves you in where there is no way to close the map dialog is quite frustrating.

I think https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Kartographer/+/1079989 should get merged as this is much easier to review and fixes the immediate issue. The changes to mw.router are lower priority.

which of the two paths forward presented here are preferable

If it helps, I'm confident we can't use addDefaultRoute as that wouldn't fix navigation from a section hash to an application hash.

tagging CTT which stewards Kartographer per mw:Maintainers.

E.g.

1. https://www.mediawiki.org/wiki/Extension:Kartographer
2. Click infobox image, then close.
3. Click map, then close.

Reader is stuck in fullscreen map. Can't close or exit.

Since I reviewed https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1080010, I'll test and review the above Kartographer fix as well.

Change #1079989 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Fix route handling when closing

https://gerrit.wikimedia.org/r/1079989

There is some discussion on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MultimediaViewer/+/1080061 on the best way to register functions for closing apps, i.e. when the path no longer matches your app, or is empty.

@Jdlrobson has proposed a second callback:
router.addRoute( Config.ROUTE_REGEXP, this.route.bind( this ), destroyMMV );

although I think this not quite flexible enough, you may have two paths within one app and exiting either would not destroy the app, but exiting both would.

@Jdlrobson's second proposal is:
router.addDestroyRoute( / list of routes not to apply to / [ Config.ROUTE_REGEXP ] () => {} )

I think this is more promising as syntactic sugar around the general approach in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MultimediaViewer/+/1080061.

In general it might be a good idea to move Router away from being a registry, as I don't see much value from being abe to silently overwrite a path registration - it is far more likely we would have multiple apps listening to the same path.

I'm moving this tracking for now, since the Kartographer part has been reviewed, please let me know if there's anything else needed from us Content-Transform-Team .

In T376391#10268858, @Esanders wrote:

There is some discussion on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MultimediaViewer/+/1080061 on the best way to register functions for closing apps, i.e. when the path no longer matches your app, or is empty.

@Jdlrobson has proposed a second callback:
router.addRoute( Config.ROUTE_REGEXP, this.route.bind( this ), destroyMMV );

although I think this not quite flexible enough, you may have two paths within one app and exiting either would not destroy the app, but exiting both would.

@Jdlrobson's second proposal is:
router.addDestroyRoute( / list of routes not to apply to / [ Config.ROUTE_REGEXP ] () => {} )

I think this is more promising as syntactic sugar around the general approach in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MultimediaViewer/+/1080061.

In general it might be a good idea to move Router away from being a registry, as I don't see much value from being abe to silently overwrite a path registration - it is far more likely we would have multiple apps listening to the same path.

Is it fair to say this is the problem the MobileFrontend OverlayManager handles? Does it make sense to generalize that some way in the MediaWiki router code or in an abstraction on top of it?