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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃悰Restrict checking cached ampDoc reference to AMP elements. #25000

Merged
merged 6 commits into from
Oct 15, 2019
Merged

馃悰Restrict checking cached ampDoc reference to AMP elements. #25000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c1b3884
Restrict checking cached ampDoc reference to AMP elements.
Oct 9, 2019
7ebe097
Handling for non-Element Nodes.
Oct 9, 2019
8dc58cf
Change approach to use `getAmpDoc`.
Oct 14, 2019
60d0f9e
Update ampdoc-impl.js
Oct 15, 2019
ec0b011
Change amp-sidebar tests (and code) to work with ampdoc changes.
Oct 15, 2019
5ff722c
Same changes for amp-sidebar 1.0
Oct 15, 2019
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
2 changes: 1 addition & 1 deletion extensions/amp-sidebar/0.1/amp-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ export class AmpSidebar extends AMP.BaseElement {
);
if (target && target.href) {
const tgtLoc = Services.urlForDoc(element).parse(target.href);
const currentHref = this.getAmpDoc().win.location.href;
const currentHref = this.getAmpDoc().getUrl();
// Important: Only close sidebar (and hence pop sidebar history entry)
// when navigating locally, Chrome might cancel navigation request
// due to after-navigation history manipulation inside a timer callback.
Expand Down
36 changes: 8 additions & 28 deletions extensions/amp-sidebar/0.1/test/test-amp-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,13 +482,8 @@ describes.realWin(
if (eventObj.initEvent) {
eventObj.initEvent('click', true, true);
}
sandbox.stub(sidebarElement, 'getAmpDoc').returns({
win: {
location: {
href: window.location.href,
},
},
});
const ampDoc = sidebarElement.getAmpDoc();
sandbox.stub(ampDoc, 'getUrl').returns(window.location.href);
anchor.dispatchEvent
? anchor.dispatchEvent(eventObj)
: anchor.fireEvent('onkeydown', eventObj);
Expand Down Expand Up @@ -519,16 +514,8 @@ describes.realWin(
if (eventObj.initEvent) {
eventObj.initEvent('click', true, true);
}
sandbox.stub(sidebarElement, 'getAmpDoc').callsFake(() => {
return {
win: {
location: {
// Mocking navigating from example.com -> localhost:9876
href: 'http://example.com',
},
},
};
});
const ampDoc = sidebarElement.getAmpDoc();
sandbox.stub(ampDoc, 'getUrl').returns('http://example.com');
anchor.dispatchEvent
? anchor.dispatchEvent(eventObj)
: anchor.fireEvent('onkeydown', eventObj);
Expand Down Expand Up @@ -559,17 +546,10 @@ describes.realWin(
if (eventObj.initEvent) {
eventObj.initEvent('click', true, true);
}
sandbox.stub(sidebarElement, 'getAmpDoc').callsFake(() => {
return {
win: {
location: {
// Mocking navigating from
// /context.html?old=context -> /context.html
href: 'http://localhost:9876/context.html?old=context',
},
},
};
});
const ampDoc = sidebarElement.getAmpDoc();
sandbox
.stub(ampDoc, 'getUrl')
.returns('http://localhost:9876/context.html?old=context');
anchor.dispatchEvent
? anchor.dispatchEvent(eventObj)
: anchor.fireEvent('onkeydown', eventObj);
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-sidebar/1.0/amp-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ export class AmpSidebar extends AMP.BaseElement {
);
if (target && target.href) {
const tgtLoc = Services.urlForDoc(element).parse(target.href);
const currentHref = this.getAmpDoc().win.location.href;
const currentHref = this.getAmpDoc().getUrl();
// Important: Only close sidebar (and hence pop sidebar history entry)
// when navigating locally, Chrome might cancel navigation request
// due to after-navigation history manipulation inside a timer callback.
Expand Down
39 changes: 11 additions & 28 deletions extensions/amp-sidebar/1.0/test/test-amp-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,13 +485,8 @@ describes.realWin(
if (eventObj.initEvent) {
eventObj.initEvent('click', true, true);
}
sandbox.stub(sidebarElement, 'getAmpDoc').returns({
win: {
location: {
href: window.location.href,
},
},
});
const ampDoc = sidebarElement.getAmpDoc();
sandbox.stub(ampDoc, 'getUrl').returns(window.location.href);
anchor.dispatchEvent
? anchor.dispatchEvent(eventObj)
: anchor.fireEvent('onkeydown', eventObj);
Expand Down Expand Up @@ -522,16 +517,9 @@ describes.realWin(
if (eventObj.initEvent) {
eventObj.initEvent('click', true, true);
}
sandbox.stub(sidebarElement, 'getAmpDoc').callsFake(() => {
return {
win: {
location: {
// Mocking navigating from example.com -> localhost:9876
href: 'http://example.com',
},
},
};
});
const ampDoc = sidebarElement.getAmpDoc();
// Mocking navigating from example.com -> localhost:9876
sandbox.stub(ampDoc, 'getUrl').returns('http://example.com');
anchor.dispatchEvent
? anchor.dispatchEvent(eventObj)
: anchor.fireEvent('onkeydown', eventObj);
Expand Down Expand Up @@ -562,17 +550,12 @@ describes.realWin(
if (eventObj.initEvent) {
eventObj.initEvent('click', true, true);
}
sandbox.stub(sidebarElement, 'getAmpDoc').callsFake(() => {
return {
win: {
location: {
// Mocking navigating from
// /context.html?old=context -> /context.html
href: 'http://localhost:9876/context.html?old=context',
},
},
};
});
const ampDoc = sidebarElement.getAmpDoc();
// Mocking navigating from /context.html?old=context -> /context.html
sandbox
.stub(ampDoc, 'getUrl')
.returns('http://localhost:9876/context.html?old=context');

anchor.dispatchEvent
? anchor.dispatchEvent(eventObj)
: anchor.fireEvent('onkeydown', eventObj);
Expand Down
27 changes: 23 additions & 4 deletions src/service/ampdoc-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,22 @@ export class AmpDocService {
return devAssert(this.singleDoc_);
}

/**
* If the node is an AMP custom element, retrieves the AmpDoc reference.
* @param {!Node} node
* @return {?AmpDoc} The AmpDoc reference, if one exists.
*/
getCustomElementAmpDocReference_(node) {
// We can only look up the AmpDoc from a custom element if it has been
// attached at some point. If it is not a custom element, one or both of
// these checks should fail.
if (!node.everAttached || typeof node.getAmpDoc !== 'function') {
return null;
}

return node.getAmpDoc();
}

/**
* Returns the instance of the ampdoc (`AmpDoc`) that contains the specified
* node. If the runtime is in the single-doc mode, the one global `AmpDoc`
Expand All @@ -135,8 +151,10 @@ export class AmpDocService {
// for the closest AmpDoc, the element might have a reference to the
// global AmpDoc, which we do not want. This occurs when using
// <amp-next-page>.
if (n.ampdoc_) {
return n.ampdoc_;

const cachedAmpDoc = this.getCustomElementAmpDocReference_(node);
if (cachedAmpDoc) {
return cachedAmpDoc;
}

// Root note: it's either a document, or a shadow document.
Expand Down Expand Up @@ -169,8 +187,9 @@ export class AmpDocService {
// for the closest AmpDoc, the element might have a reference to the
// global AmpDoc, which we do not want. This occurs when using
// <amp-next-page>.
if (n.ampdoc_) {
return n.ampdoc_;
const cachedAmpDoc = this.getCustomElementAmpDocReference_(node);
if (cachedAmpDoc) {
return cachedAmpDoc;
}

// Traverse the boundary of a friendly iframe.
Expand Down
41 changes: 33 additions & 8 deletions test/unit/test-ampdoc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,27 @@ describes.sandboxed('AmpDocService', {}, () => {
expect(service.getSingleDoc()).to.be.instanceOf(AmpDocSingle);
});

it('should not return a conflicting value on a form', () => {
const node = document.createElement('form');
// Note: Instead of actually creating an input element with a name that
// conflicts, just set this directly in case the test is ever run in
// an compiled environment.
node.getAmpDoc = 5;

const ampDoc = service.getAmpDocIfAvailable(node);
expect(ampDoc).to.equal(service.getSingleDoc());
});

it('should not return a conflicting value on a document fragment', () => {
// This is a stand-in for testing a document, without actually modifying
// the document to keep the test side-effect free.
const frag = document.createDocumentFragment();
frag.getAmpDoc = 5;

const ampDoc = service.getAmpDocIfAvailable(frag);
expect(ampDoc).to.equal(service.getSingleDoc());
});

it('should always yield the single document', () => {
expect(() => service.getAmpDoc(null)).to.throw;
expect(service.getAmpDoc(document)).to.equal(service.getSingleDoc());
Expand Down Expand Up @@ -162,7 +183,7 @@ describes.sandboxed('AmpDocService', {}, () => {

beforeEach(() => {
service = new AmpDocService(window, /* isSingleDoc */ false);
content = document.createElement('span');
content = document.createElement('amp-img');
host = document.createElement('div');
setShadowDomSupportedVersionForTesting(undefined);
if (isShadowDomSupported()) {
Expand All @@ -189,7 +210,8 @@ describes.sandboxed('AmpDocService', {}, () => {

it('should yield custom-element shadow-doc when exists', () => {
const ampDoc = {};
content.ampdoc_ = ampDoc;
content.everAttached = true;
content.getAmpDoc = () => ampDoc;
host.appendChild(content);
expect(service.getAmpDoc(content)).to.equal(ampDoc);
});
Expand All @@ -205,11 +227,12 @@ describes.sandboxed('AmpDocService', {}, () => {

// Override via custom element.
const ampDoc2 = {};
content.ampdoc_ = ampDoc2;
content.everAttached = true;
content.getAmpDoc = () => ampDoc2;
expect(service.getAmpDoc(content)).to.equal(ampDoc2);

// Fallback to cached version when custom element returns null.
content.ampdoc_ = null;
content.getAmpDoc = () => null;
expect(service.getAmpDoc(content)).to.equal(ampDoc);
});

Expand Down Expand Up @@ -299,7 +322,7 @@ describes.sandboxed('AmpDocService', {}, () => {
beforeEach(() => {
toggleExperiment(window, 'ampdoc-fie', true);
service = new AmpDocService(window, /* isSingleDoc */ true);
content = document.createElement('span');
content = document.createElement('amp-img');
host = document.createElement('div');
setShadowDomSupportedVersionForTesting(undefined);
if (isShadowDomSupported()) {
Expand Down Expand Up @@ -327,7 +350,8 @@ describes.sandboxed('AmpDocService', {}, () => {

it('should yield custom-element doc when exists', () => {
const ampDoc = {};
content.ampdoc_ = ampDoc;
content.everAttached = true;
content.getAmpDoc = () => ampDoc;
host.appendChild(content);
expect(service.getAmpDoc(content)).to.equal(ampDoc);
});
Expand All @@ -343,11 +367,12 @@ describes.sandboxed('AmpDocService', {}, () => {

// Override via custom element.
const ampDoc2 = {};
content.ampdoc_ = ampDoc2;
content.everAttached = true;
content.getAmpDoc = () => ampDoc2;
expect(service.getAmpDoc(content)).to.equal(ampDoc2);

// Fallback to cached version when custom element returns null.
content.ampdoc_ = null;
content.getAmpDoc = () => null;
expect(service.getAmpDoc(content)).to.equal(ampDoc);
});

Expand Down