WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#23955 closed defect (bug) (fixed)

Twenty Thirteen: Embedded media positioned incorrectly at <999px breakpoints

Reported by: sabreuse Owned by: markjaquith
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Bundled Theme Keywords: needs-testing has-patch
Focuses: Cc:

Description (last modified by sabreuse)

At tablet and smaller sizes, embedded media are shifted offscreen to the left rather than being centered in the layout. Patch attached.

props to alun for reporting the issue and the fix here: http://wordpress.org/support/topic/twenty-thirteen-responsive-layout-for-entry-media?replies=1

Attachments (12)

sidemedia.diff (452 bytes) - added by sabreuse 2 years ago.
2013-video-truncated.png (81.8 KB) - added by sabreuse 2 years ago.
2013-video-patched.png (206.2 KB) - added by sabreuse 2 years ago.
23955.diff (1.6 KB) - added by obenland 2 years ago.
23955.2.diff (3.0 KB) - added by obenland 2 years ago.
23955.3.diff (469 bytes) - added by obenland 2 years ago.
Re-introduce max-widths for audio post formats. There was a reason we had them.
23955.video-width.diff (1.9 KB) - added by obenland 2 years ago.
23955.video-width.2.diff (1.3 KB) - added by obenland 2 years ago.
23955.4.diff (503 bytes) - added by obenland 2 years ago.
Make sure to only alter the width on the frontend.
23955.5.diff (1.8 KB) - added by wonderboymusic 2 years ago.
23955.6.diff (2.5 KB) - added by wonderboymusic 2 years ago.
23955.7.diff (2.5 KB) - added by wonderboymusic 2 years ago.

Download all attachments as: .zip

Change History (30)

@sabreuse2 years ago

comment:1 @sabreuse2 years ago

  • Description modified (diff)

comment:2 @SergeyBiryukov2 years ago

  • Component changed from General to Bundled Theme
  • Milestone changed from Awaiting Review to 3.6
  • Version set to trunk

comment:3 follow-up: @lancewillett2 years ago

  • Keywords has-patch reporter-feedback added

@sabreuse Could you help us out by attaching before and after screenshots? Also, browser info is always helpful for repeating the bugs.

@sabreuse2 years ago

comment:4 in reply to: ↑ 3 @sabreuse2 years ago

  • Keywords reporter-feedback removed

Replying to lancewillett:

@sabreuse Could you help us out by attaching before and after screenshots? Also, browser info is always helpful for repeating the bugs.

Sure thing -- sorry I didn't think of it before:

Currently, seeing this on both Chrome & Firefox:


After patch:


@obenland2 years ago

comment:5 @obenland2 years ago

  • Keywords needs-testing added

Sabreuse's patch went in the right direction, but we need a bit more verbose. I only tested the patch with images, have not had the chance to test it with videos.

We need to account for all screen sizes, with and w/o a sidebar, LTR and RTL.

@obenland2 years ago

comment:6 @lancewillett2 years ago

In 23964:

Twenty Thirteen: better handling of media in main content area, accounting for a sidebar and all viewport width possibilities. Props sabreuse and obenland, see #23955.

comment:7 @lancewillett2 years ago

  • Keywords has-patch removed

Leaving this ticket open for more testing.

@obenland2 years ago

Re-introduce max-widths for audio post formats. There was a reason we had them.

comment:8 @lancewillett2 years ago

Seems somewhere along the line video embeds, like YouTube URLs stopped being filtered to the correct width of 724, with twentythirteen_content_width() that fires on 'template_redirect'. It's broken on the index view or any view where the video post isn't the first in the results. (Works OK on single.)

@obenland2 years ago

comment:9 @lancewillett2 years ago

In 23971:

Twenty Thirteen: reinstate Audio post format max-width layout handling, lost in r23964. See #23955, props obenland.

comment:10 @lancewillett2 years ago

In 23972:

Twenty Thirteen: adjust content_width value for video shortcodes in video post formats and on attachment templates. See #23955, props obenland.

@obenland2 years ago

Make sure to only alter the width on the frontend.

comment:11 @obenland2 years ago

.4 restricts the new width to the frontend.

@wonderboymusic2 years ago

comment:12 @wonderboymusic2 years ago

  • Keywords has-patch added

if you have a giant video and then filter only width, and it happens to equal content width, the aspect ratio is going to get effed up - I think constraints need to happen in the shortcode, which every video passes through. My patch only works if the 2013 filter isn't there. Let's discuss.

My patch is necessary anyways to constrain giant videos from rendering bigger than $content_width in the admin and on the front end

@wonderboymusic2 years ago

comment:13 @wonderboymusic2 years ago

23955.6.diff​ fixes TwentyThirteen as well

@wonderboymusic2 years ago

comment:14 @wonderboymusic2 years ago

23955.7.diff​ uses $content_width instead of hard-coding 724

comment:15 @wonderboymusic2 years ago

use .6.diff instead of .7.diff - $content_width is dynamic in some cases

comment:16 @markjaquith2 years ago

Needs refresh.

What exactly is the problem at this juncture? How can I test it to verify that the patch fixes it?

comment:17 @markjaquith2 years ago

Nevermind, doesn't need refresh.

comment:18 @markjaquith2 years ago

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

In 23989:

Constrain large videos from rendering bigger than $content_width on both frontend and backend.

props wonderboymusic. fixes #23955.

Note: See TracTickets for help on using tickets.