WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#30078 closed defect (bug) (fixed)

Video dimensions in MediaElement

Reported by: azaozz Owned by: wonderboymusic
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords:
Focuses: rtl Cc:

Description

When we add a video (both on the front-end and in wpView), we add inline style with width and height on the wrapper div. Then we add max-width: 100%; height: auto; in wp-mediaelement.css, however the height is already overridden.

That results in empty space under the video when max-width comes into effect on the wrapper div.

Attachments (3)

video-height.png (70.1 KB) - added by azaozz 6 years ago.
Screenshot_2014-10-26_12_25_45.png (970.8 KB) - added by jessepollak 6 years ago.
With no height inline style, the height is usually auto detected and works.
Screenshot_2014-10-26_12_25_55.png (999.5 KB) - added by jessepollak 6 years ago.
Failure case where no height inline style is set

Download all attachments as: .zip

Change History (10)

@azaozz
6 years ago

#1 @jessepollak
6 years ago

OK, I did some looking into this. This is happening because the inner video height is shrinking (width & height changed with JS in mediaelement.js) while the outer div.wp-video container has the fixed height. That creates the big gap of whitespace underneath.

One potential solution is to remove the inline height style — this works correctly if we: (1) start with a large screen size, then shrink the width so the max-width rule applies; (2) don't change the screen width at all. However, this fails if we start with a small screen size (where the max-width rule applies) and then expand the window width so it no longer does.

In the failure case, the width of the video stays correct, but the height is fixed at the original, shorter height (skewing the video with an incorrect aspect ratio). This happens because for some reason mediaelement.js does not change the height if it loads as less then the auto height from the set width. I'm attaching two screenshots below that show this happening. This may be a bug upstream in mediaelement.js, but I don't know the code well enough to comment on that.

@jessepollak
6 years ago

With no height inline style, the height is usually auto detected and works.

@jessepollak
6 years ago

Failure case where no height inline style is set

#2 @azaozz
6 years ago

So the solution is to remove height from all wrappers (or set it to auto).

#3 @jessepollak
6 years ago

That definitely solves the general case, but there's the edge case I mention above. If that's OK, I'm happy to upload a patch (or you can just do it — just delete [these three lines](https://github.com/WordPress/WordPress/blob/master/wp-includes/media.php#L1880).

#4 @wonderboymusic
6 years ago

I had to add the inline height due to some MEjs weirdness in the last release - since then, I had a pull request accepted upstream that fixed some of the problems. Any solution needs to be tested across all browsers and all video file types.

#5 @wonderboymusic
6 years ago

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

In 30082:

Don't hardcode height for videos - this was a workaround for MediaElement internals causing problems. Responsive videos now work properly and don't cause extra whitespace.

Fixes MediaElement by hand in the interim: https://github.com/johndyer/mediaelement/pull/1337
Video playlists were completely broken by this.

Fixes #30078.

#6 @boonebgorges
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[30082] breaks Tests_Media:test_video_shortcode_body. Looks like the expected markup needs to be updated.

#7 @wonderboymusic
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 30132:

Fix unit test for video shortcode after [30082].

Fixes #30078.

Note: See TracTickets for help on using tickets.