Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#45307 closed defect (bug) (fixed)

Can't undo auto-embeds

Reported by: afercia's profile afercia Owned by:
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.8
Component: TinyMCE Keywords:
Focuses: Cc:

Description

Seems that starting from WordPress 4.8 the "Undo" in TinyMCE is unable to undo an auto-embed.

I've tested it first on WordPress 4.7:

  • create a new post
  • write something
  • add an embed URL in its own line, tried with:
    • a YouTube embed
    • a WordPress embed
  • once the auto-embed happens, clicking the Undo button or pressing Cmd / Ctrl + Z removes the auto-embed

Then I've tested the same steps in WordPress 4.8:

  • clicking the Undo button or pressing Cmd / Ctrl + Z removes the embed but it gets immediately re-embedded
  • clicking Undo multiple times enters in an Undo / auto-embed loop

Not sure anything has changed in the auto-embeds between WordPress 4.7 and 4.8, seems to me more related to TinyMCE. For what is worth, it happens also in Gutenberg.

Attachments (3)

tweet auto embed.gif (389.7 KB) - added by afercia 5 years ago.
ff1.gif (660.6 KB) - added by afercia 5 years ago.
ff2.gif (770.1 KB) - added by afercia 5 years ago.

Download all attachments as: .zip

Change History (13)

#1 @azaozz
5 years ago

Undo seems to be working properly (again) in WP 5.2. Tested in Firefox and Chromium on Win10. @afercia could you confirm please :)

#2 @afercia
5 years ago

@azaozz I can still reproduce on trunk and 5.2.2, macOS, all browsers :) See attached animated GIF where I'm first using Cmd + Z and then the Undo button. Happens with all embed types, also with Gutenberg. Will test on Windows 10 later.

#3 @afercia
5 years ago

Update: tested also on Windows and can still reproduce.

#4 @azaozz
5 years ago

  • Milestone changed from Awaiting Review to Future Release

Ah, I see, the wpview has to be the first node in the editor. Then I can reproduce it too, every time. If it is not the first node, undo works as expected until you switch to the Text tab. Then it may start failing too (seems related to where the cursor is when switching).

Trying to figure out what's going on :)

#5 @azaozz
5 years ago

  • Milestone changed from Future Release to 5.3

Think I found it. The way undo levels are stored in the editor has changed quite a bit so we are processing some undo levels but not others.

Patch coming up.

#6 @azaozz
5 years ago

In 45631:

TinyMCE: fix adding of too many undo levels for wpviews. The HTML changes several times when a wpview is added. We only want one undo level. Also fixes cases when the cursor is next to an embeddable URL in the Text tab and the user switches to the Visual tab.

See #45307.

#7 @azaozz
5 years ago

Leaving open for now in case there are any (edge case) regressions.

#8 @desrosj
5 years ago

@afercia are you able to confirm that [45631] fixes the issue for you?

#9 follow-up: @afercia
5 years ago

I'm not sure it's fully fixed.

A single auto-embed followed by an Undo seems to work. However, then going back/forth through Undo/Redo multiple times, things get a bit weird. Seems to me at any unrelated Undo (e.g. removal of text), the embed auto-embeds again thus creating messing up the Undo/Redo history?

See GIf below, latest Firefox on macOS (similar results with Chrome).

@afercia
5 years ago

@afercia
5 years ago

#10 in reply to: ↑ 9 @azaozz
5 years ago

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

Replying to afercia:

A single auto-embed followed by an Undo seems to work. However, then going back/forth through Undo/Redo multiple times, things get a bit weird.

My guess is that when going back and forth through undo and redo fast, sometimes the embedding/wpview doesn't run fully. It is quite complex and has many "built-in" delays and timeouts. When that happens is creates a partial undo "level" which introduces the edge case.

As far as I see doing undo and redo "at a normal pace", mixed with other editing actions not just adding wpviews works as expected.

Thinking this is "as good as it gets" for now. To try to remove the remaining edge case we'll need to refactor how wpview works. That's lots of changes and out of scope here :)

Closing this as fixed for now. Lets make a new ticket with exact steps to consistently reproduce if it still happens a lot.

Last edited 5 years ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.