1168589 - Convert newtab-customize-panel into an HTML element

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ursula Sarracini (:ursula)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150522030225

Steps to reproduce:

Similar to bug 1167643, we need to move the newtab-customize-panel into mainPopupSet in browser.xul in order to fully migrate newTab.xul to newTab.xhtml

This will also need the messaging infrastructure so that newTab.xul can tell the parent to open the popup, and so we can message the child when the popup is closed.

Comment 1

[Ursula Sarracini (:ursula)]

Attachment: Move newtab-customize-panel into the browser chrome

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8616097 [details] [diff] [review]
Move newtab-customize-panel into the browser chrome

Review of attachment 8616097 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/newtab/customize.js
@@ +15,3 @@
>  
> +    this.button = document.getElementById("newtab-customize-button");
> +    window.addMessageListener("NewTab:CloseCustomizePanel", (message) => {

It'd be nice if we could get all of these functions kinda isolated from one another.

Unfortunately, RemotePages doesn't allow us to pass an object in for addMessageListener like the standard message managers. I've filed bug 1172975 about that.

In the meantime, we can probably clean this up a bit like this:

addMessageListener("NewTab:Foo", this.onFoo.bind(this));

(Note that you can omit "window" - it's implicit).

@@ +21,5 @@
> +
> +    this.button.addEventListener("click", e => this.showPanel());
> +
> +    window.addMessageListener("NewTab:ShowBlank", (message) => {
> +      let blank = message.data.blank;

blank isn't being used.

@@ +28,4 @@
>      });
> +
> +    window.addMessageListener("NewTab:ShowClassic", (message) => {
> +      let classic = message.data.classic;

What are "classic" and "enhanced" used for?

@@ +56,3 @@
>        window.open(TILES_INTRO_LINK,'new_window');
>        this._onHidden();
> +      this.button.removeAttribute("active");

Are you sure this is necessary? The attribute should be removed when the panel closes in the close message listener, no?

::: browser/components/nsBrowserGlue.js
@@ +1022,5 @@
>    /**
>     * Application shutdown handler.
>     */
>    _onQuitApplicationGranted: function () {
> +    AboutNewTab.uninit();

We should probably put this uninit somewhere lower down where the other uninits are.

@@ +1897,5 @@
>        }
>      }
>  
>      if (currentUIVersion < 19) {
> +      let detector = null;

For this and the other whitespace fixes in here, I'd actually prefer if you revert the changes. They're kinda unrelated, and might muddy up the blame on these lines for people diving into the history of a set of changes.

::: browser/modules/AboutNewTab.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + "use strict";

Nit - busted indentation lines 5-18.

@@ +36,5 @@
> +    this.pageListener.addMessageListener("NewTab:OpenCustomizePanel", this.openCustomizePanel.bind(this));
> +    this.pageListener.addMessageListener("NewTab:UpdateSelected", this.updateSelected.bind(this));
> +  },
> +
> +  openCustomizePanel: function(message){

Nit - space before {

@@ +49,5 @@
> +
> +    let boxWidth = box.screenX;
> +    let boxHeight = box.screenY;
> +    panel.hidden = false;
> +    panel.openPopupAtScreenRect("panel", x + boxWidth + width * 0.5, y + boxHeight + height);

"panel" is no good here. I *think* you can just pass the empty string to get the default.

@@ +62,5 @@
> +      this._nodes[idSuffix] = doc.getElementById("newtab-customize-" + idSuffix);
> +    }
> +
> +    panel.addEventListener("click", function(aEvent) {
> +      switch (aEvent.target.parentNode.id) {

We also never remove this click event listener, which is a problem if the panel is opened across different about:newtab instances in the same window. I believe clicks in the panel will then get dispatched to each of those newtab's, which will end up duplicating a bunch of messages / work... probably could introduce some subtle bugs there too.

What I suggest is the following:

1) Have openCustomizePanel stash the message.target into some property on AboutNewTab each time it's called. We'll need it later in the event listeners.
2) Add event listeners

panel.addEventListener("popuphidden", this);

So I think I'm going to backpedal on my original suggestion about having a single click event handler (aka event delegation). We gain some efficiency yes, but at the cost of more handling to ensure that we can properly identify what the user clicked on. Event delegation is a good idea for a list with many items - but this is a static list of few items. Also, aEvent.target.parentNode.id sounds pretty fragile (for example, if the user clicks on the padding of the newtab-customize-blank hbox, the parentNode.id is newtab-customize-panel).

So I'd add a click event handler for each of those hbox's, like:

this._nodes.blank.addEventListener("click", this);
this._nodes.classic.addEventListener("click", this);
this._nodes.enhanced.addEventListener("click", this);

Then, for all of those events above, we centralize receiving them in a handleEvent handler:

handleEvent: function (event) {
  switch(event.type) {
    case "popuphidden":
      this.onPopupHidden();
      break;
    case "click":
      this.onClick(event);
      break;
  }
},

onPopupHidden: function() {
  this.target.sendAsyncMessage("NewTab:CloseCustomizePanel");
  this.panel.hidden = true;
  // Remove event listeners here, or call a function that will do it.

  // Drop the reference to the target - we only care about it when the
  // panel is open so that we know which browser to message to.
  this.target = null;
},

onClick: function(event) {
  // currentTarget will be the name of the thing we added
  // the event handler too - regardless of what child was
  // clicked
  switch(event.currentTarget.id) {
    case "newtab-customize-blank":
      // Do blank stuff
      break;
    case "newtab-customize-enhanced":
      // etc
      break;
    // etc
  }
},

This breaking up of functions this way is, I suppose, a preference of mine - but I think it greatly eases reviewing, since it helps make each named function do one specific thing, as opposed to having a bunch of anonymous functions getting registered all over the place.

@@ +65,5 @@
> +    panel.addEventListener("click", function(aEvent) {
> +      switch (aEvent.target.parentNode.id) {
> +        case "newtab-customize-blank":
> +          let blank = doc.getElementById("newtab-customize-blank");
> +          message.target.sendAsyncMessage("NewTab:ShowBlank", {blank: blank});

This attempts to send a DOM node with ID = "newtab-customize-blank" through the message manager, which isn't going to work.

What we might want to do instead is to reduce the number of message types here. So if the user chooses "blank", we send:

"NewTab:Customize", {
  mode: "blank",
}

For the enhanced case:

let enhanced = doc.getElementById("newtab-customize-enhanced").getAttribute("selected");

"NewTab:Customize, {
  mode: "tiles",
  enhanced: enhanced,
}

@@ +85,5 @@
> +       }
> +    });
> +  },
> +
> +  updateSelected: function(message) {

We're handling this based on a message back up from content, but don't we already have enough information to deal with this in the parent? Like, we send down a message to put the about:newtab in a particular state (about:blank or something) - and the content shouldn't be able to modify what we chose - so don't we already have enough information to update the panel nodes without waiting for this message?

Comment 3

[Ursula Sarracini (:ursula)]

Attachment: Move newtab-customize-panel into the browser chrome

Comment 4

[Mike Conley (:mconley) (:⚙️)]

There have been developments.

Attachment 8620464 [details] [diff] does an admirable job of moving the customize panel into the parent process. However, it looks like the tour that is displayed on first spawn of about:newtab does some trickery to clone the panel and display it as part of the tour. This is, according to the content services team, a way of avoiding creating image assets that need to be localized, etc. Which makes sense.

The problem is that it's pretty hacky to get a panel in the parent process, to be drawn in the content area. Like, we evaluated a number of ways (including dumping a clone of the panel contents into an svg foreignObject and sending down a dataURI for the image to content, converting the contents of the panel to HTML and serializing it down to the child just for the tour), but they all seem pretty hacky.

The simpler solution seems to be to just have the panel render in content. Talking to adw, it doesn't sound like there's a really good reason why we used a XUL panel in the first place - or if there was a good reason, the reason isn't around anymore.

So Ursula will be converting the panel to HTML instead of moving it into the parent process. The communications infrastructure of her patch can still be used, so it's not a total restart.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8620464 [details] [diff] [review]
Move newtab-customize-panel into the browser chrome

Review of attachment 8620464 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling review, as this is being re-jigged to be opened in content now.

::: browser/base/content/browser-doctype.inc
@@ +21,5 @@
>  #endif
>  <!ENTITY % aboutHomeDTD SYSTEM "chrome://browser/locale/aboutHome.dtd">
>  %aboutHomeDTD;
> +<!ENTITY % newTabDTD SYSTEM "chrome://browser/locale/newTab.dtd">
> +  %newTabDTD;

Nit: Might as well be consistent with the indentation here, and move this back.

::: browser/modules/AboutNewTab.jsm
@@ +11,5 @@
> +this.EXPORTED_SYMBOLS = [ "AboutNewTab" ];
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.importGlobalProperties(['Blob', 'URL']);

These don't appear to be used here anymore.

@@ +16,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "RemotePages",
> +  "resource://gre/modules/RemotePageManager.jsm");
> +
> +const XUL_NAMESPACE = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

This is not used anymore.

Comment 6

[Ursula Sarracini (:ursula)]

Attachment: Move newtab-customize-panel into the browser chrome

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8621733 [details] [diff] [review]
Move newtab-customize-panel into the browser chrome

Hey Ursula, I'll still start reviewing this, but it looks like this won't apply on central due to this commit bitrotting you: https://hg.mozilla.org/mozilla-central/rev/e7c0ddb5f948

Can you please rebase the patch?

Comment 8

[Ursula Sarracini (:ursula)]

Attachment: Convert newtab-customize-panel into an HTML element

Comment 9

[Ursula Sarracini (:ursula)]

Attachment: Convert newtab-customize-panel into an HTML element

Comment 10

[Ursula Sarracini (:ursula)]

Attachment: Convert newtab-customize-panel into an HTML element

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8622468 [details] [diff] [review]
Convert newtab-customize-panel into an HTML element

Review of attachment 8622468 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Ursula! I think this is getting really close. Moving the panel into content has kind of changed our requirements here - and allows us to use slightly different techniques to achieve our desired result. See below.

::: browser/base/content/newtab/customize.js
@@ +8,5 @@
> +
> +  button: null,
> +  overlay: null,
> +  panel: null,
> +  enabled: null,

So we're caching the values for enabled and enhanced here...

The more caches we have, the more things can go wrong if caches go out of date or don't get invalidated properly. I suspect you might just want to use gAllPages.enabled and gAllPages.enhanced in place of this.enabled and this.enhanced.

Then, when the user clicks on one of the items down here, we send a message up to the parent. The parent updates the preference via Services.prefs.

At that point, Gecko will automatically inform the child process that the pref has changed - and we can use our nsPref:changed observer in the child to update the state.

@@ +17,1 @@
>      "classic",

I'm not wild about the _nodeIDSuffixes pattern that seems to be used a lot in these newTab scripts, but we should probably go with the flow here. Better to be consistent, than half-and-half.

So can you put button, panel and overlay in here?

@@ +39,3 @@
>    },
>  
>    _onHidden: function() {

I think _onHidden is actually an inaccurate name for this method. This method is more about doing the actual hiding, as opposed to reacting to something that has hidden.

Perhaps let's rename this to _hidePanel instead.

@@ +45,4 @@
>      });
> +    this.overlay.style.opacity = 0;
> +    this.button.removeAttribute("active");
> +    this.panel.setAttribute("hidden", true);

I don't think "hidden" will work. I think this

@@ +49,4 @@
>    },
>  
>    showPanel: function() {
> +    for (let idSuffix of this._nodeIDSuffixes) {

I don't think there's any value in setting up these event listeners each time the panel opens now, considering that we're using a unique panel per about:newtab. Same with removing the events - we probably don't need to do that now on panel close.

@@ +54,5 @@
>      }
>  
> +    this.overlay.style.display = "block";
> +    this.button.setAttribute("active", "true");
> +    this.panel.removeAttribute("hidden");

Instead of removing the hidden attribute, let's add an open or showing attribute set to true.

@@ +59,2 @@
>  
> +    setTimeout(() => {

I really don't get this setTimeout here... what's going on with it, do you know?

These event listeners can probably be set up once in the init function, and check to see whether or not the panel is showing before calling _onHidden/_hidePanel.

@@ +84,5 @@
> +    switch (event.currentTarget.id) {
> +      case "newtab-customize-blank":
> +        this.enhanced = false;
> +        this.enabled = false;
> +        sendAsyncMessage("NewTab:ShowBlank");

Instead of sending up 3 distinct messages, I think I'd prefer we send up a single message but with different arguments. Examples:

// blank

sendAsyncMessage("NewTab:Customize", {
  enabled: false,
  enhanced: false,
});

// classic

sendAsyncMessage("NewTab:Customize", {
  enabled: true,
  enhanced: false,
});

// enhanced

sendAsyncMessage("NewTab:Customize", {
  enabled: true,
  enhanced: true,
});

@@ +99,5 @@
> +        this.enhanced = !this.enhanced;
> +        sendAsyncMessage("NewTab:ShowEnhanced");
> +        break;
> +      case "newtab-customize-learn":
> +        this.panel.hidden = true;

Setting this.panel.hidden to true will not work anymore now that the panel is not XUL.

(https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/hidden is XUL-only.)

@@ +103,5 @@
> +        this.panel.hidden = true;
> +        this.showLearn();
> +        break;
> +    }
> +    this.updateSelected(this.enabled, this.enhanced);

This updateSelected will probably not be necessary anymore, since the message will go to the parent, which will cause the pref flip, which should cause nsPref:changed to fire, which should call this.updateSelected, which should then read the updated pref values.

@@ +117,5 @@
> +    window.open(TILES_INTRO_LINK,'new_window');
> +    this._onHidden();
> +  },
> +
> +  updateSelected: function(enabled, enhanced) {

We probably want to revert the changes to updateSelected - I think we want it to continue to read the prefs from gAllPages.

::: browser/base/content/newtab/newTab.css
@@ +472,5 @@
> +  position: absolute;
> +  margin-right: 40px;
> +}
> +
> +.newtab-customize-panel-container:not > #newtab-customize-panel {

This rule is busted with the :not

::: browser/base/content/newtab/newTab.xul
@@ +29,5 @@
>      </xul:hbox>
>    </xul:panel>
>  
> +  <div class="newtab-customize-panel-container">
> +    <div id="newtab-customize-panel" orient="vertical" type="arrow" hidden="true">

type="arrow" will not work with non-XUL elements like a DIV.

As for hidden, that's usually also used for XUL elements as well. For holding on to the open/close state of the panel, how about when the panel gets opened, we add a open="true" or showing="true" attribute?

Then you can use the attribute selector in newTab.css to display:none the panel when not open/showing="true"

@@ +132,2 @@
>      <input id="newtab-customize-button" type="button" title="&newtab.customize.title;"/>
> +</div>

Busted indentation.

::: browser/base/content/newtab/page.js
@@ -45,5 @@
>     * Listens for notifications specific to this page.
>     */
>    observe: function Page_observe(aSubject, aTopic, aData) {
>      if (aTopic == "nsPref:changed") {
> -      gCustomize.updateSelected();

I think we'll want to keep this so that we can react to prefs changing.

::: browser/components/nsBrowserGlue.js
@@ +22,1 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "UITour",

To be consistent, let's put a newline between these.

::: browser/modules/AboutNewTab.jsm
@@ +30,5 @@
> +    this.pageListener.addMessageListener("NewTab:ShowBlank", this.showBlank.bind(this));
> +    this.pageListener.addMessageListener("NewTab:ShowClassic", this.showClassic.bind(this));
> +    this.pageListener.addMessageListener("NewTab:ShowEnhanced", this.showEnhanced.bind(this));
> +
> +    this.enabled = Services.prefs.getBoolPref(PREF_NEWTAB_ENABLED);

We should probably talk directly to NewTabUtils.AllPages.enabled and NewTabUtils.AllPages.enhanced instead of manipulating / caching the pref values ourselves in here.

Comment 12

[Ursula Sarracini (:ursula)]

Attachment: Convert newtab-customize-panel into an HTML element

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8622636 [details] [diff] [review]
Convert newtab-customize-panel into an HTML element

Review of attachment 8622636 [details] [diff] [review]:
-----------------------------------------------------------------

Looking really good!

I noticed two things while testing:

1) The panel animation is missing.
2) Clicking the button does not cause the panel to close once it has been opened.

Also, a few small things I missed during my last review - but nothing major.

::: browser/base/content/newtab/customize.js
@@ +10,5 @@
>      "blank",
>      "button",
>      "classic",
>      "enhanced",
> +    "learn",

Might as well just put these back where they were so as to not add unnecessary blame changes to the file.

@@ +57,5 @@
>      }, 0);
>  
> +    document.addEventListener("click", this);
> +    document.addEventListener("keydown", this);
> +    event.stopPropagation();

Let's add a comment here to mention that we're stopping the event so that we don't immediately close the panel again via the click event we just added to the document.

@@ +61,5 @@
> +    event.stopPropagation();
> +  },
> +
> +  handleEvent: function(event) {
> +    switch (event.type) {

Last suggestion in here.

What's nice about the handleEvent pattern is that (like I said IRL) adding and removing the event listener is super easy, and has the right "this" binding, without requiring you to .bind(this) your event listener functions.

The drawback is that you end up with this massive handleEvent handling multiple event types.

Looking at this now, here's my final suggestion:

Have handleEvent dispatch events to two different functions, onClick, and onKeyDown, like this:

handleEvent: function(event) {
  switch(event.type) {
    case "click":
      this.onClick(event);
      break;
    case "keydown":
      this.onKeyDown(event);
      break;
  }
},

onClick: function(event) {
  // If the target is the document, then the panel must be
  // open, and the user is clicking outside of the panel (since
  // the panel should prevent any clicks from bubbling up from it),
  // so let's close it.
  if (event.currentTarget == document) {
    this.hidePanel();
    return;
  }
  
  // Otherwise, we must have clicked on one of the panel entries.
  switch(event.currentTarget.id) {
    // What you've currently got
  }
},

onKeyDown: function(event) {
  if (event.key == event.DOM_VK_ESCAPE) {
    this.hidePanel();
  }
}

@@ +70,5 @@
> +          }
> +        }
> +        break;
> +      case "keydown":
> +        if (event.key == "Escape") {

Nit - I think we normally compare these using the consts on the event, like:

if (event.keyCode == aEvent.DOM_VK_ESCAPE) {
  // foo
}

@@ +78,5 @@
>      }
>  
> +    switch (event.currentTarget.id) {
> +      case "newtab-customize-blank":
> +        gAllPages.enabled = false;

You can't set these when we run in the content process - writing to prefs will throw. You should be good to just send the enabled / enhanced values up to the parent.

@@ +102,3 @@
>  
> +  showLearn: function() {
> +    window.open(TILES_INTRO_LINK,'new_window');

Nit - space after the comma. I know it's not your code, but you're touching it.

::: browser/modules/AboutNewTab.jsm
@@ +24,5 @@
> +    this.pageListener = new RemotePages("about:newtab");
> +    this.pageListener.addMessageListener("NewTab:Customize", this.showCustomize.bind(this));
> +  },
> +
> +  showCustomize: function(message) {

showCustomize is probably the wrong name now - how about just "customize"?

@@ +25,5 @@
> +    this.pageListener.addMessageListener("NewTab:Customize", this.showCustomize.bind(this));
> +  },
> +
> +  showCustomize: function(message) {
> +    NewTabUtils.AllPages.enabled = message.data.enabled;

That's nice and clean!

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Also, it looks like this patch is based on a change that has already moved the search panel out.

I guess that hasn't happened yet on mozilla-central... was nhnt11 going to do that soonish? If not, you might need to re-base your patch on central such that the search panel is still there.

Comment 15

[Ursula Sarracini (:ursula)]

Attachment: Convert newtab-customize-panel into an HTML element

Comment 16

[Ursula Sarracini (:ursula)]

I'll ask nhnt11 for an update on his patch - newTab.xul still has the xul search panel in it in this patch though

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ursula Sarracini (:ursula) from comment #16)
> I'll ask nhnt11 for an update on his patch - newTab.xul still has the xul
> search panel in it in this patch though

Sounds good. If he's swamped, maybe you can write the patch to tear out the search panel. I figure you're pretty familiar with it at this point. :)

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8623048 [details] [diff] [review]
Convert newtab-customize-panel into an HTML element

Review of attachment 8623048 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Giving it a test run now...

::: browser/base/content/newtab/customize.js
@@ +47,5 @@
> +    if (this._nodes.panel.getAttribute("open") == "true") {
> +      return;
> +    }
> +    let nodes = this._nodes;
> +    let {panel, button, overlay} = nodes;

Might as well just do:

let {panel, button, overlay} = this._nodes;

@@ +93,5 @@
> +          sendAsyncMessage("NewTab:Customize", {enabled: true, enhanced: false});
> +        }
> +        break;
> +      case "newtab-customize-enhanced":
> +        if (!gAllPages.enabled) {

I think this case can be simplified:

sendAsyncMessage("NewTab:Customize", {enabled: true, enhanced: !gAllPages.enhanced});

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8623048 [details] [diff] [review]
Convert newtab-customize-panel into an HTML element

This looks fantastic, but two issues:

1) In RTL, the panel still anchors to the right side of the screen.
2) We're failing some tests under browser/base/content/test/newtab

You can probably fix (1) in a follow-up, but we need to get (2) fixed before you can land.

Comment 20

[Marina Samuel [:emtwo]]

Comment on attachment 8623048 [details] [diff] [review]
Convert newtab-customize-panel into an HTML element

Review of attachment 8623048 [details] [diff] [review]:
-----------------------------------------------------------------

This patch also didn't apply cleanly for me. You may need to pull the latest m-c and reapply the patch.

Also I noticed that when I open the panel from the newtab page and set very long lengths for the menu strings, it only wraps the title at 300px.

Note: Bug 1165594 resolved some RTL issues so when you rebase you may no longer have to deal with those :)

::: browser/base/content/newtab/newTab.css
@@ +503,3 @@
>  }
>  
>  #newtab-customize-title {

When I tested this patch locally, .newtab-intro-image-customize #newtab-customize-title had a max-height of 72px which originally was supposed to allow for only 2 lines of text, but now it looks like it can have more than 2 lines.

We'll prob need to change the 72px to something smaller. 40px looked good when I tried it.

Comment 21

[Ursula Sarracini (:ursula)]

Attachment: Convert newtab-customize-panel into an HTML element

Comment 22

[Ursula Sarracini (:ursula)]

Attachment: Convert newtab-customize-panel into an HTML element

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8623293 [details] [diff] [review]
Convert newtab-customize-panel into an HTML element

Review of attachment 8623293 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Just one nit that you can fix, and I'll just r=me the next patch. Thanks Ursula!

::: browser/base/content/browser-doctype.inc
@@ +18,5 @@
>  %safebrowsingDTD;
>  #endif
>  <!ENTITY % aboutHomeDTD SYSTEM "chrome://browser/locale/aboutHome.dtd">
>  %aboutHomeDTD;
> +<!ENTITY % newTabDTD SYSTEM "chrome://browser/locale/newTab.dtd">

I guess we don't need this anymore?

Comment 24

[Marina Samuel [:emtwo]]

Comment on attachment 8623293 [details] [diff] [review]
Convert newtab-customize-panel into an HTML element

Review of attachment 8623293 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with the one change we talked about for max-width.

Comment 25

[Ursula Sarracini (:ursula)]

Attachment: Convert newtab-customize-panel into an HTML element

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8623827 [details] [diff] [review]
Convert newtab-customize-panel into an HTML element

Review of attachment 8623827 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e7527ce85850

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7527ce85850