From 07ec8265353c7b3ce947c8e0cfa982db5ff16399 Mon Sep 17 00:00:00 2001 From: Zelam Ngo Date: Tue, 5 May 2015 10:32:24 -0700 Subject: [PATCH 1/6] Bugfix for router when back button is pressed, caught and canceled, it should do a window.history.forward() instead of back() --- lib/client.dart | 23 ++++- test/client_test.dart | 213 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 233 insertions(+), 3 deletions(-) diff --git a/lib/client.dart b/lib/client.dart index 97fa415..db3b7d0 100644 --- a/lib/client.dart +++ b/lib/client.dart @@ -450,6 +450,7 @@ class Router { final bool sortRoutes; bool _listen = false; WindowClickHandler _clickHandler; + int _historyLength = 0; /** * [useFragment] determines whether this Router uses pure paths with @@ -472,6 +473,9 @@ class Router { : useFragment, _window = (windowImpl == null) ? window : windowImpl, root = new RouteImpl._new() { + if (_window != null && _window.history != null) { + _historyLength = _window.history.length; + } if (clickHandler == null) { if (linkMatcher == null) { linkMatcher = new DefaultRouterLinkMatcher(); @@ -799,11 +803,18 @@ class Router { } _listen = true; if (_useFragment) { + int newHistoryLength = _window.history.length; _window.onHashChange.listen((_) { route(_normalizeHash(_window.location.hash)).then((allowed) { // if not allowed, we need to restore the browser location if (!allowed) { - _window.history.back(); + if (newHistoryLength > _historyLength) { + _window.history.back(); + } else { + _window.history.forward(); + } + } + _historyLength = _window.history.length; } }); }); @@ -811,12 +822,18 @@ class Router { } else { String getPath() => '${_window.location.pathname}${_window.location.hash}'; - + int newHistoryLength = _window.history.length; _window.onPopState.listen((_) { route(getPath()).then((allowed) { // if not allowed, we need to restore the browser location if (!allowed) { - _window.history.back(); + if (newHistoryLength > _historyLength) { + _window.history.back(); + } else { + _window.history.forward(); + } + } + _historyLength = _window.history.length; } }); }); diff --git a/test/client_test.dart b/test/client_test.dart index 76f79d1..518e09f 100644 --- a/test/client_test.dart +++ b/test/client_test.dart @@ -602,7 +602,9 @@ main() { }); }); }); + }); + group('preLeave', () { void _testAllowLeave(bool allowLeave) { var completer = new Completer(); bool barEntered = false; @@ -635,6 +637,217 @@ main() { test('should veto navigation', () { _testAllowLeave(false); }); + + bool noLeaveEntered; + bool freeLeave1Entered; + MockWindow mockWindow; + StreamController hashChangeController; + StreamController popStateEventController; + Router router; + int backCount; + int forwardCount; + int historyCount; + + void _setUpPreleave({bool useFragment}) { + Completer completer = new Completer(); + completer.complete(false); + noLeaveEntered = false; + freeLeave1Entered = false; + backCount = 0; + forwardCount = 0; + historyCount = 0; + + mockWindow = new MockWindow(); + hashChangeController = new StreamController.broadcast(onListen: () {}, + onCancel:() {}, sync:true); + + popStateEventController = new StreamController.broadcast(onListen: () {}, + onCancel:() {}, sync:true); + + mockWindow.when(callsTo('get onHashChange')) + .alwaysReturn(hashChangeController.stream); + + mockWindow.when(callsTo('get onPopState')) + .alwaysReturn(popStateEventController.stream); + + mockWindow.history.when(callsTo('back')).alwaysCall(() { + backCount++; + }); + mockWindow.history.when(callsTo('forward')).alwaysCall(() { + forwardCount++; + }); + + mockWindow.history.when(callsTo('get length')).alwaysCall(() => historyCount); + + mockWindow.location.when(callsTo('get hash')).alwaysReturn('#/foo'); + router = new Router(useFragment: true, windowImpl: mockWindow); + router.root + ..addRoute(name: 'foo', path: '/foo', + mount: (Route child) => child + ..addRoute(name: 'noLeave', path: '/noLeave', + enter: (RouteEnterEvent e) => noLeaveEntered = true, + preLeave: (RoutePreLeaveEvent e) => e.allowLeave(completer.future)) + ..addRoute(name: 'freeLeave1', path: '/freeLeave1', + enter: (RouteEnterEvent e) => freeLeave1Entered = true)); + router.listen(ignoreClick: true); + } + + void changeWindowLocation() { + historyCount++; + hashChangeController.add(new Event.eventType('KeyboardEvent', 'keyup')); + } + + void windowBackAction() { + historyCount--; + hashChangeController.add(new Event.eventType('KeyboardEvent', 'keyup')); + } + + void windowForwardAction() { + historyCount++; + hashChangeController.add(new Event.eventType('KeyboardEvent', 'keyup')); + } + + test('should handle location change cancel with useFragment', () { + _setUpPreleave(useFragment:true); + + router.route('/foo/noLeave').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + changeWindowLocation(); + hashChangeController.stream.listen((_) { + expect(backCount, 1); + }); + })); + }); + + test('should handle history.back cancel with useFragment', () { + _setUpPreleave(useFragment:true); + + router.route('/foo/freeLeave1').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + changeWindowLocation(); + hashChangeController.stream.listen((_) { + expect(backCount, 0); + expect(forwardCount, 0); + }); + })); + + router.route('/foo/noLeave').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + windowBackAction(); + hashChangeController.stream.listen((_) { + expect(backCount, 1); + expect(forwardCount, 0); + }); + })); + }); + + test('should handle history.forward cancel with useFragment', () { + _setUpPreleave(useFragment:true); + + router.route('/foo/freeLeave1').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + changeWindowLocation(); + hashChangeController.stream.listen((_) { + expect(backCount, 0); + expect(forwardCount, 0); + }); + })); + + router.route('/foo/freeLeave1').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + windowBackAction(); + hashChangeController.stream.listen((_) { + expect(backCount, 0); + expect(forwardCount, 0); + }); + })); + + router.route('/foo/noLeave').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + windowForwardAction(); + hashChangeController.stream.listen((_) { + expect(backCount, 0); + expect(forwardCount, 1); + }); + })); + }); + + test('should handle location change cancel without useFragment', () { + _setUpPreleave(useFragment:false); + + router.route('/foo/noLeave').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + changeWindowLocation(); + hashChangeController.stream.listen((_) { + expect(backCount, 1); + }); + })); + }); + + test('should handle history.back cancel without useFragment', () { + _setUpPreleave(useFragment:false); + + router.route('/foo/freeLeave1').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + changeWindowLocation(); + hashChangeController.stream.listen((_) { + expect(backCount, 0); + expect(forwardCount, 0); + }); + })); + + router.route('/foo/noLeave').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + windowBackAction(); + hashChangeController.stream.listen((_) { + expect(backCount, 1); + expect(forwardCount, 0); + }); + })); + }); + + test('should handle history.forward cancel without useFragment', () { + _setUpPreleave(useFragment:false); + + router.route('/foo/freeLeave1').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + changeWindowLocation(); + hashChangeController.stream.listen((_) { + expect(backCount, 0); + expect(forwardCount, 0); + }); + })); + + router.route('/foo/freeLeave1').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + windowBackAction(); + hashChangeController.stream.listen((_) { + expect(backCount, 0); + expect(forwardCount, 0); + }); + })); + + router.route('/foo/noLeave').then(expectAsync((_) { + expect(backCount, 0); + expect(forwardCount, 0); + windowForwardAction(); + hashChangeController.stream.listen((_) { + expect(backCount, 0); + expect(forwardCount, 1); + }); + })); + }); }); group('preEnter', () { From 51b7911d5adf89460024ea7a5eeeef602516eb00 Mon Sep 17 00:00:00 2001 From: Zelam Ngo Date: Wed, 6 May 2015 08:31:11 -0700 Subject: [PATCH 2/6] removed extra } --- lib/client.dart | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/client.dart b/lib/client.dart index db3b7d0..c5e5c7b 100644 --- a/lib/client.dart +++ b/lib/client.dart @@ -815,7 +815,6 @@ class Router { } } _historyLength = _window.history.length; - } }); }); route(_normalizeHash(_window.location.hash)); @@ -834,7 +833,6 @@ class Router { } } _historyLength = _window.history.length; - } }); }); route(getPath()); From 8b3a96c984294f794f5bf0a17fbce01cd11451bf Mon Sep 17 00:00:00 2001 From: Zelam Ngo Date: Wed, 6 May 2015 09:23:11 -0700 Subject: [PATCH 3/6] Bug fix for test --- test/client_test.dart | 176 ++++++++++++++++++++---------------------- 1 file changed, 84 insertions(+), 92 deletions(-) diff --git a/test/client_test.dart b/test/client_test.dart index 518e09f..845d9e3 100644 --- a/test/client_test.dart +++ b/test/client_test.dart @@ -9,11 +9,14 @@ import 'dart:html'; import 'package:unittest/unittest.dart'; import 'package:mock/mock.dart'; +import 'package:dart.testing/google3_html_config.dart' as g3; import 'package:route_hierarchical/client.dart'; +import 'package:route_hierarchical/url_pattern.dart'; import 'util/mocks.dart'; main() { + g3.useGoogle3HtmlConfiguration(); unittestConfiguration.timeout = new Duration(seconds: 1); test('paths are routed to routes added with addRoute', () { @@ -659,10 +662,10 @@ main() { mockWindow = new MockWindow(); hashChangeController = new StreamController.broadcast(onListen: () {}, - onCancel:() {}, sync:true); + onCancel:() {}, sync:false); popStateEventController = new StreamController.broadcast(onListen: () {}, - onCancel:() {}, sync:true); + onCancel:() {}, sync:false); mockWindow.when(callsTo('get onHashChange')) .alwaysReturn(hashChangeController.stream); @@ -707,16 +710,27 @@ main() { hashChangeController.add(new Event.eventType('KeyboardEvent', 'keyup')); } + void expectBack(int count) { + mockWindow.history.getLogs(callsTo('back')) + .verify(happenedExactly(count)); + } + + void expectForward(int count) { + mockWindow.history.getLogs(callsTo('forward')) + .verify(happenedExactly(count)); + } + test('should handle location change cancel with useFragment', () { _setUpPreleave(useFragment:true); router.route('/foo/noLeave').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); + expectBack(0); + expectForward(0); changeWindowLocation(); - hashChangeController.stream.listen((_) { - expect(backCount, 1); - }); + + hashChangeController.close().then(expectAsync((_) { + expectBack(1); + })); })); }); @@ -724,23 +738,19 @@ main() { _setUpPreleave(useFragment:true); router.route('/foo/freeLeave1').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); + expectBack(0); + expectForward(0); changeWindowLocation(); - hashChangeController.stream.listen((_) { - expect(backCount, 0); - expect(forwardCount, 0); - }); - })); - router.route('/foo/noLeave').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); - windowBackAction(); - hashChangeController.stream.listen((_) { - expect(backCount, 1); - expect(forwardCount, 0); - }); + router.route('/foo/noLeave').then(expectAsync((_) { + expectBack(0); + expectForward(0); + windowBackAction(); + hashChangeController.close().then(expectAsync((_) { + expectBack(0); + expectForward(1); + })); + })); })); }); @@ -748,33 +758,25 @@ main() { _setUpPreleave(useFragment:true); router.route('/foo/freeLeave1').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); + expectBack(0); + expectForward(0); changeWindowLocation(); - hashChangeController.stream.listen((_) { - expect(backCount, 0); - expect(forwardCount, 0); - }); - })); - - router.route('/foo/freeLeave1').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); - windowBackAction(); - hashChangeController.stream.listen((_) { - expect(backCount, 0); - expect(forwardCount, 0); - }); - })); - router.route('/foo/noLeave').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); - windowForwardAction(); - hashChangeController.stream.listen((_) { - expect(backCount, 0); - expect(forwardCount, 1); - }); + router.route('/foo/freeLeave1').then(expectAsync((_) { + expectBack(0); + expectForward(0); + windowBackAction(); + + router.route('/foo/noLeave').then(expectAsync((_) { + expectBack(0); + expectForward(0); + windowForwardAction(); + hashChangeController.close().then(expectAsync((_) { + expectBack(1); + expectForward(0); + })); + })); + })); })); }); @@ -782,12 +784,13 @@ main() { _setUpPreleave(useFragment:false); router.route('/foo/noLeave').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); + expectBack(0); + expectForward(0); changeWindowLocation(); - hashChangeController.stream.listen((_) { - expect(backCount, 1); - }); + + hashChangeController.close().then(expectAsync((_) { + expectBack(1); + })); })); }); @@ -795,23 +798,19 @@ main() { _setUpPreleave(useFragment:false); router.route('/foo/freeLeave1').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); + expectBack(0); + expectForward(0); changeWindowLocation(); - hashChangeController.stream.listen((_) { - expect(backCount, 0); - expect(forwardCount, 0); - }); - })); - router.route('/foo/noLeave').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); - windowBackAction(); - hashChangeController.stream.listen((_) { - expect(backCount, 1); - expect(forwardCount, 0); - }); + router.route('/foo/noLeave').then(expectAsync((_) { + expectBack(0); + expectForward(0); + windowBackAction(); + hashChangeController.close().then(expectAsync((_) { + expectBack(0); + expectForward(1); + })); + })); })); }); @@ -819,33 +818,25 @@ main() { _setUpPreleave(useFragment:false); router.route('/foo/freeLeave1').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); + expectBack(0); + expectForward(0); changeWindowLocation(); - hashChangeController.stream.listen((_) { - expect(backCount, 0); - expect(forwardCount, 0); - }); - })); - router.route('/foo/freeLeave1').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); - windowBackAction(); - hashChangeController.stream.listen((_) { - expect(backCount, 0); - expect(forwardCount, 0); - }); - })); - - router.route('/foo/noLeave').then(expectAsync((_) { - expect(backCount, 0); - expect(forwardCount, 0); - windowForwardAction(); - hashChangeController.stream.listen((_) { - expect(backCount, 0); - expect(forwardCount, 1); - }); + router.route('/foo/freeLeave1').then(expectAsync((_) { + expectBack(0); + expectForward(0); + windowBackAction(); + + router.route('/foo/noLeave').then(expectAsync((_) { + expectBack(0); + expectForward(0); + windowForwardAction(); + hashChangeController.close().then(expectAsync((_) { + expectBack(1); + expectForward(0); + })); + })); + })); })); }); }); @@ -2001,3 +1992,4 @@ main() { /// An alias for Router.root.findRoute(path) r(Router router, String path) => router.root.findRoute(path); + From d3f838cd4dfaeed2fb3e53d210eeb8df1f28fdd6 Mon Sep 17 00:00:00 2001 From: Zelam Ngo Date: Wed, 6 May 2015 10:05:28 -0700 Subject: [PATCH 4/6] final bugfixes --- lib/client.dart | 4 ++-- test/client_test.dart | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/client.dart b/lib/client.dart index c5e5c7b..6e225e0 100644 --- a/lib/client.dart +++ b/lib/client.dart @@ -803,9 +803,9 @@ class Router { } _listen = true; if (_useFragment) { - int newHistoryLength = _window.history.length; _window.onHashChange.listen((_) { route(_normalizeHash(_window.location.hash)).then((allowed) { + int newHistoryLength = _window.history.length; // if not allowed, we need to restore the browser location if (!allowed) { if (newHistoryLength > _historyLength) { @@ -821,9 +821,9 @@ class Router { } else { String getPath() => '${_window.location.pathname}${_window.location.hash}'; - int newHistoryLength = _window.history.length; _window.onPopState.listen((_) { route(getPath()).then((allowed) { + int newHistoryLength = _window.history.length; // if not allowed, we need to restore the browser location if (!allowed) { if (newHistoryLength > _historyLength) { diff --git a/test/client_test.dart b/test/client_test.dart index 845d9e3..b19b672 100644 --- a/test/client_test.dart +++ b/test/client_test.dart @@ -9,14 +9,11 @@ import 'dart:html'; import 'package:unittest/unittest.dart'; import 'package:mock/mock.dart'; -import 'package:dart.testing/google3_html_config.dart' as g3; import 'package:route_hierarchical/client.dart'; -import 'package:route_hierarchical/url_pattern.dart'; import 'util/mocks.dart'; main() { - g3.useGoogle3HtmlConfiguration(); unittestConfiguration.timeout = new Duration(seconds: 1); test('paths are routed to routes added with addRoute', () { From 1155d322937f48b0118964dca89210e386afca6d Mon Sep 17 00:00:00 2001 From: Zelam Ngo Date: Wed, 6 May 2015 10:09:12 -0700 Subject: [PATCH 5/6] Clean up unused vars --- test/client_test.dart | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/test/client_test.dart b/test/client_test.dart index b19b672..6ee9ed5 100644 --- a/test/client_test.dart +++ b/test/client_test.dart @@ -638,23 +638,15 @@ main() { _testAllowLeave(false); }); - bool noLeaveEntered; - bool freeLeave1Entered; MockWindow mockWindow; StreamController hashChangeController; StreamController popStateEventController; Router router; - int backCount; - int forwardCount; int historyCount; void _setUpPreleave({bool useFragment}) { Completer completer = new Completer(); completer.complete(false); - noLeaveEntered = false; - freeLeave1Entered = false; - backCount = 0; - forwardCount = 0; historyCount = 0; mockWindow = new MockWindow(); @@ -670,13 +662,6 @@ main() { mockWindow.when(callsTo('get onPopState')) .alwaysReturn(popStateEventController.stream); - mockWindow.history.when(callsTo('back')).alwaysCall(() { - backCount++; - }); - mockWindow.history.when(callsTo('forward')).alwaysCall(() { - forwardCount++; - }); - mockWindow.history.when(callsTo('get length')).alwaysCall(() => historyCount); mockWindow.location.when(callsTo('get hash')).alwaysReturn('#/foo'); @@ -685,10 +670,10 @@ main() { ..addRoute(name: 'foo', path: '/foo', mount: (Route child) => child ..addRoute(name: 'noLeave', path: '/noLeave', - enter: (RouteEnterEvent e) => noLeaveEntered = true, + enter: (RouteEnterEvent e) {}, preLeave: (RoutePreLeaveEvent e) => e.allowLeave(completer.future)) ..addRoute(name: 'freeLeave1', path: '/freeLeave1', - enter: (RouteEnterEvent e) => freeLeave1Entered = true)); + enter: (RouteEnterEvent e) {})); router.listen(ignoreClick: true); } From c9819c98851985e47a8c79674b469753abd2fb40 Mon Sep 17 00:00:00 2001 From: Zelam Ngo Date: Mon, 11 May 2015 14:55:39 -0700 Subject: [PATCH 6/6] Not a perfect solution, but better than current. Browser history limits at 50, where we change strategy to window.location.replace. Will destroy history data, but better than recursive backing. --- lib/client.dart | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/lib/client.dart b/lib/client.dart index 6e225e0..9384b77 100644 --- a/lib/client.dart +++ b/lib/client.dart @@ -792,6 +792,24 @@ class Router { [kvPair.substring(0, splitPoint), kvPair.substring(splitPoint + 1)]; } + void _handleRouteResponse(bool allowed) { + int newHistoryLength = _window.history.length; + // if not allowed, we need to restore the browser location + if (!allowed) { + if (_historyLength < 50) { + if (newHistoryLength > _historyLength) { + _window.history.back(); + } else { + _window.history.forward(); + } + } else { + window.location.replace(_oldLocation); + } + } + _historyLength = _window.history.length; + _oldLocation = window.location.href; + } + /** * Listens for window history events and invokes the router. On older * browsers the hashChange event is used instead. @@ -803,37 +821,16 @@ class Router { } _listen = true; if (_useFragment) { + _window.onHashChange.listen((_) { - route(_normalizeHash(_window.location.hash)).then((allowed) { - int newHistoryLength = _window.history.length; - // if not allowed, we need to restore the browser location - if (!allowed) { - if (newHistoryLength > _historyLength) { - _window.history.back(); - } else { - _window.history.forward(); - } - } - _historyLength = _window.history.length; - }); + route(_normalizeHash(_window.location.hash)).then(_handleRouteResponse); }); route(_normalizeHash(_window.location.hash)); } else { String getPath() => '${_window.location.pathname}${_window.location.hash}'; _window.onPopState.listen((_) { - route(getPath()).then((allowed) { - int newHistoryLength = _window.history.length; - // if not allowed, we need to restore the browser location - if (!allowed) { - if (newHistoryLength > _historyLength) { - _window.history.back(); - } else { - _window.history.forward(); - } - } - _historyLength = _window.history.length; - }); + route(getPath()).then(_handleRouteResponse); }); route(getPath()); }