Page MenuHomePhabricator

Add download link to embedded files

Authored by chad on Dec 2 2016, 8:59 PM.
Referenced Files
Unknown Object (File)
Tue, Apr 9, 7:48 PM
Unknown Object (File)
Tue, Apr 9, 7:44 PM
Unknown Object (File)
Sat, Apr 6, 9:05 AM
Unknown Object (File)
Sat, Apr 6, 9:03 AM
Unknown Object (File)
Sat, Apr 6, 9:03 AM
Unknown Object (File)
Sat, Apr 6, 9:02 AM
Unknown Object (File)
Thu, Apr 4, 10:32 AM
Unknown Object (File)
Thu, Apr 4, 2:22 AM



Ref T3612. Doesn't render correctly, need help please. Adds a download icon into the renderfilelinkview to allow easier downloads.

Test Plan

Click on link, get download, click on file, get lightbox.

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

chad retitled this revision from to Add download link to embedded files.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.
chad added a task: T3612: Lightbox v2.

Getting a double render here I think, not sure what I'm setting incorrectly (the form triggers it).

pasted_file (133×340 px, 5 KB)

If there is no easy way to do this, lmk, I'll just maybe re-think if Files should be lightboxable at all.

Oh, sorry, missed this. Let me take a look...

The browser inspector is misleading. The issue is that we're emitting this HTML:

  class="phabricator-remarkup-embed-layout-link "
  <span class="visual-only phui-icon-view phui-font-fa fa-file-picture-o" aria-hidden="true"></span>
  <span class="phabricator-remarkup-embed-layout-info-block">
    <span class="phabricator-remarkup-embed-layout-name">psyduck.png</span>
    <span class="phabricator-remarkup-embed-layout-info">490 KB</span>
  <form action="#" method="POST" class="embed-download-form" data-sigil="embed-download-form">
    <input type="hidden" name="__csrf__" value="B@qw2kw2zt247d96d8816fcd9a" />
    <input type="hidden" name="__form__" value="1" />
    <a class="phui-icon-circle hover-green" href="#">
      <span class="visual-only phui-icon-view phui-font-fa fa-download" aria-hidden="true"></span>

More simply:


I think you can't have a form in a link, and you definitely can't have a link in a link.

I think browsers are silently reorganizing the outer <a> and <form> tags to be valid, which makes it appear that a double render has occurred.

Changing getTagName() to div instead of a fixes the apparent double render (although we'll need more work to make the element actually work).

This took me a bit, but here's how I ultimately figured this out:

  • Changed renderFileLink() to return this:
return array(
  • Looked at the raw page source. The DOM inspector sometimes lies about this stuff, and reflects changes made by Javascript. You can compare the DOM inspector to the raw source to find problems caused by Javascript or by bad HTML tag soup: if the raw page source looks good but the DOM inspector looks broken, either the browser "fixed" something for you or the problem is with some kind of JS thing.
  • Copy/pasted the separated block of stuff of raw source stuff into my text editor.
  • Manually reformatted the tags to be readable, revealing the <a><form><a>.
epriestley edited edge metadata.

ok you can have this back now

This revision now requires changes to proceed.Dec 7 2016, 9:21 PM

This "improvement" to the source is particularly bizarre and I haven't seen browsers make it before but I guess we learned something new today?

chad edited edge metadata.
  • switch to divs

I think this all works as expected, though downloading the file now changes submit button to 0.5 opacity and never clears and I'm not sure how to skip that feature.

Alternatively, I believe you can add the sigil download to stop the disable-on-submit behavior.

This revision is now accepted and ready to land.Jan 3 2017, 6:48 PM
This revision was automatically updated to reflect the committed changes.