WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#39272 closed defect (bug) (fixed)

Twenty Seventeen: Incorrect $content_width

Reported by: celloexpressions Owned by: laurelfulford
Milestone: 4.7.1 Priority: normal
Severity: major Version: 4.7
Component: Bundled Theme Keywords: has-patch fixed-major
Focuses: Cc:

Description

Twenty Seventeen defines a wider content width on the front page in
twentyseventeen_content_width():

$content_width = 700;

if ( twentyseventeen_is_frontpage() ) {
	$content_width = 1120;
}

But the front page does not use a different content width than other pages. The only case I can think of where it might change is when the one column layout option is used.

Also, I'm not seeing an actual content width of 700px anywhere. Depending on screen size, it looks like it's roughly 520px (+/- 5px).

Incorrectly setting the $content_width causes media embeds to end up with the wrong aspect ratio, potentially among other issues. I'm surprised that this was not picked up sooner; I hadn't looked closely through functions.php but noticed this in passing while working on adding some inline docs there.

Attachments (7)

39272.patch (497 bytes) - added by sstoqnov 4 months ago.
Remove front-page content_width change
39272.1.patch (1.9 KB) - added by laurelfulford 3 months ago.
39272.2.patch (1.2 KB) - added by sstoqnov 3 months ago.
Combine all content_width checks in twentyseventeen_content_width function
39272.3.patch (1.2 KB) - added by sstoqnov 3 months ago.
Change comments from 39272.2.patch to match WordPress coding standards
39272.4.patch (1.6 KB) - added by sstoqnov 3 months ago.
Set the default content width at after_setup_theme. Change twentyseventeen_content_width function hook to template_redirect
39272.5.patch (1.6 KB) - added by sstoqnov 3 months ago.
Remove tab spaces on empty lines
39272.6.patch (1.5 KB) - added by sstoqnov 3 months ago.

Download all attachments as: .zip

Change History (25)

@sstoqnov
4 months ago

Remove front-page content_width change

#1 @sstoqnov
4 months ago

  • Keywords has-patch reporter-feedback added; needs-patch removed

It seems that the content width check was added so it can be removed the check in front-page.php in the following ticket
https://github.com/WordPress/twentyseventeen/pull/371/files#diff-78cd5aa3783a74555c9938a2a81d01c6

I checked the front-page.php revisions in github and it seems that this is there from the initial commit.
Not sure why it was added.
https://github.com/WordPress/twentyseventeen/blob/69482880b4ae2ec145f60df417423665581a5653/front-page.php

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


3 months ago

#3 @davidakennedy
3 months ago

  • Keywords needs-refresh added; has-patch reporter-feedback removed
  • Owner set to laurelfulford
  • Status changed from new to assigned

Thanks for the ticket @celloexpressions!

I missed this in all my code reviews, and it's something that should be addressed as soon as possible.

Thanks for the patch @sstoqnov, but we'll need to cover more cases than just removing that code.

The widths break down like this:

Static Front Page:
Two Column: 525px (524.31px)
One Column: 644px

Regular Pages:
Two Column: 525px (524.31px)
One Column: 740px

Blog:
Two Column (with sidebar): 525px (524.31px)
One column (no sidebar): 740px

I'm going to assign this to @laurelfulford so she can take a stab at a patch. We can all help test though.

#4 @laurelfulford
3 months ago

  • Keywords has-patch added; needs-refresh removed

Thanks for catching this @celloexpressions!

39272.1.patch builds off the patch @sstoqnov submitted, plus:

  • Changes the default $content_width to 525
  • Updates the $content_width on the front-page.php to 644px when the one column layout is used.
  • Updates the $content_width on pages to 740px when the one column layout is used.
  • Updates the $content_width on single posts when no sidebar widgets are assigned.

The blog index, archive and search all show the sidebar widgets as well, but when they're not present it doesn't affect the width of the content area.

Last edited 3 months ago by laurelfulford (previous) (diff)

#5 @celloexpressions
3 months ago

Rather than changing it in each template, would it work to keep all of the checks in functions.php with conditional for the possible different sizes? That would be easier to follow and override in child themes.

I'm actually not sure how well the editor handles applying different content widths on different views, but at least the frontend should be correct with the changes either way.

@sstoqnov
3 months ago

Combine all content_width checks in twentyseventeen_content_width function

#6 @sstoqnov
3 months ago

  • Keywords needs-testing added

Thank for the comment @davidakennedy.

@celloexpressions I agree with you that will be better to have everything in functions.php, but after spending some time I reallized that using after_setup_theme action doesn't allow us to use conditional tags. The previous check that I have removed also doesn't work, because twentyseventeen_is_frontpage always rerun false.

We can use wp hook, so that everything we need will be loaded.

In 39272.2.patch I replaced after_setup_theme with wp action and combine the good work from @laurelfulford in twentyseventeen_content_width function.

#7 @davidakennedy
3 months ago

  • Keywords dev-feedback added

Thanks for the work here @laurelfulford and @sstoqnov!

I'd definitely prefer keeping all the content width changes in one function to make it easier to understand and work with for developers.

Some tiny feedback for 39272.2.patch @sstoqnov, there are a few comments without spaces after the //. Also, can we end those in a full stop (period)? That's per coding standards we're trying to improve in #39152. :) Otherwise, it's looking good.

In testing, this worked well. I can verify that the after_setup_theme hook isn't sufficient here. From reading the source code, it does seem like the wp hook would be best to use. But I'd like to get a second opinion to be sure. @obenland Is wp the best hook to use here since the code needs to use some conditional tags and theme functions?

@sstoqnov
3 months ago

Change comments from 39272.2.patch to match WordPress coding standards

#8 @sstoqnov
3 months ago

@davidakennedy Agree, I have updated my patch to match the coding standards.

Regarding the hook, this was the first one that all conditional tags can be used. I agree that there may have a better one.

#9 @obenland
3 months ago

  • Keywords dev-feedback removed

In Twenty Thirteen we used template_redirect, wp would work just as well probably.
In any case I'd split this up and define a default value at after_theme_setup so that plugins have something to work with at init.

@sstoqnov
3 months ago

Set the default content width at after_setup_theme. Change twentyseventeen_content_width function hook to template_redirect

#10 @davidakennedy
3 months ago

  • Keywords needs-refresh added

Thanks for the feedback helpful @obenland!

Thanks for the refreshed patch @sstoqnov! It's looking good, and I think this is almost ready. One tiny point of feedback: there are tab spaces on the empty lines in the code. Can you fix those?

Otherwise, this tested well, and I don't see any issues.

@sstoqnov
3 months ago

Remove tab spaces on empty lines

#11 @sstoqnov
3 months ago

  • Keywords needs-refresh removed

Hey @davidakennedy, thank you for your comment, I have updated my patch.

Last edited 3 months ago by sstoqnov (previous) (diff)

#12 @laurelfulford
3 months ago

  • Keywords needs-refresh added; has-patch needs-testing removed

Thanks @sstoqnov!

I took a peek and it looks like there are still two tabs on empty lines remaining -- on line 57 and line 206. The others are fixed.

Can you please submit one more patch with these removed -- thank you!

@sstoqnov
3 months ago

#13 @sstoqnov
3 months ago

  • Keywords needs-refresh removed

Hey @laurelfulford, thank you for finding this. I am sure that the spaces have been removed in my last patch, but you are right that they still exist. Maybe I have uploaded a wrong page... however a new patch has been uploaded.

Thank you!

#14 @sstoqnov
3 months ago

  • Keywords has-patch added

#15 @davidakennedy
3 months ago

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

In 39635:

Twenty Seventeen: Fix incorrect $content_width value in theme

This addresses a major bug. Incorrectly setting the $content_width causes media embeds to end up with the wrong aspect ratio, among other issues. This fix uses template_redirect, to ensure conditional theme tags can be used. It also defines a default value at after_theme_setup so that plugins have something to work with at init.

Props sstoqnov, laurelfulford, obenland.

Fixes #39272.

#16 @davidakennedy
3 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#17 @dd32
3 months ago

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

In 39650:

Twenty Seventeen: Fix incorrect $content_width value in theme.

This addresses a major bug. Incorrectly setting the $content_width causes media embeds to end up with the wrong aspect ratio, among other issues. This fix uses template_redirect, to ensure conditional theme tags can be used. It also defines a default value at after_theme_setup so that plugins have something to work with at init.

Props sstoqnov, laurelfulford, obenland.
Merges [39635] to the 4.7 branch.
Fixes #39272.

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


3 months ago

Note: See TracTickets for help on using tickets.