Verify that oembed is working for YouTube

[boonebgorges]

From http://commons.gc.cuny.edu/?p=44237

I am able to embed Twitter posts by cutting and pasting the URL as text. I used to be able to do this with YouTube, but you now have to “add media” for it to embed

I haven't verified, but this should be checked

[christianwach]

Works for me in latest Chrome (OSX): http://cdev.gc.cuny.edu/papers/image-tests/

The YouTube URL must be on a line of its own though.

[christianwach]

Oh, but it's wildly oversized :(

[r-a-y]

I can also confirm that YouTube oEmbeds work.

Oh, but it's wildly oversized :(

I've looked into using JS to resize IFRAMES dynamically when the browser is resized. But, there is another issue with post revisions.

If you add a new YouTube URL so it will trigger oEmbed and then disable editing, you'll see the YouTube video in its IFRAME. If you re-enable editing and make some changes, the YouTube URL gets replaced with the oEmbed IFRAME contents... this isn't great because it will create a new post revision.

I just tested this on a new install with just FEE and FEE does not cause the YouTube URL to turn to an IFRAME when updating. I think this issue and the other issues caused by autosave revisions via wptexturize(), etc. (#42, #71) are because of the comment tracking code.

My guess is the copy_original() JS function:
https://github.com/cuny-academic-commons/social-paper/blob/master/assets/js/changes.js#L1030-L1044

@christianwach - Do we need to copy the reader mode contents back into the editor? If we strip the paragraph data-incom attributes at the server level, we wouldn't need the copy_original() JS function would we? Or instead of copying the contents back to the editor, we strip the data-incom attributes in the copy_original() function.

[christianwach]

@r-a-y The copy_original() function was really just a shortcut that happened to work with basic content. I can now see that it's causing issues and should be able to do a better job of applying Inline Comments attributes to the editor content tomorrow.

[christianwach]

In response to the comment from @r-a-y above, I've been writing a custom method to selectively apply data-incom attributes on fee-on - to replace copy_original() - and thought I was making reasonable progress. However, I have now hit something of a wall which started with oEmbeds but has since widened considerably.

So, having discovered that copy_original() was too crude because it failed to account for the way that oEmbeds work (in .fee-content-original the YouTube URL is converted to an <iframe> wrapped in a <p> tag - whilst in .fee-content-body it is converted to a wp-view object with the URL present as the content of the data-wpview-text attribute) I set about "manually" parsing the content of .fee-content-body to apply data-incom attributes as required. This was going okay despite:

• the mis-match between the number of paragraph tags present in each (I use one of the <p> tags inside the wp-view object to 'hold' the data-incom attribute which is applied to the enclosing <p> tag in .fee-content-original)
• the resultant need to rewrite all the code that relied on <p> tags being top-level (nextAll('p') only grabs siblings) and in an uninterrupted sequence (nextAll('p') stops traversing when it encounters an element that is not a <p> tag)
• additionally realising that undo and redo have not yet been accounted for (there is placeholder code which reminded me of this after a bit of head-scratching over out-of-sync data in the change-tracking array)

And then, like Ray, I revisited a vanilla WP FEE install.

I had been under the impression that clicking the "Leave" button in the "The changes you made will be lost if you navigate away from this page" dialog would, well, discard the changes in the editor. However, the vanilla WP FEE install showed me that this wasn't that case at all. The changes in the editor remain when returning to the editor. I now see that it was actually the copy_original() function that was responsible for this apparently sensible behaviour. This can be verified in the SP context by excluding 'changes.js'.

To my mind, WP FEE's behaviour is counter-intuitive. The "Leave" button does not suggest to me that the content of the editor remains unchanged, especially given the text in the dialog. I'm open to other people's views, however. The consequences seem to be:

• If changes are to be discarded (as I would expect) then the original editor content must be cached somewhere since WP FEE doesn't do this for us. This cache will need to be updated on each fee-after-save and possibly elsewhere.
• if the default WP FEE behaviour is to be respected, there will be mis-matches between the data-incom values in .fee-content-original and .fee-content-body. I can't even begin to think my way through that right now.

Apologies if my brain dump is rather haphazard, but I hadn't expected that replacing copy_original()
would open such a proverbial can of worms. I'd appreciate any/all thoughts on this.

[boonebgorges]

Thanks for plugging away at this, @christianwach.

To my mind, WP FEE's behaviour is counter-intuitive.

I certainly agree that the behavior isn't accurately described by the text of the dialog. What counts as "intuitive" in this case is not clear to me, though. I think it would be perfectly natural, for instance, if FEE didn't show the notice at all when clicking "Disable Editing", but only beforeunload and when clicking an external link. That way, you're only reminded that you're going to lose content when you are actually going to lose it.

Setting aside the "proper" behavior of the Leave button for a moment, can you remind me what problem copy_original() was intended to solve in the first place? I'm playing around with the editor with the copy_orginal() call commented it out, and it seems that - aside from the misleading Leave message when clicking Disable Editing - most of these other problems go away.

[christianwach]

can you remind me what problem copy_original() was intended to solve in the first place?

@boonebgorges Thanks for your feedback.

The goal of 'changes.js' is to maintain (as best we can) the associations between comments and paragraphs during edits. However, in order to track changes to the editor content (and apply those changes to the data-incom-comment attribute values of each comment to maintain linkage) the paragraphs in the editor need to have identifiers. The Inline Comments data-incom values are those identifiers. However, because Inline Comments (rightly) only adds data-incom attributes to the non-editable content, the attributes somehow need to be applied to the editor content as well.

As I tried to explain above, copy_original() was a convenient way to apply the data-incom attributes to the editor content. It did so adequately for static markup, but failed when there were differences between the original and editor content, as is the case with oEmbeds.

[christianwach]

I think it would be perfectly natural, for instance, if FEE didn't show the notice at all when clicking "Disable Editing", but only beforeunload and when clicking an external link.

I'm afraid I don't agree with you here @boonebgorges. When I click "Disable Editing", I expect the visible content to show the content of the editor in "Read" mode. By default, FEE does not do this, which (for me) produces a strange, dissonant effect when toggling between "Read" and "Edit" modes thereafter. It then appears that I am not editing the content, but rather have two parallel "contents", which is true, but counter-intuitive - to me at least.

[boonebgorges]

When I click "Disable Editing", I expect the visible content to show the content of the editor in "Read" mode

I'm not saying that this seems wrong, just not that it seems obviously right.

If I'm understanding the situation correctly:

1. We need a way to identify, in the editor, the data-incom atts of each paragraph
2. Doing this by putting the data-incom markup into the editor is causing isDirty problems

Is it possible for us to accomplish 1 without changing the editor content? What if we had a paragraph map, built on fee-on, which matched incom paragraph numbers to "raw" (ie, editor) paragraphs? Then, on cut/paste/delete/etc actions, we check against the map to determine any necessary comment moves.

[boonebgorges]

Here's an attempt to sidestep the whole issue: 831e171

Requires the following modification to FEE: cuny-academic-commons/wp-front-end-editor@3dc1d47

One way of looking at our current problems is this: FEE determines dirtiness by comparing the editor content with the "original" content. My suggestion is that we stop FEE from doing this. Instead, we set an isDirty=false flag; we flip it to true on change; we flip it to false after save; and we override wp.fee.isDirty() to check the value of this flag. This way, we can do whatever we want to the editor content and not worry about FEE throwing a tantrum.

@christianwach @r-a-y Would appreciate your thoughts on this technique.

[christianwach]

@boonebgorges Very handy! I think the modification of FEE is logical regardless of what SP does with it. More generally, I think it could make sense to maintain an SP fork of FEE since it is a basic requirement of the project and FEE doesn't seem to be getting any TLC right now. Although it would be sensible to minimise the changes made to that fork.

Hooking into ed.onChange would mean that the method changes.js uses to track content changes would have to accommodate this flag, since adding data-incom attributes to the editor content is more than likely to trigger it. That doesn't look difficult to implement, however, and the benefits of having control of isDIrty definitely outweighs this.

[christianwach]

@boonebgorges @r-a-y I've added a new branch called "no-copy" that handles oEmbeds by replacing copy_original() with a more sophisticated add_atts() method.

For me, even without 831e171, it never seems to trigger the isDirty dialog, particularly now that I filter editor.getContent() to remove data-incom attributes. In theory, this should prevent erroneous auto-saved revisions too.

I haven't applied the new logic to cut/paste yet, so please just test with enter, delete and printing chars for now.

EDIT: logic applied to cut and paste

[boonebgorges]

Thanks for your work on this, @christianwach. The approach looks good to me, and so far I haven't managed to make FEE throw any notices. Would appreciate a look from @r-a-y before merging.

[christianwach]

@boonebgorges Thanks for looking and glad it seems good to you. To clarify: my comment about it working without your isDirty fixes doesn't mean we don't need your code! Indeed, I can't think how SP would signal that the group settings need saving without taking control of it from FEE.

I'll tackle undo redo asap and check the rest in the context of 831e171.

[r-a-y]

I've tested the no-copy branch and everything seems to be working.

@christianwach - Merge at will!

[christianwach]

@r-a-y I fixed an edge case (when enter is pressed with the cursor at the beginning of a para) and merged no-copy.

I have bumped an issue on the TinyMCE issue queue for the Gecko/Webkit difference in classes when merging paragraphs. This still needs to be addressed in SP's change-tracking code.

For undo/redo tracking, I created a new branch which implements comment and tracker data sync with TinyMCE's editor.undoManager levels. Please test!

[christianwach]

Most of the oEmbed providers I have tested work okay, though some have needed code tweaks in order that they don't cause a mismatch between Read and Edit modes. The following, however, is the scenario for Twitter embeds, which is giving me a right headache.

NB, I am using a filter on the_content (which is as yet not committed) to replace

<p><script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script></p>

with

<div><script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script></div>

This at least removes the script container from the equation. Otherwise, it is parsed by IC but is not viewable due to the <script> tag (the only content) being display: none.

Read Mode

a) Inline Comments parses content before Tweet has been rendered

In this case, IC picks up the two paragraphs in, for example:

<blockquote class="twitter-tweet" width="550"><p lang="en" dir="ltr">Perspective <a href="https://t.co/GymOsgKll2">pic.twitter.com/GymOsgKll2</a></p>
<p>&mdash; Christian Wach (@interactivist) <a href="https://twitter.com/interactivist/status/666956609919320064">November 18, 2015</a></p></blockquote>

These paragraphs are then removed from the DOM by the async Javascript, leading to subsequent paragraphs being numbered incorrectly.

b) Inline Comments parses content after Tweet has been rendered

IC picks up no paragraphs in the tweet, despite there being one, eg:

<p class="Tweet-text e-entry-title" lang="en" dir="ltr">Perspective </p>

I presume this is because it's inside the <iframe>.

Edit Mode:

Sometimes the Tweet renders, other times it doesn't.

a) On first clicking "Enable Editing", tweets do not render

Example wp-view code:

<div class="wpview-wrap" data-wpview-text="https%3A%2F%2Ftwitter.com%2Finteractivist%2Fstatus%2F666956609919320064" data-wpview-type="embedURL"><p class="wpview-selection-before">&nbsp;</p><div class="wpview-body" contenteditable="false"><div class="wpview-content wpview-type-embedURL"><blockquote class="twitter-tweet" width="550"><p lang="en" dir="ltr">Perspective <a href="https://t.co/GymOsgKll2">pic.twitter.com/GymOsgKll2</a></p>— Christian Wach (@interactivist) <a href="https://twitter.com/interactivist/status/666956609919320064">November 18, 2015</a></blockquote><script async="" src="//platform.twitter.com/widgets.js" charset="utf-8"></script></div><div class="wpview-overlay"></div></div><p class="wpview-selection-after">&nbsp;</p></div>

b) On clicking "Update", the tweet renders

---

I need to match the number of paragraphs in Edit Mode to the number in Read Mode. So, at present, I disallow any paras from being parsed in Edit Mode, but this leads to a mismatch when IC creates the bubbles before the tweet has been rendered. Conversely, if I allow two paragraphs within the wp-view object, it leads to a mismatch when IC creates the bubbles after the tweet has been rendered.

Unfortunately, there is no way of knowing whether or not the Tweet has rendered when IC parses the content. So there is always the possibility of a mismatch - the result of which is that reassigning comments in Read Mode can produce incorrect associations.