Make WordPress Core

Opened 22 months ago

Closed 21 months ago

Last modified 21 months ago

#35630 closed enhancement (fixed)

oEmbed Link Handling Can't Be Refired

Reported by: JamesDiGioia Owned by: swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch commit
Focuses: Cc:


On page load, the oEmbed script adds a click handler to anchor tags to postMessage to the parent window to handle and redirect the user. I have two questions/issues about this.

First, i'm doing syntax highlighting in the iframe, and part of that is to add (if the user is logged in) a link to the edit the code on the backend. Because the initial oEmbed script has already run by the time this happens, I have to manually add that handler myself, which results in some code duplication between my plugin and WordPress. Is it possible to mitigate this issue by making the mechanism available outside of the IIFE scope?

Attachments (3)

wp-embed-template.diff (1.2 KB) - added by JamesDiGioia 22 months ago.
35630.1.diff (1.2 KB) - added by JamesDiGioia 22 months ago.
Removed extra space, proper naming.
35630.diff (986 bytes) - added by swissspidy 21 months ago.

Download all attachments as: .zip

Change History (13)

#1 @swissspidy
22 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to swissspidy
  • Status changed from new to assigned

Thanks for your report. Supporting lazy-loaded content sounds like something we should support automatically.

We're currently using

var links = document.getElementsByTagName( 'a' );
for ( i = 0; i < links.length; i++ ) {
	links[ i ].addEventListener( 'click', linkClickHandler );

whereas we might just use something like

document.addEventListener( 'click', 'linkClickHandler' );

This would require/allow us to to simplify the logic inside linkClickHandler().

If this is already enough, we could look into making things available under the wp namespace at a later point, e.g. functions like wp.embed.send_some_stuff().

#2 @JamesDiGioia
22 months ago

  • Keywords has-patch added; needs-patch removed

First patch, but I think I did this right. I didn't really see a way to simplify the logic that much, since we still need to check the parentNode.

22 months ago

Removed extra space, proper naming.

#3 @JamesDiGioia
22 months ago

Friend of mine suggested the diff name needs to be <ticket#>.<patch#>.diff. This rectifies that and removes the extra line break I added by accident.

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

22 months ago

#5 @chriscct7
22 months ago

@swissspidy can you review the latest patch for merge potential?

#6 @swissspidy
22 months ago

  • Milestone changed from Future Release to 4.5
  • Status changed from assigned to reviewing

#7 @swissspidy
21 months ago

  • Keywords commit added

I haven't found any problems with document.addEventListener() being slow or anything, so that looks good to me.

#8 @swissspidy
21 months ago

  • Summary changed from oEmed Link Handling Can't Be Refired to oEmbed Link Handling Can't Be Refired

21 months ago

#9 @swissspidy
21 months ago

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

In 36637:

Embeds: Make the click event handler work for dynamically added links.

Props JamesDiGioia.
Fixes #35630.

#10 @johnbillion
21 months ago

  • Version changed from trunk to 4.4
Note: See TracTickets for help on using tickets.