Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#52564 closed defect (bug) (fixed)

Twenty Twenty-One has unit-less CSS variable unfit for calc()

Reported by: nico23's profile nico23 Owned by: ryelle's profile ryelle
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: css Cc:

Description

I was building something with calc on top of --global--admin-bar--height and found out through painful trail and error that why my code would not work.

--my-var: 10px;

calc(var(--my-var) + var(--global--admin-bar--height, 0px));

The issue with this is that calc NEEDS a unit. Otherwise, it fails! And because the value on root the default value will never be used. For my case I think it's actually better to have no value on root at all (for default) but the root value should be 0px because its pretty much made for calc.

:root {
	/* Admin-bar height */
	--global--admin-bar--height: 0; // should be 0px
}

.admin-bar {
	--global--admin-bar--height: 32px;
}
@media only screen and (max-width: 782px) {

	.admin-bar {
		--global--admin-bar--height: 46px;
	}
}

The theme actually uses it with calc.

	.admin-bar .primary-navigation > .primary-menu-container {
		height: calc(100vh - var(--global--admin-bar--height));
	}

I have not tested this code, but I am pretty sure this will NOT work. When the admin bar is not active because calc will fail with unit-less value 0.

I think WordPress core should maybe just include this CSS on all pages so both themes and plugins can utilize it and not have to repeat the code. There is already global admin bar CSS included so why not with this.

Change History (11)

#1 @joyously
2 years ago

CSS variables work best with no units, because you can't manipulate them like strings. And because of that, it's best if they are percentages.
But you can use a unitless variable in calc(), by multiplying by the unit, like 1px or 1rem or 1%.

#2 @ryelle
2 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.7
  • Summary changed from Twenty Twenty has unit-less CSS variable unfit for calc() to Twenty Twenty-One has unit-less CSS variable unfit for calc()

How interesting! A unitless value is allowed in calc for multiplication and division, but not in addition/subtraction. Always something new to learn with CSS :)

The value is 0 by default for logged out users who don't have an admin bar, so you don't need to pass through a default in var. You're right that the usage in the theme doesn't work, but it doesn't actually cause a bug. That said, we should fix it - the :root value should get changed to 0px so all instances of this variable use the same unit.

Targeting this to 5.7 since it would be a quick fix if someone can write a patch soon.

#3 @nico23
2 years ago

  • Keywords needs-patch removed
  • Summary changed from Twenty Twenty-One has unit-less CSS variable unfit for calc() to Twenty Twenty has unit-less CSS variable unfit for calc()

@joyously

I meant the 21 theme obviously.

I do not get what you are saying. Work best with no units? I showed the opposite this BREAKS and not "works best". Then you follow up and say they should be percentages? I do not get what you are saying.

As I said with this case the --global--admin-bar--height is like the perfect example for a variable used to calc() positioning with (very likely with addition). It literally BREAKS and has unexpected consequences because you can not use it with in a unit calculation. Well not in addition/substation to be perfectly correct as @ryelle just pointed out.

I copied it to my plugin anyway, but what do you think about just adding this to core? I mean WP already puts out some .admin-bar CSS anyway when you logged in. To not logged-in users the value could either be omitted completely, so people can use their own 0px default or just include it with 0px

I do not understand why this CSS spec does this, its possible there is a good reason but its pretty annoying to run into, especially when you work with SCSS rules that actually say you should use unitless 0 instead of 0px, 0em ... So I have to put an ignore comment for the lines in or globally allow the rules.

Last edited 2 years ago by nico23 (previous) (diff)

This ticket was mentioned in PR #1018 on WordPress/wordpress-develop by nextgenthemes.


2 years ago
#4

  • Keywords has-patch added

Make value not break in css calc() subsctation/addition.

Trac ticket: https://core.trac.wordpress.org/ticket/52564

#5 @ryelle
2 years ago

  • Owner set to ryelle
  • Status changed from new to reviewing

#6 @ryelle
2 years ago

I copied it to my plugin anyway, but what do you think about just adding this to core?

I like that idea. I'm going to mark this ticket fixed when I commit your theme fix, so could you open another ticket for this idea? We've started some discussions on custom properties related to colors, but this is another place where that could help out developers. Thanks!

#7 @ryelle
2 years ago

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

In 50388:

Twenty Twenty-One: Add the px unit to the admin bar height custom property.

The admin bar height custom property is used in calc functions, which require consistent unit use, even when the value is zero. A unitless value returns an invalid value for the "height" property, where this variable is used. This also changes the stylelint rule in the theme to allow zero values with a unit, just in custom properties.

Props nico23.
Fixes #52564.

#8 @ryelle
2 years ago

  • Summary changed from Twenty Twenty has unit-less CSS variable unfit for calc() to Twenty Twenty-One has unit-less CSS variable unfit for calc()

ryelle commented on PR #1018:


2 years ago
#9

This was committed in r50388 — thanks for your work!

This ticket was mentioned in Slack in #core-css by ryelle. View the logs.


2 years ago

#11 @desrosj
2 years ago

In 50975:

Twenty Twenty-One: Re-add px unit to the admin bar height custom property.

The px unit added in [50388] to prevent invalid values when using calc functions was inadvertently removed in [50972].

This re-adds the unit to ensure the issue reported in #52564 remains fixed. The same stylelint rule adjustment made in [50388] to .stylelintrc-css.json has also been copied over to stylelintrc.json file, which is the configuration file used when linting .scss files.

Props ocean90.
See #52624, #52564.

Note: See TracTickets for help on using tickets.