Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#35153 closed defect (bug) (fixed)

Default link target for media files is none

Reported by: clorith's profile Clorith Owned by: azaozz's profile azaozz
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch fixed-major
Focuses: ui Cc:

Description

in r33729 the default link target for images was changed to None, which is all fine and dandy, but this also affects other media elements such as documents, PDF files and so forth.

Normal files should still link to the media file as default, as it's not helpful to the users to just have a file name inserted when they are inserting an actual file.

Attachments (5)

35153.diff (1.1 KB) - added by eherman24 8 years ago.
Set default 'Link To' parameter to 'Attachment Page' for certain attachment mime types
35153-2.diff (1.1 KB) - added by eherman24 8 years ago.
Set default 'Link To' parameter to 'Media File' for certain attachment mime types
35153-3.diff (858 bytes) - added by eherman24 8 years ago.
Media file whos mime types != image/* default link to 'Media File'
35153-4.diff (903 bytes) - added by eherman24 8 years ago.
Regenerated patch from root checkout
35153.5.diff (2.4 KB) - added by azaozz 8 years ago.

Download all attachments as: .zip

Change History (26)

#1 @Clorith
8 years ago

  • Summary changed from Defautl link target for media files is none to Default link target for media files is none

#2 @SergeyBiryukov
8 years ago

  • Milestone changed from Awaiting Review to 4.4.1

#3 @SergeyBiryukov
8 years ago

Normal files should still link to the media file as default, as it's not helpful to the users to just have a file name inserted when they are inserting an actual file.

Seeing similar reports on support forums as well.

Last edited 8 years ago by SergeyBiryukov (previous) (diff)

#4 @jorbin
8 years ago

  • Owner set to wonderboymusic
  • Status changed from new to assigned

@liljimmi, @janhenckens, @eherman24, @wonderboymusic - This seems to have been introduced in [33729] which you all worked on, can you please take a look.

@eherman24
8 years ago

Set default 'Link To' parameter to 'Attachment Page' for certain attachment mime types

@eherman24
8 years ago

Set default 'Link To' parameter to 'Media File' for certain attachment mime types

#5 @eherman24
8 years ago

  • Keywords has-patch 2nd-opinion added

I've attached a first patch here. Sorry for posting two patches. The first one was linking to the attachment page, where it should have been linking to the actual media file itself.

I've gone ahead and added a check when a media element is selected in the media library. If the selected media file's mime type is found in the array, the 'Link To' for the element sets to 'file' (Media File).

Some additional feedback is needed, as I've defined a new array of default mime types to set the 'Link To' parameter to 'File'.

var mimeTypeDefaultLinkToAttachmentPage = [
   'pdf',
   'csv',
   'zip',
   'gz|gzip',
   'rar',
   '7z',
   'doc',
   'docx',
];

I got a list of acceptable mime types from the codex article get_allowed_mime_types() (https://codex.wordpress.org/Function_Reference/get_allowed_mime_types), under 'Default allowed mime types'.

I'm sure additional mime types should be added to this list, but I'm not entirely sure which is best to have on this list and which is best left off.

#6 @Clorith
8 years ago

I'm thinking it would be safer to reverse that check?

If the mime is image/* then link to None, but if it isn't, link to the media file, as you are more likely to have an infinite amount of file variations than you are image types.

@eherman24
8 years ago

Media file whos mime types != image/* default link to 'Media File'

#7 follow-up: @eherman24
8 years ago

@Clorith Yea, I think that does make a bit more sense. Moving forward it's probably best not to have an array of file types to maintain.

The new patch checks the mime type and for anything not matching 'image/' will default to 'file' - or 'Media File' in the media library 'Link To' dropdown.

#8 in reply to: ↑ 7 @janhenckens
8 years ago

@Clorith @eherman24 Yeah, only setting it to none for images is a saner approch. Tested the patch against 4.4, looks good.

Do we want to revert anything from #31467 or can that stay in? (the goal there was to not link images by default and we ended up not linking anything by default)

#9 follow-up: @jorbin
8 years ago

I'm torn on this since it sounds like the image_default_link_type option isn't going to be respected. Perhaps there needs to be a second option for other uploads?

@eherman24 thanks for the patch. In the future, please try to generate the patch from the root of your checkout. Can you refresh it please?

@melchoyce would love your UX thoughts here since you were a champion of the original change.

@eherman24
8 years ago

Regenerated patch from root checkout

#10 @eherman24
8 years ago

@jorbin Sorry about that. It's been a little bit since I created a patch. I've gone ahead and regenerated the patch from the root of my install.

#11 @SergeyBiryukov
8 years ago

#35247 was marked as a duplicate.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


8 years ago

#13 @jorbin
8 years ago

  • Owner changed from wonderboymusic to azaozz

#14 in reply to: ↑ 9 @azaozz
8 years ago

Replying to jorbin:

Perhaps there needs to be a second option for other uploads?

Yeah, it's logical to have different link options for images and other uploads. The default workflow is different:

  • Images are better not wrapped in a link, see #31467.
  • All other files should link to the file (the old behaviour), or optionally to the attachment page.

In addition this is affected by letting the user settings override the options, see #35101. Even if we change the default in image_default_link_type, the user can set it to none and still trigger the above error when inserting attachments that are not images.

For 4.4.1 we need to ensure the default cannot be none for non-image attachments. This should still respect the image_default_link_type option when changed by a plugin.

@azaozz
8 years ago

#15 @azaozz
8 years ago

  • Keywords needs-testing dev-feedback added; 2nd-opinion removed

In 35153.5.diff:

  • Check if an attachment is image and if not ensure the link is set to something else than 'none'.
  • Introduces wp.media.controller.Library.isImageAttachment().

While testing this discovered another bug: changing the settings object in defaultDisplaySettings() changes this._defaultDisplaySettings as it is a reference to it. So clicking on "embeddable" attachment sets this._defaultDisplaySettings.link = 'embed' which is used for all other attachments that are clicked after that. Had to clone this._defaultDisplaySettings to keep it unchanged.

This ticket was mentioned in Slack in #core by azaozz. View the logs.


8 years ago

#17 @nacin
8 years ago

LGTM, @azaozz.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


8 years ago

#19 @azaozz
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

Media: when inserting an attachment in the editor and it is not an image, ensure the link is set to something else than none.

Props eherman24, azaozz.
Fizes #35153 for trunk.

#20 @azaozz
8 years ago

  • Keywords fixed-major added; needs-testing dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 4.4.

#21 @azaozz
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 36167:

Media: when inserting an attachment in the editor and it is not an image, ensure the link is set to something else than none.

Props eherman24, azaozz.
Fixes #35153 for 4.4.1.

Note: See TracTickets for help on using tickets.