WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#31158 closed defect (bug) (fixed)

oEmbeds don't work if text is formatted

Reported by: siobhan Owned by: azaozz
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Embeds Keywords: has-patch
Focuses: javascript Cc:

Description

I was teaching a class on WordPress a few weeks ago and I showed the students how to use oEmbeds. A number of the students were frustrated because they couldn't get their oEmbeds to work. When I went to help them out we realised it was because they were pasting it into the text in a place where formatting what applied. IF an oEmbed link is wrapped in a <strong> tag, for example, it doesn't parse. This can be very confusing to users.

Example:

Back end

https://cldup.com/BoqjH9N9E1.png

Front end

https://cldup.com/dHjdNbp0Bc.png

Attachments (4)

31158.patch (1.2 KB) - added by iseulde 5 years ago.
31158.2.patch (1.4 KB) - added by iseulde 5 years ago.
31158.3.patch (2.0 KB) - added by azaozz 5 years ago.
31158.4.patch (1.2 KB) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (24)

#1 @siobhan
6 years ago

  • Summary changed from OEmbeds don't work if text is formatted to oEmbeds don't work if text is formatted

#2 follow-up: @danielbachhuber
6 years ago

oEmbed is tough because the links essentially have to be perfectly formatted for it to work.

Idea: When TinyMCE thinks it could be an embed, but isn't sure, we could have a little tooltip that says "did you mean to embed this?"

#3 in reply to: ↑ 2 @siobhan
6 years ago

Replying to danielbachhuber:

oEmbed is tough because the links essentially have to be perfectly formatted for it to work.

The problem is that users don't know that they have to be perfectly formatted, so they think that there's something wrong with WordPress.

Idea: When TinyMCE thinks it could be an embed, but isn't sure, we could have a little tooltip that says "did you mean to embed this?"

Something like that could work - at least a way to alert the user that they're doing something wrong and that they should correct it.

#4 @helen
6 years ago

Sometimes I think we'd be better off being more liberal about oEmbeds, letting them display if they're within other formatting and then providing a way to not show that embed if desired (which would wrap it in a span with a special class or something).

#6 follow-up: @danielbachhuber
6 years ago

If we want to make some changes, I'd be happy to test against our users and see where / when they complain.

@siobhan have something specific to suggest ?

#7 in reply to: ↑ 6 @siobhan
5 years ago

Replying to danielbachhuber:

If we want to make some changes, I'd be happy to test against our users and see where / when they complain.

@siobhan have something specific to suggest ?

As Helen said, I think it would be good to be more liberal with oEmbeds. Users don't know about any coding issues related to displaying embeds within formatted text. They just want to dump a video in the middle of a paragraph that's edited bold.

@danielbachhuber - it'd be interesting to see how users react to such a change. I'm not sure if they would even notice - it's more of a measure to prevent frustration rather than being an enhancement.

#8 @helen
5 years ago

Related: #25387

I could see this potentially working: if all you're left with is a URL after stripping all tags from the paste, strip those tags and turn it into an oEmbed. Provide some kind of button or other UI for "don't embed this URL" (I think this might be the tricky part), which would then wrap in an element or perhaps restore the original paste.

#9 @iseulde
5 years ago

  • Milestone changed from Awaiting Review to 4.2

#10 @DrewAPicture
5 years ago

  • Keywords needs-patch added

If we're still planning to try and fix this issue in 4.2, we're going to need a patch in the next few days. Otherwise, please mark it for future release.

@iseulde
5 years ago

@iseulde
5 years ago

#11 @iseulde
5 years ago

  • Focuses javascript added
  • Keywords has-patch added; needs-patch removed

#12 @azaozz
5 years ago

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

In 31817:

TinyMCE: strip tags from pasted URLs before testing if they are embeddable.
Props siobhan, iseulde. Fixes #31158.

#13 @azaozz
5 years ago

In 31819:

TinyMCE: small cleanup for stripping of tags from pasted URLs. See #31158.

@azaozz
5 years ago

#14 @azaozz
5 years ago

@wonderboymusic not sure what is broken, embeds seem to work here.

Just in case going to revert the changes and add cleanup for the pasted URL only. Will look again after beta2.

#15 @azaozz
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#16 @azaozz
5 years ago

In 31832:

TinyMCE: revert stripping of tags from pasted URLs on beforeSetContent [31817] and [31819]. Clean up URLs on pastePreProcess.
See #31158.

#17 @wonderboymusic
5 years ago

[31832] fixed everything

#18 @iseulde
5 years ago

@siobhan: Is the problem pasting a url that already has some formatting from somewhere else, or pasting the url in an empty paragraph but already bold/... Or both? In other words is the pasted link bold, or is the paragraph before you paste bold?

@azaozz
5 years ago

#19 @azaozz
5 years ago

In 31158.4.patch:

  • Improve the test if the paragraph doesn't contain text.
  • Strip all inline tags before adding the URL (that is eventually converted to wpView).

Seems to work well in all major browsers. Will catch cases where the users have made a new paragraph, then clicked on the B or I buttons, etc.

One "side effect": when pasting an URL that is not embeddable and was copied from another web page, and was bold or italic, it is converted to plain text, the bold/italic is removed. As that is very rare, thinking it shouldn't prevent this fix.

#20 @azaozz
5 years ago

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

In 31856:

TinyMCE: when pasting an URL, check if the node it is pasted at is empty and remove any empty inline child elements.
Fixes #31158.

Note: See TracTickets for help on using tickets.