Close the issue dialog (in the old treatment, this will cause the browser to leave the site!)

This is actually a long standing issue and a problem with all of our overlays. It requires an overhaul of the OverlayManager to be fixed so is tricky.
It's not just happening for the old treatment - it happens for the new one too.

The problem is that you started with a hash fragment url (https://readers-web-master.wmflabs.org/wiki/Naugadh#/issues/all)
The close icon in the overlay calls window.history.back()
The same happens if I send you to https://readers-web-master.wmflabs.org/wiki/Naugadh#/editor/0 - you cannot get out of the editor.

I'm not able to replicate the issue you are seeing, but Im guessing it's working by design - it's navigating in browser history - so it's sending you back to your last location. To fix it, we'd need OverlayManager to detect when it starts in a state and use use replace/pushState to avoid this situation. is this the bug you are reporting (see also T150105, T178414, T201339, T62848#655232)?

The parenthetical, "...(In the old treatment, this will cause the browser to leave the site!)" is T178414. However, the issue this task captures seems to be distinct from the others since it refers to visiting a page with a URL fragment directly. I've added a gif to help supplement the description.

I noticed that several of the other issues you mentioned have been declined. I would like to encourage that tasks against the Overlays and OverlayManager be kept open. These are core components of MobileFrontend and they're in need of major improvement or replacement. This is evident in both the code, which has become a chore to work with, and the user experience, which is subpar in certain parts as shown in bugs like this. I recognize it will be challenging to fix these issues or replace the implementations outright, but that's what we should do IMHO and I don't think we should give up on trying to make such changes in a near future. If we can't afford to even track important issues like this in our backlog, it gives little hope for the road ahead.

The parenthetical, "...(In the old treatment, this will cause the browser to leave the site!)" is T178414. However, the issue this task captures seems to be distinct from the others since it refers to visiting a page with a URL fragment directly. I've added a gif to help supplement the description.

I don't think that's true. I've updated the gif with what I'm seeing and I'm seeing the problem with both treatments and am thus convinced this is another variation of the T178414 bug and that "[Bug] Page issues sometimes reopens dialog" should be renamed "[Bug] Cannot escape page issues overlay when start session with #/issues/all fragment" or something similar.

I noticed that several of the other issues you mentioned have been declined. I would like to encourage that tasks against the Overlays and OverlayManager be kept open.

Sure, I did offer that option back in 2017 :) https://phabricator.wikimedia.org/T62848#3694812
While I am sympathetic that this feels broken, this is how browser history works. If you enter https://readers-web-master.wmflabs.org/wiki/Naugadh#/issues/all, that becomes the first item in your browser history. If you call history.back() (as our overlay does) there is nothing to go back to, so you stay at that URL. I see this issue with all our overlays.

The good news is that replaceState and pushState are now available (they were not viable in 2014 when the original bug got declined - https://phabricator.wikimedia.org/T62848#655232) and that it feels feasible that we could detect when the mobile site boots in a hash bang state and use replace/pushState to add the missing entry on the user's behalf.

While this is annoying, I do think it's an edge case that as engineers we hit the most often (I don't see many hash bangs on twitter for instance).

Is our understanding of this bug the same and should we generalise it? Possibly as part of the MobileFrontend.js refactor we might be able to finally tackle this, but I don't think fixing this is related to the page issues project.

To be honest after a brief analysis I am not sure this is possible without completely rewriting our overlay management system so that the close button does not call window.history.back (but then that will break the back/forward behavioir we currently have)

Possibly we can fix this behaviour when in a new tab as I'd hope window.history.length === 1 and back button not available.

To be talked about in a future estimation.

Seems more like a longstanding bug with mobile then something to be done for page issues.

I would go further (and did in our estimation) and say this is actually a 13 pointer as the overlay manager is the most important part of web navigation so any modifications to this (especially one which touches every overlay) carries a lot of risk.

Maybe we can keep this as a bug with unknown pointage and split off an overlay refactor epic. This might help us understand what problems we're trying to solve with overlays and then what are options are for reimplementing them.

Will fold this into requirements of new OverlayManager i'm trying to build...

I think this code inside the Overlaymanager getSingleton function should be enough to fix this:

		hash = window.location.hash;
		if ( hash ) {
			history.replaceState( null, null, '#' );
			history.pushState( null, null, hash );
		}

It basically replaces the current stack item with '#' on startup and then pops it back on, so there is always something to go back to.
This does however been that on subsequent refreshes extra entries will be placed on the stack, but that's probably more acceptable than the status quo.
@Esanders @matmarex thoughts?

That sounds good to me.

This does however been that on subsequent refreshes extra entries will be placed on the stack, but that's probably more acceptable than the status quo.

I think we can avoid this by passing the state parameter to replaceState/pushState and then checking history.state on page load to see if it's the same. Like this:

	var hash = window.location.hash;
	var state = history.state;

	if ( hash && state !== 'foo' ) {
		history.replaceState( 'foo', null, '#' );
		history.pushState( 'foo', null, hash );
	}

Sounds good to me. Will happily review a patch if you feel the urge to write one!

Change 531545 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] Allow closing an overlay without leaving the page when it was directly navigated to

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

Okay, I took a shot. It works!

Turns out we also need to do this when opening an overlay normally, but luckily that's easy.

Notes from testing:

• On desktop Opera browser, the browser back button actually skips over our entry (but the overlay close button, calling history.back(), does not). This is interesting and possibly intended as mitigation for some history abuse they encountered in the wild. Not a big problem.
• On desktop Edge browser, history.state is not preserved on refresh, so our trick to avoid duplicate entries doesn't work. Not a big problem.
• On desktop Firefox and iOS Safari it works exactly as expected.

8 points is a bit outdated and from a time when we thought this would involve rewriting the whole thing from scratch.

Change 531545 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Allow closing an overlay without leaving the page when it was directly navigated to

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