From b1611b998e783e679d402a85101b87be2fd2481c Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Fri, 5 Jul 2019 13:46:07 +0200 Subject: [PATCH 1/7] Refactor BookmarkPanelHub.jsm --- lib/BookmarkPanelHub.jsm | 132 +++++++++++++++++++++------------------ 1 file changed, 72 insertions(+), 60 deletions(-) diff --git a/lib/BookmarkPanelHub.jsm b/lib/BookmarkPanelHub.jsm index b6b21a0484..6cd478e142 100644 --- a/lib/BookmarkPanelHub.jsm +++ b/lib/BookmarkPanelHub.jsm @@ -19,6 +19,19 @@ ChromeUtils.defineModuleGetter( "resource://gre/modules/PrivateBrowsingUtils.jsm" ); +const MARKUP_IDENTIFIERS = { + messageHeader: "editBookmarkPanelRecommendationTitle", + headerClass: "cfrMessageHeader", + contentParagraph: "editBookmarkPanelRecommendationContent", + ctaBtn: "editBookmarkPanelRecommendationCta", + // Div that contains header, message and cta + messageContainer: "cfrMessageContainer", + // Bookmark panel element where the messageContainer will be attached + panelAnchor: "editBookmarkPanelRecommendation", + // Bookmark panel info button used to toggle the message + infoButton: "editBookmarkPanelInfoButton", +}; + class _BookmarkPanelHub { constructor() { this._id = "BookmarkPanelHub"; @@ -27,8 +40,7 @@ class _BookmarkPanelHub { this._addImpression = null; this._dispatch = null; this._initialized = false; - this._response = null; - this._l10n = null; + this._state = null; this.messageRequest = this.messageRequest.bind(this); this.toggleRecommendation = this.toggleRecommendation.bind(this); @@ -45,17 +57,15 @@ class _BookmarkPanelHub { this._handleMessageRequest = handleMessageRequest; this._addImpression = addImpression; this._dispatch = dispatch; - this._l10n = new DOMLocalization(); this._initialized = true; } uninit() { - this._l10n = null; this._initialized = false; this._handleMessageRequest = null; this._addImpression = null; this._dispatch = null; - this._response = null; + this._state = null; } /** @@ -89,31 +99,33 @@ class _BookmarkPanelHub { triggerId: this._trigger.id, }); - return this.onResponse(response, target, win); + return this.onResponse(target, win, response); + } + + insertFTLIfNeeded(win) { + // Only insert localization files if we need to show a message + win.MozXULElement.insertFTLIfNeeded("browser/newtab/asrouter.ftl"); + win.MozXULElement.insertFTLIfNeeded("browser/branding/sync-brand.ftl"); } /** * If the response contains a message render it and send an impression. * Otherwise we remove the message from the container. */ - onResponse(response, target, win) { - this._response = { - ...response, + onResponse(target, win, response) { + this._state = { + message: response, collapsed: false, - target, - win, url: target.url, }; if (response && response.content) { - // Only insert localization files if we need to show a message - win.MozXULElement.insertFTLIfNeeded("browser/newtab/asrouter.ftl"); - win.MozXULElement.insertFTLIfNeeded("browser/branding/sync-brand.ftl"); - this.showMessage(response.content, target, win); + this.insertFTLIfNeeded(win); + this.showMessage(target, win, response.content); this.sendImpression(); this.sendUserEventTelemetry("IMPRESSION", win); } else { - this.hideMessage(target); + this.removeMessage(target); } target.infoButton.disabled = !response; @@ -121,20 +133,23 @@ class _BookmarkPanelHub { return !!response; } - showMessage(message, target, win) { - if (this._response && this._response.collapsed) { - this.toggleRecommendation(false); + showMessage(target, win, message) { + const { document } = win; + if (this._state.collapsed) { + this.toggleRecommendation(target, false); return; } const createElement = elem => target.document.createElementNS("http://www.w3.org/1999/xhtml", elem); - let recommendation = target.container.querySelector("#cfrMessageContainer"); + let recommendation = document.getElementById( + MARKUP_IDENTIFIERS.messageContainer + ); if (!recommendation) { recommendation = createElement("div"); const headerContainer = createElement("div"); - headerContainer.classList.add("cfrMessageHeader"); - recommendation.setAttribute("id", "cfrMessageContainer"); + headerContainer.classList.add(MARKUP_IDENTIFIERS.headerClass); + recommendation.setAttribute("id", MARKUP_IDENTIFIERS.messageContainer); recommendation.addEventListener("click", async e => { target.hidePopup(); const url = await FxAccounts.config.promiseEmailFirstURI("bookmark"); @@ -157,25 +172,25 @@ class _BookmarkPanelHub { close.style.color = message.color; close.addEventListener("click", e => { this.sendUserEventTelemetry("DISMISS", win); - this.collapseMessage(); + this.collapseMessage(target); target.close(e); }); const title = createElement("h1"); - title.setAttribute("id", "editBookmarkPanelRecommendationTitle"); + title.setAttribute("id", MARKUP_IDENTIFIERS.messageHeader); const content = createElement("p"); - content.setAttribute("id", "editBookmarkPanelRecommendationContent"); + content.setAttribute("id", MARKUP_IDENTIFIERS.contentParagraph); const cta = createElement("button"); - cta.setAttribute("id", "editBookmarkPanelRecommendationCta"); + cta.setAttribute("id", MARKUP_IDENTIFIERS.ctaBtn); // If `string_id` is present it means we are relying on fluent for translations if (message.text.string_id) { - this._l10n.setAttributes( + document.l10n.setAttributes( close, message.close_button.tooltiptext.string_id ); - this._l10n.setAttributes(title, message.title.string_id); - this._l10n.setAttributes(content, message.text.string_id); - this._l10n.setAttributes(cta, message.cta.string_id); + document.l10n.setAttributes(title, message.title.string_id); + document.l10n.setAttributes(content, message.text.string_id); + document.l10n.setAttributes(cta, message.cta.string_id); } else { close.setAttribute("title", message.close_button.tooltiptext); title.textContent = message.title; @@ -243,60 +258,57 @@ class _BookmarkPanelHub { } if (target.infoButton.checked) { // If it was ever collapsed we need to cancel the state - this._response.collapsed = false; + this._state.collapsed = false; target.container.removeAttribute("disabled"); } else { target.container.setAttribute("disabled", "disabled"); } } - collapseMessage() { - this._response.collapsed = true; - this.toggleRecommendation(false); + collapseMessage(target) { + this._state.collapsed = true; + this.toggleRecommendation(target, false); } _removeContainer(target) { - if (target || (this._response && this._response.target)) { - const container = ( - target || this._response.target - ).container.querySelector("#cfrMessageContainer"); - if (container) { - this._restorePanelHeight(this._response.win); - container.remove(); - } + const container = target.document.getElementById( + MARKUP_IDENTIFIERS.messageContainer + ); + if (container) { + this._restorePanelHeight(this._response.win); + container.remove(); } } - hideMessage(target) { + removeMessage(target) { this._removeContainer(target); - this.toggleRecommendation(false); - this._response = null; + this.toggleRecommendation(target, false); + this._state = null; } _forceShowMessage(target, message) { const doc = target.browser.ownerGlobal.gBrowser.ownerDocument; const win = target.browser.ownerGlobal.window; const panelTarget = { - container: doc.getElementById("editBookmarkPanelRecommendation"), - infoButton: doc.getElementById("editBookmarkPanelInfoButton"), + container: doc.getElementById(`${MARKUP_IDENTIFIERS.panelAnchor}`), + infoButton: doc.getElementById(`${MARKUP_IDENTIFIERS.infoButton}`), document: doc, - close: e => { - e.stopPropagation(); - this.toggleRecommendation(false); - }, + }; + panelTarget.close = e => { + e.stopPropagation(); + this.toggleRecommendation(panelTarget, false); }; // Remove any existing message - this.hideMessage(panelTarget); + this.removeMessage(panelTarget); + // Reset the reference to the panel elements - this._response = { target: panelTarget, win }; - // Required if we want to preview messages that include fluent strings - win.MozXULElement.insertFTLIfNeeded("browser/newtab/asrouter.ftl"); - win.MozXULElement.insertFTLIfNeeded("browser/branding/sync-brand.ftl"); - this.showMessage(message.content, panelTarget, win); + this._state = { message, collapsed: false }; + this.insertFTLIfNeeded(win); + this.showMessage(panelTarget, win, message.content); } sendImpression() { - this._addImpression(this._response); + this._addImpression(this._state.message); } sendUserEventTelemetry(event, win) { @@ -307,8 +319,8 @@ class _BookmarkPanelHub { ) ) { this._sendTelemetry({ - message_id: this._response.id, - bucket_id: this._response.id, + message_id: this._state.message.id, + bucket_id: this._state.message.id, event, }); } From 4be9946634ccad6976f9703d77b8482fb40071ac Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Fri, 5 Jul 2019 14:59:40 +0200 Subject: [PATCH 2/7] Update tests --- test/unit/lib/BookmarkPanelHub.test.js | 161 ++++++++++++++----------- 1 file changed, 90 insertions(+), 71 deletions(-) diff --git a/test/unit/lib/BookmarkPanelHub.test.js b/test/unit/lib/BookmarkPanelHub.test.js index b5707c8632..7a2c2e76d7 100644 --- a/test/unit/lib/BookmarkPanelHub.test.js +++ b/test/unit/lib/BookmarkPanelHub.test.js @@ -49,6 +49,7 @@ describe("BookmarkPanelHub", () => { classList: { add: sandbox.stub() }, appendChild: sandbox.stub(), querySelector: sandbox.stub(), + remove: sandbox.stub(), children: [], style: {}, getBoundingClientRect: sandbox.stub(), @@ -108,7 +109,6 @@ describe("BookmarkPanelHub", () => { assert.equal(instance._addImpression, fakeAddImpression); assert.equal(instance._handleMessageRequest, fakeHandleMessageRequest); assert.equal(instance._dispatch, fakeDispatch); - assert.ok(instance._l10n); assert.isTrue(instance._initialized); }); it("should return early if not initialized", async () => { @@ -123,7 +123,7 @@ describe("BookmarkPanelHub", () => { sandbox.restore(); }); it("should not re-request messages for the same URL", async () => { - instance._response = { url: "foo.com", content: true }; + instance._state = { url: "foo.com", message: { content: true } }; fakeTarget.url = "foo.com"; sandbox.stub(instance, "showMessage"); @@ -178,6 +178,23 @@ describe("BookmarkPanelHub", () => { }); it("should insert the appropriate ftl files with translations", () => { instance.onResponse({ content: "content" }, fakeTarget, fakeWindow); + sandbox.stub(instance, "removeMessage"); + fakeTarget = { infoButton: { disabled: true } }; + }); + it("should show a message when called with a response", () => { + instance.onResponse(fakeTarget, fakeWindow, { content: "content" }); + + assert.calledOnce(instance.showMessage); + assert.calledWithExactly( + instance.showMessage, + fakeTarget, + fakeWindow, + "content" + ); + assert.calledOnce(instance.sendImpression); + }); + it("should insert the appropriate ftl files with translations", () => { + instance.onResponse(fakeTarget, fakeWindow, { content: "content" }); assert.calledTwice(fakeWindow.MozXULElement.insertFTLIfNeeded); assert.calledWith( @@ -192,7 +209,7 @@ describe("BookmarkPanelHub", () => { it("should dispatch a user impression", () => { sandbox.spy(instance, "sendUserEventTelemetry"); - instance.onResponse({ content: "content" }, fakeTarget, fakeWindow); + instance.onResponse(fakeTarget, fakeWindow, { content: "content" }); assert.calledOnce(instance.sendUserEventTelemetry); assert.calledWithExactly( @@ -211,7 +228,7 @@ describe("BookmarkPanelHub", () => { isBrowserPrivateStub.returns(true); sandbox.spy(instance, "sendUserEventTelemetry"); - instance.onResponse({ content: "content" }, fakeTarget, fakeWindow); + instance.onResponse(fakeTarget, fakeWindow, { content: "content" }); assert.calledOnce(instance.sendUserEventTelemetry); assert.calledWithExactly( @@ -222,39 +239,41 @@ describe("BookmarkPanelHub", () => { assert.notCalled(fakeDispatch); }); it("should hide existing messages if no response is provided", () => { - instance.onResponse(null, fakeTarget); + instance.onResponse(fakeTarget, fakeWindow, null); - assert.calledOnce(instance.hideMessage); - assert.calledWithExactly(instance.hideMessage, fakeTarget); + assert.calledOnce(instance.removeMessage); + assert.calledWithExactly(instance.removeMessage, fakeTarget); }); }); describe("#showMessage.collapsed=false", () => { beforeEach(() => { instance.init(fakeHandleMessageRequest, fakeAddImpression, fakeDispatch); sandbox.stub(instance, "toggleRecommendation"); - sandbox.stub(instance, "_response").value({ collapsed: false }); + sandbox + .stub(instance, "_state") + .value({ collapsed: false, message: fakeMessage }); }); it("should create a container", () => { - fakeTarget.container.querySelector.returns(false); + fakeWindow.document.getElementById.returns(null); - instance.showMessage(fakeMessage, fakeTarget); + instance.showMessage(fakeTarget, fakeWindow, fakeMessage); assert.equal(fakeTarget.document.createElementNS.callCount, 6); assert.calledOnce(fakeTarget.container.appendChild); assert.notCalled(fakeL10n.setAttributes); }); it("should create a container (fluent message)", () => { - fakeTarget.container.querySelector.returns(false); + fakeWindow.document.getElementById.returns(null); - instance.showMessage(fakeMessageFluent, fakeTarget); + instance.showMessage(fakeTarget, fakeWindow, fakeMessageFluent); assert.equal(fakeTarget.document.createElementNS.callCount, 6); assert.calledOnce(fakeTarget.container.appendChild); }); it("should set l10n attributes", () => { - fakeTarget.container.querySelector.returns(false); + fakeWindow.document.getElementById.returns(null); - instance.showMessage(fakeMessageFluent, fakeTarget); + instance.showMessage(fakeTarget, fakeWindow, fakeMessageFluent); assert.equal(fakeL10n.setAttributes.callCount, 4); }); @@ -272,16 +291,14 @@ describe("BookmarkPanelHub", () => { ); }); it("should reuse the container", () => { - fakeTarget.container.querySelector.returns(true); - - instance.showMessage(fakeMessage, fakeTarget); + instance.showMessage(fakeTarget, fakeWindow, fakeMessage); assert.notCalled(fakeTarget.container.appendChild); }); it("should open a tab with FxA signup", async () => { - fakeTarget.container.querySelector.returns(false); + fakeWindow.document.getElementById.returns(null); - instance.showMessage(fakeMessage, fakeTarget, fakeWindow); + instance.showMessage(fakeTarget, fakeWindow, fakeMessage); // Call the event listener cb await fakeContainer.addEventListener.firstCall.args[1](); @@ -289,9 +306,9 @@ describe("BookmarkPanelHub", () => { }); it("should send a click event", async () => { sandbox.stub(instance, "sendUserEventTelemetry"); - fakeTarget.container.querySelector.returns(false); + fakeWindow.document.getElementById.returns(null); - instance.showMessage(fakeMessage, fakeTarget, fakeWindow); + instance.showMessage(fakeTarget, fakeWindow, fakeMessage); // Call the event listener cb await fakeContainer.addEventListener.firstCall.args[1](); @@ -304,9 +321,9 @@ describe("BookmarkPanelHub", () => { }); it("should send a click event", async () => { sandbox.stub(instance, "sendUserEventTelemetry"); - fakeTarget.container.querySelector.returns(false); + fakeWindow.document.getElementById.returns(null); - instance.showMessage(fakeMessage, fakeTarget, fakeWindow); + instance.showMessage(fakeTarget, fakeWindow, fakeMessage); // Call the event listener cb await fakeContainer.addEventListener.firstCall.args[1](); @@ -318,11 +335,11 @@ describe("BookmarkPanelHub", () => { ); }); it("should collapse the message", () => { - fakeTarget.container.querySelector.returns(false); sandbox.spy(instance, "collapseMessage"); - instance._response.collapsed = false; + instance._state.collapsed = false; + fakeWindow.document.getElementById.returns(null); - instance.showMessage(fakeMessage, fakeTarget, fakeWindow); + instance.showMessage(fakeTarget, fakeWindow, fakeMessage); // Show message calls it once so we need to reset instance.toggleRecommendation.reset(); // Call the event listener cb @@ -330,15 +347,16 @@ describe("BookmarkPanelHub", () => { assert.calledOnce(instance.collapseMessage); assert.calledOnce(fakeTarget.close); - assert.isTrue(instance._response.collapsed); + assert.isTrue(instance._state.collapsed); assert.calledOnce(instance.toggleRecommendation); }); it("should send a dismiss event", () => { sandbox.stub(instance, "sendUserEventTelemetry"); sandbox.spy(instance, "collapseMessage"); - instance._response.collapsed = false; + instance._state.collapsed = false; + fakeWindow.document.getElementById.returns(null); - instance.showMessage(fakeMessage, fakeTarget, fakeWindow); + instance.showMessage(fakeTarget, fakeWindow, fakeMessage); // Call the event listener cb fakeContainer.addEventListener.secondCall.args[1](); @@ -350,10 +368,10 @@ describe("BookmarkPanelHub", () => { ); }); it("should call toggleRecommendation `true`", () => { - instance.showMessage(fakeMessage, fakeTarget, fakeWindow); + instance.showMessage(fakeTarget, fakeWindow, fakeMessage); assert.calledOnce(instance.toggleRecommendation); - assert.calledWithExactly(instance.toggleRecommendation, true); + assert.calledWithExactly(instance.toggleRecommendation, fakeTarget, true); }); }); describe("#showMessage.collapsed=true", () => { @@ -364,84 +382,77 @@ describe("BookmarkPanelHub", () => { sandbox.stub(instance, "toggleRecommendation"); }); it("should return early if the message is collapsed", () => { - instance.showMessage(); + instance.showMessage(fakeTarget, fakeWindow); assert.calledOnce(instance.toggleRecommendation); - assert.calledWithExactly(instance.toggleRecommendation, false); + assert.calledWithExactly( + instance.toggleRecommendation, + fakeTarget, + false + ); }); }); - describe("#hideMessage", () => { - let removeStub; + describe("#removeMessage", () => { beforeEach(() => { sandbox.stub(instance, "toggleRecommendation"); - removeStub = sandbox.stub(); - fakeTarget = { - container: { - querySelector: sandbox.stub().returns({ remove: removeStub }), - }, - }; - instance._response = { win: fakeWindow }; }); it("should remove the message", () => { - instance.hideMessage(fakeTarget); + instance.removeMessage(fakeTarget); - assert.calledOnce(removeStub); + assert.calledOnce(fakeContainer.remove); }); it("should call toggleRecommendation `false`", () => { - instance.hideMessage(fakeTarget); + instance.removeMessage(fakeTarget); assert.calledOnce(instance.toggleRecommendation); - assert.calledWithExactly(instance.toggleRecommendation, false); + assert.calledWithExactly( + instance.toggleRecommendation, + fakeTarget, + false + ); }); }); describe("#toggleRecommendation", () => { - let target; beforeEach(() => { - target = { - infoButton: {}, - container: { - setAttribute: sandbox.stub(), - removeAttribute: sandbox.stub(), - }, - }; - sandbox.stub(instance, "_response").value({ target }); + instance._state = {}; }); it("should check infoButton", () => { - instance.toggleRecommendation(true); + instance.toggleRecommendation(fakeTarget, true); - assert.isTrue(target.infoButton.checked); + assert.isTrue(fakeTarget.infoButton.checked); }); it("should uncheck infoButton", () => { - instance.toggleRecommendation(false); + instance.toggleRecommendation(fakeTarget, false); - assert.isFalse(target.infoButton.checked); + assert.isFalse(fakeTarget.infoButton.checked); }); it("should uncheck infoButton", () => { - target.infoButton.checked = true; + fakeTarget.infoButton.checked = true; - instance.toggleRecommendation(); + instance.toggleRecommendation(fakeTarget); - assert.isFalse(target.infoButton.checked); + assert.isFalse(fakeTarget.infoButton.checked); }); it("should disable the container", () => { - target.infoButton.checked = true; + fakeTarget.infoButton.checked = true; - instance.toggleRecommendation(); + instance.toggleRecommendation(fakeTarget); - assert.calledOnce(target.container.setAttribute); + assert.calledOnce(fakeTarget.container.setAttribute); }); it("should enable container", () => { - target.infoButton.checked = false; + fakeTarget.infoButton.checked = false; - instance.toggleRecommendation(); + instance.toggleRecommendation(fakeTarget); - assert.calledOnce(target.container.removeAttribute); + assert.calledOnce(fakeTarget.container.removeAttribute); + assert.isFalse(instance._state.collapsed); }); }); describe("#_forceShowMessage", () => { it("should call showMessage with the correct args", () => { sandbox.spy(instance, "showMessage"); - sandbox.stub(instance, "hideMessage"); + sandbox.stub(instance, "removeMessage"); instance._forceShowMessage(fakeTarget, { content: fakeMessage }); @@ -465,9 +476,17 @@ describe("BookmarkPanelHub", () => { sandbox.spy(instance, "showMessage"); sandbox.stub(instance, "toggleRecommendation"); sandbox.stub(instance, "sendUserEventTelemetry"); + // Force rendering the message + fakeWindow.document.getElementById + .returns(null) + .withArgs("editBookmarkPanelRecommendation") + .returns(fakeContainer); instance._forceShowMessage(fakeTarget, { content: fakeMessage }); + assert.calledTwice(fakeContainer.addEventListener); + + const [target] = instance.showMessage.firstCall.args; const [ , eventListenerCb, @@ -476,13 +495,13 @@ describe("BookmarkPanelHub", () => { instance.toggleRecommendation.reset(); eventListenerCb({ stopPropagation: sandbox.stub() }); - assert.calledWithExactly(instance.toggleRecommendation, false); + assert.calledWithExactly(instance.toggleRecommendation, target, false); }); }); describe("#sendImpression", () => { beforeEach(() => { instance.init(fakeHandleMessageRequest, fakeAddImpression, fakeDispatch); - instance._response = "foo"; + instance._state = { message: "foo" }; }); it("should dispatch an impression", () => { instance.sendImpression(); From 606bd31a76e497ff8ecf20d85e67fd43d883e783 Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Wed, 17 Jul 2019 15:18:04 +0200 Subject: [PATCH 3/7] Fix tests --- lib/BookmarkPanelHub.jsm | 37 +++++++++-------------- test/unit/lib/BookmarkPanelHub.test.js | 42 ++++++++------------------ 2 files changed, 26 insertions(+), 53 deletions(-) diff --git a/lib/BookmarkPanelHub.jsm b/lib/BookmarkPanelHub.jsm index 6cd478e142..1bb2fca6f9 100644 --- a/lib/BookmarkPanelHub.jsm +++ b/lib/BookmarkPanelHub.jsm @@ -58,6 +58,7 @@ class _BookmarkPanelHub { this._addImpression = addImpression; this._dispatch = dispatch; this._initialized = true; + this._state = {}; } uninit() { @@ -82,19 +83,14 @@ class _BookmarkPanelHub { return false; } - if ( - this._response && - this._response.win === win && - this._response.url === target.url && - this._response.content - ) { - this.showMessage(this._response.content, target, win); + if (this._state.url === target.url && this._state.message) { + this.showMessage(target, win, this._state.message.content); return true; } // If we didn't match on a previously cached request then make sure // the container is empty - this._removeContainer(target); + this._removeContainer(target, win); const response = await this._handleMessageRequest({ triggerId: this._trigger.id, }); @@ -125,7 +121,7 @@ class _BookmarkPanelHub { this.sendImpression(); this.sendUserEventTelemetry("IMPRESSION", win); } else { - this.removeMessage(target); + this.removeMessage(target, win); } target.infoButton.disabled = !response; @@ -206,7 +202,7 @@ class _BookmarkPanelHub { target.container.appendChild(recommendation); } - this.toggleRecommendation(true); + this.toggleRecommendation(target, true); this._adjustPanelHeight(win, recommendation); } @@ -244,12 +240,7 @@ class _BookmarkPanelHub { document.getElementById("editBookmarkPanelImage").style.height = ""; } - toggleRecommendation(visible) { - if (!this._response) { - return; - } - - const { target } = this._response; + toggleRecommendation(target, visible) { if (visible === undefined) { // When called from the info button of the bookmark panel target.infoButton.checked = !target.infoButton.checked; @@ -270,18 +261,18 @@ class _BookmarkPanelHub { this.toggleRecommendation(target, false); } - _removeContainer(target) { + _removeContainer(target, win) { const container = target.document.getElementById( MARKUP_IDENTIFIERS.messageContainer ); if (container) { - this._restorePanelHeight(this._response.win); + this._restorePanelHeight(win); container.remove(); } } - removeMessage(target) { - this._removeContainer(target); + removeMessage(target, win) { + this._removeContainer(target, win); this.toggleRecommendation(target, false); this._state = null; } @@ -290,8 +281,8 @@ class _BookmarkPanelHub { const doc = target.browser.ownerGlobal.gBrowser.ownerDocument; const win = target.browser.ownerGlobal.window; const panelTarget = { - container: doc.getElementById(`${MARKUP_IDENTIFIERS.panelAnchor}`), - infoButton: doc.getElementById(`${MARKUP_IDENTIFIERS.infoButton}`), + container: doc.getElementById(MARKUP_IDENTIFIERS.panelAnchor), + infoButton: doc.getElementById(MARKUP_IDENTIFIERS.infoButton), document: doc, }; panelTarget.close = e => { @@ -299,7 +290,7 @@ class _BookmarkPanelHub { this.toggleRecommendation(panelTarget, false); }; // Remove any existing message - this.removeMessage(panelTarget); + this.removeMessage(panelTarget, win); // Reset the reference to the panel elements this._state = { message, collapsed: false }; diff --git a/test/unit/lib/BookmarkPanelHub.test.js b/test/unit/lib/BookmarkPanelHub.test.js index 7a2c2e76d7..ab6d7bce26 100644 --- a/test/unit/lib/BookmarkPanelHub.test.js +++ b/test/unit/lib/BookmarkPanelHub.test.js @@ -135,7 +135,7 @@ describe("BookmarkPanelHub", () => { it("should call handleMessageRequest", async () => { fakeHandleMessageRequest.resolves(fakeMessage); - await instance.messageRequest(fakeTarget, {}); + await instance.messageRequest(fakeTarget, fakeWindow); assert.calledOnce(fakeHandleMessageRequest); assert.calledWithExactly(fakeHandleMessageRequest, { @@ -145,14 +145,14 @@ describe("BookmarkPanelHub", () => { it("should call onResponse", async () => { fakeHandleMessageRequest.resolves(fakeMessage); - await instance.messageRequest(fakeTarget, {}); + await instance.messageRequest(fakeTarget, fakeWindow); assert.calledOnce(instance.onResponse); assert.calledWithExactly( instance.onResponse, - fakeMessage, fakeTarget, - {} + fakeWindow, + fakeMessage ); }); }); @@ -161,23 +161,6 @@ describe("BookmarkPanelHub", () => { instance.init(fakeHandleMessageRequest, fakeAddImpression, fakeDispatch); sandbox.stub(instance, "showMessage"); sandbox.stub(instance, "sendImpression"); - sandbox.stub(instance, "hideMessage"); - fakeTarget = { infoButton: { disabled: true } }; - }); - it("should show a message when called with a response", () => { - instance.onResponse({ content: "content" }, fakeTarget, fakeWindow); - - assert.calledOnce(instance.showMessage); - assert.calledWithExactly( - instance.showMessage, - "content", - fakeTarget, - fakeWindow - ); - assert.calledOnce(instance.sendImpression); - }); - it("should insert the appropriate ftl files with translations", () => { - instance.onResponse({ content: "content" }, fakeTarget, fakeWindow); sandbox.stub(instance, "removeMessage"); fakeTarget = { infoButton: { disabled: true } }; }); @@ -242,7 +225,7 @@ describe("BookmarkPanelHub", () => { instance.onResponse(fakeTarget, fakeWindow, null); assert.calledOnce(instance.removeMessage); - assert.calledWithExactly(instance.removeMessage, fakeTarget); + assert.calledWithExactly(instance.removeMessage, fakeTarget, fakeWindow); }); }); describe("#showMessage.collapsed=false", () => { @@ -376,9 +359,8 @@ describe("BookmarkPanelHub", () => { }); describe("#showMessage.collapsed=true", () => { beforeEach(() => { - sandbox - .stub(instance, "_response") - .value({ collapsed: true, target: fakeTarget }); + instance.init({}); + instance._state.collapsed = true; sandbox.stub(instance, "toggleRecommendation"); }); it("should return early if the message is collapsed", () => { @@ -397,12 +379,12 @@ describe("BookmarkPanelHub", () => { sandbox.stub(instance, "toggleRecommendation"); }); it("should remove the message", () => { - instance.removeMessage(fakeTarget); + instance.removeMessage(fakeTarget, fakeWindow); assert.calledOnce(fakeContainer.remove); }); it("should call toggleRecommendation `false`", () => { - instance.removeMessage(fakeTarget); + instance.removeMessage(fakeTarget, fakeWindow); assert.calledOnce(instance.toggleRecommendation); assert.calledWithExactly( @@ -457,12 +439,12 @@ describe("BookmarkPanelHub", () => { instance._forceShowMessage(fakeTarget, { content: fakeMessage }); assert.calledOnce(instance.showMessage); - assert.calledOnce(instance.hideMessage); + assert.calledOnce(instance.removeMessage); assert.calledWithExactly( instance.showMessage, - fakeMessage, sinon.match.object, - fakeWindow + fakeWindow, + fakeMessage ); }); it("should insert required fluent files", () => { From 5f81bb61b307eed6f845f88dad8affddc163b44a Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Thu, 6 Jun 2019 17:08:01 +0200 Subject: [PATCH 4/7] (nobug) - Open the bookmark panel when previewing the FxA message --- lib/BookmarkPanelHub.jsm | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/BookmarkPanelHub.jsm b/lib/BookmarkPanelHub.jsm index 1bb2fca6f9..d5ec47ec91 100644 --- a/lib/BookmarkPanelHub.jsm +++ b/lib/BookmarkPanelHub.jsm @@ -277,9 +277,16 @@ class _BookmarkPanelHub { this._state = null; } - _forceShowMessage(target, message) { - const doc = target.browser.ownerGlobal.gBrowser.ownerDocument; + async _forceShowMessage(target, message) { const win = target.browser.ownerGlobal.window; + // Bookmark the page to force the panel to show and + // remove the bookmark when the panel is hidden + win.StarUI.panel.addEventListener("popupshown", () => { + win.StarUI._removeBookmarksOnPopupHidden = true; + }, {once: true}); + await win.PlacesCommandHook.bookmarkPage(); + + const doc = target.browser.ownerGlobal.gBrowser.ownerDocument; const panelTarget = { container: doc.getElementById(MARKUP_IDENTIFIERS.panelAnchor), infoButton: doc.getElementById(MARKUP_IDENTIFIERS.infoButton), From 5cd56a540d15f0928e5526a09360c8e308d36a73 Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Thu, 6 Jun 2019 18:07:29 +0200 Subject: [PATCH 5/7] Merge open bookmark panel PR --- lib/BookmarkPanelHub.jsm | 10 +++++--- test/unit/lib/BookmarkPanelHub.test.js | 35 +++++++++++++++++++++----- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/lib/BookmarkPanelHub.jsm b/lib/BookmarkPanelHub.jsm index d5ec47ec91..62c072eda8 100644 --- a/lib/BookmarkPanelHub.jsm +++ b/lib/BookmarkPanelHub.jsm @@ -281,9 +281,13 @@ class _BookmarkPanelHub { const win = target.browser.ownerGlobal.window; // Bookmark the page to force the panel to show and // remove the bookmark when the panel is hidden - win.StarUI.panel.addEventListener("popupshown", () => { - win.StarUI._removeBookmarksOnPopupHidden = true; - }, {once: true}); + win.StarUI.panel.addEventListener( + "popupshown", + () => { + win.StarUI._removeBookmarksOnPopupHidden = true; + }, + { once: true } + ); await win.PlacesCommandHook.bookmarkPage(); const doc = target.browser.ownerGlobal.gBrowser.ownerDocument; diff --git a/test/unit/lib/BookmarkPanelHub.test.js b/test/unit/lib/BookmarkPanelHub.test.js index ab6d7bce26..1ff5babf2c 100644 --- a/test/unit/lib/BookmarkPanelHub.test.js +++ b/test/unit/lib/BookmarkPanelHub.test.js @@ -67,6 +67,13 @@ describe("BookmarkPanelHub", () => { MozXULElement: { insertFTLIfNeeded: sandbox.stub() }, document, requestAnimationFrame: x => x(), + StarUI: { + panel: { + addEventListener: sandbox.stub(), + }, + _removeBookmarksOnPopupHidden: false, + }, + PlacesCommandHook: { bookmarkPage: sandbox.stub().resolves() }, }; fakeTarget = { document, @@ -432,11 +439,11 @@ describe("BookmarkPanelHub", () => { }); }); describe("#_forceShowMessage", () => { - it("should call showMessage with the correct args", () => { + it("should call showMessage with the correct args", async () => { sandbox.spy(instance, "showMessage"); sandbox.stub(instance, "removeMessage"); - instance._forceShowMessage(fakeTarget, { content: fakeMessage }); + await instance._forceShowMessage(fakeTarget, { content: fakeMessage }); assert.calledOnce(instance.showMessage); assert.calledOnce(instance.removeMessage); @@ -447,14 +454,14 @@ describe("BookmarkPanelHub", () => { fakeMessage ); }); - it("should insert required fluent files", () => { + it("should insert required fluent files", async () => { sandbox.stub(instance, "showMessage"); - instance._forceShowMessage(fakeTarget, { content: fakeMessage }); + await instance._forceShowMessage(fakeTarget, { content: fakeMessage }); assert.calledTwice(fakeWindow.MozXULElement.insertFTLIfNeeded); }); - it("should insert a message you can collapse", () => { + it("should insert a message you can collapse", async () => { sandbox.spy(instance, "showMessage"); sandbox.stub(instance, "toggleRecommendation"); sandbox.stub(instance, "sendUserEventTelemetry"); @@ -464,7 +471,7 @@ describe("BookmarkPanelHub", () => { .withArgs("editBookmarkPanelRecommendation") .returns(fakeContainer); - instance._forceShowMessage(fakeTarget, { content: fakeMessage }); + await instance._forceShowMessage(fakeTarget, { content: fakeMessage }); assert.calledTwice(fakeContainer.addEventListener); @@ -479,6 +486,22 @@ describe("BookmarkPanelHub", () => { assert.calledWithExactly(instance.toggleRecommendation, target, false); }); + it("should open the bookmark panel when force showing the message", async () => { + sandbox.stub(instance, "showMessage"); + sandbox.stub(instance, "removeMessage"); + + await instance._forceShowMessage(fakeTarget, { content: fakeMessage }); + const [ + , + popupshownCb, + ] = fakeWindow.StarUI.panel.addEventListener.firstCall.args; + + assert.isFalse(fakeWindow.StarUI._removeBookmarksOnPopupHidden); + // call the event listener + popupshownCb(); + + assert.isTrue(fakeWindow.StarUI._removeBookmarksOnPopupHidden); + }); }); describe("#sendImpression", () => { beforeEach(() => { From 58967c9515d19f61f9be7817a471ba33986eeee8 Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Fri, 26 Jul 2019 15:20:22 +0200 Subject: [PATCH 6/7] Allow preview endpoints to trigger bookmark panel messages --- lib/ASRouter.jsm | 7 ++++++- lib/BookmarkPanelHub.jsm | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/ASRouter.jsm b/lib/ASRouter.jsm index b1e565a337..dc4b7d24fb 100644 --- a/lib/ASRouter.jsm +++ b/lib/ASRouter.jsm @@ -1487,7 +1487,12 @@ class _ASRouter { } else { await this.setState({ lastMessageId: message ? message.id : null }); } - await this._sendMessageToTarget(message, target, trigger); + await this._sendMessageToTarget( + message, + target, + trigger, + Boolean(previewMsgs.length) + ); } handleMessageRequest({ triggerId, template, returnAll = false }) { diff --git a/lib/BookmarkPanelHub.jsm b/lib/BookmarkPanelHub.jsm index 62c072eda8..84537bceb2 100644 --- a/lib/BookmarkPanelHub.jsm +++ b/lib/BookmarkPanelHub.jsm @@ -274,7 +274,7 @@ class _BookmarkPanelHub { removeMessage(target, win) { this._removeContainer(target, win); this.toggleRecommendation(target, false); - this._state = null; + this._state = {}; } async _forceShowMessage(target, message) { From 15fe30a384ae8310b930b0431b8286cda4dfbe90 Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Tue, 30 Jul 2019 10:52:10 +0200 Subject: [PATCH 7/7] Update mochitest --- test/browser/browser_asrouter_bookmarkpanel.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/browser/browser_asrouter_bookmarkpanel.js b/test/browser/browser_asrouter_bookmarkpanel.js index db89dcec07..ce3c21c26f 100644 --- a/test/browser/browser_asrouter_bookmarkpanel.js +++ b/test/browser/browser_asrouter_bookmarkpanel.js @@ -26,7 +26,6 @@ add_task(async function test_fxa_message_shown() { const [msg] = PanelTestProvider.getMessages(); const response = BookmarkPanelHub.onResponse( - msg, { container: document.getElementById("editBookmarkPanelRecommendation"), infoButton: document.getElementById("editBookmarkPanelInfoButton"), @@ -36,7 +35,8 @@ add_task(async function test_fxa_message_shown() { url: testURL, document, }, - window + window, + msg ); Assert.ok(response, "We sent a valid message");