WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 3 weeks ago

#49318 closed enhancement (fixed)

Twenty-Twenty: content font CSS selector is too important

Reported by: alexandreb3 Owned by: ianbelanger
Milestone: 5.4 Priority: normal
Severity: normal Version: 5.3
Component: Bundled Theme Keywords: has-patch commit dev-reviewed fixed-major
Focuses: css Cc:

Description

The CSS selector used to define the content font-family is too important.

https://themes.trac.wordpress.org/browser/twentytwenty/1.1/style.css?rev=122235#L3524

.entry-content p,
.entry-content ol,
.entry-content ul,
.entry-content dl,
.entry-content dt {
        font-family: NonBreakingSpaceOverride, "Hoefler Text", Garamond, "Times New Roman", serif;
        letter-spacing: normal;
}

As a result, it's very difficult to override without custom CSS (especially for beginners that doen't code). Even a page builder can't edit the font without custom CSS (example with Elementor here https://youtu.be/FW-6rbDd4WI)

Why not only use this to set the default font ?

.entry-content{
        font-family: NonBreakingSpaceOverride, "Hoefler Text", Garamond, "Times New Roman", serif;
        letter-spacing: normal;
}

This way the main font-family is set for all the content and can be easily customized without custom CSS.

Attachments (4)

49318.diff (602 bytes) - added by larrach 2 months ago.
first patch
49318.1.diff (1.2 KB) - added by audrasjb 2 months ago.
Added RTL styles
49318.2.diff (2.2 KB) - added by ianbelanger 4 weeks ago.
Fixes the heading font and a few other elements that needed it
49318.3.diff (2.2 KB) - added by ianbelanger 3 weeks ago.
Rearranged selectors as per Sergey's recommendation

Download all attachments as: .zip

Change History (25)

@larrach
2 months ago

first patch

#1 @larrach
2 months ago

  • Keywords has-patch needs-testing added

Hi,
I made a first patch for this ticket, I think it's worth testing this patch to look for indesirables effects.
cheers
Rach

@audrasjb
2 months ago

Added RTL styles

#2 @audrasjb
2 months ago

  • Keywords dev-feedback added; needs-testing removed
  • Milestone changed from Awaiting Review to 5.4

Hi @alexandreb3 and welcome to WordPress Core Trac,

This demand makes sense to me.

49318.1.diff should fix the issue for both LTR and RTL stylesheets. It works fine on my side on a fresh install.

Ping @ianbelanger for a detailed review.

#3 @ianbelanger
2 months ago

  • Keywords commit added
  • Owner set to ianbelanger
  • Status changed from new to reviewing

This looks good @audrasjb, marking for commit

#4 @ianbelanger
2 months ago

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

In 47133:

Bundled Themes: Twenty Twenty content font CSS selector is too important.

This makes the font family selector for entry-content less specific and thus easier to override.

Props alexandreb3, larrach, audrasjb.
Fixes #49318.

#5 @ianbelanger
2 months ago

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

Reopening for backport

#6 @audrasjb
2 months ago

  • Keywords needs-dev-note added

#7 @audrasjb
2 months ago

  • Keywords dev-feedback commit fixed-major removed
  • Milestone changed from 5.3.3 to 5.4
  • Resolution set to fixed
  • Status changed from reopened to closed

Hi,
As per yesterday’s devchat and previous discussions in the minor release squad, let's move all Bundled Themes tickets from milestone 5.3.3 to 5.4 as there is no plan for a 5.3.3 release for now (except if it's a security release, which may of course happen at any time).

#8 @ianbelanger
8 weeks ago

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

Hi @audrasjb, this has already been committed and then reopened for backport, that is why it was in the 5.3.3 milestone and tagged with fixed-major. I am going to move it back into 5.3.3 and reopen. Thanks

Last edited 8 weeks ago by ianbelanger (previous) (diff)

#9 @alexandreb3
7 weeks ago

Hi,

With additional testing I noticed that the enhancement had an impact on content headings (they don't have the correct font anymore).

To fix this, we need to update the code as the following :

/* Font Families ----------------------------- */
​
.entry-content{
	font-family: NonBreakingSpaceOverride, "Hoefler Text", Garamond, "Times New Roman", serif;
	letter-spacing: normal;
}
​
.entry-content h1,
.entry-content h2,
.entry-content h3,
.entry-content h4,
.entry-content h5,
.entry-content h6,
.entry-content cite,
.entry-content figcaption,
.entry-content .wp-caption-text {
	font-family: -apple-system, BlinkMacSystemFont, "Helvetica Neue", Helvetica, sans-serif;
}
​
@supports ( font-variation-settings: normal ) {
​
	.entry-content h1,
	.entry-content h2,
	.entry-content h3,
	.entry-content h4,
	.entry-content h5,
	.entry-content h6,
	.entry-content cite,
	.entry-content figcaption,
	.entry-content .wp-caption-text {
		font-family: "Inter var", -apple-system, BlinkMacSystemFont, "Helvetica Neue", Helvetica, sans-serif;
	}
}

#10 @audrasjb
4 weeks ago

  • Keywords needs-dev-note fixed-major removed
  • Resolution set to fixed
  • Status changed from reopened to closed

#11 @audrasjb
4 weeks ago

  • Milestone changed from 5.3.3 to 5.4

#12 @audrasjb
4 weeks ago

Just noting this ticket need to be reopened once 5.4 RC1 is out, to fix comment 9 issue.
I'll take care of reopening it tomorrow.

#13 @audrasjb
4 weeks ago

  • Keywords needs-patch needs-design added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening this ticket to handle the issue reported in comment number 9.

This ticket was mentioned in Slack in #core-themes by ianbelanger. View the logs.


4 weeks ago

@ianbelanger
4 weeks ago

Fixes the heading font and a few other elements that needed it

#15 @ianbelanger
4 weeks ago

  • Keywords has-patch commit added; needs-patch needs-design removed

49318.2.diff should take care of the font issues introduced by [47133]. Since we are officially in the RC phase, I need another committer to review before I can commit this. @SergeyBiryukov could you take a look?

#16 @sabernhardt
3 weeks ago

Would it be better to revert the change (for now)? It seems adding font styles to the headings trades one set of selectors for another instead of simplifying them. And people probably have used the original set already to override fonts in the customizer or a child theme.

If I'm wrong, just say so. :)

#17 @SergeyBiryukov
3 weeks ago

  • Keywords dev-reviewed added

49318.2.diff looks good to me, though the list of selectors (e.g. adding table and address) seems somewhat arbitrary. If those are necessary, could they be placed next to cite and figcaption, for consistency?

Reverting the original change for now would indeed be another option. I'm leaving the decision to @ianbelanger as the Bundled Theme component maintainer.

Last edited 3 weeks ago by SergeyBiryukov (previous) (diff)

@ianbelanger
3 weeks ago

Rearranged selectors as per Sergey's recommendation

#18 @ianbelanger
3 weeks ago

  • Status changed from reopened to reviewing

#19 @ianbelanger
3 weeks ago

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

In 47439:

Bundled Themes: Twenty Twenty content font CSS selector is too important - updated.

This adds more selectors for headings, tables, addresses, cite, figcaption, file and caption blocks to make the font-family match as before [47133].

Props alexandreb3, SergeyBiryukov.
Fixes #49318.

#20 @ianbelanger
3 weeks ago

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

Since trunk is now the 5.5 branch, I believe that this needs to be backported into 5.4.

#21 @SergeyBiryukov
3 weeks ago

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

In 47440:

Bundled Themes: Twenty Twenty content font CSS selector is too important - updated.

This adds more selectors for headings, tables, addresses, cite, figcaption, file and caption blocks to make the font-family match as before [47133].

Props alexandreb3, ianbelanger.
Reviewed by SergeyBiryukov.
Merges [47439] to the 5.4 branch.
Fixes #49318.

Note: See TracTickets for help on using tickets.