890556 - Draggable handles for html5 media do not exist on initial load

Status: VERIFIED FIXED

(Webmaker Graveyard :: popcorn.js, defect)

Description

[Scott [:thecount] Downe]

Add a clip, I used http://ia601200.us.archive.org/29/items/Children_Must_Learn_The/0883_Children_Must_Learn_The_14_22_40_00_3mb.ogv

Expected: a media clip on the stage with draggable handles.
Actual: it is stuck where it is.

If you save and refresh it'll be fine.

Comment 1

[Matthew Schranz [:mjschranz]]

Turns out this is a bug with Popcorn.smart. See https://github.com/mozilla/popcorn-js/blob/master/modules/player/popcorn.player.js#L396-L405

This code basically overwrites anything that might have already been inside the containing element we pass to smart.

I'm wondering if these draggable/resize handles ever worked with HTML5 media.

Comment 2

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/popcorn-js/pull/331

I can add tests for this if you feel the need. Probably should.

Comment 3

[Scott [:thecount] Downe]

Comment on attachment 772110 [details] [review]
https://github.com/mozilla/popcorn-js/pull/331

The new draggable handles do not work. The click events did not get cloned with it.

Would love it if we found a solution that didn't need to clone, and worked in IE9.

Comment 4

[Scott [:thecount] Downe]

I wonder if IE9 is going to be fine with this: http://pastebin.com/FXdZkmSU

Comment 5

[Matthew Schranz [:mjschranz]]

Comment on attachment 772110 [details] [review]
https://github.com/mozilla/popcorn-js/pull/331

Updated.

Comment 6

[Scott [:thecount] Downe]

Comment on attachment 772110 [details] [review]
https://github.com/mozilla/popcorn-js/pull/331

Close, but the new container needs so css, or else the video's aspect ratio and height are off.

Comment 7

[[github robot]]

Commit pushed to master at https://github.com/mozilla/popcorn-js

https://github.com/mozilla/popcorn-js/commit/bb226a9a541e5d1f0b5343ffadf0c67bcef431b6
Fix Bug 890556 - Popcorn.smart shouldn't overwrite any existing child elements inside it's container when creating HTML5 media.

Comment 8

[George Marshall]

Attachment: This is a minor patch to fix an issue with broken player unit test.

Comment 9

[George Marshall]

Comment on attachment 8454167 [details] [diff] [review]
This is a minor patch to fix an issue with broken player unit test.

>https://github.com/mozilla/popcorn-js/pull/402

Comment 10

[Matthew Schranz [:mjschranz]]

George,

Could you file a new bug describing the issue you've found and submit your patch there?

Thanks for the patch! Awesome.

Comment 11

[George Marshall]

Does this really deserve a new ticket? I am not sure why the original fix was accepted because it does not pass the unit tests in "modules/player/popcorn.player.unit.html".

Comment 12

[Matthew Schranz [:mjschranz]]

Every change deserves a bug to be filled. That we can can track changes that were made in the future incase we need to understand why something was done.

The normal course of action would be to reference this bug in that new ticket.