Closed Bug 480889 Opened 11 years ago Closed 11 years ago

Nested <source> elements appear in video's childNodes

Categories

(Core :: Audio/Video, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

(Keywords: fixed1.9.1)

Attachments

(4 files, 2 obsolete files)

Attached file testcase
If a video element has a descendant source element which is deeply nested in the video's descendants, the source element will still appear in the video's childNodes array. e.g.:

<video>
  <div>
    <div>
      <source ... >
    </div>
  </div>
</video>

The video element's childNodes will be text node, div, source, text node. The source shouldn't appear as a direct child of the video, it should be a child of its parent div element.

See attached testcase.

Occurs in: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090301 Minefield/3.2a1pre (.NET CLR 3.5.30729) Ubiquity/0.1.4
The same issue occurs with applet so it doesn't appear to be a video specific bug:
<html>
  <body>
    <applet code="com.fluendo.player.Cortado.class" archive="http://tinyvid.tv/static/cortado.jar" width="480" height="272">
      <div>
        <param name="url" value="http://tinyvid.tv/file/t5v1inpguy9t.ogg"></param>
      </div>
      <param name="duration" value="313.353"></param>
    </applet>
  </body>
 </html>

In this example the DIV node appears in DOM inspector, and the param is successfully used by the applet to set the URL.
Attached file applet test case (obsolete) —
Attached file applet testcase
Fix bad html
Attachment #364842 - Attachment is obsolete: true
Attached file XHTML test case
Simliar to first testcase, but xhtml. The source tag isn't hoisted up to be a child of the video element, so the html parser must be doing something to cause that.
Attached patch Patch v1 - do what <applet> does (obsolete) — Splinter Review
This fixes it for me... I just copied what the param tag does in the html element table. eHTMLTag_source had its mParentBits set to kNone, but eHTMLTag_param has it set to kSpecial. If I copy that, and set eHTMLTag_source's mParentBits to kSpecial, they behave the same way - nested source elements no longer appear as children of the video element. I guess mParentBits==kSpecial tells the parser to look at the mSpecialParents field?
Assignee: nobody → chris
This was recently done in bug 364188.
Comment on attachment 364848 [details] [diff] [review]
Patch v1 - do what <applet> does

Blake, can you review this? It's very similar to the patch you reviewed for bug 364188.
Attachment #364848 - Flags: superreview?(mrbkap)
Attachment #364848 - Flags: review?(mrbkap)
Requesting blocking on this bug, since without this we iterate through a <video>'s child <source> elements incorrectly when considering what resource to load.
Blocks: 479859
Flags: blocking1.9.1?
Comment on attachment 364848 [details] [diff] [review]
Patch v1 - do what <applet> does

Sure. For what it's worth, this means that <source> elements will be allowed to appear outside of <video> and <audio> elements (since kSpecial means that any flow (inline) or block tag can contain it) but that's not really a big deal, I think.
Attachment #364848 - Flags: superreview?(mrbkap)
Attachment #364848 - Flags: superreview+
Attachment #364848 - Flags: review?(mrbkap)
Attachment #364848 - Flags: review+
(In reply to comment #9)
> For what it's worth, this means that <source> elements will be allowed to
> appear outside of <video> and <audio> elements.

Hmm... This makes a parser mochitest which checks that <source> don't appear outside of <video> and <audio> fail... I'll need to remove that test I guess...
(Per HTML5 <source> can appear pretty much anywhere.)
Flags: blocking1.9.1? → wanted1.9.1+
Same as patch v1, but changes mochitest so that it expects <source> to be visible anywhere, as per Anne's comment Re: HTML5 spec. Carrying forward r+/sr+
Attachment #364848 - Attachment is obsolete: true
Attachment #365279 - Flags: superreview+
Attachment #365279 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Pushed: http://hg.mozilla.org/mozilla-central/rev/7d08c79abb4e
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [baking for 1.9.1]
You need to log in before you can comment on or make changes to this bug.