Make WordPress Core

Opened 16 years ago

Closed 16 years ago

#8850 closed defect (bug) (fixed)

WYSIWYG editor fails to correct invalid video embed code

Reported by: fd's profile fd Owned by: azaozz's profile azaozz
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.7
Component: TinyMCE Keywords: youtube embed tinymce editor video wysiwyg
Focuses: Cc:

Description

The WYSIWYG editor changes this:

<object width="425" height="344"><param name="movie" value="http://www.youtube.com/v/h9zb_Gr8iLc&hl=en&fs=1"></param><param name="allowFullScreen" value="true"></param><param name="allowscriptaccess" value="always"></param><embed src="http://www.youtube.com/v/h9zb_Gr8iLc&hl=en&fs=1" type="application/x-shockwave-flash" allowscriptaccess="always" allowfullscreen="true" width="425" height="344"></embed></object>

to this:

<object width="425" height="344" data="http://www.youtube.com/v/h9zb_Gr8iLc&amp;hl=en&amp;fs=1" type="application/x-shockwave-flash"><param name="allowFullScreen" value="true" /><param name="allowscriptaccess" value="always" /><param name="src" value="http://www.youtube.com/v/h9zb_Gr8iLc&amp;hl=en&amp;fs=1" /><param name="allowfullscreen" value="true" /></object>

Note the "allowFullScreen" param is duplicated, the embed tag has been removed, the object tag has been modified, and an additional param "src" has been added. The video still shows up on the blog itself but it does not show up in Google Reader.

To reproduce:

  1. Start a new post.
  2. Switch to Code view.
  3. Insert YouTube embed code.
  4. Switch to Visual view.
  5. Switch to Code view.

I am logged in as admin using WordPress 2.7. The "correct invalidly nested XHTML" option is unchecked.

Change History (8)

#1 @azaozz
16 years ago

  • Summary changed from WYSIWYG editor mangles video embed code to WYSIWYG editor corrects invalid video embed code

This is HTML validity issue. The <embed> tag is invalid in both HTML 4 and XHTML 1. Also all attributes like name have to be in lower case in XHTML 1. It seems TinyMCE has properly converted "allowFullScreen" to lower case but hasn't removed the invalid version. Will pass this upstream to the TinyMCE team.

If you need to support the non-standard tags for embedding video in your RSS feed, perhaps use a plugin specifically written for that.

#2 follow-up: @fd
16 years ago

  • Summary changed from WYSIWYG editor corrects invalid video embed code to WYSIWYG editor fails to correct invalid video embed code

I see what you did there. How you changed the name of the bug so that it's actually a feature. ;-)

It would be just an HTML validity issue if all it was doing was removing the <embed> tag. But it's doing a lot more than that.

For one, elements and attributes must be lowercase but the value of an attribute does not have to be (except for attributes that have a pre-defined set of values like the type attribute on an <input> tag). "allowFullScreen" is a value, not an attribute, it is valid and should be left alone.

Also, it has added data and type attributes to the (already valid) object tag. I didn't put them there and there is no reason for them to be there. They're optional attributes.

It added a whole extra <param> with name="src" for no good reason.

And, crucially I think, it deleted the (perfectly valid) <param> with name="movie" pointing to the actual source of the video.

I'm going to stick with "mangling" when describing this behavior.

For what it's worth, this is the embed code handed out by YouTube and it is going to affect a lot of 2.7 users.

#3 @mrmist
16 years ago

I took the embed code suggested for a ranom youtube video and stuck it into my test site in the way you describe.

The result was a correctly embedded video which works, and passes XHMTML 1.0 validation.

That would make it look more like a google reader issue, as a valid working page is being shown.

#4 @mrmist
16 years ago

ranom = random.

#5 in reply to: ↑ 2 @azaozz
16 years ago

Replying to fd:

It would be just an HTML validity issue if all it was doing was removing the <embed> tag. But it's doing a lot more than that.

...

Also, it has added data and type attributes to the (already valid) object tag. I didn't put them there and there is no reason for them to be there. They're optional attributes.

It added a whole extra <param> with name="src" for no good reason.

And, crucially I think, it deleted the (perfectly valid) <param> with name="movie" pointing to the actual source of the video.

Unfortunately the <object> tag is not supported well in many browsers. Only a few seem to understand the movie param but most understand src. Also it seems some browsers need data and type to be able to show the video.

I'm going to stick with "mangling" when describing this behavior.

It seems to produce a valid <object> tag from an invalid one pasted by the user (from YouTube or not). It also seems to preserve all attributes and makes it compatible with all popular browsers without using invalid HTML. The only bug seems to be the double allowFullScreen param that most likely will be fixed in the next version.

#6 @fd
16 years ago

The original <object> tag and all of the <param> tags were already valid. The only tag that was invalid was the <embed> tag. And only because it is deprecated, not because it wasn't well-formed.

And something really interesting I just discovered: if you use the [youtube] quicktag on WordPress.com, it does it the YouTube way with the <embed> tag.

I think we may have a difference in ideology (which is fine). I understand that it's important to try and improve compatibility for broad user acceptance. But I also think that if I type valid tags (like the <param name="movie"...> or <object> tags) into the HTML editor then the editor shouldn't alter or delete them. Maybe a new option on the Writing pane could be added: "Don't automatically improve/correct XHTML"

I did some more testing with Google Reader and, unfortunately, the only form it seems to like is the one with the <embed> tag. Since Google Reader has the largest market share among feed aggregators, I'm forced to use the <embed> tag which means I'll have to use a plugin and do it like WordPress.com does.

#7 @fd
16 years ago

Where I said 'quicktag' I meant 'shortcode'. The [youtube=] shortcode.

#8 @azaozz
16 years ago

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

Fixed in TinyMCE 3.2.2 [10791]

Note: See TracTickets for help on using tickets.