Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#41259 closed defect (bug) (fixed)

Editing an image link in the Editor produces one additional, empty link

Reported by: afercia's profile afercia Owned by: azaozz's profile azaozz
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.8
Component: Editor Keywords: has-screenshots has-patch needs-testing
Focuses: Cc:

Description

I can reproduce this consistently in WordPress 4.8 but only using Chrome (v. 59) or Safari (10) and only if an image is the first thing in the post content. To reproduce:

  • insert an image as first thing in the content
  • when inserting the image, link it to the "Attachment Page"
  • save the post
  • on the front-end, inspect the HTML: everything looks fine
  • edit the post and update the image link to, for example, "Media File" or set a "Custom URL"
  • save the post
  • inspect the HTML in the front end: the previous link to the "Attachment Page" is still there (before the image) and it's empty

Seems this happens whatever link is set. Tested on two different machines, macOS and Windows. Couldn't reproduce using Firefox, so seems something specific to webkit browsers. Couldn't reproduce in WordPress 4.7.

Note: you can't see the doubled, empty link switching the Editor to "Text mode" because TinyMCE will strip it out. Not sure why the empty link doesn't get removed when saving the post.

Screenshots:

https://cldup.com/-fHYEoCUPd.png

Happens also with an image after another image:

https://cldup.com/0FKdDgTHwr.png

On Windows:

https://cldup.com/7SR2nTCIJK.png

Attachments (1)

41259.patch (535 bytes) - added by azaozz 8 years ago.

Download all attachments as: .zip

Change History (17)

#1 @afercia
8 years ago

  • Milestone changed from Awaiting Review to 4.8.1

Moving to 4.8.1 for consideration, as it seems a regression from 4.7.

#2 @jbpaul17
8 years ago

  • Keywords needs-patch added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

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


8 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

#6 @xkon
8 years ago

I've been doing some tests trying to tackle this but without getting any results as I don't even know where to start, the only files I didn't go through was the tinyMCE ones.

Extra information:

I've found out that there's always an extra <a></a> even when using the Link button from the editor ( not editing with the Image Editor ), if you do it while on Visual tab and just change to Text you will always see an extra empty <a> that is always the 'last' used. So the extra <a> gets generated either way you try to edit the 'current' link.

Apparently when you are trying to change the link instead of getting replaced it is like creating a new one and moving the image in there, thus leaving the old one empty without an <img> tag in it. Some times when you hit the Update button it gets removed but sometimes it doesn't ( usually it does not ).

I don't know if this is a tinyMCE thing or there's somewhere an extra function when we Update the posts / pages that doesn't get triggered always.

Hopefully this can help a bit to get this fixed.

Best regards,
Konstantinos

Last edited 8 years ago by xkon (previous) (diff)

#7 @spocke
8 years ago

The bug here seems to be in the hasTextContent function in /wp-includes/js/tinymce/plugins/wpeditimage/plugin.js that function checks if the link has any text contents however since the introduction of the inline boundaries for links there is a character in there to place the caret the zwnbsp char or \ufeff so the function needs to trim those. So the function should be something like:

function hasTextContent( node ) {
  return node && !! ( node.textContent || node.innerText ).replace(/\ufeff/g, '');
}

#8 @xkon
8 years ago

Thank you @spocke although I don't know if anything changed but I haven't been able to reproduce this bug again since I pulled the latest dev svn yesterday night. I don't know if anything changed in general or maybe this was a bug combined with another that got fixed. I've been doing image updates and new posts for the past 5 mins + plus I'm not getting second links anymore they are getting stripped out as soon as I update the post. /cc @afercia ( maybe you could retest this also as you've originally found how to reproduce it? )

Best regards,
Konstantinos

#9 @afercia
8 years ago

I can still reproduce this just following the steps described in the ticket description. /cc @xkon

#10 @xkon
8 years ago

Sorry @afercia you are correct I had messed up files. Same file as spocke says plugin.js arround 410 line we have :

if ( linkNode = dom.getParent( imageNode, 'a' ) ) {

This is what is causing the extra <a> to appear as it moves the image out of any link that has extra content. I've just tested it with console.logs and that's what triggers every time the empty <a> is created.

I'll play around with that when I get home in case I find a solution. I'll also check spockes code asap.


Editing post, just to confirm that Spockes fix works on my end.

Best regards,
Konstantinos

Last edited 8 years ago by xkon (previous) (diff)

#11 @azaozz
8 years ago

Yeah, can reproduce it too. Spocke's patch fixes it here.

@azaozz
8 years ago

#12 @azaozz
8 years ago

  • Keywords needs-patch removed

In 41259.patch: fix hasTextContent() to remove the link padding chars before testing for text (props spocke).

As this bug is triggered very rarely, maybe we can punt it to 4.9. Not sure if it's worth it updating wp-editor.js.gz just for it.

#13 @azaozz
8 years ago

  • Keywords has-patch needs-testing added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

#15 @jbpaul17
8 years ago

  • Milestone changed from 4.8.1 to 4.9

Punting per today's bug scrub in #core.

#16 @azaozz
8 years ago

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

In 41319:

TinyMCE: fix cases where an additional empty link is created while updating an image with a link.

Props spocke.
Fixes #41259.

Note: See TracTickets for help on using tickets.