WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#29841 closed defect (bug) (fixed)

Don't replace non embeddable URLs with a loading placeholder

Reported by: iseulde Owned by: iseulde
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.0
Component: TinyMCE Keywords:
Focuses: javascript Cc:

Description

Continuing from #29268.

This is visually annoying.
Also, the cursor is set back to the start of the url.

Attachments (9)

29841.patch (4.5 KB) - added by iseulde 6 years ago.
29841.2.patch (5.0 KB) - added by iseulde 6 years ago.
29841.3.patch (25.7 KB) - added by iseulde 6 years ago.
29841.4.patch (36.0 KB) - added by iseulde 6 years ago.
29841.5.patch (37.4 KB) - added by iseulde 6 years ago.
29841.6.patch (41.8 KB) - added by iseulde 6 years ago.
29841.7.patch (42.5 KB) - added by iseulde 6 years ago.
29841.8.patch (42.1 KB) - added by iseulde 6 years ago.
29841.9.patch (41.9 KB) - added by iseulde 6 years ago.

Download all attachments as: .zip

Change History (18)

#1 @iseulde
6 years ago

  • Owner set to avryl
  • Status changed from new to accepted

#2 @SergeyBiryukov
6 years ago

Duplicate of #29686?

#3 @iseulde
6 years ago

Oh, I thought that was about urls still embedding in the editor when oembed is disabled.

#4 @helen
6 years ago

I think we can keep the two tickets separate - the other for if you want to turn off oEmbed (although perhaps there's a better/different way to accomplish that?), and this for this issue that was noted pre-release but not addressed. Not sure what our options are - whitelisting in JS, a la wpautop? Something else?

#5 @iseulde
6 years ago

Nah, we just shouldn't use a loading placeholder. Only replace the URL when there's something to replace it with. We'll need to change the way wpviews load though.

@iseulde
6 years ago

#6 @iseulde
6 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Future Release to 4.2

Initially I wanted to approach this differently by not setting a placeholder, but that wouldn't be ideal for shortcodes (ideally the user should never see the shortcode).

Instead it would be better check the url with the regexes of the providers. So this patch converts them for JS and adds an additional check before replacing with a loading placeholder. I'm not a regex expert, so this should definitely be checked/tested.

This patch will also set the cursor to the end of the view if the cursor is within the view so you can continue typing.

#7 @azaozz
6 years ago

Don't think we can reuse the same regex as in PHP. There are quite a few syntax differences between PCRE and JS. It's true we can keep the default regex compatible, but plugins can add there too. Trying to convert from PCRE to JS syntax is no fun.

Thinking we should go with the first option: having another arg on wp.mce.views.toViews() to indicate that we don't want a loading placeholder while waiting for the response. This would be a good addition that may be useful in other contexts too.

We can get this working in couple of ways: keep a reference to the content of the <p> node in the TinyMCE plugin, fire an event when the server sends the embed code, check if the node content/text hasn't changed, then either add the whole wpView or leave it as-is if no match. Or we can match that node to a particular ajax request and replace on completion.

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

#8 @iseulde
6 years ago

  • Keywords has-patch needs-testing removed

I think we have to fix #29526 first.

@iseulde
6 years ago

@iseulde
6 years ago

@iseulde
6 years ago

@iseulde
6 years ago

@iseulde
6 years ago

@iseulde
6 years ago

@iseulde
6 years ago

@iseulde
6 years ago

#9 @iseulde
6 years ago

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

No more annoying loaders! :)
See #31412 and [31546].

Note: See TracTickets for help on using tickets.