888293 - Inconsistency in Popcorn Embed shell in Firefox

Status: VERIFIED FIXED

(Webmaker Graveyard :: Make Valet, defect)

Description

[Brett Gaylor [:brett]]

Feel free to duplicate this on another mobile view bug or other, but I'm noticing that Popcorn projects really aren't fitting the browser window.

I'm looking at this on FF desktop, and you can see that I have to scroll down to get controls.  This isn't awesome.

https://dl.dropboxusercontent.com/u/7968133/images/dougvideo.png

Comment 1

[Brett Gaylor [:brett]]

Some additional screenshots show difference between Firefox and Chrome.

https://dl.dropboxusercontent.com/u/7968133/images/firefoxfullscreen.png

https://dl.dropboxusercontent.com/u/7968133/images/chromefullscreen.png

Comment 2

[iamjayakumars]

I'm also facing the same issue in Firefox Nightly....

Comment 3

[Scott [:thecount] Downe]

So, kinda a weird bug, and do not yet fully understand it. 

It looks like it is only old makes that have this.

Republishing might fix it.

Not happening on my system, only happens on some systems, might be related to old makes and retina screens? Just a hunch at this point.

Comment 4

[iamjayakumars]

I created a popcorn recently, in that also i'm facing the same issue

Check the images

http://www.flickr.com/photos/98095128@N03/9407477767/

http://www.flickr.com/photos/98095128@N03/9407477767/

Comment 5

[iamjayakumars]

Hi, I remixed a popcorn, in that also i'm facing the same issue
*https://iamjayakumars.makes.org/popcorn/1a64

here is screenshot
*http://www.flickr.com/photos/98095128@N03/9410291556/

Comment 6

[Scott [:thecount] Downe]

Attachment: https://github.com/mozilla/popcorn.webmaker.org/pull/146

Comment 7

[Scott [:thecount] Downe]

Comment on attachment 783919 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/146

I'm fairly certain this is caused by media queries being optimized to common screen sizes, triggering on width only. So as soon as someone's common screen includes something like a mac doc, this fails to pick up on it. It is no longer considered common.

Or other screens that change up the height.

I made a patch that makes height trigger media query as well as width. I don't see why we don't do this anyway. We do in the editor.

Comment 8

[Matthew Schranz [:mjschranz]]

Comment on attachment 783919 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/146

I'm no CSS ninja so it's harder for me to truly review it well but I have some questions in there.

Also, it seems like the height changes are going overboard and causing this to be much smaller than we intended.

Master: https://dl.dropboxusercontent.com/u/8586348/embed_master.png

Branch: https://dl.dropboxusercontent.com/u/8586348/embed_patch.png

Can we possibly make it not so extreme?

Comment 9

[Scott [:thecount] Downe]

I cannot use the height you're seeing on master, it is cutting off some of the controls.

What you're asking is to change the preset player sizes to better fit your screen.

We can review the current screen sizes we snap to, if you believe yours is a common one, we can add it.

Can you give me the values, though?

This is a bit of a silly problem. I wonder if we could support completely dynamic player sizes instead of these pre set media query sizes?

Comment 10

[Scott [:thecount] Downe]

I think Kate needs to take a look at this at some point.

Comment 11

[Brett Gaylor [:brett]]

The two screenshots that matt showed actually look fine to me.  I assume he is looking at this on a large monitor - on a laptop that is probably a pretty perfect size.

Comment 12

[Matthew Schranz [:mjschranz]]

The screenshots I showed are both on a 15" laptop screen.

Comment 13

[Scott [:thecount] Downe]

Attachment: https://github.com/mozilla/make-valet/pull/32

If Jon is busy, Matt can do the bulk of the review.

I think Jon just needs to do a quick sanity check that I'm loading and structuring things in a make-valet like way.

I made sure this still works in thimble.

What we end up with is a popcorn embed that fills as much space as it can, is fully displayed, and keeps aspect ratio.

Comment 14

[Scott [:thecount] Downe]

Depends on bug 936622, that is, bug 936622 needs to be on prod before this can be on prod.

Comment 15

[Jon Buckley [:jbuck]]

Comment on attachment 831739 [details] [review]
https://github.com/mozilla/make-valet/pull/32

This works well, but I wonder if this can be accomplished entirely with CSS? That would be a nicer solution, for sure.

Comment 16

[Matthew Schranz [:mjschranz]]

Do you mean using media queries Jon? Because that's something we specifically don't want to do.

Comment 17

[Scott [:thecount] Downe]

There is a hack using paddingBottom as a percentage to get us half way there, all css.

Unfortunatly it does not work for paddingRight, thus we need some javascript. I am however only using it for the one part, I do use css where I can.

Anyway, it is a valid complaint and I'll take another look to see if there is a pure css solution for all cases.

Comment 18

[Scott [:thecount] Downe]

So, here is how far I can get this with all css, using the padding-bottom hack: http://jsfiddle.net/4vDph/

Resize that, and you'll see it keeps the aspect ratio by resizing the height.

It however does not limit the height to the container's height, just the width to the container's width. Height detection is ultimatly what we need to solve the above problem of bottom portions being hidden when a browser window is not close to 16:9 or is failing to get the proper media query.

We actually already do the above solution with padding-bottom, it was just hidden under media queries which were needed for text, which had no css solution. We now do text resizing with onresize anyway. We still do it, I use onresize to pick up with padding-bottom left off.

Anyway, I'm never going to say something is impossible. In order to say that I would have to be omniscient. I can however tell you my current research I cannot find a css only solution that fits our needs. Nor can I find a reason for onresize being problematic.

Comment 19

[Scott [:thecount] Downe]

Staged: https://github.com/mozilla/make-valet/commit/678a3ee6f4184c381cb3445803b30b854ec842ec

Needs verification.