From 578adaa9df675609f1c9f481e02546d7662974b9 Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Fri, 29 May 2020 16:40:59 +0000 Subject: [PATCH] Bug 1631415 - Reuse the remote-text custom element for CFR layouts r=k88hudson Differential Revision: https://phabricator.services.mozilla.com/D72380 --- .../components/CustomElements/paragraph.js | 11 +- browser/components/newtab/lib/ASRouter.jsm | 4 +- .../components/newtab/lib/CFRPageActions.jsm | 121 +++++++++--------- browser/components/newtab/lib/RemoteL10n.jsm | 28 ++++ .../components/newtab/lib/ToolbarPanelHub.jsm | 63 +++------ .../test/browser/browser_asrouter_cfr.js | 8 -- 6 files changed, 116 insertions(+), 119 deletions(-) diff --git a/browser/components/newtab/components/CustomElements/paragraph.js b/browser/components/newtab/components/CustomElements/paragraph.js index 483f709ea908..a8e332980139 100644 --- a/browser/components/newtab/components/CustomElements/paragraph.js +++ b/browser/components/newtab/components/CustomElements/paragraph.js @@ -21,9 +21,14 @@ const attributes = {}; for (let name of this.getAttributeNames()) { if (name.startsWith("fluent-variable-")) { - attributes[name.replace(/^fluent-variable-/, "")] = this.getAttribute( - name - ); + let value = this.getAttribute(name); + // Attribute value is a string, in some cases that is not useful + // for example instantiating a Date object will fail. We try to + // convert all possible integers back. + if (value.match(/^\d+/)) { + value = parseInt(value, 10); + } + attributes[name.replace(/^fluent-variable-/, "")] = value; } } diff --git a/browser/components/newtab/lib/ASRouter.jsm b/browser/components/newtab/lib/ASRouter.jsm index bc56e5104a4a..2eef03adba72 100644 --- a/browser/components/newtab/lib/ASRouter.jsm +++ b/browser/components/newtab/lib/ASRouter.jsm @@ -1348,6 +1348,7 @@ class _ASRouter { } break; case "cfr_doorhanger": + case "milestone_message": if (force) { CFRPageActions.forceRecommendation(target, message, this.dispatch); } else { @@ -1382,9 +1383,6 @@ class _ASRouter { case "update_action": MomentsPageHub.executeAction(message); break; - case "milestone_message": - CFRPageActions.showMilestone(target, message, this.dispatch, { force }); - break; default: try { target.sendAsyncMessage(OUTGOING_MESSAGE_NAME, { diff --git a/browser/components/newtab/lib/CFRPageActions.jsm b/browser/components/newtab/lib/CFRPageActions.jsm index 11bf37b68a8d..3990ba559a2c 100644 --- a/browser/components/newtab/lib/CFRPageActions.jsm +++ b/browser/components/newtab/lib/CFRPageActions.jsm @@ -307,6 +307,15 @@ class PageAction { }); } + maybeLoadCustomElement(win) { + if (!win.customElements.get("remote-text")) { + Services.scriptloader.loadSubScript( + "resource://activity-stream/data/custom-elements/paragraph.js", + win + ); + } + } + /** * getStrings - Handles getting the localized strings vs message overrides. * If string_id is not defined it assumes you passed in an override @@ -508,6 +517,8 @@ class PageAction { } async _renderMilestonePopup(message, browser) { + this.maybeLoadCustomElement(this.window); + let { content, id } = message; let { primary } = content.buttons; @@ -532,21 +543,18 @@ class PageAction { reachedMilestone = milestone; } } - if (typeof message.content.heading_text === "string") { - // This is a test environment. - panelTitle = message.content.heading_text; - headerLabel.value = panelTitle; - } else { - RemoteL10n.l10n.setAttributes( - headerLabel, - content.heading_text.string_id, - { + if (headerLabel.firstChild) { + headerLabel.firstChild.remove(); + } + headerLabel.appendChild( + RemoteL10n.createElement(this.window.document, "span", { + content: message.content.heading_text, + attributes: { blockedCount: reachedMilestone, date: monthName, - } - ); - await RemoteL10n.l10n.translateElements([headerLabel]); - } + }, + }) + ); // Use the message layout as a CSS selector to hide different parts of the // notification template markup @@ -631,6 +639,8 @@ class PageAction { // eslint-disable-next-line max-statements async _renderPopup(message, browser) { + this.maybeLoadCustomElement(this.window); + const { id, content, modelVersion } = message; const headerLabel = this.window.document.getElementById( @@ -683,7 +693,14 @@ class PageAction { const author = this.window.document.getElementById( "cfr-notification-author" ); - author.textContent = await this.getStrings(content.text); + if (author.firstChild) { + author.firstChild.remove(); + } + author.appendChild( + RemoteL10n.createElement(this.window.document, "span", { + content: content.text, + }) + ); primaryActionCallback = () => { this._blockMessage(id); this.dispatchUserAction(primary.action); @@ -748,10 +765,13 @@ class PageAction { for (let step of content.descriptionDetails.steps) { // This li is a generic xul element with custom styling const li = this.window.document.createXULElement("li"); - RemoteL10n.l10n.setAttributes(li, step.string_id); + li.appendChild( + RemoteL10n.createElement(this.window.document, "span", { + content: step, + }) + ); stepsContainer.appendChild(li); } - await RemoteL10n.l10n.translateElements([...stepsContainer.children]); } await this._renderPinTabAnimation(); @@ -759,8 +779,15 @@ class PageAction { default: panelTitle = await this.getStrings(content.addon.title); await this._setAddonAuthorAndRating(this.window.document, content); + if (footerText.firstChild) { + footerText.firstChild.remove(); + } // Main body content of the dropdown - footerText.textContent = await this.getStrings(content.text); + footerText.appendChild( + RemoteL10n.createElement(this.window.document, "span", { + content: content.text, + }) + ); options = { popupIconURL: content.addon.icon }; footerLink.value = await this.getStrings({ @@ -1022,46 +1049,6 @@ const CFRPageActions = { return url; }, - /** - * Show Milestone notification. - * @param browser The browser for the recommendation - * @param recommendation The recommendation to show - * @param dispatchToASRouter A function to dispatch resulting actions to - * @return Did adding the recommendation succeed? - */ - async showMilestone(browser, message, dispatchToASRouter, options = {}) { - let win = null; - const { id, content, personalizedModelVersion } = message; - - // If we are forcing via the Admin page, the browser comes in a different format - if (options.force) { - win = browser.browser.ownerGlobal; - RecommendationMap.set(browser.browser, { - id, - content, - retain: true, - modelVersion: personalizedModelVersion, - }); - } else { - win = browser.ownerGlobal; - RecommendationMap.set(browser, { - id, - content, - retain: true, - modelVersion: personalizedModelVersion, - }); - } - - if (!PageActionMap.has(win)) { - PageActionMap.set(win, new PageAction(win, dispatchToASRouter)); - } - - await PageActionMap.get(win).showMilestonePopup(); - PageActionMap.get(win).addImpression(message); - - return true; - }, - /** * Force a recommendation to be shown. Should only happen via the Admin page. * @param browser The browser for the recommendation @@ -1084,8 +1071,13 @@ const CFRPageActions = { } if (content.skip_address_bar_notifier) { - await PageActionMap.get(win).showPopup(); - PageActionMap.get(win).addImpression(recommendation); + if (recommendation.template === "milestone_message") { + await PageActionMap.get(win).showMilestonePopup(); + PageActionMap.get(win).addImpression(recommendation); + } else { + await PageActionMap.get(win).showPopup(); + PageActionMap.get(win).addImpression(recommendation); + } } else { await PageActionMap.get(win).showAddressBarNotifier(recommendation, true); } @@ -1129,9 +1121,16 @@ const CFRPageActions = { } if (content.skip_address_bar_notifier) { - await PageActionMap.get(win).showPopup(); - PageActionMap.get(win).addImpression(recommendation); + if (recommendation.template === "milestone_message") { + await PageActionMap.get(win).showMilestonePopup(); + PageActionMap.get(win).addImpression(recommendation); + } else { + // Tracking protection messages + await PageActionMap.get(win).showPopup(); + PageActionMap.get(win).addImpression(recommendation); + } } else { + // Doorhanger messages await PageActionMap.get(win).showAddressBarNotifier(recommendation, true); } return true; diff --git a/browser/components/newtab/lib/RemoteL10n.jsm b/browser/components/newtab/lib/RemoteL10n.jsm index 72dcf915d81f..e0324ad044ad 100644 --- a/browser/components/newtab/lib/RemoteL10n.jsm +++ b/browser/components/newtab/lib/RemoteL10n.jsm @@ -27,6 +27,34 @@ class _RemoteL10n { this._l10n = null; } + createElement(doc, elem, options = {}) { + let node; + if (options.content && options.content.string_id) { + node = doc.createElement("remote-text"); + } else { + node = doc.createElementNS("http://www.w3.org/1999/xhtml", elem); + } + if (options.classList) { + node.classList.add(options.classList); + } + this.setString(node, options); + + return node; + } + + // If `string_id` is present it means we are relying on fluent for translations. + // Otherwise, we have a vanilla string. + setString(el, { content, attributes = {} }) { + if (content && content.string_id) { + for (let [fluentId, value] of Object.entries(attributes)) { + el.setAttribute(`fluent-variable-${fluentId}`, value); + } + el.setAttribute("fluent-remote-id", content.string_id); + } else { + el.textContent = content; + } + } + /** * Creates a new DOMLocalization instance with the Fluent file from Remote Settings. * diff --git a/browser/components/newtab/lib/ToolbarPanelHub.jsm b/browser/components/newtab/lib/ToolbarPanelHub.jsm index d000b323841b..03f44819aa25 100644 --- a/browser/components/newtab/lib/ToolbarPanelHub.jsm +++ b/browser/components/newtab/lib/ToolbarPanelHub.jsm @@ -13,6 +13,7 @@ XPCOMUtils.defineLazyModuleGetters(this, { Preferences: "resource://gre/modules/Preferences.jsm", SpecialMessageActions: "resource://messaging-system/lib/SpecialMessageActions.jsm", + RemoteL10n: "resource://activity-stream/lib/RemoteL10n.jsm", }); XPCOMUtils.defineLazyServiceGetter( this, @@ -267,13 +268,13 @@ class _ToolbarPanelHub { _createMessageElements(win, doc, message, previousDate) { const { content } = message; - const messageEl = this._createElement(doc, "div"); + const messageEl = RemoteL10n.createElement(doc, "div"); messageEl.classList.add("whatsNew-message"); // Only render date if it is different from the one rendered before. if (content.published_date !== previousDate) { messageEl.appendChild( - this._createElement(doc, "p", { + RemoteL10n.createElement(doc, "p", { classList: "whatsNew-message-date", content: new Date(content.published_date).toLocaleDateString( "default", @@ -287,14 +288,14 @@ class _ToolbarPanelHub { ); } - const wrapperEl = this._createElement(doc, "button"); + const wrapperEl = RemoteL10n.createElement(doc, "button"); wrapperEl.doCommand = () => this._dispatchUserAction(win, message); wrapperEl.classList.add("whatsNew-message-body"); messageEl.appendChild(wrapperEl); if (content.icon_url) { wrapperEl.classList.add("has-icon"); - const iconEl = this._createElement(doc, "img"); + const iconEl = RemoteL10n.createElement(doc, "img"); iconEl.src = content.icon_url; iconEl.classList.add("whatsNew-message-icon"); if (content.icon_alt && content.icon_alt.string_id) { @@ -308,7 +309,7 @@ class _ToolbarPanelHub { wrapperEl.appendChild(this._createMessageContent(win, doc, content)); if (content.link_text) { - const anchorEl = this._createElement(doc, "a", { + const anchorEl = RemoteL10n.createElement(doc, "a", { classList: "text-link", content: content.link_text, }); @@ -329,35 +330,39 @@ class _ToolbarPanelHub { const wrapperEl = new win.DocumentFragment(); wrapperEl.appendChild( - this._createElement(doc, "h2", { + RemoteL10n.createElement(doc, "h2", { classList: "whatsNew-message-title", content: content.title, + attributes: this.state.contentArguments, }) ); switch (content.layout) { case "tracking-protections": wrapperEl.appendChild( - this._createElement(doc, "h4", { + RemoteL10n.createElement(doc, "h4", { classList: "whatsNew-message-subtitle", content: content.subtitle, + attributes: this.state.contentArguments, }) ); wrapperEl.appendChild( - this._createElement(doc, "h2", { + RemoteL10n.createElement(doc, "h2", { classList: "whatsNew-message-title-large", content: this.state.contentArguments[ content.layout_title_content_variable ], + attributes: this.state.contentArguments, }) ); break; } wrapperEl.appendChild( - this._createElement(doc, "p", { + RemoteL10n.createElement(doc, "p", { content: content.body, classList: "whatsNew-message-content", + attributes: this.state.contentArguments, }) ); @@ -367,28 +372,28 @@ class _ToolbarPanelHub { _createHeroElement(win, doc, message) { this.maybeLoadCustomElement(win); - const messageEl = this._createElement(doc, "div"); + const messageEl = RemoteL10n.createElement(doc, "div"); messageEl.setAttribute("id", "protections-popup-message"); messageEl.classList.add("whatsNew-hero-message"); - const wrapperEl = this._createElement(doc, "div"); + const wrapperEl = RemoteL10n.createElement(doc, "div"); wrapperEl.classList.add("whatsNew-message-body"); messageEl.appendChild(wrapperEl); wrapperEl.appendChild( - this._createElement(doc, "h2", { + RemoteL10n.createElement(doc, "h2", { classList: "whatsNew-message-title", content: message.content.title, }) ); wrapperEl.appendChild( - this._createElement(doc, "p", { + RemoteL10n.createElement(doc, "p", { classList: "protections-popup-content", content: message.content.body, }) ); if (message.content.link_text) { - let linkEl = this._createElement(doc, "a", { + let linkEl = RemoteL10n.createElement(doc, "a", { classList: "text-link", content: message.content.link_text, }); @@ -402,21 +407,6 @@ class _ToolbarPanelHub { return messageEl; } - _createElement(doc, elem, options = {}) { - let node; - if (options.content && options.content.string_id) { - node = doc.createElement("remote-text"); - } else { - node = doc.createElementNS("http://www.w3.org/1999/xhtml", elem); - } - if (options.classList) { - node.classList.add(options.classList); - } - this._setString(node, options.content); - - return node; - } - async _contentArguments() { const { defaultEngine } = Services.search; // Between now and 6 weeks ago @@ -458,21 +448,6 @@ class _ToolbarPanelHub { }; } - // If `string_id` is present it means we are relying on fluent for translations. - // Otherwise, we have a vanilla string. - _setString(el, stringObj) { - if (stringObj && stringObj.string_id) { - for (let [fluentId, value] of Object.entries( - this.state.contentArguments || {} - )) { - el.setAttribute(`fluent-variable-${fluentId}`, value); - } - el.setAttribute("fluent-remote-id", stringObj.string_id); - } else { - el.textContent = stringObj; - } - } - async _showAppmenuButton(win) { this.maybeInsertFTL(win); await this._showElement( diff --git a/browser/components/newtab/test/browser/browser_asrouter_cfr.js b/browser/components/newtab/test/browser/browser_asrouter_cfr.js index 4ed6f6020e6a..98f1a62e301d 100644 --- a/browser/components/newtab/test/browser/browser_asrouter_cfr.js +++ b/browser/components/newtab/test/browser/browser_asrouter_cfr.js @@ -195,14 +195,6 @@ function trigger_cfr_panel( } clearNotifications(); - if (recommendation.template === "milestone_message") { - return CFRPageActions.showMilestone( - browser, - recommendation, - // Use the real AS dispatch method to trigger real notifications - ASRouter.dispatch - ); - } return CFRPageActions.addRecommendation( browser, trigger,