Add local navigation to Differential

Summary:
Adds a flexible navigation menu to diffs that shows you your current position in the diff.

Anticipating some "this is the best thing ever" and some "this is the wosrt thing ever" on this, but let's see how much pushback we get? It seems pretty good to me.

Test Plan: Will attach screenshots.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Maniphest Tasks: T1633, T1591

Differential Revision: https://secure.phabricator.com/D3355

scripts/celerity_mapper.php

@@ -121,7 +121,6 @@ $package_spec = array(
 
     'differential-inline-comment-editor',
     'javelin-behavior-differential-dropdown-menus',
-    'javelin-behavior-buoyant',
   ),
   'diffusion.pkg.css' => array(
     'diffusion-commit-view-css',

src/__phutil_library_map__.php

@@ -547,6 +547,7 @@ phutil_register_library_map(array(
     'PhabricatorAccessLog' => 'infrastructure/PhabricatorAccessLog.php',
     'PhabricatorActionListView' => 'view/layout/PhabricatorActionListView.php',
     'PhabricatorActionView' => 'view/layout/PhabricatorActionView.php',
+    'PhabricatorAnchorView' => 'view/layout/PhabricatorAnchorView.php',
     'PhabricatorApplication' => 'applications/base/PhabricatorApplication.php',
     'PhabricatorApplicationApplications' => 'applications/meta/application/PhabricatorApplicationApplications.php',
     'PhabricatorApplicationAudit' => 'applications/audit/application/PhabricatorApplicationAudit.php',
@@ -1683,6 +1684,7 @@ phutil_register_library_map(array(
     'Phabricator404Controller' => 'PhabricatorController',
     'PhabricatorActionListView' => 'AphrontView',
     'PhabricatorActionView' => 'AphrontView',
+    'PhabricatorAnchorView' => 'AphrontView',
     'PhabricatorApplicationApplications' => 'PhabricatorApplication',
     'PhabricatorApplicationAudit' => 'PhabricatorApplication',
     'PhabricatorApplicationAuth' => 'PhabricatorApplication',

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -194,7 +194,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
             array(
               'href' => $request_uri
                 ->alter('large', 'true')
-                ->setFragment('differential-review-toc'),
+                ->setFragment('toc'),
             ),
             'Show All Files Inline').
         "</strong>");
@@ -395,12 +395,22 @@ final class DifferentialRevisionViewController extends DifferentialController {
     PhabricatorFeedStoryNotification::updateObjectNotificationViews(
       $user, $revision->getPHID());
 
-    return $this->buildStandardPageResponse(
+    $top_anchor = id(new PhabricatorAnchorView())
+      ->setAnchorName('top')
+      ->setNavigationMarker(true);
+
+    $nav = $this->buildSideNavView($revision, $changesets);
+    $nav->selectFilter('');
+    $nav->appendChild(
       array(
         $reviewer_warning,
+        $top_anchor,
         $revision_detail,
         $page_pane,
-      ),
+      ));
+
+    return $this->buildApplicationPage(
+      $nav,
       array(
         'title' => 'D'.$revision->getID().' '.$revision->getTitle(),
       ));
@@ -996,4 +1006,79 @@ final class DifferentialRevisionViewController extends DifferentialController {
     return id(new AphrontRedirectResponse())->setURI($file->getBestURI());
 
   }
+
+  private function buildSideNavView(
+    DifferentialRevision $revision,
+    array $changesets) {
+
+    $nav = new AphrontSideNavFilterView();
+    $nav->setBaseURI(new PhutilURI('/D'.$revision->getID()));
+    $nav->setFlexible(true);
+
+    $nav->addFilter('top', 'D'.$revision->getID(), '#top',
+      $relative = false,
+      'phabricator-active-nav-focus');
+
+    $tree = new PhutilFileTree();
+    foreach ($changesets as $changeset) {
+      $tree->addPath($changeset->getFilename(), $changeset);
+    }
+
+    require_celerity_resource('phabricator-filetree-view-css');
+
+    $filetree = array();
+
+    $path = $tree;
+    while (($path = $path->getNextNode())) {
+      $data = $path->getData();
+
+      $name = $path->getName();
+      $style = 'padding-left: '.(2 + (3 * $path->getDepth())).'px';
+
+      $href = null;
+      if ($data) {
+        $href = '#'.$data->getAnchorName();
+        $icon = 'phabricator-filetree-icon-file';
+      } else {
+        $name .= '/';
+        $icon = 'phabricator-filetree-icon-dir';
+      }
+
+      $icon = phutil_render_tag(
+        'span',
+        array(
+          'class' => 'phabricator-filetree-icon '.$icon,
+        ),
+        '');
+
+      $name_element = phutil_render_tag(
+        'span',
+        array(
+          'class' => 'phabricator-filetree-name',
+        ),
+        phutil_escape_html($name));
+
+      $filetree[] = javelin_render_tag(
+        $href ? 'a' : 'span',
+        array(
+          'href' => $href,
+          'style' => $style,
+          'title' => $name,
+          'class' => 'phabricator-filetree-item',
+        ),
+        $icon.$name_element);
+    }
+    $tree->destroy();
+
+    $filetree =
+      '<div class="phabricator-filetree">'.
+        implode("\n", $filetree).
+      '</div>';
+    $nav->addFilter('toc', 'Table of Contents', '#toc');
+    $nav->addCustomBlock($filetree);
+    $nav->addFilter('comment', 'Add Comment', '#comment');
+    $nav->setActive(true);
+
+    return $nav;
+  }
 }

src/applications/differential/field/specification/DifferentialRevisionIDFieldSpecification.php

@@ -118,7 +118,7 @@ final class DifferentialRevisionIDFieldSpecification
         $body[] = null;
         $body[] = 'CHANGE SINCE LAST DIFF';
         $body[] = '  '.PhabricatorEnv::getProductionURI(
-          "/D{$this->id}?vs={$old}&id={$new}#differential-review-toc");
+          "/D{$this->id}?vs={$old}&id={$new}#toc");
       }
     }
 

src/applications/differential/view/DifferentialAddCommentView.php

@@ -203,6 +203,10 @@ final class DifferentialAddCommentView extends AphrontView {
     $panel_view->addClass('aphront-panel-flush');
 
     return
+      id(new PhabricatorAnchorView())
+        ->setAnchorName('comment')
+        ->setNavigationMarker(true)
+        ->render().
       '<div class="differential-add-comment-panel">'.
         $panel_view->render().
         '<div class="aphront-panel-preview aphront-panel-flush">'.

src/applications/differential/view/DifferentialChangesetDetailView.php

@@ -93,9 +93,6 @@ final class DifferentialChangesetDetailView extends AphrontView {
 
     $display_filename = $changeset->getDisplayFilename();
 
-    $buoyant_begin = $this->renderBuoyant($display_filename);
-    $buoyant_end   = $this->renderBuoyant(null);
-
     $output = javelin_render_tag(
       'div',
       array(
@@ -109,39 +106,17 @@ final class DifferentialChangesetDetailView extends AphrontView {
         'class' => $class,
         'id'    => $id,
       ),
-      $buoyant_begin.
+      id(new PhabricatorAnchorView())
-      phutil_render_tag(
+        ->setAnchorName($changeset->getAnchorName())
-        'a',
+        ->setNavigationMarker(true)
-        array(
+        ->render().
-          'name' => $changeset->getAnchorName(),
-        ),
-        '').
       $buttons.
       '<h1>'.phutil_escape_html($display_filename).'</h1>'.
       '<div style="clear: both;"></div>'.
-      $this->renderChildren().
+      $this->renderChildren());
-      $buoyant_end);
 
 
     return $output;
   }
 
-  private function renderBuoyant($text) {
-    return javelin_render_tag(
-      'div',
-      array(
-        'sigil' => 'buoyant',
-        'meta'  => array(
-          'text' => $text,
-        ),
-        'style' => ($text === null)
-          // Current CSS spacing rules cause the "end" anchor to appear too
-          // late in the display document. Shift it up a bit so we drop the
-          // buoyant header sooner. This reduces confusion when using keystroke
-          // navigation.
-          ? 'bottom: 60px; position: absolute;'
-          : null,
-      ),
-      '');
-  }
 }

src/applications/differential/view/DifferentialChangesetListView.php

@@ -110,10 +110,6 @@ final class DifferentialChangesetListView extends AphrontView {
 
     $changesets = $this->changesets;
 
-    // TODO: Restore this once we make it through the redesign, it has funky
-    // interactions with things and there are various reports that it's slow.
-    // Javelin::initBehavior('buoyant', array());
-
     Javelin::initBehavior('differential-toggle-files', array());
 
     $output = array();

src/applications/differential/view/DifferentialDiffTableOfContentsView.php

@@ -222,8 +222,11 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
     );
 
     return
-      '<div id="differential-review-toc" '.
+      id(new PhabricatorAnchorView())
-        'class="differential-toc differential-panel">'.
+        ->setAnchorName('toc')
+        ->setNavigationMarker(true)
+        ->render().
+      '<div class="differential-toc differential-panel">'.
         $editor_link.
         $reveal_link.
         '<h1>Table of Contents</h1>'.

src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php

@@ -216,7 +216,7 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView {
     return
       '<div class="differential-revision-history differential-panel">'.
         '<h1>Revision Update History</h1>'.
-        '<form action="#differential-review-toc">'.
+        '<form action="#toc">'.
           '<table class="differential-revision-history-table">'.
             '<tr>'.
               '<th>Diff</th>'.

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -182,7 +182,7 @@ final class DiffusionCommitController extends DiffusionController {
     } else {
       $change_panel = new AphrontPanelView();
       $change_panel->setHeader("Changes (".number_format($count).")");
-      $change_panel->setID('differential-review-toc');
+      $change_panel->setID('toc');
 
       if ($count > self::CHANGES_LIMIT) {
         $show_all_button = phutil_render_tag(

src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php

@@ -162,7 +162,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
               '/D'.$revision->getID().
               '?vs='.$vs_diff->getID().
               '&id='.$diff->getID().
-              '#differential-review-toc');
+              '#toc');
             $editor->setChangedByCommit($changed_by_commit);
           }
 

src/view/layout/AphrontSideNavFilterView.php

@@ -45,6 +45,12 @@ final class AphrontSideNavFilterView extends AphrontView {
   private $showApplicationMenu;
   private $user;
   private $currentApplication;
+  private $active;
+
+  public function setActive($active) {
+    $this->active = $active;
+    return $this;
+  }
 
   public function setCurrentApplication(PhabricatorApplication $current) {
     $this->currentApplication = $current;
@@ -102,6 +108,11 @@ final class AphrontSideNavFilterView extends AphrontView {
     }
   }
 
+  public function addCustomBlock($block) {
+    $this->items[] = array('custom', null, $block);
+    return $this;
+  }
+
   public function addLabel($name) {
     $this->items[] = array('label', null, $name);
     return $this;
@@ -150,6 +161,7 @@ final class AphrontSideNavFilterView extends AphrontView {
     $view->setFlexNav($this->flexNav);
     $view->setFlexible($this->flexible);
     $view->setShowApplicationMenu($this->showApplicationMenu);
+    $view->setActive($this->active);
     if ($this->user) {
       $view->setUser($this->user);
     }
@@ -159,6 +171,9 @@ final class AphrontSideNavFilterView extends AphrontView {
     foreach ($this->items as $item) {
       list($type, $key, $name) = $item;
       switch ($type) {
+        case 'custom':
+          $view->addNavItem($name);
+          break;
         case 'spacer':
           $view->addNavItem('<br />');
           break;

src/view/layout/AphrontSideNavView.php

@@ -24,6 +24,7 @@ final class AphrontSideNavView extends AphrontView {
   private $showApplicationMenu;
   private $user;
   private $currentApplication;
+  private $active;
 
   public function setUser(PhabricatorUser $user) {
     $this->user = $user;
@@ -55,6 +56,11 @@ final class AphrontSideNavView extends AphrontView {
     return $this;
   }
 
+  public function setActive($active) {
+    $this->active = $active;
+    return $this;
+  }
+
   public function render() {
     $view = new AphrontNullView();
     $view->appendChild($this->items);
@@ -153,6 +159,14 @@ final class AphrontSideNavView extends AphrontView {
           'collapseKey' => $key,
         ));
 
+      if ($this->active && $local_id) {
+        Javelin::initBehavior(
+          'phabricator-active-nav',
+          array(
+            'localID' => $local_id,
+          ));
+      }
+
       $header_part =
         '<div class="phabricator-nav-head">'.
           '<div class="phabricator-nav-head-tablet">'.

src/view/layout/PhabricatorAnchorView.php

@@ -0,0 +1,61 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class PhabricatorAnchorView extends AphrontView {
+
+  private $anchorName;
+  private $navigationMarker;
+
+  public function setAnchorName($name) {
+    $this->anchorName = $name;
+    return $this;
+  }
+
+  public function setNavigationMarker($marker) {
+    $this->navigationMarker = $marker;
+    return $this;
+  }
+
+  public function render() {
+    $marker = null;
+    if ($this->navigationMarker) {
+      $marker = javelin_render_tag(
+        'legend',
+        array(
+          'class' => 'phabricator-anchor-navigation-marker',
+          'sigil' => 'marker',
+          'meta'  => array(
+            'anchor' => $this->anchorName,
+          ),
+        ),
+        '');
+    }
+
+    $anchor = phutil_render_tag(
+      'a',
+      array(
+        'name'  => $this->anchorName,
+        'id'    => $this->anchorName,
+        'class' => 'phabricator-anchor-view',
+      ),
+      '');
+
+    return $marker.$anchor;
+  }
+
+}

src/view/layout/PhabricatorTransactionView.php

@@ -122,11 +122,14 @@ final class PhabricatorTransactionView extends AphrontView {
 
     if ($this->anchorName) {
       Javelin::initBehavior('phabricator-watch-anchor');
-      $info[] = phutil_render_tag(
+
+      $anchor = id(new PhabricatorAnchorView())
+        ->setAnchorName($this->anchorName)
+        ->render();
+
+      $info[] = $anchor.phutil_render_tag(
         'a',
         array(
-          'name'  => $this->anchorName,
-          'id'    => $this->anchorName,
           'href'  => '#'.$this->anchorName,
         ),
         phutil_escape_html($this->anchorText));

webroot/rsrc/css/application/base/standard-page-view.css

@@ -6,6 +6,22 @@
   background: #ffffff;
 }
 
+.phabricator-anchor-view,
+.phabricator-anchor-navigation-marker {
+  position: absolute;
+  margin-top: -15px;
+}
+
+
+
+.device-desktop .phabricator-anchor-view,
+.device-desktop .phabricator-anchor-navigation-marker {
+  /* On desktops, move the anchor up more so the menu bar doesn't obscure the
+     content. This is the menu bar height (44px) plus the anchor adjustment
+     (15px). */
+  margin-top: -59px;
+}
+
 .phabricator-chromeless-page .phabricator-standard-page {
   background:           transparent;
   border-width:         0px;
@@ -101,21 +117,3 @@ a.handle-disabled {
   font-size: 11px;
   font-family: "Verdana";
 }
-
-.buoyant {
-  position: fixed;
-  top: 0px;
-  left: 0px;
-  z-index: 8;
-
-  padding: 6px;
-  color: #dddddd;
-  font-size: 11px;
-  opacity: 0.90;
-  width: 100%;
-
-  background: #222222;
-  border-bottom: 1px solid #dfdfdf;
-  cursor: pointer;
-
-}

webroot/rsrc/css/application/differential/revision-comment.css

@@ -23,7 +23,7 @@
   bottom: 0;
   right: 0;
   left: 0;
-  z-index: 5;
+  z-index: 8;
   overflow: auto;
   max-height: 375px;
   max-width: none;

webroot/rsrc/css/layout/phabricator-filetree-view.css

@@ -0,0 +1,67 @@
+/**
+ * @provides phabricator-filetree-view-css
+ */
+
+.phabricator-filetree {
+  background: #fcfcfc;
+  border-style: solid;
+  border-width: 1px 0;
+  border-color: #a0a0a0;
+  padding: 4px 0;
+}
+
+/* NOTE: Until the whole side nav situation gets cleaned up, we need to be
+   highly specific in specifying selectors here, to override side nav styles.
+*/
+
+.phabricator-filetree .phabricator-filetree-item {
+  margin: 0;
+  padding: 0;
+}
+
+.phabricator-filetree span.phabricator-filetree-icon {
+  background-repeat: no-repeat;
+  background-position: 0 2px;
+  width: 16px;
+  height: 20px;
+  padding: 0;
+  float: left;
+}
+
+.phabricator-filetree span.phabricator-filetree-name {
+  padding: 0;
+  margin-left: 20px;
+  font-size: 11px;
+  font-weight: normal;
+  line-height: 20px;
+  white-space: nowrap;
+}
+
+.phabricator-filetree span.phabricator-filetree-item
+  .phabricator-filetree-name {
+  color: #666666;
+}
+
+.phabricator-filetree a.phabricator-filetree-item
+  .phabricator-filetree-name {
+  color: #3b5998;
+}
+
+.phabricator-filetree a.phabricator-filetree-item:hover
+  .phabricator-filetree-name {
+  color: #f9f9f9;
+}
+
+
+.phabricator-filetree-icon-file {
+  background-image: url(/rsrc/image/icon/fatcow/page_white_text.png);
+}
+
+.phabricator-filetree-icon-dir {
+  background-image: url(/rsrc/image/icon/fatcow/folder.png);
+}
+
+.phabricator-nav-local
+  a.phabricator-active-nav-focus {
+  background: #9caccf;
+}

webroot/rsrc/js/application/core/behavior-active-nav.js

@@ -0,0 +1,86 @@
+/**
+ * @provides javelin-behavior-phabricator-active-nav
+ * @requires javelin-behavior
+ *           javelin-stratcom
+ *           javelin-vector
+ *           javelin-dom
+ *           javelin-uri
+ */
+
+JX.behavior('phabricator-active-nav', function(config) {
+
+  var local = JX.$(config.localID);
+
+  /**
+   * Select the navigation item corresponding to a given anchor.
+   */
+  var selectnav = function(anchor) {
+    var links = JX.DOM.scry(local, 'a');
+    var link;
+    var link_anchor;
+    var selected;
+    for (var ii = 0; ii < links.length; ii++) {
+      link = links[ii];
+      link_anchor = JX.$U(link.href).getFragment();
+
+      selected = (link_anchor == anchor);
+      JX.DOM.alterClass(
+        link,
+        'phabricator-active-nav-focus',
+        selected);
+    }
+  }
+
+
+  /**
+   * Identify the current anchor based on the document scroll position.
+   */
+  var updateposition = function() {
+    // Find all the markers in the document.
+    var scroll_position = JX.Vector.getScroll().y;
+    var document_size = JX.Vector.getDocument();
+    var viewport_size = JX.Vector.getViewport();
+
+    // If we're scrolled all the way down, we always want to select the last
+    // anchor.
+    var is_at_bottom = (viewport_size.y + scroll_position >= document_size.y);
+
+    var markers = JX.DOM.scry(document.body, 'legend', 'marker');
+
+    // Sort the markers by Y position, descending.
+    var markinfo = [];
+    for (var ii = 0; ii < markers.length; ii++) {
+      markinfo.push({
+        marker: markers[ii],
+        position: JX.$V(markers[ii]).y
+      });
+    }
+    markinfo.sort(function(u, v) { return (v.position - u.position); });
+
+    // Find the first marker above the current scroll position, or the first
+    // marker in the document if we're above all the markers.
+    var active = null;
+    for (var ii = 0; ii < markinfo.length; ii++) {
+      active = markinfo[ii].marker;
+      if (markinfo[ii].position <= scroll_position) {
+        break;
+      }
+      if (is_at_bottom) {
+        break;
+      }
+    }
+
+    // If we get above the first marker, select it.
+    selectnav(active && JX.Stratcom.getData(active).anchor);
+  }
+
+  var pending = null;
+  var onviewportchange = function(e) {
+    pending && clearTimeout(pending);
+    pending = setTimeout(updateposition, 100);
+  }
+
+  JX.Stratcom.listen('scroll', null, onviewportchange);
+  JX.Stratcom.listen('resize', null, onviewportchange);
+  JX.Stratcom.listen('hashchange', null, onviewportchange);
+});

webroot/rsrc/js/application/core/behavior-buoyant.js

@@ -1,91 +0,0 @@
-/**
- * @provides javelin-behavior-buoyant
- * @requires javelin-behavior
- *           javelin-stratcom
- *           javelin-vector
- *           javelin-dom
- */
-
-JX.behavior('buoyant', function() {
-
-  // The display element which shows the "buoyant" header to the user.
-  var element = JX.$N('div', {className : 'buoyant'});
-
-  // Keeps track of whether we're currently showing anything or not.
-  var visible = false;
-
-  // If we're showing something, the positional DOM element that triggered the
-  // currently shown header.
-  var active_marker = null;
-
-  // When the header is clicked, jump to the element that triggered it.
-  JX.DOM.listen(element, 'click', null, function(e) {
-    window.scrollTo(0, JX.$V(active_marker).y - 40);
-  });
-
-  function hide() {
-    if (visible) {
-      JX.DOM.remove(element);
-      visible = false;
-    }
-  }
-
-  function show(text) {
-    if (!visible) {
-      document.body.appendChild(element);
-      visible = true;
-    }
-    JX.DOM.setContent(element, text);
-  }
-
-  var onviewportchange = function(e) {
-
-    // If we're currently showing a header but we've scrolled back up past its
-    // marker, hide it.
-
-    var scroll_position = JX.Vector.getScroll().y;
-    if (visible && (scroll_position < JX.$V(active_marker).y)) {
-      hide();
-    }
-
-    // Find all the markers in the document.
-
-    var markers = JX.DOM.scry(document.body, 'div', 'buoyant');
-
-    // Sort the markers by Y position, descending.
-
-    var markinfo = [];
-    for (var ii = 0; ii < markers.length; ii++) {
-      markinfo.push({
-        marker: markers[ii],
-        position: JX.$V(markers[ii]).y
-      });
-    }
-    markinfo.sort(function(u, v) { return (v.position - u.position); });
-
-    // Find the first marker above the current scroll position.
-
-    for (var ii = 0; ii < markinfo.length; ii++) {
-      if (markinfo[ii].position > scroll_position) {
-        // This marker is below the current scroll position, so ignore it.
-        continue;
-      }
-
-      // We've found a marker. Display it as appropriate;
-
-      active_marker = markinfo[ii].marker;
-      var text = JX.Stratcom.getData(active_marker).text;
-      if (text) {
-        show(text);
-      } else {
-        hide();
-      }
-
-      break;
-    }
-
-  }
-
-  JX.Stratcom.listen('scroll', null, onviewportchange);
-  JX.Stratcom.listen('resize', null, onviewportchange);
-});

webroot/rsrc/js/application/differential/behavior-keyboard-nav.js

@@ -219,7 +219,7 @@ JX.behavior('differential-keyboard-navigation', function(config) {
 
   new JX.KeyboardShortcut('t', 'Jump to the table of contents.')
     .setHandler(function(manager) {
-      var toc = JX.$('differential-review-toc');
+      var toc = JX.$('toc');
       manager.scrollTo(toc);
     })
     .register();
@@ -231,7 +231,7 @@ JX.behavior('differential-keyboard-navigation', function(config) {
         return;
       }
       JX.Stratcom.invoke('differential-toggle-file', null, {
-        diff: JX.DOM.scry(changesets[cursor], 'table', 'differential-diff'),
+        diff: JX.DOM.scry(changesets[cursor], 'table', 'differential-diff')
       });
     })
     .register();