Skip to content

File Block: Set the download attribute as empty#10948

Merged
Copons merged 2 commits into
masterfrom
update/download-attribute-valueless
Oct 23, 2018
Merged

File Block: Set the download attribute as empty#10948
Copons merged 2 commits into
masterfrom
update/download-attribute-valueless

Conversation

@Copons

@Copons Copons commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

Description

Replaces #10693

Set the File Block's "Download" button's download attribute as empty.

The addition of the download attribute for a tags to the wp_kses whitelist has been recently blocked (cc @pento):

The download attribute doesn't work on cross-origin links (eg, any site that uses a CDN for hosting uploads). I don't know that we necessarily need to account for this, but it is something to consider.

It's also a risk to allow the download filename to be set: for example, an author could upload my_definitely_not_suspicious_file.txt, but then set the download attribute to be CLICK_ME.bat, which isn't great. If we do allow the download attribute, it should only be allowed with no value.

Note

The attribute has been set as empty string (download="") instead of valueless (download or download={ true }) because in the latter case it was always stripped away at the first block update, breaking it altogether.

How has this been tested?

Manually on a local environment, checking for regressions.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@Copons Copons added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks labels Oct 23, 2018
@Copons Copons self-assigned this Oct 23, 2018
@Copons Copons requested review from pento and talldan October 23, 2018 11:58
@vindl vindl added this to the API Freeze milestone Oct 23, 2018
@gziolo gziolo requested a review from a team October 23, 2018 12:56

@tofumatt tofumatt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test here would be appreciated, but looks good to me.

@tofumatt

Copy link
Copy Markdown
Member

(Oh, never mind: you'll need to fix your snapshot test though; that test failure is legitimate.)

@Copons Copons force-pushed the update/download-attribute-valueless branch from 9f044df to 47d6bde Compare October 23, 2018 16:26
@Copons Copons merged commit f0831a1 into master Oct 23, 2018
@Copons Copons deleted the update/download-attribute-valueless branch October 23, 2018 16:56
@pento

pento commented Oct 24, 2018

Copy link
Copy Markdown
Member

This isn't going to work, KSES expects a valueless attribute to not have a value at all, not to have an empty value. (eg, <a download> is allowed, <a download=""> is not.)

@Copons

Copons commented Oct 24, 2018

Copy link
Copy Markdown
Contributor Author

@pento I'm not familiar with the serialization process of a block, but here for some reasons <a download> is stripped away on save/reload, while <a download=""> isn't (also see the inline comment in the previous version).

EDIT: scratch everything, this is why.

@youknowriad youknowriad modified the milestones: API Freeze, 4.2 Oct 24, 2018
antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018
@mtias mtias added the [Block] File Affects the File Block label Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] File Affects the File Block [Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants