WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 4 months ago

Last modified 2 months ago

#38511 closed defect (bug) (fixed)

Copy paste a long shortcode wpview (video, gallery... ) is broken in Firefox

Reported by: bduclos Owned by: iseulde
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.6.1
Component: TinyMCE Keywords: has-patch
Focuses: Cc:

Description

Steps to reproduce:

  • In firefox, copy the following code
    [video width="640" height="480" mp4="https://videos.files.wordpress.com/GbdhpGF3/video-25313e7f01_hd.mp4" poster="https://i2.wp.com/videos.files.wordpress.com/GbdhpGF3/video-25313e7f01_scruberthumbnail_0.jpg"][/video]
    
  • In Visual Mode, select the video view, copy it (Ctrl+c) and paste it (Ctrl+v) below. Switch to Text mode. Multiple newlines are added which results in the shortcode being broken on both back-end and front-end.
    [video width="640" height="480" 
    mp4="https://videos.files.wordpress.com/GbdhpGF3/video-25313e7f01_hd.mp4"
    
    poster="https://i2.wp.com/videos.files.wordpress.com/GbdhpGF3/video-25313e7f01_scruberthumbnail_0.jpg"][/video]
    

The same happens with the gallery shortcode or any wpview shortcode where the shortcode text is a bit long.
(e.g. [gallery link="file" columns="5" size="large" ids="495,499,498,533,664,638,577,575,497,773,943,670,668,496,494,487,479,673,675,676" orderby="rand"] )

http://i.imgur.com/mpY1xhN.gif

Attachments (10)

38511.patch (454 bytes) - added by iseulde 4 months ago.
38511.2.patch (686 bytes) - added by iseulde 4 months ago.
38511.3.patch (457 bytes) - added by iseulde 4 months ago.
38511.4.patch (1.4 KB) - added by gitlost 4 months ago.
Remove newlines in pastepreprocess and workaround refreshContentEditable.
38511.5.patch (7.5 KB) - added by iseulde 4 months ago.
38511.6.patch (8.2 KB) - added by iseulde 4 months ago.
38511.7.patch (8.7 KB) - added by azaozz 4 months ago.
38511.8.patch (8.6 KB) - added by iseuldebot 4 months ago.
Compare
38511.10.patch (797 bytes) - added by iseuldebot 4 months ago.
38511.11.patch (871 bytes) - added by azaozz 4 months ago.

Download all attachments as: .zip

Change History (36)

This ticket was mentioned in Slack in #core-editor by iseulde. View the logs.


4 months ago

#2 @azaozz
4 months ago

Can't reproduce in Firefox on Win10 (or any other browser). Tested with the above example in trunk and 4.6. Looks like "something" is adding line breaks before shortcode attributes on purpose. Could this be caused by a plugin? Or perhaps there were some "invisible" characters in the shortcodes when they were pasted in the editor for the first time that aren't in the example above?

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

#3 @bduclos
4 months ago

On both a fresh WP trunk install and a fresh WP 4.6.1 install (no plugins), on Win10 and Firefox latest version (49.0.2), retested with the 2 following methods but still having the same issue:

  • Paste the following video shortcode in Text mode and copy paste the wpview in Visual mode.
[video width="640" height="480" mp4="https://videos.files.wordpress.com/GbdhpGF3/video-25313e7f01_hd.mp4" poster="https://i2.wp.com/videos.files.wordpress.com/GbdhpGF3/video-25313e7f01_scruberthumbnail_0.jpg"][/video]
  • In Visual Mode, click on Add Media, create a gallery with a lot of images and set its settings (Link To, Columns, Random order, Size) different than the default ones in order to generate a long shortcode string. Insert the gallery, copy and paste the wpview.

http://i.imgur.com/hqB1T7E.gif

#4 @iseulde
4 months ago

I can reproduce this now in Firefox. It's breaking the line at certain points such as space, dot, dash etc.

#5 @iseulde
4 months ago

Actually I can't reproduce entirely. Just after switching to text and back the blocks are blank.

#6 @lukecavanagh
4 months ago

@bduclos

I can reproduce a similar issue in Firefox Developer 49.0a2 using WP 4.6.1.

@iseulde
4 months ago

@iseulde
4 months ago

#7 @iseulde
4 months ago

  • Milestone changed from Awaiting Review to Future Release

This is a really strange issue... Scripts are run at first (so the contents set), but then shortly afterwards the whole iframe unloads. Try the patch above and you'll see it "test"in the console, then "unload". I've been looking for hours but couldn't find the cause so far.

#8 @iseulde
4 months ago

I found a link! If editor scrolling is disabled, all works well.

@iseulde
4 months ago

This ticket was mentioned in Slack in #core-editor by iseulde. View the logs.


4 months ago

#10 @gitlost
4 months ago

This is a really strange issue... Scripts are run at first (so the contents set), but then shortly afterwards the whole iframe unloads.

This is caused by the quirks routine refreshContentEditable() which (Firefox-only) reloads the body if there's no selection. Calling this early (so that later calls are a noop) is one way to workaround this issue.

The OP is caused by Firefox prettyprinting text/html into the clipboard, wrapping long lines by inserting newlines. Shortcodes with attributes then get wpautoped causing them not to parse. The following patch uses the existing pastepreprocess action in "wpviews/plugin.js" to remove them.

@gitlost
4 months ago

Remove newlines in pastepreprocess and workaround refreshContentEditable.

#11 @iseulde
4 months ago

@gitlost, that looks right.

Test code (see https://github.com/tinymce/tinymce/blob/master/js/tinymce/classes/util/Quirks.js):

(function() {
  var body, parent;
  body = tinyMCE.activeEditor.getBody();
  parent = body.parentNode;
  parent.removeChild(body);
  parent.appendChild(body);
  body.focus();
})()

If you run this the iframes will be empty.

#12 @iseulde
4 months ago

I'd prefer fixing this so that whenever the whole content reloads by something, the iframes contents are reloaded as well.

We can still trigger the quirks function beforehand though to prevent excessive reloading. Instead of running editor.quirks.refreshContentEditable() we can run editor.focus() which runs the former internally.

@iseulde
4 months ago

#13 @iseulde
4 months ago

Added a new patch that also removes the timeout that was once added to fix this issue. See [29513].

Last edited 4 months ago by iseulde (previous) (diff)

@iseulde
4 months ago

#14 follow-up: @iseulde
4 months ago

Finding more bugs and mistakes while doing this.

Patch 6:

  • Set focus before rendering.
  • Empty views on hide. Missed in #36434.
  • Fix argument in wp.mce.views.render.
  • Remove timeout added in [29513].
  • Rerender views if they are unloaded.

@azaozz
4 months ago

#15 in reply to: ↑ 14 @azaozz
4 months ago

  • Keywords has-patch commit added

Replying to iseulde:

38511.6.patch looks good. In 38511.7.patch also added the removal of line breaks from shortcodes from 38511.4.patch (props @gitlost). Moved it to createInstance() as discussed in Slack, but it makes sense to try to fix that in Firefox for all pasted content. Not sure if we should do that, probably better to be handled upstream in the paste plugin.

Also noticing (non-fatal) mediaelement.js errors in Firefox and IE11 when switching to the Text editor while a movie is playing. Seems pausing the player and removal of player instances fails in these browsers. This is not caused by the above patch.

#16 @iseulde
4 months ago

  • Milestone changed from Future Release to 4.7
  • Owner set to iseulde
  • Status changed from new to reviewing

#17 @bduclos
4 months ago

Other bug: when you copy paste a view, the toolbar with the edit/delete buttons appears above the pasted view while the view is not selected (clicking on the buttons does nothing). Bug on all browsers.

#18 @iseulde
4 months ago

Normally we'll never have more than one shortcode in createInstance, because they are already matched, so I'm not sure if the extra loop is necessary. Also, I don't think it would be bad to disallow line breaks in general in these view texts as it will lead to nothing but problems.

Ideally this is handled by mce indeed, but it doesn't seem like we can wait for that. Let's fix it like this for now.

#19 @iseulde
4 months ago

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

In 39282:

TinyMCE views: fix Firefox issues.

  • Set focus before rendering to prevent reload in Firefox.
  • Rerender views if they are unloaded.
  • Remove timeout added in [29513].
  • Fix argument in wp.mce.views.render.
  • Empty views on hide. Missed in #36434.

Props gitlost, azaozz, iseulde.
Fixes #38511.

#20 follow-up: @iseulde
4 months ago

@bduclos Could you create a now ticket for the other bug?

#21 in reply to: ↑ 20 @bduclos
4 months ago

Replying to iseulde:

@bduclos Could you create a now ticket for the other bug?

#38849

Last edited 2 months ago by SergeyBiryukov (previous) (diff)

#22 @iseulde
4 months ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The commit here causes the page to automatically scroll down when it loads.

#23 follow-up: @iseulde
4 months ago

Patch only sets focus when new content is loaded in the editor, and it's not the initial load. That happens when switching from text to visual.

#24 @iseulde
4 months ago

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

In 39299:

TinyMCE: fix automatic scroll on page load.

Set focus when new content is loaded in the editor, but not when it's the initial load.

Fixes #38511.

#25 in reply to: ↑ 23 @azaozz
4 months ago

Replying to iseulde:

Yeah, we've had problems with calling editor.focus() before. It behaves quite differently in different browsers when called on load or during load.

How about we go back to editor.quirks.refreshContentEditable();? I know it used to be a private function, but seems it is not any more (the leading underscore was removed): https://github.com/tinymce/tinymce/commit/b4576a4c1f96801241c710ad51dfacc605d24e1f.

It is better for this as it only runs in Firefox and only when needed/there is no selection.

@azaozz
4 months ago

#26 @azaozz
4 months ago

In 39334:

TinyMCE: avoid calling editor.focus() on loading the content in the editor. It may trigger scroll-into-view in the browser. Call the quirks fix in TinyMCE directly.

Props gitlost.
Fixes #38511.

Note: See TracTickets for help on using tickets.