make Differential comment panel flexible width and nip a bit at other flexible width issues

Summary:
all sorts of stuff

 - made comment form width flexible
 - made margins element specific rather than part of differential-primary-pane
 - made box elements all veritically align left and right until code stuff
 - re-factored width calculaton stuff a bunch so only the code section has to suffer from max-width calculations; everything else can flex
 - made colspan 3 for rightmost table header element. this is so the "View Options" UI element ends up lining up correctly with the "Show All Lines" element just below

Test Plan: looked at revision view and changeset view and it all looked hot. note I did not test what things looked like with different word wrap values; that should still work given the re-factoring and not re-design here. also toggled haunted panel mode and it looked good.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2006

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

src/__celerity_resource_map__.php

@@ -722,7 +722,7 @@ celerity_register_resource_map(array(
   ),
   'differential-changeset-view-css' =>
   array(
-    'uri' => '/res/8ef2b180/rsrc/css/application/differential/changeset-view.css',
+    'uri' => '/res/dfc639d1/rsrc/css/application/differential/changeset-view.css',
     'type' => 'css',
     'requires' =>
     array(
@@ -731,7 +731,7 @@ celerity_register_resource_map(array(
   ),
   'differential-core-view-css' =>
   array(
-    'uri' => '/res/2303c309/rsrc/css/application/differential/core.css',
+    'uri' => '/res/7901260e/rsrc/css/application/differential/core.css',
     'type' => 'css',
     'requires' =>
     array(
@@ -782,7 +782,7 @@ celerity_register_resource_map(array(
   ),
   'differential-revision-comment-css' =>
   array(
-    'uri' => '/res/ff510704/rsrc/css/application/differential/revision-comment.css',
+    'uri' => '/res/5e613a7f/rsrc/css/application/differential/revision-comment.css',
     'type' => 'css',
     'requires' =>
     array(
@@ -1181,7 +1181,7 @@ celerity_register_resource_map(array(
   ),
   'javelin-behavior-differential-keyboard-navigation' =>
   array(
-    'uri' => '/res/559fbf58/rsrc/js/application/differential/behavior-keyboard-nav.js',
+    'uri' => '/res/0a013420/rsrc/js/application/differential/behavior-keyboard-nav.js',
     'type' => 'js',
     'requires' =>
     array(
@@ -1578,7 +1578,7 @@ celerity_register_resource_map(array(
   ),
   'javelin-behavior-phabricator-nav' =>
   array(
-    'uri' => '/res/6e550189/rsrc/js/application/core/behavior-phabricator-nav.js',
+    'uri' => '/res/3d04f9ab/rsrc/js/application/core/behavior-phabricator-nav.js',
     'type' => 'js',
     'requires' =>
     array(
@@ -2586,7 +2586,7 @@ celerity_register_resource_map(array(
   ),
   'phabricator-nav-view-css' =>
   array(
-    'uri' => '/res/766628f5/rsrc/css/aphront/phabricator-nav-view.css',
+    'uri' => '/res/84381dcf/rsrc/css/aphront/phabricator-nav-view.css',
     'type' => 'css',
     'requires' =>
     array(
@@ -3149,7 +3149,7 @@ celerity_register_resource_map(array(
       'uri' => '/res/pkg/805a58d7/core.pkg.js',
       'type' => 'js',
     ),
-    '2ba14b3d' =>
+    '7d861806' =>
     array(
       'name' => 'differential.pkg.css',
       'symbols' =>
@@ -3169,10 +3169,10 @@ celerity_register_resource_map(array(
         12 => 'differential-local-commits-view-css',
         13 => 'inline-comment-summary-css',
       ),
-      'uri' => '/res/pkg/2ba14b3d/differential.pkg.css',
+      'uri' => '/res/pkg/7d861806/differential.pkg.css',
       'type' => 'css',
     ),
-    '8136e4a6' =>
+    58152763 =>
     array(
       'name' => 'differential.pkg.js',
       'symbols' =>
@@ -3195,7 +3195,7 @@ celerity_register_resource_map(array(
         15 => 'differential-inline-comment-editor',
         16 => 'javelin-behavior-differential-dropdown-menus',
       ),
-      'uri' => '/res/pkg/8136e4a6/differential.pkg.js',
+      'uri' => '/res/pkg/58152763/differential.pkg.js',
       'type' => 'js',
     ),
     'c8ce2d88' =>
@@ -3291,7 +3291,7 @@ celerity_register_resource_map(array(
     'aphront-dialog-view-css' => '10a01616',
     'aphront-error-view-css' => '10a01616',
     'aphront-form-view-css' => '10a01616',
-    'aphront-headsup-action-list-view-css' => '2ba14b3d',
+    'aphront-headsup-action-list-view-css' => '7d861806',
     'aphront-headsup-view-css' => '10a01616',
     'aphront-list-filter-view-css' => '10a01616',
     'aphront-pager-view-css' => '10a01616',
@@ -3301,36 +3301,36 @@ celerity_register_resource_map(array(
     'aphront-tokenizer-control-css' => '10a01616',
     'aphront-tooltip-css' => '10a01616',
     'aphront-typeahead-control-css' => '10a01616',
-    'differential-changeset-view-css' => '2ba14b3d',
+    'differential-changeset-view-css' => '7d861806',
-    'differential-core-view-css' => '2ba14b3d',
+    'differential-core-view-css' => '7d861806',
-    'differential-inline-comment-editor' => '8136e4a6',
+    'differential-inline-comment-editor' => '58152763',
-    'differential-local-commits-view-css' => '2ba14b3d',
+    'differential-local-commits-view-css' => '7d861806',
-    'differential-results-table-css' => '2ba14b3d',
+    'differential-results-table-css' => '7d861806',
-    'differential-revision-add-comment-css' => '2ba14b3d',
+    'differential-revision-add-comment-css' => '7d861806',
-    'differential-revision-comment-css' => '2ba14b3d',
+    'differential-revision-comment-css' => '7d861806',
-    'differential-revision-comment-list-css' => '2ba14b3d',
+    'differential-revision-comment-list-css' => '7d861806',
-    'differential-revision-history-css' => '2ba14b3d',
+    'differential-revision-history-css' => '7d861806',
-    'differential-revision-list-css' => '2ba14b3d',
+    'differential-revision-list-css' => '7d861806',
-    'differential-table-of-contents-css' => '2ba14b3d',
+    'differential-table-of-contents-css' => '7d861806',
     'diffusion-commit-view-css' => 'c8ce2d88',
     'diffusion-icons-css' => 'c8ce2d88',
-    'inline-comment-summary-css' => '2ba14b3d',
+    'inline-comment-summary-css' => '7d861806',
     'javelin-behavior' => 'c50bbf3a',
     'javelin-behavior-aphront-basic-tokenizer' => 'dd024ca1',
-    'javelin-behavior-aphront-drag-and-drop' => '8136e4a6',
+    'javelin-behavior-aphront-drag-and-drop' => '58152763',
-    'javelin-behavior-aphront-drag-and-drop-textarea' => '8136e4a6',
+    'javelin-behavior-aphront-drag-and-drop-textarea' => '58152763',
     'javelin-behavior-aphront-form-disable-on-submit' => '805a58d7',
     'javelin-behavior-audit-preview' => '5e68be89',
-    'javelin-behavior-differential-accept-with-errors' => '8136e4a6',
+    'javelin-behavior-differential-accept-with-errors' => '58152763',
-    'javelin-behavior-differential-add-reviewers-and-ccs' => '8136e4a6',
+    'javelin-behavior-differential-add-reviewers-and-ccs' => '58152763',
-    'javelin-behavior-differential-comment-jump' => '8136e4a6',
+    'javelin-behavior-differential-comment-jump' => '58152763',
-    'javelin-behavior-differential-diff-radios' => '8136e4a6',
+    'javelin-behavior-differential-diff-radios' => '58152763',
-    'javelin-behavior-differential-dropdown-menus' => '8136e4a6',
+    'javelin-behavior-differential-dropdown-menus' => '58152763',
-    'javelin-behavior-differential-edit-inline-comments' => '8136e4a6',
+    'javelin-behavior-differential-edit-inline-comments' => '58152763',
-    'javelin-behavior-differential-feedback-preview' => '8136e4a6',
+    'javelin-behavior-differential-feedback-preview' => '58152763',
-    'javelin-behavior-differential-keyboard-navigation' => '8136e4a6',
+    'javelin-behavior-differential-keyboard-navigation' => '58152763',
-    'javelin-behavior-differential-populate' => '8136e4a6',
+    'javelin-behavior-differential-populate' => '58152763',
-    'javelin-behavior-differential-show-more' => '8136e4a6',
+    'javelin-behavior-differential-show-more' => '58152763',
     'javelin-behavior-diffusion-commit-graph' => '5e68be89',
     'javelin-behavior-diffusion-pull-lastmodified' => '5e68be89',
     'javelin-behavior-maniphest-batch-selector' => '7707de41',
@@ -3340,12 +3340,12 @@ celerity_register_resource_map(array(
     'javelin-behavior-maniphest-transaction-preview' => '7707de41',
     'javelin-behavior-phabricator-autofocus' => '805a58d7',
     'javelin-behavior-phabricator-keyboard-shortcuts' => '805a58d7',
-    'javelin-behavior-phabricator-object-selector' => '8136e4a6',
+    'javelin-behavior-phabricator-object-selector' => '58152763',
     'javelin-behavior-phabricator-oncopy' => '805a58d7',
     'javelin-behavior-phabricator-tooltips' => '805a58d7',
     'javelin-behavior-phabricator-watch-anchor' => '805a58d7',
     'javelin-behavior-refresh-csrf' => '805a58d7',
-    'javelin-behavior-repository-crossreference' => '8136e4a6',
+    'javelin-behavior-repository-crossreference' => '58152763',
     'javelin-behavior-workflow' => '805a58d7',
     'javelin-dom' => 'c50bbf3a',
     'javelin-event' => 'c50bbf3a',
@@ -3367,23 +3367,23 @@ celerity_register_resource_map(array(
     'maniphest-task-summary-css' => '7839ae2d',
     'maniphest-transaction-detail-css' => '7839ae2d',
     'phabricator-app-buttons-css' => '10a01616',
-    'phabricator-content-source-view-css' => '2ba14b3d',
+    'phabricator-content-source-view-css' => '7d861806',
     'phabricator-core-buttons-css' => '10a01616',
     'phabricator-core-css' => '10a01616',
     'phabricator-directory-css' => '10a01616',
-    'phabricator-drag-and-drop-file-upload' => '8136e4a6',
+    'phabricator-drag-and-drop-file-upload' => '58152763',
     'phabricator-dropdown-menu' => '805a58d7',
     'phabricator-flag-css' => '10a01616',
     'phabricator-jump-nav' => '10a01616',
     'phabricator-keyboard-shortcut' => '805a58d7',
     'phabricator-keyboard-shortcut-manager' => '805a58d7',
     'phabricator-menu-item' => '805a58d7',
-    'phabricator-object-selector-css' => '2ba14b3d',
+    'phabricator-object-selector-css' => '7d861806',
     'phabricator-paste-file-upload' => '805a58d7',
     'phabricator-prefab' => '805a58d7',
     'phabricator-project-tag-css' => '7839ae2d',
     'phabricator-remarkup-css' => '10a01616',
-    'phabricator-shaped-request' => '8136e4a6',
+    'phabricator-shaped-request' => '58152763',
     'phabricator-standard-page-view' => '10a01616',
     'phabricator-tooltip' => '805a58d7',
     'phabricator-transaction-view-css' => '10a01616',

src/__phutil_library_map__.php

@@ -226,6 +226,7 @@ phutil_register_library_map(array(
     'DifferentialChangesetParser' => 'applications/differential/parser/DifferentialChangesetParser.php',
     'DifferentialChangesetParserTestCase' => 'applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php',
     'DifferentialChangesetViewController' => 'applications/differential/controller/DifferentialChangesetViewController.php',
+    'DifferentialCodeWidthSensitiveView' => 'applications/differential/view/DifferentialCodeWidthSensitiveView.php',
     'DifferentialComment' => 'applications/differential/storage/DifferentialComment.php',
     'DifferentialCommentEditor' => 'applications/differential/editor/DifferentialCommentEditor.php',
     'DifferentialCommentMail' => 'applications/differential/mail/DifferentialCommentMail.php',
@@ -1473,9 +1474,10 @@ phutil_register_library_map(array(
     'DifferentialChangeSetTestCase' => 'PhabricatorTestCase',
     'DifferentialChangeset' => 'DifferentialDAO',
     'DifferentialChangesetDetailView' => 'AphrontView',
-    'DifferentialChangesetListView' => 'AphrontView',
+    'DifferentialChangesetListView' => 'DifferentialCodeWidthSensitiveView',
     'DifferentialChangesetParserTestCase' => 'ArcanistPhutilTestCase',
     'DifferentialChangesetViewController' => 'DifferentialController',
+    'DifferentialCodeWidthSensitiveView' => 'AphrontView',
     'DifferentialComment' =>
     array(
       0 => 'DifferentialDAO',
@@ -1526,7 +1528,7 @@ phutil_register_library_map(array(
     'DifferentialManiphestTasksFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialNewDiffMail' => 'DifferentialReviewRequestMail',
     'DifferentialPathFieldSpecification' => 'DifferentialFieldSpecification',
-    'DifferentialPrimaryPaneView' => 'AphrontView',
+    'DifferentialPrimaryPaneView' => 'DifferentialCodeWidthSensitiveView',
     'DifferentialReplyHandler' => 'PhabricatorMailReplyHandler',
     'DifferentialResultsTableView' => 'AphrontView',
     'DifferentialRevertPlanFieldSpecification' => 'DifferentialFieldSpecification',

src/applications/differential/controller/DifferentialChangesetViewController.php

@@ -250,18 +250,20 @@ final class DifferentialChangesetViewController extends DifferentialController {
     $detail->appendChild($output);
     $detail->setVsChangesetID($left_source);
 
-    $output =
+    $panel = id(new DifferentialPrimaryPaneView())
-      id(new DifferentialPrimaryPaneView())
+      ->setLineWidthFromChangesets(array($changeset));
-        ->setLineWidthFromChangesets(array($changeset))
+
-        ->appendChild(
+    $panel->appendChild(phutil_render_tag('div',
-          '<div class="differential-review-stage" '.
+      array(
-            'id="differential-review-stage">'.
+        'class' => 'differential-review-stage',
-            $detail->render().
+        'id'    => 'differential-review-stage',
-          '</div>');
+        'style' => "max-width: {$panel->calculateSideBySideWidth()}px;"
+      ), $detail->render())
+    );
 
     return $this->buildStandardPageResponse(
       array(
-        $output
+        $panel
       ),
       array(
         'title' => 'Changeset View',

src/applications/differential/controller/DifferentialDiffViewController.php

@@ -116,6 +116,7 @@ final class DifferentialDiffViewController extends DifferentialController {
     $details = id(new DifferentialChangesetListView())
       ->setChangesets($changesets)
       ->setVisibleChangesets($changesets)
+      ->setLineWidthFromChangesets($changesets)
       ->setRenderingReferences($refs)
       ->setStandaloneURI('/differential/changeset/')
       ->setUser($request->getUser());

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -285,6 +285,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
     }
 
     $changeset_view = new DifferentialChangesetListView();
+    $changeset_view->setLineWidthFromChangesets($changesets);
     $changeset_view->setChangesets($changesets);
     $changeset_view->setVisibleChangesets($visible_changesets);
 

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -1521,7 +1521,7 @@ final class DifferentialChangesetParser {
             implode(' • ', $contents).
           '</td>'.
           '<th class="show-context-line">'.$context_line.'</td>'.
-          '<td colspan="2" class="show-context">'.$context.'</td>');
+          '<td colspan="3" class="show-context">'.$context.'</td>');
 
         $html[] = $container;
 

src/applications/differential/view/DifferentialAddCommentView.php

@@ -102,6 +102,7 @@ final class DifferentialAddCommentView extends AphrontView {
     $form = new AphrontFormView();
     $form
       ->setWorkflow(true)
+      ->setFlexible(true)
       ->setUser($this->user)
       ->setAction($this->actionURI)
       ->addHiddenInput('revision_id', $revision->getID())
@@ -164,8 +165,6 @@ final class DifferentialAddCommentView extends AphrontView {
 
     $diff = $revision->loadActiveDiff();
     $warnings = mpull($this->auxFields, 'renderWarningBoxForRevisionAccept');
-    $lint_warning = null;
-    $unit_warning = null;
 
     Javelin::initBehavior(
       'differential-accept-with-errors',
@@ -192,8 +191,6 @@ final class DifferentialAddCommentView extends AphrontView {
         'inline'    => 'inline-comment-preview',
       ));
 
-    $panel_view = new AphrontPanelView();
-    $panel_view->appendChild($form);
     $warning_container = '<div id="warnings">';
     foreach ($warnings as $warning) {
       if ($warning) {
@@ -201,18 +198,9 @@ final class DifferentialAddCommentView extends AphrontView {
       }
     }
     $warning_container .= '</div>';
-    $panel_view->appendChild($warning_container);
-    if ($lint_warning) {
-      $panel_view->appendChild($lint_warning);
-    }
-    if ($unit_warning) {
-      $panel_view->appendChild($unit_warning);
-    }
 
-    $panel_view->setHeader($is_serious ? 'Add Comment' : 'Leap Into Action');
+    $header = id(new PhabricatorHeaderView())
-
+      ->setHeader($is_serious ? 'Add Comment' : 'Leap Into Action');
-    $panel_view->addClass('aphront-panel-accent');
-    $panel_view->addClass('aphront-panel-flush');
 
     return
       id(new PhabricatorAnchorView())
@@ -220,7 +208,9 @@ final class DifferentialAddCommentView extends AphrontView {
         ->setNavigationMarker(true)
         ->render().
       '<div class="differential-add-comment-panel">'.
-        $panel_view->render().
+        $header->render().
+        $form->render().
+        $warning_container.
         '<div class="aphront-panel-preview aphront-panel-flush">'.
           '<div id="comment-preview">'.
             '<span class="aphront-panel-preview-loading-text">'.

src/applications/differential/view/DifferentialChangesetListView.php

@@ -16,7 +16,8 @@
  * limitations under the License.
  */
 
-final class DifferentialChangesetListView extends AphrontView {
+final class DifferentialChangesetListView
+  extends DifferentialCodeWidthSensitiveView {
 
   private $changesets = array();
   private $visibleChangesets = array();
@@ -209,6 +210,7 @@ final class DifferentialChangesetListView extends AphrontView {
       array(
         'class' => 'differential-review-stage',
         'id'    => 'differential-review-stage',
+        'style' => "max-width: {$this->calculateSideBySideWidth()}px; ",
       ),
       implode("\n", $output));
   }

src/applications/differential/view/DifferentialCodeWidthSensitiveView.php

@@ -0,0 +1,55 @@
+<?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.
+ */
+
+abstract class DifferentialCodeWidthSensitiveView extends AphrontView {
+
+  private $lineWidth = 80;
+
+  private function setLineWidth($width) {
+    $this->lineWidth = $width;
+    return $this;
+  }
+
+  public function getLineWidth() {
+    return $this->lineWidth;
+  }
+
+  public function setLineWidthFromChangesets(array $changesets) {
+    assert_instances_of($changesets, 'DifferentialChangeset');
+    if (empty($changesets)) {
+      return $this;
+    }
+
+    $max = 1;
+    foreach ($changesets as $changeset) {
+      $max = max($max, $changeset->getWordWrapWidth());
+    }
+    $this->setLineWidth($max);
+
+    return $this;
+  }
+
+  public function calculateSideBySideWidth() {
+    // Width of the constant-width elements (like line numbers, padding,
+    // and borders).
+    $const = 148;
+    $width = ceil(((1162 - $const) / 80) * $this->getLineWidth()) + $const;
+    return  max(1162, $width);
+  }
+
+}

src/applications/differential/view/DifferentialPrimaryPaneView.php

@@ -16,36 +16,16 @@
  * limitations under the License.
  */
 
-final class DifferentialPrimaryPaneView extends AphrontView {
+final class DifferentialPrimaryPaneView
+  extends DifferentialCodeWidthSensitiveView {
 
-  private $lineWidth = 80;
   private $id;
 
-  public function setLineWidth($width) {
-    $this->lineWidth = $width;
-    return $this;
-  }
-
   public function setID($id) {
     $this->id = $id;
     return $this;
   }
 
-  public function setLineWidthFromChangesets(array $changesets) {
-    assert_instances_of($changesets, 'DifferentialChangeset');
-    if (empty($changesets)) {
-      return $this;
-    }
-
-    $max = 1;
-    foreach ($changesets as $changeset) {
-      $max = max($max, $changeset->getWordWrapWidth());
-    }
-    $this->setLineWidth($max);
-
-    return $this;
-  }
-
   public function render() {
 
     // This is chosen somewhat arbitrarily so the math works out correctly
@@ -53,15 +33,9 @@ final class DifferentialPrimaryPaneView extends AphrontView {
     // need some tweaking, but when lineWidth = 80, the computed pixel width
     // should be 1162px or something along those lines.
 
-    // Width of the constant-width elements (like line numbers, padding,
-    // and borders).
-    $const = 148;
-    $width = ceil(((1162 - $const) / 80) * $this->lineWidth) + $const;
-    $width = max(1162, $width);
-
     // Override the 'td' width rule with a more specific, inline style tag.
     // TODO: move this to <head> somehow.
-    $td_width = ceil((88 / 80) * $this->lineWidth);
+    $td_width = ceil((88 / 80) * $this->getLineWidth());
     $style_tag = phutil_render_tag(
       'style',
       array(
@@ -73,7 +47,6 @@ final class DifferentialPrimaryPaneView extends AphrontView {
       'div',
       array(
         'class' => 'differential-primary-pane',
-        'style' => "max-width: {$width}px",
         'id'    => $this->id,
       ),
       $style_tag.$this->renderChildren());

webroot/rsrc/css/application/differential/changeset-view.css

@@ -2,6 +2,16 @@
  * @provides differential-changeset-view-css
  */
 
+.differential-review-stage {
+  margin: 0 2em 0 2em;
+
+  /**
+   * This is potentially overridden by a 'style' attribute which selects a
+   * different width based on the maximum character width of the diff.
+   */
+  max-width:        1162px;
+}
+
 .differential-changeset {
   z-index: 2;
   position: relative;

webroot/rsrc/css/application/differential/core.css

@@ -3,19 +3,11 @@
  */
 
 .differential-primary-pane {
-  margin:           0 0 0 2em;
   padding-bottom:   2em;
-
-  /**
-   * This is potentially overridden by a 'style' attribute which selects a
-   * different width based on the maximum character width of the diff.
-   */
-  max-width:        1162px;
 }
 
 .differential-panel {
-  margin:       25px 0 0 0;
+  margin:       25px 2em 0 2em;
-  max-width:    1120px;
   border:       1px solid #666622;
   background:   #efefdf;
   padding:      15px 20px;

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

@@ -3,11 +3,7 @@
  */
 
 .differential-comment-list {
-  max-width: 1162px;
+  margin: 0 2em 0 2em;
-}
-
-.differential-add-comment-panel {
-  max-width: 1162px;
 }
 
 /*  Spooky haunted panel which floats on the bottom of the screen.
@@ -26,12 +22,10 @@
   z-index: 8;
   overflow: auto;
   max-height: 375px;
-  max-width: none;
 
   box-shadow: 0 0 4px #000;
   -webkit-box-shadow: 0 0 4px #000;
   -moz-box-shadow: 0 0 4px #000;
-
 }
 
 .differential-haunt-mode-2 .differential-add-comment-panel {