Page MenuHomePhabricator

Add an navigation method in OOjs Router that replaces the current entry in document history (instead of creating a new one).
Needs ReviewPublic

Authored by JGirault on Sep 16 2016, 2:04 PM.

Details

Maniphest Tasks
T135692: OOjs Router should support replacing the current entry in the history
Reviewers
Jdlrobson
Patch without arc
git checkout -b D353 && curl -L https://phabricator.wikimedia.org/D353?download=true | git apply
Summary
  • Rewrite the router code to use the History API instead of the hashchange event when possible.
  • Abstract the differences between pushState method and hashchange method in OOjs Router.
  • Change signature of navigate to support options.replace = true:
  • Remove router.back() hack, as this should be possible with the new "history replace" method.

Note: this is unrelated to the MediaWiki pages' history.

Diff Detail

Repository
rMW MediaWiki
Branch
pushstate
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 966
Build 1400: arc lint + arc unit

Event Timeline

JGirault updated this revision to Diff 949.Sep 16 2016, 2:04 PM
JGirault retitled this revision from to Add an navigation method in OOjs Router that replaces the current entry in document history (instead of creating a new one)..
JGirault updated this object.
JGirault edited the test plan for this revision. (Show Details)
Jdlrobson edited edge metadata.Sep 26 2016, 6:09 PM

Such a change should probably have some unit tests too (operating on a stubbed history object)

resources/lib/oojs-router/oojs-router.js
154

Any reason why you can't just do !path and drop brackets?

157

I see a few uses of this method in MobileFrontend.
It's used where we need to do things after it has been successful.

could you elaborate on why you remove this and what the migration path would be?

https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.search/SearchOverlay.js#L231

resources/lib/oojs-router/oojs-router.js
154

I can't think of any, I guess it must work

157

I only see one use case for it, @ https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.search/SearchOverlay.js#L231-L233

I think what you're trying to do is simply;

window.location.replace( $link.attr( 'href' ) );

but I might be wrong.

This current router is useful when we're doing routing in a "single page" scenario. If MobileFrontend was navigating through articles without reloading the page (using the router), we could add an option to the router to enable/disable hash fragments, so one could do:

router.navigate( '/wiki/San_Francisco', { hash: false } );
router.navigate ('/search/San');

This would allow to replace router.back with:

this.router.replace( $link.attr( 'href' ), { hash: false } );

I think the best would be the opposite, i.e. make the hash fragment an option, and move towards being able to use popstate/pushState everywhere.

Jdlrobson added inline comments.Oct 21 2016, 12:30 AM
resources/lib/oojs-router/oojs-router.js
154

Also what about query string?

157

To rephrase ...
We can't merge this code unless we either retain the back method or update MobileFrontend to do something different. Otherwise this will break MobileFrontend.

From what I remember, MobileFrontend's overlay system works by subscribing to hash change events and then using back to navigate back to the state where the overlay wasn't open.

Yes, I suspect this could probably do this with replaceState once it's available, but I'd advise keeping the method here for the time being until we can find the right migration path.