Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#19389 closed defect (bug) (fixed)

Subsequent image insertion after captioned image causes breakage

Reported by: hew Owned by: azaozz
Milestone: 3.5 Priority: normal
Severity: normal Version: 2.7
Component: TinyMCE Keywords: needs-testing
Focuses: Cc:


Repro (trunk):

  1. Insert an image with a caption. The Visual Editor should now report a path like this:
    Path: div » dl.wp-caption alignnone » dd.wp-caption-dd
  1. Immediately click "Add Media" and insert another image again (don't move cursor or anything else) with another caption. The insertion should mangle the previous image and leave a path like this in the Visual Editor:
    Path: div » dl.wp-caption alignnone » dd.wp-caption-dd » div » dl.wp-caption alignnone » dd.wp-caption-dd

Bonus: Enter can be pressed in the Visual Editor after step #1. That inserts a new paragraph as expected with the Visual Editor reporting "Path: p". Toggle to the HTML editor and then back to the Visual Editor. The caption element is now active (Path: div » dl.wp-caption alignnone » dd.wp-caption-dd) and an insertion at this point will cause chaos (step #2).

Repro (3.2.1):
Same as trunk except that the "Bonus" (toggling to HTML and back to Visual) does not apply.

Expected behavior:
That a subsequent image insertion would be cleanly inserted without requiring manual cursor movement.

Attachments (2)

19389.patch (2.9 KB) - added by azaozz 3 years ago.
19389-2.patch (944 bytes) - added by azaozz 3 years ago.

Download all attachments as: .zip

Change History (18)

#1 @nacin
4 years ago

  • Version set to 3.2.1

Not much worse in trunk, setting to 3.2.1.

#3 @azaozz
4 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Version changed from 3.2.1 to 2.7

This has been around since image captions were introduced back in 2.7. The problem is that to place the caret in contentEditable mode the browsers need a block level element with some sort of content in it. FF has <br data-mce-bogus="1">, WebKit has U+00A0 (UTF for non-breaking space), etc. So for placing the caret we would need to insert something like <p><br data-mce-bogus="1"></p> or <p>u\00a0</p> after the caption which brings other problems when another caption is not inserted in the editor straight away.

The current behavior is a compromise. On the other hand we probably can re-examine the process of inserting a shortcode in TinyMCE and converting it to a bunch of DOM elements as the browsers have evolved a bit in that area since the last time.

#4 follow-up: @hew
4 years ago

I consider myself rather careful yet I inadvertently encountered this problem multiple times trying to arrange some screenshots in a post. It can be pretty painful as a user if the situation arises. The re-examination idea sounds neat.

#5 in reply to: ↑ 4 @azaozz
4 years ago

Replying to hew:

We can also make the whole image caption <dl> non-editable in the editor. That will prevent inserting anything where it doesn't belong but will also prevent editing the actual caption text which is possible at the moment. This had some problems when trying to drag the image before, seems to be working better now. Still needs some serious testing for all possible actions in all browsers though.

#6 @DrewAPicture
4 years ago

  • Cc xoodrew@… added

#8 @azaozz
3 years ago

  • Keywords needs-patch added
  • Milestone changed from Future Release to 3.5
  • Owner set to azaozz
  • Status changed from new to reviewing

Will make a patch that adds another <p> after each caption (as described in comment 3) so this can be tested in action.

Could also be fixed by looking at the caret position before inserting a caption. If it's already in a caption, can try to move it after.

3 years ago

#9 @azaozz
3 years ago

  • Keywords needs-testing added; needs-patch removed

#10 @azaozz
3 years ago

  • Milestone 3.5 deleted
  • Resolution set to duplicate
  • Status changed from reviewing to closed

This is handled in the new media workflow. Closing as a subset of #21390.

#11 follow-up: @nacin
3 years ago

  • Milestone set to 3.5

Worth pursuing?

#12 @nacin
3 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

#13 in reply to: ↑ 11 @azaozz
3 years ago

Replying to nacin:

Yes, been testing few things that would fix this and any 'send_to_editor()' while a captioned image is selected. Will have updated patch later today.

#14 @trepmal
3 years ago

#22457 was marked as a duplicate.

3 years ago

#15 @azaozz
3 years ago

19389-2.patch works well in all supported browsers. Couldn't find any side effects/interactions with other functionality too so thinking to commit it for further testing.

#16 @azaozz
3 years ago

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

In 22644:

TinyMCE: place the caret after/under images with captions when inserting content, fixes #19389

Note: See TracTickets for help on using tickets.