Bug 1420945 - Keep the existing items in the Recent Highlights section while opening the Library panel. r=Gijs draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Thu, 30 Nov 2017 21:28:00 +0000
changeset 705865 801c08d3bb1d539270e97f1d25fdd7fa58c48802
parent 703662 cad9c9573579698c223b4b6cb53ca723cd930ad2
child 742508 f0c972100da97f32ebcd099db819892e6cd66cab
push id91634
push userpaolo.mozmail@amadzone.org
push dateThu, 30 Nov 2017 21:28:58 +0000
reviewersGijs
bugs1420945
milestone59.0a1
Bug 1420945 - Keep the existing items in the Recent Highlights section while opening the Library panel. r=Gijs In order to display the Library view sooner, the Recent Highlights section is populated asynchronously after the view is shown. When the Library button is moved to the overflow menu, this prevents the view height animation from including this section. This can be worked around after the first time the view is opened, and is currently done by reusing the last known height. This logic causes issues with other subviews, but to fix this the Library view has to be changed to keep the existing highlight items while fetching the new ones. MozReview-Commit-ID: AGs7nh4CsYE
browser/components/customizableui/content/panelUI.inc.xul
browser/components/customizableui/content/panelUI.js
--- a/browser/components/customizableui/content/panelUI.inc.xul
+++ b/browser/components/customizableui/content/panelUI.inc.xul
@@ -624,20 +624,22 @@
                        label="&libraryDownloads.label;"
                        closemenu="none"
                        oncommand="DownloadsSubview.show(this);"/>
         <toolbarbutton id="appMenu-library-remotetabs-button"
                        class="subviewbutton subviewbutton-iconic subviewbutton-nav"
                        label="&appMenuRemoteTabs.label;"
                        closemenu="none"
                        oncommand="PanelUI.showSubView('PanelUI-remotetabs', this)"/>
-        <toolbarseparator/>
+        <toolbarseparator hidden="true"/>
         <label value="&appMenuRecentHighlights.label;"
+               hidden="true"
                class="subview-subheader"/>
         <toolbaritem id="appMenu-library-recentHighlights"
+                     hidden="true"
                      orient="vertical"
                      smoothscroll="false"
                      flatList="true"
                      tooltip="bhTooltip">
           <!-- Recent Highlights will go here -->
         </toolbaritem>
       </vbox>
     </panelview>
--- a/browser/components/customizableui/content/panelUI.js
+++ b/browser/components/customizableui/content/panelUI.js
@@ -23,16 +23,17 @@ const PanelUI = {
    * getters are set in init, and memoizing happens after the first retrieval.
    */
   get kElements() {
     return {
       mainView: "appMenu-mainView",
       multiView: "appMenu-multiView",
       helpView: "PanelUI-helpView",
       libraryView: "appMenu-libraryView",
+      libraryRecentHighlights: "appMenu-library-recentHighlights",
       menuButton: "PanelUI-menu-button",
       panel: "appMenu-popup",
       notificationPanel: "appMenu-notification-popup",
       addonNotificationContainer: "appMenu-addon-banners",
       overflowFixedList: "widget-overflow-fixed-list",
       overflowPanel: "widget-overflow",
       navbar: "nav-bar",
     };
@@ -300,17 +301,17 @@ const PanelUI = {
       case "MozDOMFullscreen:Entered":
       case "MozDOMFullscreen:Exited":
       case "fullscreen":
       case "activate":
         this._updateNotifications();
         break;
       case "ViewShowing":
         if (aEvent.target == this.libraryView) {
-          this.onLibraryViewShowing(aEvent.target);
+          this.onLibraryViewShowing(aEvent.target).catch(Cu.reportError);
         }
         break;
     }
   },
 
   get isReady() {
     return !!this._isReady;
   },
@@ -502,42 +503,61 @@ const PanelUI = {
   /**
    * When the Library view is showing, we can start fetching and populating the
    * list of Recent Highlights.
    * This is done asynchronously and may finish when the view is already visible.
    *
    * @param {panelview} viewNode The library view.
    */
   async onLibraryViewShowing(viewNode) {
-    if (this._loadingRecentHighlights) {
-      return;
-    }
-    this._loadingRecentHighlights = true;
-
     // Since the library is the first view shown, we don't want to add a blocker
-    // to the event, which would make PanelMultiView wait to show it.
-    let container = this.clearLibraryRecentHighlights();
-    if (!this.libraryRecentHighlightsEnabled) {
-      this._loadingRecentHighlights = false;
+    // to the event, which would make PanelMultiView wait to show it. Instead,
+    // we keep the space currently reserved for the items, but we hide them.
+    if (this._loadingRecentHighlights || !this.libraryRecentHighlightsEnabled) {
       return;
     }
 
+    // Make the elements invisible synchronously, before the view is shown.
+    this.makeLibraryRecentHighlightsInvisible();
+
+    // Perform the rest asynchronously while protecting from re-entrancy.
+    this._loadingRecentHighlights = true;
+    try {
+      await this.fetchAndPopulateLibraryRecentHighlights();
+    } finally {
+      this._loadingRecentHighlights = false;
+    }
+  },
+
+  /**
+   * Fetches the list of Recent Highlights and replaces the items in the Library
+   * view with the results.
+   */
+  async fetchAndPopulateLibraryRecentHighlights() {
     let highlights = await NewTabUtils.activityStreamLinks.getHighlights({
       // As per bug 1402023, hard-coded limit, until Activity Stream develops a
       // richer list.
       numItems: 6,
       withFavicons: true
+    }).catch(ex => {
+      // Just hide the section if we can't retrieve the items from the database.
+      Cu.reportError(ex);
+      return [];
     });
-    // If there's nothing to display, or the panel is already hidden, get out.
-    let multiView = viewNode.panelMultiView;
-    if (!highlights.length || (multiView && multiView.getAttribute("panelopen") != "true")) {
-      this._loadingRecentHighlights = false;
+
+    // Since the call above is asynchronous, the panel may be already hidden
+    // at this point, but we still prepare the items for the next time the
+    // panel is shown, so their space is reserved. The part of this function
+    // that adds the elements is the least expensive anyways.
+    this.clearLibraryRecentHighlights();
+    if (!highlights.length) {
       return;
     }
 
+    let container = this.libraryRecentHighlights;
     container.hidden = container.previousSibling.hidden =
       container.previousSibling.previousSibling.hidden = false;
     let fragment = document.createDocumentFragment();
     for (let highlight of highlights) {
       let button = document.createElement("toolbarbutton");
       button.classList.add("subviewbutton", "highlight", "subviewbutton-iconic", "bookmark-item");
       let title = highlight.title || highlight.url;
       button.setAttribute("label", title);
@@ -546,31 +566,39 @@ const PanelUI = {
       button.setAttribute("onclick", "PanelUI.onLibraryHighlightClick(event)");
       if (highlight.favicon) {
         button.setAttribute("image", highlight.favicon);
       }
       button._highlight = highlight;
       fragment.appendChild(button);
     }
     container.appendChild(fragment);
+  },
 
-    this._loadingRecentHighlights = false;
+  /**
+   * Make all nodes from the 'Recent Highlights' section invisible while we
+   * refresh its contents. This is done while the Library view is opening to
+   * avoid showing potentially stale items, but still keep the space reserved.
+   */
+  makeLibraryRecentHighlightsInvisible() {
+    for (let button of this.libraryRecentHighlights.children) {
+      button.style.visibility = "hidden";
+    }
   },
 
   /**
    * Remove all the nodes from the 'Recent Highlights' section and hide it as well.
    */
   clearLibraryRecentHighlights() {
-    let container = document.getElementById("appMenu-library-recentHighlights");
+    let container = this.libraryRecentHighlights;
     while (container.firstChild) {
       container.firstChild.remove();
     }
     container.hidden = container.previousSibling.hidden =
       container.previousSibling.previousSibling.hidden = true;
-    return container;
   },
 
   /**
    * Event handler; invoked when an item of the Recent Highlights is clicked.
    *
    * @param {MouseEvent} event Click event, originating from the Highlight.
    */
   onLibraryHighlightClick(event) {