WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#22842 closed defect (bug) (fixed)

Object Within Video?

Reported by: miqrogroove Owned by: nacin
Milestone: 3.5 Priority: highest omg bbq
Severity: blocker Version: 3.5
Component: TinyMCE Keywords: commit
Focuses: Cc:

Description

Following up to #22790,

By looking in the TinyMCE Media plugin, I just wanted to wonder out loud if there is a concern about elements other than embed.

From the plugin source we have:

ed.schema.addValidElements('object[id|style|width|height|classid|codebase|*],param[name|value],embed[id|style|width|height|type|src|*],video[*],audio[*],source[*]');

So are the HTML 5 audio, video, source, etc. elements working? I did a quick comparison to the included schema file then tested what looked like a potential problem area.

For example, code found at http://www.w3schools.com/html/html_videos.asp

<video width="320" height="240" controls="controls">
  <source src="movie.mp4" type="video/mp4">
  <source src="movie.ogg" type="video/ogg">
  <source src="movie.webm" type="video/webm">
  <object data="movie.mp4" width="320" height="240">
    <embed src="movie.swf" width="320" height="240">
  </object> 
</video>

When pasted into the Text tab in 3.5 and switched to Visual and back, the code looks quite different afterward.

Does object need to be a valid child of video? And so on?

Attachments (8)

22842.diff (1.6 KB) - added by nacin 3 years ago.
Toss in the towel.
22842.patch (892 bytes) - added by azaozz 3 years ago.
22842.3.diff (2.1 KB) - added by koopersmith 3 years ago.
22842.4.diff (2.8 KB) - added by koopersmith 3 years ago.
22842.5.diff (4.7 KB) - added by koopersmith 3 years ago.
22842.6.diff (8.1 KB) - added by koopersmith 3 years ago.
22842.7.diff (10.7 KB) - added by koopersmith 3 years ago.
22842.8.diff (11.4 KB) - added by koopersmith 3 years ago.

Download all attachments as: .zip

Change History (35)

comment:1 @nacin3 years ago

  • Milestone changed from Awaiting Review to 3.5
  • Priority changed from normal to highest omg bbq
  • Severity changed from normal to blocker

This causes a JS error and is a regression.

comment:2 @miqrogroove3 years ago

Also: If the whitespace is removed from between the elements, the behavior on subsequent tab switches becomes amusing.

@nacin3 years ago

Toss in the towel.

@azaozz3 years ago

comment:3 @azaozz3 years ago

Both @nacin and I were patching this simultaneously and finished at precisely the same time :)

At this point I'm leaning towards 22842.diff​ (revert back to html4 schema). 22842.patch works for video and audio but wondering how many more errors are there in the html5 schema...

@koopersmith3 years ago

comment:4 @koopersmith3 years ago

attachment:22842.3.diff updates the underlying HTML5 schema to support flow-based elements where approprate (i.e. video, audio, and object) — note that embed is included in that group. It also adds support for the html5 track element. This makes our additional adjustments in the WP tinymce plugin redundant, so those have been removed.

comment:5 @azaozz3 years ago

22842.3.diff works well here. Tested with all problematic/recently reported tags and attributes.

@koopersmith3 years ago

comment:6 @koopersmith3 years ago

attachment:22842.4.diff is the result of auditing the HTML5 spec for every attribute listed in attachment:22842.diff, to ensure that the resulting schema will include everything possible with that patch.

We've added:

  • Appropriate child elements to canvas.
  • Additional attributes to audio and video.
  • The summary element.
  • command elements can contain other command elements.

@koopersmith3 years ago

comment:7 @koopersmith3 years ago

attachment:22842.5.diff adds support for the bdi and data elements.

comment:8 @sabreuse3 years ago

.5 works well for me

comment:9 follow-up: @miqrogroove3 years ago

22842.5.diff is a huge improvement.

I may have broken it here though. Try pasting into Text tab:

<details>blah <summary><span>hello</span> blah</summary> </details>

Then switch tabs repeatedly.

comment:10 in reply to: ↑ 9 @koopersmith3 years ago

Replying to miqrogroove:

22842.5.diff is a huge improvement.

I may have broken it here though. Try pasting into Text tab:

<details>blah <summary><span>hello</span> blah</summary> </details>

Then switch tabs repeatedly.

Given that we're just introducing support for the summary element and that TinyMCE itself supports an extremely limited (and quite janky) interaction with the details and summary elements, we're going to consider "support" as meaning "doesn't delete the node". The non-breaking spaces that pile up can be addressed in a future WP or TinyMCE release.

comment:11 @miqrogroove3 years ago

Test case posted above behaves the same in 3.4.2. Put it on the ignore list.

comment:12 @miqrogroove3 years ago

Appears to be deleting code here:

<details>Hello world.<summary><data value="93" /></summary></details>

comment:13 @miqrogroove3 years ago

Sorry, ignore that one too. I think I had my 3.4.2 window still open.

comment:14 @miqrogroove3 years ago

Here we go:

<table><tr><td> <data value="93">19+74 hexes (93 total)</data></td></tr></table>

comment:15 @miqrogroove3 years ago

That works:

<table><tr><td><span><data value="93">19+74 hexes (93 total)</data></span></td></tr></table>

+1 commit 22842.5.diff​

This thing is ready.

@koopersmith3 years ago

comment:16 @azaozz3 years ago

22842.6.diff is even better than 22842.5.diff, thinking we are covering a lot of tags and attributes that aren't even supported in the browsers yet or are just being added.

comment:17 @azaozz3 years ago

  • Keywords commit added

+1 to commit

comment:18 @sabreuse3 years ago

+1 to azaozz's +1

@koopersmith3 years ago

comment:19 @koopersmith3 years ago

attachment:22842.7.diff removes and accounts for the extra (now redundant) HTML4-compat code that was committed on #22790.

@koopersmith3 years ago

comment:20 @koopersmith3 years ago

attachment:22842.8.diff audits global attributes, phrasing content, and flow content groups, adding back any elements that the spec still defines. Corrects mathml to math, adds back s and u, and adds numerous missing attributes.

comment:21 @nacin3 years ago

In 23149:

Delete the patch of TinyMCE's schema; new attempt. see #22790. see #22842. for the 3.5 branch.

comment:22 @nacin3 years ago

In 23150:

Delete the patch of TinyMCE's schema; new attempt. see #22790. see #22842. actually for the 3.5 branch.

comment:23 @nacin3 years ago

In 23151:

Add TinyMCE's Schema.js to prepare for patching it. see #22790. see #22842. for trunk.

comment:24 @nacin3 years ago

In 23152:

Add TinyMCE's Schema.js to prepare for patching it. see #22790. see #22842. for the 3.5 branch.

comment:25 @nacin3 years ago

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

In 23153:

Restore the duck punch of TinyMCE's schema from [23120], along with updated rules for all HTML5 elements, as painstakingly audited against the HTML5 spec.

Remove conflicting and unnecessary code in the WP TinyMCE plugin.

Fixes all known regressions when working with the embed, object, video, audio, source, and param elements in TinyMCE.

props markjaquith, koopersmith.
fixes #22790, #22842.
fixes #22842.

comment:26 @nacin3 years ago

In 23154:

Restore the duck punch of TinyMCE's schema from [23120], along with updated rules for all HTML5 elements, as painstakingly audited against the HTML5 spec.

Remove conflicting and unnecessary code in the WP TinyMCE plugin.

Fixes all known regressions when working with the embed, object, video, audio, source, and param elements in TinyMCE.

props markjaquith, koopersmith.
fixes #22790, #22842.

Merges [23153] to the 3.5 branch.

comment:27 @miqrogroove3 years ago

Impressive!

Best wishes on the 3.5 release.

Happy Holidays!

Note: See TracTickets for help on using tickets.