Simplify form/lightbox stuff

Summary:
I broke this a bit a few revisions ago by making `phabricator_render_csrf_magic()` return array instead of string without actually grepping for callsites.

Instead of building a form in JS, build it in PHP and just change its action in JS. This is simpler and doesn't require us to expose CSRF magic in more places.

This change triggered a rather subtle bug: if we rendered a normal form on the page (and thus installed the "disable forms when submitted" behavior), the download button would incorrectly disable after being clicked. This doesn't currently happen in Files because we don't put a form on the same page as the download button.

Instead, make the "disable" behavior check if the form is downloading stuff, and not disable stuff if it is. Then mark the lightbox and Files form as both download forms.

Test Plan: Used lightbox; used Files. Verified download buttons download the right stuff and behave correctly. Grepped for other download forms, but all the other places where we write "download" don't appear to actually be download links.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Maniphest Tasks: T2432

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

Author: epriestley

src/applications/files/controller/PhabricatorFileInfoController.php

@@ -72,6 +72,7 @@ private function buildActionView(PhabricatorFile $file) {
         id(new PhabricatorActionView())
           ->setUser($user)
           ->setRenderAsForm(true)
+          ->setDownload(true)
           ->setName(pht('Download File'))
           ->setIcon('download')
           ->setHref($file->getViewURI()));

src/view/AphrontTagView.php

@@ -56,8 +56,8 @@ final public function addSigil($sigil) {
     return $this;
   }
 
-  final public function getSigil() {
-    return $this->sigil;
+  final public function getSigils() {
+    return $this->sigils;
   }
 
   public function addClass($class) {

src/view/form/AphrontFormView.php

@@ -58,8 +58,6 @@ public function render() {
     }
     require_celerity_resource('aphront-form-view-css');
 
-    Javelin::initBehavior('aphront-form-disable-on-submit');
-
     $layout = new AphrontFormLayoutView();
 
     if (!$this->flexible) {

src/view/layout/PhabricatorActionView.php

@@ -8,6 +8,16 @@ final class PhabricatorActionView extends AphrontView {
   private $disabled;
   private $workflow;
   private $renderAsForm;
+  private $download;
+
+  public function setDownload($download) {
+    $this->download = $download;
+    return $this;
+  }
+
+  public function getDownload() {
+    return $this->download;
+  }
 
   public function setHref($href) {
     $this->href = $href;
@@ -73,12 +83,20 @@ public function render() {
           ),
           $this->name);
 
+        $sigils = array();
+        if ($this->workflow) {
+          $sigils[] = 'workflow';
+        }
+        if ($this->download) {
+          $sigils[] = 'download';
+        }
+
         $item = phabricator_render_form(
           $this->user,
           array(
             'action'    => $this->href,
             'method'    => 'POST',
-            'sigil'     => $this->workflow ? 'workflow' : null,
+            'sigil'     => implode(' ', $sigils),
           ),
           $item);
       } else {

src/view/page/PhabricatorStandardPageView.php

@@ -108,36 +108,55 @@ protected function willRenderPage() {
 
     Javelin::initBehavior('workflow', array());
 
-    $current_token = null;
     $request = $this->getRequest();
+    $user = null;
     if ($request) {
       $user = $request->getUser();
-      if ($user) {
-        $current_token = $user->getCSRFToken();
-        $download_form = phabricator_render_form_magic($user);
-        $default_img_uri =
-          PhabricatorEnv::getCDNURI(
-            '/rsrc/image/icon/fatcow/document_black.png'
-          );
-
-        Javelin::initBehavior(
-          'lightbox-attachments',
-          array(
-            'defaultImageUri' => $default_img_uri,
-            'downloadForm'    => $download_form,
-          ));
-      }
     }
 
+    if ($user) {
+      $default_img_uri =
+        PhabricatorEnv::getCDNURI(
+          '/rsrc/image/icon/fatcow/document_black.png'
+        );
+      $download_form = phabricator_render_form(
+        $user,
+        array(
+          'action' => '#',
+          'method' => 'POST',
+          'class'  => 'lightbox-download-form',
+          'sigil'  => 'download',
+        ),
+        phutil_tag(
+          'button',
+          array(),
+          pht('Download')));
+
+      Javelin::initBehavior(
+        'lightbox-attachments',
+        array(
+          'defaultImageUri' => $default_img_uri,
+          'downloadForm'    => $download_form,
+        ));
+    }
+
+    Javelin::initBehavior('aphront-form-disable-on-submit');
     Javelin::initBehavior('toggle-class', array());
     Javelin::initBehavior('konami', array());
+
+    $current_token = null;
+    if ($user) {
+      $current_token = $user->getCSRFToken();
+    }
+
     Javelin::initBehavior(
       'refresh-csrf',
       array(
         'tokenName' => AphrontRequest::getCSRFTokenName(),
         'header'    => AphrontRequest::getCSRFHeaderName(),
         'current'   => $current_token,
       ));
+
     Javelin::initBehavior('device');
 
     if ($console) {

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

@@ -23,6 +23,12 @@ JX.behavior('aphront-form-disable-on-submit', function(config) {
 
     root = e.getNode('tag:form');
 
+    // If the form is a "download" form, don't disable it on submit because
+    // we won't transition off the page.
+    if (JX.Stratcom.hasSigil(root, 'download')) {
+      return;
+    }
+
     // Open the form to a new tab in Firefox explicitly (automatic in Chrome).
     // We render some buttons as links so users may be confused that the links
     // don't open to new tabs with Ctrl+click as normal links.

webroot/rsrc/js/application/core/behavior-lightbox-attachments.js

@@ -15,7 +15,7 @@ JX.behavior('lightbox-attachments', function (config) {
   var next         = null;
   var x_margin     = 40;
   var y_margin     = 100;
-  var downloadForm = JX.$H(config.downloadForm);
+  var downloadForm = JX.$H(config.downloadForm).getFragment().firstChild;
 
   function loadLightBox(e) {
     if (!e.isNormalClick()) {
@@ -133,33 +133,11 @@ JX.behavior('lightbox-attachments', function (config) {
                            },
                            'Image '+current+' of '+total+'.'+extra_status
                            );
-    var form = JX.$N('form',
-                     {
-                       action     : target_data.dUri,
-                       method     : 'POST',
-                       className  : 'lightbox-download-form'
-                     },
-                     downloadForm
-                    );
-    JX.DOM.appendContent(form, JX.$N('button', {}, 'Download'));
-    JX.DOM.listen(form,
-                  'click',
-                  null,
-                  function (e) {
-                    e.prevent();
-                    form.submit();
-                    // Firefox and probably IE need this trick to work.
-                    // Removing a form from the DOM while its submitting is
-                    // tricky business.
-                    setTimeout(JX.bind(null, closeLightBox, e), 0);
-                  }
-                 );
+
     var downloadSpan = JX.$N('span',
                             {
                               className : 'lightbox-download'
-                            },
-                            form
-                            );
+                            });
     var statusHTML = JX.$N('div',
                            {
                              className : 'lightbox-status'
@@ -169,6 +147,10 @@ JX.behavior('lightbox-attachments', function (config) {
     JX.DOM.appendContent(lightbox, statusHTML);
     JX.DOM.alterClass(document.body, 'lightbox-attached', true);
     JX.Mask.show('jx-dark-mask');
+
+    downloadForm.action = target_data.dUri;
+    downloadSpan.appendChild(downloadForm);
+
     document.body.appendChild(lightbox);
 
     JX.Busy.start();