WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#29146 closed defect (bug) (fixed)

Audio/Video in Twenty Thirteen are lacking bottom margin

Reported by: wonderboymusic Owned by: mikeyarce
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.6
Component: Bundled Theme Keywords: good-first-bug has-patch commit
Focuses: ui Cc:

Description

This is probably a regression, just want to track with a ticket.

Posts that have audio and video: the comment link is too close to the media.

Attachments (10)

thirteen-audio-video.png (174.5 KB) - added by obenland 3 years ago.
Screen Shot 2014-08-07 at 2.42.00 PM.png (279.8 KB) - added by wonderboymusic 3 years ago.
trac-29146.png (343.1 KB) - added by amcreative 3 years ago.
Screenshot with video from WordPress.tv
29146.diff (457 bytes) - added by mikeyarce 3 years ago.
fix bottom margin for video, videopress, and audio embeds
29146.1.diff (568 bytes) - added by mikeyarce 3 years ago.
updated path
29146.2.diff (397 bytes) - added by jeanyoungkim 3 years ago.
29146.3.diff (652 bytes) - added by ccprice 3 years ago.
29146.4.diff (708 bytes) - added by DavidTheMachine 3 years ago.
change filepaths so patch will apply
29146.5.diff (1.5 KB) - added by mikeyarce 3 years ago.
Fixes bottom margin in Twenty Ten, Twenty Eleven, and Twenty Thirteen for the Video Player, VideoPress, and Audio Player elements in the Standard Post Format. Tested all default themes up to Twenty Fifteen - all other themes either have top margin in the footer .entry-meta or aren't affected.
29146.6.diff (1.8 KB) - added by gregrickaby 3 years ago.
Includes all fixes in .5, includes support for .format-audio

Download all attachments as: .zip

Change History (34)

#1 @obenland
3 years ago

The above shows how it looks to me. Could you share a screenshot of how it looks on your end?

#2 @wonderboymusic
3 years ago

yeah - so I think the issue is non-post-format'd media

#3 @obenland
3 years ago

Right, with an embed as the last element that happens to not be wrapped in a paragraph.

#4 @obenland
3 years ago

Is this a 2013 issue or an inconsistency in how embeds are output?

#5 @wonderboymusic
3 years ago

I was testing the MediaElement upgrade (still hasn't landed) in the past 5 themes, my notes tell me that 2013 was the only problem. I then noticed that the problem persisted without the upgrade.

@amcreative
3 years ago

Screenshot with video from WordPress.tv

#6 in reply to: ↑ description @amcreative
3 years ago

I was not able to replicate this issue. Above is a screenshot using a video from WordPress.tv as a standard post and a video post format. I also embedded a video from Youtube with similar results.

#7 @kraftbj
3 years ago

I could replicate it when using a [video]or [audio] shortcode. YouTube oEmbed isn't impacted since it places the video within <p> tags.

The <div> tags for the embeds are what's lacking.

@mikeyarce
3 years ago

fix bottom margin for video, videopress, and audio embeds

#8 @kraftbj
3 years ago

@mikeyarce - could you update the patch to be relative to root? (e.g. for src/wp-content/themes/twentythirteen/style.css instead of simply style.css)

Makes it easier to test (e.g. grunt patch:29146 would be able to apply the patch). :)

@mikeyarce
3 years ago

updated path

#9 @kraftbj
3 years ago

  • Keywords has-patch added; needs-patch removed

@jeanyoungkim
3 years ago

@ccprice
3 years ago

#10 @wonderboymusic
3 years ago

Helped a few of my colleagues navigate this ticket - the CSS rules need to be scoped to standard post format when the audio or video shortcode is not wrapped in a <p>

#11 @obenland
3 years ago

Looking at 29146.3.diff:
We currently don't have a Post Format section for standard post formats in style.css, so we should probably add that and combine the styles. Does the selector have to be that specific? Wouldn't .format-standard .wp-video suffice?

I also wonder why is this only a bug in Twenty Thirteen, how do other default themes handle that scenario?

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


3 years ago

#13 @obenland
3 years ago

  • Milestone changed from 4.0 to Future Release
  • Version changed from trunk to 3.6

Let's look at it in 4.1 again.

@DavidTheMachine
3 years ago

change filepaths so patch will apply

#14 @DavidTheMachine
3 years ago

I'm currently researching obenland's question in comment comment:11, and discovered that patch 29146.3 wasn't applying, so I fixed it.

Last edited 3 years ago by DavidTheMachine (previous) (diff)

#15 @DavidTheMachine
3 years ago

In response to comment:11:

I imported theme-unit-test.xml into the developer install, and checked the default themes as released with 4.0.

The table below shows whether the elements have a bottom margin applied by the style.

twentyten twentyeleven twentytwelve twentythirteen twentyfourteen
Post Format: Audio no no yes yes yes
Post Format: Video (WordPress.TV) yes no yes no -
Post Format: Video (VideoPress) - - no no no
Post Format: Video (YouTube) yes yes yes yes yes

- Theme does not support the underlying shortcode, even with Jetpack installed

Last edited 3 years ago by DavidTheMachine (previous) (diff)

#16 @DrewAPicture
3 years ago

  • Owner set to mikeyarce
  • Status changed from new to assigned

#17 @mikeyarce
3 years ago

@kraftbj and I will be looking at this ticket in the next week and putting together the final patches for all default themes.

#18 @obenland
3 years ago

Is this a themes issue though, or an issue with the different ways that videos are output?

#19 @kraftbj
3 years ago

@obenland - Not totally sure. We'll check that out. Either way, aiming to have something to test in time for Beta 1.

@mikeyarce
3 years ago

Fixes bottom margin in Twenty Ten, Twenty Eleven, and Twenty Thirteen for the Video Player, VideoPress, and Audio Player elements in the Standard Post Format. Tested all default themes up to Twenty Fifteen - all other themes either have top margin in the footer .entry-meta or aren't affected.

#20 @lancewillett
3 years ago

  • Keywords needs-testing added
  • Milestone changed from Future Release to 4.1

If another person could apply latest patch and test -- and report back, we can get this in.

@gregrickaby
3 years ago

Includes all fixes in .5, includes support for .format-audio

#21 @balbert
3 years ago

applied 29146.6.diff and appears to be working in TwentyTen, TwentyEleven and TwentyThriteen for both video and audio players. I tested using the theme unit test data for post lists and video and audio post formats.

This ticket was mentioned in Slack in #core by blobaugh. View the logs.


3 years ago

#23 @lancewillett
3 years ago

  • Keywords commit added; needs-testing removed

#24 @lancewillett
3 years ago

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

In 30371:

Twenty Ten, Eleven, Thirteen: add bottom margin to audio and video players.

Fixes #29146. Props mikeyarce, gregrickaby, DavidTheMachine, jeanyoungkim, ccprice.

Note: See TracTickets for help on using tickets.