WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#29476 closed enhancement (fixed)

Hide the 'Title' field on the 'Insert from URL' tab until it's needed

Reported by: johnbillion Owned by: helen
Milestone: 4.2 Priority: high
Severity: normal Version: 3.5
Component: Media Keywords: has-patch
Focuses: Cc:

Description

As helen mentioned in IRC, the 'Title' field on the 'Insert from URL' tab in the media manager should remain hidden until we've confirmed (via AJAX) that a URL can't be embedded.

Marking as 4.0. I'll let helen/nacin/MarkJaquith make the call.

Attachments (3)

29476.diff (1.0 KB) - added by johnbillion 5 years ago.
29476.2.diff (1.5 KB) - added by johnbillion 5 years ago.
29476.patch (2.8 KB) - added by iseulde 4 years ago.

Download all attachments as: .zip

Change History (24)

@johnbillion
5 years ago

@johnbillion
5 years ago

#1 @johnbillion
5 years ago

  • Keywords has-patch 2nd-opinion added

Oops, first patch missed off media-template.php.

29476.2.diff implements the behaviour described above and also changes the 'Title' heading to 'Link Text' as per helen's request. This is an existing string which is used on the nav menu screen when adding a link to a menu item.

Related: #29473 (with independent patch).

#2 @johnbillion
5 years ago

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

#3 @helen
5 years ago

  • Keywords commit added

In conjunction with #29473 (so that you actually see embeds when they work), this looks lovely.

#4 @helen
5 years ago

  • Keywords dev-feedback removed

#5 @helen
5 years ago

  • Keywords commit removed
  • Milestone changed from 4.0 to Future Release
  • Version changed from trunk to 3.5

It has been explained to me that media straddles a line between frontend and admin contexts for translators, and thus the string swap would pose a problem. So, punting from 4.0.

However, I still think this is something we should do, and go further with it: throttle the input (in the case of somebody manually typing something in, unlikely to be very common but of course still possible), show a message that no embeddable media was found, and perhaps change the button to say "Insert link".

#6 @wonderboymusic
5 years ago

  • Milestone changed from Future Release to 4.2

We can probably use most of this, so let's look at it

#7 @DrewAPicture
5 years ago

Patch needs to be refreshed (media-views.js doesn't exist anymore).

Also seems to be some unaddressed i18n/accessibility concerns with the interaction of toggling the title field in various contexts.

#8 @DrewAPicture
5 years ago

  • Keywords needs-refresh added

#9 @wonderboymusic
5 years ago

In 31632:

In the modal state for Embed previews, only show the Title field when the preview fails.

Props johnbillion, wonderboymusic.
See #29476.

#10 @helen
5 years ago

  • Owner set to helen
  • Resolution set to fixed
  • Status changed from new to closed

In 31642:

Media: UI tweaks for Insert from URL.

"Title" field is now labeled as "Link Text", to reflect how it's actually used. It's also hidden whenever the embed is updating, to make it clearer that something is happening. Embed dimension fields are shown below the preview, much like image property fields are shown below.

fixes #29476, see #31139.

#11 @iseulde
4 years ago

  • Focuses administration removed
  • Keywords has-patch needs-refresh removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The last commit breaks the link text. It is not inserted correctly to the editor.

@iseulde
4 years ago

#12 @iseulde
4 years ago

  • Keywords has-patch added

#13 @helen
4 years ago

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

In 32055:

Insert from URL: Make sure the link text is actually used.

Turns out there were more pieces to renaming the field.

props iseulde.
fixes #29476.

#14 @azaozz
4 years ago

  • Priority changed from normal to high
  • Resolution fixed deleted
  • Status changed from closed to reopened

This seems to have introduced a regression when editing an [embed] shortcode, see https://core.trac.wordpress.org/ticket/32006#comment:9.

Reverting [31642] and [32055] seems to fix it but needs more testing/looking into.

I'm not very sure what "Link text" has to do when inserting media. I know we fall back to inserting a link for types of media we cannot embed, but linking the file name seems better in these (rare) cases? The user will usually have to edit that anyway. Maybe removing the "Title" field completely would be best?

Last edited 4 years ago by azaozz (previous) (diff)

#15 @afercia
4 years ago

Noticed one small thing:

  • paste an embeddable URL
  • then empty the field
  • at this point url is an empty string
  • the AJAX call fires
  • the "Link Text" field appears

You will also see an error in your console:

<b>Notice</b>:  Undefined variable: url in <b>.../ajax-actions.php</b> on line <b>2749

that's because

if ( url && url.length < 6 ) {
  ...

should also check when url is falsey.

#16 @afercia
4 years ago

Also, not sure why 6 characters :) http:// is made by 7 characters but maybe I'm missing something.

#17 @helen
4 years ago

I don't really know why that "Title" field existed originally, I guess for a better fallback for non-embeddable URLs. I think we either give the user the ability to specify link text (and thus make it clear that it will insert a link), or else get rid of the field and insert just the URL, no link.

#18 @helen
4 years ago

In 32247:

Avoid a PHP notice when adding an embed via Insert from URL.

props iseulde.
see #32006, #29476.

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


4 years ago

#20 @helen
4 years ago

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

#32045 opened for the stuck view issue, going to leave this as fixed for now as it's hard to trace bugs like that against a ticket with a completely unrelated summary. If we need to do something against this ticket, it'll still be here. Any bigger changes such as removing the field entirely should likely be done in another release.

@afercia That is probably worth a separate ticket, if you didn't open one already (haven't read new tickets today).

#21 @afercia
4 years ago

@helen thanks, just opened #32059 for the empty url check

Note: See TracTickets for help on using tickets.