Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#35630 closed enhancement (fixed)

oEmbed Link Handling Can't Be Refired

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

Description

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 8 years ago.
Patch1
35630.1.diff (1.2 KB) - added by JamesDiGioia 8 years ago.
Removed extra space, proper naming.
35630.diff (986 bytes) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (13)

#1 @swissspidy
8 years 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().

@JamesDiGioia
8 years ago

Patch1

#2 @JamesDiGioia
8 years 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.

@JamesDiGioia
8 years ago

Removed extra space, proper naming.

#3 @JamesDiGioia
8 years 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.


8 years ago

#5 @chriscct7
8 years ago

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

#6 @swissspidy
8 years ago

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

#7 @swissspidy
8 years 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
8 years ago

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

@swissspidy
8 years ago

#9 @swissspidy
8 years 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
8 years ago

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