Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix local navigation history management by handling hash navigations #5961

Merged
merged 11 commits into from Nov 18, 2016
Merged

Fix local navigation history management by handling hash navigations #5961

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c9142a5
Fix local navigation history management by handling hash navigations …
Nov 1, 2016
6b730b8
Fix natural and virtual history and doc-click
Nov 8, 2016
2e66c7f
change api.
Nov 9, 2016
ac47fe4
fix comments
Nov 10, 2016
de90625
fix comments
Nov 10, 2016
798c565
Tests and fixes
Nov 16, 2016
ec8d448
fix types
Nov 16, 2016
9f083ff
try-finally and dev.assert starts with #
Nov 17, 2016
7fe396c
Use lastNavigatedHash_ instead of ignorePopstate
Nov 18, 2016
3aa40f5
fixes
Nov 18, 2016
b30b6a0
add comment
Nov 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/service/history-impl.js
View file
Open in desktop
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

import {Pass} from '../pass';
import {fromClass, getServiceForDoc} from '../service';
import {getMode} from '../mode';
import {dev} from '../log';
Expand Down Expand Up @@ -246,9 +245,9 @@ class HistoryBindingInterface {

/**
* Replaces the state for local target navigation.
* @param target
* @param unusedTarget
*/
replaceStateForTarget(target) {}
replaceStateForTarget(unusedTarget) {}
}


Expand Down Expand Up @@ -557,7 +556,7 @@ export class HistoryBindingNatural_ {
*/
replaceStateForTarget(target) {
this.whenReady_(() => {
let hash = target.indexOf('#') == 0 ? target : `#${target}`;
const hash = target[0] == '#' ? target : `#${target}`;
this.ignoreUpcomingPopstate_ = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the events are synchronous, does it mean that replace can fail? Please check. If so, we may need a try/finally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it fails if the location is invalid or different origin than executing script.

We should probably limit the replaceStateForTarget to be just for hash navigation. Which is we kinda do with the first like appending a # but maybe we should just handle the explicit # and throw if the target passed does not start with a #.

What do you think?

I'll also add the try-finally

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG. Should definitely be limited to the same everything - only hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// TODO(mkhatib, #6095): Chrome iOS will add extra states for location.replace.
this.win.location.replace(hash);
Expand Down Expand Up @@ -618,7 +617,7 @@ export class HistoryBindingVirtual_ {
* @param {!./viewer-impl.Viewer} viewer
*/
constructor(win, viewer) {
/** @private @const {!Window} */
/** @const {!Window} */
this.win = win;

/** @private @const {!./viewer-impl.Viewer} */
Expand All @@ -637,7 +636,7 @@ export class HistoryBindingVirtual_ {

/** @override */
replaceStateForTarget(target) {
let hash = target.indexOf('#') == 0 ? target : `#${target}`;
const hash = target[0] == '#' ? target : `#${target}`;
this.win.location.replace(hash);
}

Expand Down
107 changes: 52 additions & 55 deletions test/functional/test-document-click.js
View file
Open in desktop
Expand Up @@ -38,26 +38,30 @@ describe('test-document-click onDocumentElementClick_', () => {
let preventDefaultSpy;
let scrollIntoViewSpy;
let querySelectorSpy;
let replaceLocSpy;
let viewport;
let timerFuncSpy;
let replaceStateForTargetSpy;
let replaceStateForTargetPromise;
let replaceStateForTargetResolver;

beforeEach(() => {
replaceStateForTargetPromise = new Promise(resolve => {
replaceStateForTargetResolver = resolve;
});
sandbox = sinon.sandbox.create();
preventDefaultSpy = sandbox.spy();
scrollIntoViewSpy = sandbox.spy();
timerFuncSpy = sandbox.spy();
replaceLocSpy = sandbox.spy();
timerFuncSpy = sandbox.stub();
elem = {nodeType: 1};
getElementByIdSpy = sandbox.stub();
querySelectorSpy = sandbox.stub();
replaceStateForTargetSpy = sandbox.stub();
tgt = document.createElement('a');
tgt.href = 'https://www.google.com';
win = {
document: {},
location: {
href: 'https://www.google.com/some-path?hello=world#link',
replace: replaceLocSpy,
},
setTimeout: fn => {
timerFuncSpy();
Expand Down Expand Up @@ -95,6 +99,10 @@ describe('test-document-click onDocumentElementClick_', () => {
};
history = {
push: () => Promise.resolve(),
replaceStateForTarget: hash => {
replaceStateForTargetSpy(hash);
return replaceStateForTargetPromise;
},
};
installTimerService(win);
installDocumentInfoServiceForDoc(ampdoc);
Expand All @@ -111,6 +119,10 @@ describe('test-document-click onDocumentElementClick_', () => {
win.location.href = 'https://www.google.com/some-path?hello=world#link';
});

afterEach(() => {
sandbox.restore();
});

it('should not do anything on path change', () => {
tgt.href = 'https://www.google.com/some-other-path';
onDocumentElementClick_(evt, ampdoc, viewport, history);
Expand Down Expand Up @@ -159,6 +171,10 @@ describe('test-document-click onDocumentElementClick_', () => {
tgt.href = 'https://www.google.com/some-path?hello=world#test';
});

afterEach(() => {
sandbox.restore();
});

it('should call getElementById on document', () => {
getElementByIdSpy.returns(elem);
expect(getElementByIdSpy.callCount).to.equal(0);
Expand Down Expand Up @@ -200,33 +216,39 @@ describe('test-document-click onDocumentElementClick_', () => {
onDocumentElementClick_(evt, ampdoc, viewport, history);
expect(getElementByIdSpy.callCount).to.equal(1);
expect(scrollIntoViewSpy.callCount).to.equal(0);
expect(replaceLocSpy.callCount).to.equal(1);
expect(replaceLocSpy.args[0][0]).to.equal('#test');
expect(replaceStateForTargetSpy.callCount).to.equal(1);
expect(replaceStateForTargetSpy.args[0][0]).to.equal('#test');
});

it('should call scrollIntoView if element with id is found', () => {
getElementByIdSpy.returns(elem);

expect(replaceLocSpy.callCount).to.equal(0);
expect(replaceStateForTargetSpy.callCount).to.equal(0);
expect(scrollIntoViewSpy.callCount).to.equal(0);
onDocumentElementClick_(evt, ampdoc, viewport, history);
expect(scrollIntoViewSpy.callCount).to.equal(2);
expect(timerFuncSpy).to.be.calledOnce;
expect(replaceLocSpy.callCount).to.equal(1);
expect(replaceLocSpy.args[0][0]).to.equal('#test');
expect(replaceStateForTargetSpy.callCount).to.equal(1);
expect(replaceStateForTargetSpy.args[0][0]).to.equal('#test');
replaceStateForTargetResolver();
return replaceStateForTargetPromise.then(() => {
expect(scrollIntoViewSpy.callCount).to.equal(2);
expect(timerFuncSpy).to.be.calledOnce;
});
});

it('should call scrollIntoView if element with name is found', () => {
getElementByIdSpy.returns(null);
querySelectorSpy.returns(elem);

expect(replaceLocSpy.callCount).to.equal(0);
expect(replaceStateForTargetSpy.callCount).to.equal(0);
expect(scrollIntoViewSpy.callCount).to.equal(0);
onDocumentElementClick_(evt, ampdoc, viewport, history);
expect(scrollIntoViewSpy.callCount).to.equal(2);
expect(timerFuncSpy).to.be.calledOnce;
expect(replaceLocSpy.callCount).to.equal(1);
expect(replaceLocSpy.args[0][0]).to.equal('#test');
replaceStateForTargetResolver();
return replaceStateForTargetPromise.then(() => {
expect(scrollIntoViewSpy.callCount).to.equal(2);
expect(timerFuncSpy).to.be.calledOnce;
expect(replaceStateForTargetSpy.callCount).to.equal(1);
expect(replaceStateForTargetSpy.args[0][0]).to.equal('#test');
});
});

it('should use escaped css selectors', () => {
Expand All @@ -243,63 +265,38 @@ describe('test-document-click onDocumentElementClick_', () => {
expect(querySelectorSpy).to.be.calledWith('a[name="test\\"hello"]');
});

it('should call location.replace before scrollIntoView', () => {
it('should call replaceStateForTarget before scrollIntoView', () => {
getElementByIdSpy.returns(null);
querySelectorSpy.returns(elem);

const ops = [];
win.location.replace = () => {
ops.push('location.replace');
};
viewport.scrollIntoView = () => {
ops.push('scrollIntoView');
};
onDocumentElementClick_(evt, ampdoc, viewport, history);

expect(timerFuncSpy).to.be.calledOnce;
expect(ops).to.have.length(3);
expect(ops[0]).to.equal('location.replace');
expect(ops[1]).to.equal('scrollIntoView');
expect(replaceStateForTargetSpy).to.have.been.calledOnce;
expect(scrollIntoViewSpy).to.not.be.called;
replaceStateForTargetResolver();
return replaceStateForTargetPromise.then(() => {
expect(timerFuncSpy).to.be.calledOnce;
expect(scrollIntoViewSpy).to.be.calledTwice;
});
});

it('should push and pop history state', () => {
let historyOnPop;
const historyPushStub = sandbox.stub(history, 'push', onPop => {
historyOnPop = onPop;
});
sandbox.stub(history, 'push');

// Click -> push.
onDocumentElementClick_(evt, ampdoc, viewport, history);
expect(scrollIntoViewSpy.callCount).to.equal(0);
expect(replaceLocSpy.callCount).to.equal(1);
expect(replaceLocSpy.args[0][0]).to.equal('#test');
expect(historyPushStub.callCount).to.equal(1);
expect(historyOnPop).to.exist;

// Pop.
historyOnPop();
expect(replaceLocSpy.callCount).to.equal(2);
expect(replaceLocSpy.args[1][0]).to.equal('#');
expect(replaceStateForTargetSpy.callCount).to.equal(1);
expect(replaceStateForTargetSpy.args[0][0]).to.equal('#test');
});

it('should push and pop history state with pre-existing hash', () => {
win.location.href = 'https://www.google.com/some-path?hello=world#first';
let historyOnPop;
const historyPushStub = sandbox.stub(history, 'push', onPop => {
historyOnPop = onPop;
});
sandbox.stub(history, 'push');

// Click -> push.
onDocumentElementClick_(evt, ampdoc, viewport, history,
/* isIosSafari*/ true, /* isIframed */ false);
expect(historyPushStub.callCount).to.equal(1);
expect(replaceLocSpy.callCount).to.equal(1);
expect(replaceLocSpy.args[0][0]).to.equal('#test');

// Pop.
historyOnPop();
expect(replaceLocSpy.callCount).to.equal(2);
expect(replaceLocSpy.args[1][0]).to.equal('#first');
expect(replaceStateForTargetSpy.callCount).to.equal(1);
expect(replaceStateForTargetSpy.args[0][0]).to.equal('#test');
});
});

Expand Down