#22790 closed defect (bug) (fixed)
Duplicating Embed-Codes
Reported by: | Bichareh | Owned by: | ryan |
---|---|---|---|
Milestone: | 3.5 | Priority: | highest omg bbq |
Severity: | blocker | Version: | 3.5 |
Component: | TinyMCE | Keywords: | has-patch commit |
Focuses: | Cc: |
Description
Weird issue in WP 3.5 RC4...
If I paste any embed-code in html-mode, change into visual mode and back to html, it duplicates the code.
e.g. this code...
Before (1301 character):
<object id="flashObj" width="630" height="534" classid="clsid:D27CDB6E-AE6D-11cf-96B8-444553540000" codebase="http://download.macromedia.com/pub/shockwave/cabs/flash/swflash.cab#version=9,0,47,0"><param name="movie" value="http://c.brightcove.com/services/viewer/federated_f9?isVid=1" /><param name="bgcolor" value="#FFFFFF" /><param name="flashVars" value="videoId=1999115218001&playerID=18295496001&playerKey=AQ~~,AAAAABumiUQ~,d-Jwax1bUNkyqTy8Pr8lycLwfYHEbxsR&domain=embed&dynamicStreaming=true" /><param name="base" value="http://admin.brightcove.com" /><param name="seamlesstabbing" value="false" /><param name="allowFullScreen" value="true" /><param name="swLiveConnect" value="true" /><param name="allowScriptAccess" value="always" /><embed src="http://c.brightcove.com/services/viewer/federated_f9?isVid=1" bgcolor="#FFFFFF" flashVars="videoId=1999115218001&playerID=18295496001&playerKey=AQ~~,AAAAABumiUQ~,d-Jwax1bUNkyqTy8Pr8lycLwfYHEbxsR&domain=embed&dynamicStreaming=true" base="http://admin.brightcove.com" name="flashObj" width="630" height="534" seamlesstabbing="false" type="application/x-shockwave-flash" allowFullScreen="true" swLiveConnect="true" allowScriptAccess="always" pluginspage="http://www.macromedia.com/shockwave/download/index.cgi?P1_Prod_Version=ShockwaveFlash"/></object>
After (2153 character):
<object width="630" height="534" classid="clsid:D27CDB6E-AE6D-11cf-96B8-444553540000" codebase="http://download.macromedia.com/pub/shockwave/cabs/flash/swflash.cab#version=9,0,47,0"><param name="movie" value="http://c.brightcove.com/services/viewer/federated_f9?isVid=1" /><param name="bgcolor" value="#FFFFFF" /><param name="flashVars" value="videoId=1999115218001&playerID=18295496001&playerKey=AQ~~,AAAAABumiUQ~,d-Jwax1bUNkyqTy8Pr8lycLwfYHEbxsR&domain=embed&dynamicStreaming=true" /><param name="base" value="http://admin.brightcove.com" /><param name="seamlesstabbing" value="false" /><param name="allowFullScreen" value="true" /><param name="swLiveConnect" value="true" /><param name="allowScriptAccess" value="always" /></object><object width="630" height="534" classid="clsid:d27cdb6e-ae6d-11cf-96b8-444553540000" codebase="http://download.macromedia.com/pub/shockwave/cabs/flash/swflash.cab#version=6,0,40,0" bgcolor="#FFFFFF"><param name="src" value="http://c.brightcove.com/services/viewer/federated_f9?isVid=1" /><param name="flashvars" value="videoId=1999115218001&playerID=18295496001&playerKey=AQ~~,AAAAABumiUQ~,d-Jwax1bUNkyqTy8Pr8lycLwfYHEbxsR&domain=embed&dynamicStreaming=true" /><param name="base" value="http://admin.brightcove.com" /><param name="seamlesstabbing" value="false" /><param name="allowfullscreen" value="true" /><param name="swliveconnect" value="true" /><param name="allowscriptaccess" value="always" /><param name="pluginspage" value="http://www.macromedia.com/shockwave/download/index.cgi?P1_Prod_Version=ShockwaveFlash" /><embed width="630" height="534" type="application/x-shockwave-flash" src="http://c.brightcove.com/services/viewer/federated_f9?isVid=1" flashvars="videoId=1999115218001&playerID=18295496001&playerKey=AQ~~,AAAAABumiUQ~,d-Jwax1bUNkyqTy8Pr8lycLwfYHEbxsR&domain=embed&dynamicStreaming=true" base="http://admin.brightcove.com" seamlesstabbing="false" allowfullscreen="true" swliveconnect="true" allowscriptaccess="always" pluginspage="http://www.macromedia.com/shockwave/download/index.cgi?P1_Prod_Version=ShockwaveFlash" bgcolor="#FFFFFF" /></object>
Attachments (14)
Change History (58)
#5
@
12 years ago
Tested on IE10, Opera 12.10, Chrome 23, Safari 6 and Firefox 17. Happens on all of them.
#6
@
12 years ago
- Component changed from Editor to TinyMCE
- Milestone changed from Awaiting Review to 3.5
- Severity changed from normal to critical
So, azaozz has narrowed this down to the change from TinyMCE's "schema" parameter, which we changed in 3.5 to go from HTML4 to HTML5 handling.
You can confirm by going into wp-includes/class-wp-editor.php and removing the 'schema' => 'html5' line at line 348.
I am currently diffing the entire tags and attributes list here: https://github.com/tinymce/tinymce/blob/master/jscripts/tiny_mce/classes/html/Schema.js. (It's packed, so not very fun.)
#7
@
12 years ago
THESE ELEMENTS WERE IN THE HTML4 SCHEMA AND ARE NOT IN HTML5
applet, tt, u, s, strike, big, font, basefont, acronym, dir, center, noframes, isindex
These elements are new to the HTML5 schema
html, head, title, link, meta, section, nav, article, aside, hgroup, header, footer, dialog, mark, progress, meter, time, ruby, rt, rp, figure, figcaption, embed, details, command, source, audio, video, datalist, keygen, output, canvas, mathml, svg, wbr
#8
@
12 years ago
Yeah, they are "not supported" in html5: http://www.w3schools.com/tags/default.asp
We have two options: either revert back to html4 and add all the html5 tags, or keep the html5 schema and allow the obsolete/not-supported tags. Seems the second option is better as HTML 5.0 allows more attributes on all tags and generally is a bit more liberal than HTML 4/XHTML 1.
#9
@
12 years ago
From HTML4 schema to HTML5, a bunch of elements had their attributes removed. I excluded xml:lang and lang from the results — those should be available globally, probably.
script --- language, xml:space
style --- xml:space
object --- declare, codebase, codetype, archive, standby, tabindex, align, border, hspace, vspace
param --- valuetype, type
p --- align
a --- tabindex, onfocus, onblur, charset, name, hreflang, rev, shape, coords
br --- clear
h1 --- align
img --- name, longdesc, align, border, hspace, vspace
h2 --- align
iframe --- longdesc, frameborder, marginwidth, marginheight, scrolling, align
h3 --- align
input --- tabindex, onfocus, onblur, usemap, onselect, onchange, align
select --- tabindex, onfocus, onblur, onchange
textarea --- tabindex, onfocus, onblur, onselect, onchange
label --- onfocus, onblur
button --- tabindex, onfocus, onblur
h4 --- align
h5 --- align
h6 --- align
div --- align
ul --- type, compact
li --- type
ol --- type, compact
dl --- compact
menu --- compact
pre --- width, xml:space
hr --- align, noshade, size, width
legend --- align
table --- summary, width, frame, rules, cellspacing, cellpadding, align, bgcolor
caption --- align
col --- width, align, char, charoff, valign
colgroup --- width, align, char, charoff, valign
thead --- align, char, charoff, valign
tr --- align, char, charoff, valign, bgcolor
th --- abbr, axis, align, char, charoff, valign, nowrap, bgcolor, width, height
form --- onsubmit, onreset, accept
td --- abbr, axis, scope, align, char, charoff, valign, nowrap, bgcolor, width, height
tfoot --- align, char, charoff, valign
tbody --- align, char, charoff, valign
area --- tabindex, onfocus, onblur, nohref
body --- onload, onunload, background, bgcolor, text, link, vlink, alink
#10
@
12 years ago
From HTML4 to HTML5, a number of elements received *more* attributes. I have excluded these attributes, as they seem to be have been added all over: accesskey, draggable, item, hidden, itemprop, role, spellcheck, subject.
script --- class, dir, style, title, onclick, ondblclick, onmousedown, onmouseup, onmouseover, onmousemove, onmouseout, onkeypress, onkeydown, onkeyup, async
style --- class, style, onclick, ondblclick, onmousedown, onmouseup, onmouseover, onmousemove, onmouseout, onkeypress, onkeydown, onkeyup, scoped
object --- form
param --- class, dir, style, title, onclick, ondblclick, onmousedown, onmouseup, onmouseover, onmousemove, onmouseout, onkeypress, onkeydown, onkeyup
a --- ping, media
br --- dir, onclick, ondblclick, onmousedown, onmouseup, onmouseover, onmousemove, onmouseout, onkeypress, onkeydown, onkeyup
iframe --- dir, onclick, ondblclick, onmousedown, onmouseup, onmouseover, onmousemove, onmouseout, onkeypress, onkeydown, onkeyup, sandbox, seamless
input --- autocomplete, autofocus, form, formaction, formenctype, formmethod, formnovalidate, formtarget, height, list, max, min, multiple, pattern, placeholder, required, step, width, files
select --- autofocus, form
textarea --- autofocus, form, maxlength, placeholder, required, wrap
label --- form
button --- autofocus, form, formaction, formenctype, formmethod, formnovalidate, formtarget
ol --- reversed
menu --- type, label
fieldset --- disabled, form, name
form --- autocomplete, novalidate
area --- media, rel, ping, type
base --- class, dir, style, title, onclick, ondblclick, onmousedown, onmouseup, onmouseover, onmousemove, onmouseout, onkeypress, onkeydown, onkeyup
#11
follow-up:
↓ 13
@
12 years ago
In case anyone would find looking at the output in a readable way:
https://gist.github.com/4228771
A summary of the changes from before to after:
- <object> loses its id attr.
- <object> is split in two.
- the first <object> loses its nested <embed> but retains all <param>s correctly.
- the second <object> inherits a bgcolor attr, which previously existed in only on a <param> and on the <embed>
- the second <object> has it's codebase attr rewritten to a value not previously present.
- the <embed> object loses it's name attr.
- the second's <param>s appear to be rewritten based on the attrs of the <embed>: the "movie" and "bgcolor" <param>s are gone, but there's now a "src" and "pluginspage". It further appears "width", "height", "bgcolor" where simply hoisted past the <param> rewrite to attr on the object.
This looks to me to be some flash back-compatibility rewrite wrapper (version 9 down to 6) in TinyMCE. Perhaps this is activated by the 'schema' => 'html5' flag?
#12
@
12 years ago
the 5. from above is the evidence for the Compat rewriting: version=9,0,47,0
becomes version=6,0,40,0
#13
in reply to:
↑ 11
@
12 years ago
Replying to WraithKenny:
This looks to me to be some flash back-compatibility rewrite wrapper (version 9 down to 6) in TinyMCE. Perhaps this is activated by the 'schema' => 'html5' flag?
This summary is awesome, thank you! Can you do this same thing in 3.4.2? TinyMCE does do some rewriting to ensure it works in contenteditable. At this point I am only interested in differences between 3.4.2 and 3.5.
#14
@
12 years ago
the 3.4.2 comparison (but it isn't a clean install): https://gist.github.com/4229053
#15
@
12 years ago
The rewrite present in 3.4.2:
- doesn't double the object.
- doesn't strip the id attr.
- does strip the "movie" and "bgcolor" params, (same as 3.5)
- hoists the bgcolor into the object tag (same)
- converts the version hash on codebase attr (same)
- "src" param created from embed attr (same)
- duplicates "flashvars", "swliveconnect", allowScriptAccess" and "pluginspage" by hoisting from embed attr. (sort of the same, 3.5 looks to clean up the dups. better)
- <embed> has it's name converted to id.
- embed attrs otherwise left intact but some are doubled. (mostly the same).
#16
@
12 years ago
if the first object of the 3.5 version could be cleaned out, it'd be much closer.
#17
@
12 years ago
22790.patch fixes the regression. It still doesn't preserve the <object> exactly but that's how it works in 3.4 and earlier. It's worth it extending/modifying the 'media' plugin for TinyMCE by adding an option there to fully preserve the <object> tags.
#19
follow-up:
↓ 32
@
12 years ago
there are differences still but the double object is gone. the id attr on object gets removed still for example.
#20
@
12 years ago
- Owner set to ryan
- Resolution set to fixed
- Status changed from new to closed
In 23112:
#21
@
12 years ago
- Resolution fixed deleted
- Severity changed from critical to major
- Status changed from closed to reopened
Re-opening and lowering severity for additional investigation of remaining attributes and children.
#22
@
12 years ago
For the elements entirely removed from the HTML5 schema, here are their HTML4 schema definitions:
applet[id|class|style|title|codebase|archive|code|object|alt|name|width|height|align|hspace|vspace] tt[id|class|style|title|lang|xml:lang|dir|onclick|ondblclick|onmousedown|onmouseup|onmouseover|onmousemove|onmouseout|onkeypress|onkeydown|onkeyup] u[id|class|style|title|lang|xml:lang|dir|onclick|ondblclick|onmousedown|onmouseup|onmouseover|onmousemove|onmouseout|onkeypress|onkeydown|onkeyup] s[id|class|style|title|lang|xml:lang|dir|onclick|ondblclick|onmousedown|onmouseup|onmouseover|onmousemove|onmouseout|onkeypress|onkeydown|onkeyup] strike[id|class|style|title|lang|xml:lang|dir|onclick|ondblclick|onmousedown|onmouseup|onmouseover|onmousemove|onmouseout|onkeypress|onkeydown|onkeyup] big[id|class|style|title|lang|xml:lang|dir|onclick|ondblclick|onmousedown|onmouseup|onmouseover|onmousemove|onmouseout|onkeypress|onkeydown|onkeyup] font[id|class|style|title|lang|xml:lang|dir|size|color|face] basefont[id|size|color|face] acronym[id|class|style|title|lang|xml:lang|dir|onclick|ondblclick|onmousedown|onmouseup|onmouseover|onmousemove|onmouseout|onkeypress|onkeydown|onkeyup] dir[id|class|style|title|lang|xml:lang|dir|onclick|ondblclick|onmousedown|onmouseup|onmouseover|onmousemove|onmouseout|onkeypress|onkeydown|onkeyup|compact] center[id|class|style|title|lang|xml:lang|dir|onclick|ondblclick|onmousedown|onmouseup|onmouseover|onmousemove|onmouseout|onkeypress|onkeydown|onkeyup] noframes[id|class|style|title|lang|xml:lang|dir|onclick|ondblclick|onmousedown|onmouseup|onmouseover|onmousemove|onmouseout|onkeypress|onkeydown|onkeyup] isindex[id|class|style|title|lang|xml:lang|dir|prompt]
#23
@
12 years ago
22790-2.patch extends the html5 schema in TinyMCE with all tags and attributes from the html4 schema, more or less merging the two. That way we have good HTML 5.0 compatibility and good back-compat with existing posts.
#24
@
12 years ago
You can include tinymce-schemas.php into a PHP file and you'll end up with two arrays: $html4 and $html5. These both represent the schemas in https://github.com/tinymce/tinymce/blob/master/jscripts/tiny_mce/classes/html/Schema.js. I took that file and executed it, then logged the two function returns as JSON, and there you are.
The format is as follows:
element => object( attributes => object( (attribute1) => {} (attribute2) => {} (attribute3) => {} (attribute4) => {} ), attributeOrder => array( attribute1, attribute2, attribute3, attribute4 ), children => object( (element) => {} (element) => {} (element) => {} ), ), element => object( ...
So, if you pluck the attributeOrder key, you'll end up with all valid attributes for that object. This is the primary focus.
You can also look at the children key for elements that are valid immediately inside this element.
#25
@
12 years ago
tinymce-test.php is what I used to generate the comparison comments above.
We can extend this further to:
- Double-check azaozz's work.
- Ensure that there are no changes to allowed children.
Hack away. Sorry for the crudeness.
#26
@
12 years ago
22790-3.patch is the same as 22790-2.patch but fixes xml:lang
so it's not forced but verified.
#27
@
12 years ago
- Keywords has-patch added
Three approaches here:
- I manually went into getHTML5 and added elements, attributes, and children. (You can see this in 22790.getHTML5.diff.)
- koopersmith wrote a script to merge getHTML5 and getHTML4. You can see this in 22790.schema.debug.diff. Look for things like "WordPress," "sane," "nacin," and "Koop".
- azaozz added everything in the TinyMCE plugin.
Then, Koop used his script to check both my work and azaozz's work. There were a few issues in my work that we resolved 22790.schema.2.diff includes those changes).
azaozz's patch is missing lang
and xml:lang
in a number of places. (Which I don't really care about... should we?)
Additionally, azaozz's patch ignores children. (How could you?!) It only adds elements and attributes. That means that in azaozz's patch, tinymce.activeEditor.schema.isValidChild( 'div', 'big' ) returns false. For me and koop, it returns true.
But, it seems like TinyMCE doesn't care about children validness. (Insert parental joke here?) <div><big>Big</big></div>
works as long as <big>
is registered with addValidElements, even if it is missing from div's children. Which makes us all wonder if it is a bug, an incomplete API, or just something we're missing.
azaozz is now pinging TinyMCE's Spocke to discuss. Assuming we can just ignore the plight of the children, I am +1 on azaozz's patch, primarily because his results has been backed up by two other methods and a lot of manpower, and secondarily since we've already been doing exactly that all release, for other elements and attributes.
#28
@
12 years ago
An excellent summary. If we can go with azaozz's patch, we should, especially since it's based on what we've been using all cycle. Otherwise, we have the other well-tested overrides waiting in the wings.
#31
@
12 years ago
After a long work session in IRC, it was determined that addValidElements was insufficient for new elements (acronym, strike, etc.), because it did not set those elements to be allowed children for all other elements. This is a problem because to fix it, we'd need to call addValidChildren for essentially every element. The elements would still work even if they were not allowed; per Spocke they would be treated as custom elements and kind of like spans. But tinymce.activeEditor.schema.isValidChild would return false, and we didn't want to get into the ramifications of that.
At that point, we decided to go with Koop's straightforward duck punch. [23120], and bumpbot in [23121]. The wp-tinymce.js.gz file now includes a minified version of wp-includes/js/tinymce/wp-tinymce-schema.js immediately after core, and before plugins. That will allow our version of Schema.js to replace theirs.
Nice work, everyone.
#32
in reply to:
↑ 19
@
12 years ago
Replying to WraithKenny:
...the id attr on object gets removed still for example.
Been looking why this is still happening. The syntax for adding embed
as a valid child to object
is different from the syntax for adding valid attributes, so we should be doing addValidChildren('+object[embed]')
instead of addValidChildren('object[*]')
.
Here is the description of the TinyMCE API function, the `valid_children` init setting that gets passed directly to that function, and an example of TinyMCE core using it.
Note: this is a fix for 22790.patch/[23112] to make TinyMCE properly recognize <object><embed /></object> tags, it doesn't affect the rest of the schema.
#33
@
12 years ago
- Priority changed from normal to highest omg bbq
- Resolution fixed deleted
- Severity changed from major to blocker
- Status changed from closed to reopened
<object></object> without an <embed /> inside now gets stripped entirely.
#34
@
12 years ago
Seems to be caused by addValidChildren('object[*]')
interfering with the MCE media plugin. 22790-valid-children.patch fixes this as well.
#35
@
12 years ago
- Keywords commit added
I've looked through MCE in 3.5 and 3.4 and the various code paths, and this appears to be the correct fix. Allowing all children is too broad, but registering the embed tag without adding it as a child causes large amounts of breakage when the TinyMCE media plugin reads an object.
To quote azaozz from IRC:
It works in 3.4 as the <embed> tag is not mentioned as being allowed child of any other tag, so the check skips it.
The TinyMCE HTML5 schema adds the embed tag, so things begin breaking. This fixes them. +1 for commit.
What browser is returning this issue? All?