WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.34k stars 4.13k forks source link

Do not embed PDF automatically in file block #34825

Open sabernhardt opened 3 years ago

sabernhardt commented 3 years ago

Originally reported on a Fixing WordPress forum topic and Trac #54062

By default, the new embeddable PDF option (PR #30857) is toggled on, so people who do not want to embed their PDFs need to uncheck the toggle every time.

Show inline embed option is checked by default

Fixing this may be as simple as switching the displayPreview from true to false: https://github.com/WordPress/gutenberg/blob/8714f668dde8120068028f0975adac5fdaa8f30e/packages/block-library/src/file/edit.js#L125

sabernhardt commented 3 years ago

I'm hopeful this could be backported to the 5.8 branch.

For further consideration, though, could the toggle start with the user's choice from a previous PDF file block after the first?

karmatosed commented 3 years ago

I think having it remember the choice works, but I guess I would like to know really how many users want this option first. If it's more likely to be something people want embedded, then I'd say leave it on works - it also means it's discovered. If it is off it would need the discovery of the feature also.

sabernhardt commented 3 years ago

The discussion on #19077 gives a few reasons why people might choose not to embed a PDF file, including accessibility concerns and poor mobile support. Even users who like embedding a PDF probably would not want multiple embeds on the same page.

For discoverability, the inline embed option is already at the top of the Block settings sidebar panel. That makes it more findable than the Text link settings and the Download link option.

(also, it's the File block)

Ferellie commented 2 years ago

This is a major annoyance for myself and my collegues as we're often required to add many PDF documents at once. Further more we are constantly being asked by clients to make off by default as they too are getting frustrated.

I've been looking around to find possible solutions and every post I read explains the frustration this is causing. As mentioned by Sabernhardt, it's not like it's hard to see / find for those that do want to use it.

millab commented 2 years ago

I see this as an issue that should not rely on a per-user last-used preference. The problem I see is that I need to know that every user's experience is the same once I've created a template or page for them to update.

In my use case, I have a 5x5 table with approximately 20 PDF files linked. At the moment, every time a user updates a PDF in the table it changes that formatted link to an embed, which then requires the user to select the embed and then disable it again. They may need to edit other pages that do have an embed, so can't rely on last chosen setting. Ideally a file link would hold its link after replacement of a file rather than converting to an embed without warning

fabiankaegy commented 2 years ago

Looking at this I do think that it would be nice to have a way for theme authors to define wether they want to support the rich preview of files or not. There are many different usecases where it makes perfect sense to show the inline preview. But there also are many instances where from a design perspective it doesn't make sense. And being able to define that support in the settings of a theme.json file for example would be really cool.

I'm torn wether it should be just a boolean to enable / dissable support globally or wether it should give you more granular controll to choose wether the default for certain types should be true / false but still letting the end user make the final call.

{
    "settings": {
        "blocks": {
            "core/file": {
                 "enablePreview: false
            }
        }
    }
}

or

{
    "settings": {
        "blocks": {
            "core/file": {
                 "enablePreview": {
                     "default": false,
                     "pdf": true
                 }
            }
        }
    }
}

just as two examples for what a possible syntax could look like.

Pauxster commented 2 years ago

Looking at this I do think that it would be nice to have a way for theme authors to define wether they want to support the rich preview of files or not. There are many different usecases where it makes perfect sense to show the inline preview. But there also are many instances where from a design perspective it doesn't make sense. And being able to define that support in the settings of a theme.json file for example would be really cool.

I'm torn wether it should be just a boolean to enable / dissable support globally or wether it should give you more granular controll to choose wether the default for certain types should be true / false but still letting the end user make the final call.

{
    "settings": {
        "blocks": {
            "core/file": {
                 "enablePreview: false
            }
        }
    }
}

or

{
    "settings": {
        "blocks": {
            "core/file": {
                 "enablePreview": {
                     "default": false,
                     "pdf": true
                 }
            }
        }
    }
}

just as two examples for what a possible syntax could look like.

It didn't working for me

fabiankaegy commented 2 years ago

@Pauxster Sorry for the confusion here. My comment was a suggestion for the development team on how an API for this could look. But that has not been implemented and is not an actual option right now. Only a suggestion from my side.

fabianpimminger commented 2 years ago

+1 … there should be an option to completely disable this file preview.

matt-carboncoop commented 2 years ago

Auto-embedding PDFs is a huge step backwards in terms of accessibility, speed and cross platform compatibility. There were already a number of quality third party plugins that provided this functionality for those that needed it, so not sure it brings any benefit to counterbalance it's disadvantages.

hspot commented 2 years ago

The default to embed PDF is a major annoyance for our users. We definitely like to be able to default to having a link to the file.

mrwweb commented 2 years ago

Does anyone know if it's possible to change the default right now? I tried doing so with a registerBlockVariation like this:

/* DOES NOT WORK IN PRACTICE! */
wp.blocks.registerBlockVariation(
    'core/file',
    {
        isDefault: true,
        attributes: {
            displayPreview: false
        },
    }
);

This inserts a block with displayPreview set to false, but then that setting is changed when inserting a PDF.

mr-manuel commented 2 years ago

I don't know, if some one other have a better workaround, but in the meanwhile here is mine.

Create a cronjob with this command, which changes the necessary file (browser cache must be deleted after modification):

sed -i 's/url.endsWith(".pdf");/url.endsWith(".pdff");/' /path/to/wordpress/installation/wp-includes/js/dist/block-library.min.js

This is my cronjob entry, so that it searches 3 times a day for the string, in case an automatic update was done.

5 6,12,18 * * * su www-data -c "sed -i 's/url.endsWith(".pdf");/url.endsWith(".pdff");/' /path/to/wordpress/installation/wp-includes/js/dist/block-library.min.js"

I hope for a better solution by Wordpress.

Edit: Actually I'm using Wordpress 5.9.1

gregdev commented 2 years ago

It's for issues like this that people (in this case rightfully) blast Gutenberg. This is a major usability issue.

nebulousGirl commented 2 years ago

I also wish to disable to embed by default.

ncc1701v commented 2 years ago

Strongly in favor

sabernhardt commented 2 years ago

Using JSON may be better, but I made a PR for the simpler method.

jacqueschoquette commented 1 year ago

Plus one for this change. This is a major interruption to workflow. Has anyone figured out a temporary solution to change this default behaviour for now?

slingshotdesign commented 1 year ago

I'm also in favour of an option to switch this on or off by default. It is really confusing for content creators and at the moment everyone I know switches off the Show Inline Emded.

Was this feature implemented for accessibility reasons? Is it better for accessibility than a link to a pdf?

joedolson commented 1 year ago

This is definitely not for accessibility; the PDF previews are more likely to be harmful to accessibility.

Now that user settings can be stored in user meta (https://github.com/WordPress/gutenberg/pull/39795), getting the user choice to be remembered can be much more broadly applied, and this preference should definitely be registered there.

I'd still prefer the default state for new users to have the embed off; embedding a file preview is, in my opinion, the exceptional use, not the normative use.

nickvanballegooijen commented 1 year ago

Even when I re-upload a new PDF document for the same block, the previewer setting is enabled again. Very frustrating when you have to manage documents weekly.

Has somebody find a workaround like a plugin or something for php functions?

sabernhardt commented 1 year ago

I unassigned myself. If setting the default to false is sufficient, the PR could be merged. However, I do not have the expertise to work with user preferences.

emcgal commented 1 year ago

Just looking for something on the file block. 1) The preview isn't working at the moment using the Astra theme but a lot of native blocks are so I am assuming this is Astra.

2) However the download isn't happening for pdfs at the moment on mobile. On desktop, it is opening in another tab even though I have it turned off. Anyone experiencing similar issues as I haven't had these issues in the past.

Thanks

EarthmanWeb commented 12 months ago

I've run into this as well, and confirmed this...

Currently in the file block code in block-library.js the default is not set, thus it should default to false...

Except: in the case when a PDF is uploaded, which is when it is hardcoded to default to true, as noted in the OP's post:

displayPreview: isPdf ? true : undefined,

Expected behaviour, as per the Block Hooks guide in the Codex would state than a pdf upload should also adhere to a hook filter defining the behaviour for the displayPreview, allowing it to be set to true, OR false, as per the end user's preference.

// WordPress block hook to set core file block defaults
wp.hooks.addFilter(
  "blocks.registerBlockType",
  "sps/sps-set-core-file-block-defaults",
  function (settings, name) {
    if (name === "core/file") {
      console.log("loaded hook", name);
      console.log("loaded hook PRE", settings.attributes);

      let assign = lodash.assign({}, settings, {
        attributes: lodash.assign({}, settings.attributes, {
          // set 'show download button' default to false (this works)
          showDownloadButton: {
            type: "boolean",
            default: false,
          },
          // set 'Open in New Tab' to false (this works)
          textLinkTarget: {
            attribute: "target",
            selector: "a:not([download])",
            source: "attribute",
            type: "string",
            default: "_blank",
          },
          // set 'Show inline Embed' for PDF's to default to false (this *doesn't work)
          displayPreview: {
            type: "boolean",
            default: false,
          },
        }),
      });

      console.log("loaded hook POST", assign.attributes);
      return assign;
    }
    return settings;
  }
);

One can confirm by the console.log entries in the code above that the default is indeed set, however due to the code in block-library.js the hook value is overridden

While the PR suggested by @sabernhardt would help disable this functionality, it does not, as the PR title suggests, "make PDF embedding optional " - it disables it completely. That is likely why it has not been approved, but that's just a guess on my part.

Generally the goal in any core modification would be to preserve the existing behaviour, as much as possible, and then to also allow an override to provide a user-defined experience for the new behaviour. Currently there is no easy way I can think of to preserve the existing behaviour, which is:

Now, I don't think we would want to allow a user to change the default for other file types to 'true' because it doesn't work for any other file types except PDF's - in my testing, that just causes the file to be downloaded in the editor screen.

One could argue that embedding PDF's by default goes against accessibility standard, and I would agree, but rather than hardcoding it to never allow the embed, I think the option should be available still, for those users who prefer and have grown accustomed to this feature.

To meet the needs of those users, and to provide the ability to disable, I would suggest this simple modification to the PR provided, which would be to modify as follows:

displayPreview: isPdf && displayPreview ? true : undefined,

This, in my initial testing, does the following - it :

// WordPress block hook to set core file block defaults
wp.hooks.addFilter(
  "blocks.registerBlockType",
  "sps/sps-set-core-file-block-defaults",
  function (settings, name) {
    if (name === "core/file") {
      return lodash.assign({}, settings, {
        attributes: lodash.assign({}, settings.attributes, {
          // set 'Show inline Embed' for PDF's  to default to true (this works with the mod above)
          displayPreview: {
            type: "boolean",
            default: false,
          },
        }),
      });
    }
    return settings;
  }
);

I'll add these comments to the existing PR, in the hopes that it will be reviewed and accepted with this modification and rationale.

interfaceslivres commented 11 months ago

Any good news on this issue?

CdrMarks commented 2 months ago

I would like for the block to remember the embed choice when replacing the file.

Iliya5digital commented 1 week ago

I want too! It annoys me so much when I upload a large number of pdf files and when the upload of pdf files to the site is finished, it immediately starts loading the pdf file preview widget itself after uploading the pdf file to the site itself and I don’t need it, why the hell did I need your pdf file widget (it’s annoying for me).