WordPress.org

Make WordPress Core

Opened 8 weeks ago

Last modified 8 weeks ago

#43897 reopened defect (bug)

Coding standards: audit a few occurrences of CSS font-weight using keywords instead of numeric values

Reported by: afercia Owned by: afercia
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: ui, coding-standards Cc:

Description

Just noticed a few occurrences of font-weight: bold; or font-weight: normal; have been recently introduced in core.

As per the CSS Coding Standards:

Font weights should be defined using numeric values (e.g. 400 instead of normal, 700 rather than bold).

Rationale: see [37740] Use numeric font weights instead of keywords.

When Open Sans was in use, the 300, 400, and 600 weights were loaded. 400 is the equivalent of normal; however, bold is equivalent to 700, not 600. With the move to system fonts, we need to be specific rather than relying on the lack of a 700 weight.

Attachments (1)

43897.diff (1.4 KB) - added by afercia 8 weeks ago.

Download all attachments as: .zip

Change History (15)

@afercia
8 weeks ago

#1 @afercia
8 weeks ago

  • Keywords has-patch added; needs-patch removed

#2 @afercia
8 weeks ago

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

In 43018:

Coding standards: Use numeric font weights instead of keywords.

Fixes #43897.

#3 @afercia
8 weeks ago

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

Reopening for 4.9.6 consideration: some of the bold weights are used in the new Privacy Policy settings screen.

#4 @netweb
8 weeks ago

  • Focuses coding-standards added

#5 @SergeyBiryukov
8 weeks ago

Was going to merge, but it looks like [43018] depends on the styles added in [42967] and [42978], so they need to be merged first.

#6 @afercia
8 weeks ago

There's one occurrence of font weight 700 in the theme details, that appears only when a theme is a child theme and should be changed to 600. See the name of the parent theme in the screenshot below:

https://cldup.com/sb2uNtd-zz.png

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

#7 @afercia
8 weeks ago

In 43021:

Coding standards: Change one occurrence of font weight 700 to 600.

Props chetan200891.
Amends [43018].
See #43897.

#8 @afercia
8 weeks ago

Note: there are several occurrences of font weight set to normal in font shorthand declarations. Normally, they should be changed to 400 but, as far as I see, they're all related to dashicons. For example:

font: normal 20px/1 dashicons;

So I'm not fully sure they should be changed. Noticed also in few cases, the shorthand syntax misses the font weight, for example (it should fallback to the initial value normal):

font: 20px/.5 dashicons;

#9 @afercia
8 weeks ago

In 43041:

Coding standards: Change a few occurrences of font weight 700 to 600 in multisite signup and activate.

Props chetan200891.
Amends [43018].
See #43897.

#10 @ocean90
8 weeks ago

  • Keywords fixed-major removed
  • Milestone changed from 4.9.6 to 5.0

Moving to next major milestone due to the lack of significant changes that would warrant the back-port.

#11 @SergeyBiryukov
8 weeks ago

Anything left here?

#12 follow-up: @afercia
8 weeks ago

Anything left here?

Haven't found other occurrences, so everything should be OK. As per the milestoning, I'm not happy to release something that goes against the coding standards, even if it's just about a bold value.

#13 in reply to: ↑ 12 @ocean90
8 weeks ago

Replying to afercia:

I'm not happy to release something that goes against the coding standards, even if it's just about a bold value.

Did you take a look at all the privacy code? It doesn't follow the coding standards in a few places. I'd be more concerned if something gets released which doesn't follow a11y standards, like https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/user.php?rev=43085&marks=1352-1355,1394-1397#L1352.

#14 @afercia
8 weeks ago

Worth a ticket.

Note: See TracTickets for help on using tickets.