Page MenuHomePhabricator

[Bug] impossible to navigate back to article if start from hash fragment link
Closed, ResolvedPublic

Description

This is a longstanding issue. It applies to issues, editor and talk overlays. It is impossible to read the article if you follow a url which contains a hash fragment.

Steps to Reproduce

  1. open fresh browser tab
  2. Visit the all issue link in the new OR old treatment: https://readers-web-master.wmflabs.org/wiki/Naugadh#/issues/all
  3. Close the issue dialog

Expected Results

  • The dialog is dismissed and I can read the article Naugadh

Actual Results

  • The dialog is dismissed and I am ejected from the mobile site.

Environments Observed

  • readers-web-master

Browser Version

  • Chromium v68.0.3440.75

OS Version

  • Ubuntu v18.04 64b

Device Model

  • Desktop

Device Language

  • English

developer notes

Exiting an overlay relies on the back button. A solution will thus need to detect that the user has entered the mobile experience in the hash fragment state. The OverlayManager when initialising will need to be able to detect that it started with a hash fragment beginning with "/". When this is the case, using history replaceState and then a pushState call we should inject a new item without the hash fragment state before the current before opening the overlay.

One downside of this approach is refreshing the page will impact back behaviour as it will insert an unnecessary item in history.

Ideally, we would be able to detect that the history queue is empty or the previous url has a different domain but detecting the previous page in history seems problematic. See
https://stackoverflow.com/questions/3588315/how-to-check-if-the-user-can-go-back-in-browser-history-or-not#3588420

Do not worry about browsers that do not support the history api.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 13 2018, 3:56 PM

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)?

Niedzielski updated the task description. (Show Details)Aug 15 2018, 3:18 PM

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.

Jdlrobson updated the task description. (Show Details)Aug 15 2018, 10:51 PM
Jdlrobson updated the task description. (Show Details)Aug 15 2018, 11:01 PM

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.

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptAug 21 2018, 8:10 AM
Jdlrobson renamed this task from [Bug] Page issues sometimes reopens dialog to [Bug] impossible to navigate back to article if start from hash fragment link.Aug 22 2018, 11:31 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Aug 22 2018, 11:38 PM

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.

ovasileva lowered the priority of this task from Medium to Low.Aug 28 2018, 4:38 PM
ovasileva set the point value for this task to 8.

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...

Restricted Application added a project: VisualEditor. · View Herald TranscriptAug 10 2019, 3:43 PM

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?

stepstate
Load page on #/editor['#/editor']
startup script runs['#', '#/editor']
Refresh page['#', '#/editor']
startup script runs['#', '#', '#/editor']

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.
Jdlrobson removed the point value for this task.Aug 21 2019, 9:30 PM

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

matmarex closed this task as Resolved.Aug 21 2019, 10:59 PM
matmarex claimed this task.

\o/

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptAug 21 2019, 10:59 PM