Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#53605 closed defect (bug) (fixed)

Twenty Twenty-One: Investigate duplicate rules in `ie-editor.css`

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.9
Component: Bundled Theme Keywords: has-patch good-first-bug commit
Focuses: Cc:

Description

Spun off from ticket:53560#comment:19:

It seems weird that these rules are fully duplicated in the assets/css/ie-editor.css file:

select {
	border: 3px solid #39414d;
	border-radius: 0;
	color: #28303d;
	font-size: 1.125rem;
	-moz-appearance: none;
	-webkit-appearance: none;
	appearance: none;
	padding: 10px 30px 10px 10px;
	background: #fff url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10' fill='%2328303d'><polygon points='0,0 10,0 5,5'/></svg>") no-repeat;
	background-position: right 10px top 60%;
}

select:focus {
	border: 3px solid #39414d;
	border-radius: 0;
	color: #28303d;
	font-size: 1.125rem;
	-moz-appearance: none;
	-webkit-appearance: none;
	appearance: none;
	padding: 10px 30px 10px 10px;
	background: #fff url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10' fill='%2328303d'><polygon points='0,0 10,0 5,5'/></svg>") no-repeat;
	background-position: right 10px top 60%;
}

while they are properly combined in assets/css/style-editor.css:

select,
select:focus {
	border: var(--form--border-width) solid var(--form--border-color);
	border-radius: var(--form--border-radius);
	color: var(--form--color-text);
	font-size: var(--form--font-size);
	-moz-appearance: none;
	-webkit-appearance: none;
	appearance: none;
	padding: var(--form--spacing-unit) calc(3 * var(--form--spacing-unit)) var(--form--spacing-unit) var(--form--spacing-unit);
	background: var(--global--color-white) url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10' fill='%2328303d'><polygon points='0,0 10,0 5,5'/></svg>") no-repeat;
	background-position: right var(--form--spacing-unit) top 60%;
}

Looking closer, ie-editor.css has a lot more unnecessarily duplicated rules, for example:

.has-background .has-link-color a {
	color: #28303d;
}

.has-background.has-link-color a {
	color: #28303d;
}

I would expect to see this instead:

.has-background .has-link-color a,
.has-background.has-link-color a {
	color: #28303d;
}

Could it be an issue with PostCSS configuration? I've noticed that postcss.config.js for the theme requires the postcss-discard-duplicates module, but it seems like it's not being used, or I'm misunderstanding how it works.

Attachments (2)

53605.diff (1.2 KB) - added by Shaharyar10 3 years ago.
Created patch.
53605.1.diff (2.5 KB) - added by Shital Patel 3 years ago.
I added css in ie-editor.css and ie.css . please check and let me know if any changes.

Download all attachments as: .zip

Change History (17)

#1 @SergeyBiryukov
3 years ago

  • Reporter changed from desrosj to SergeyBiryukov

Thanks for moving this to a new ticket! Just changing the Reporter field for clarity, since it was my comment :)

#2 follow-up: @ryelle
3 years ago

The postcss-discard-duplicates plugin gets rid of duplicate rules, it wouldn't combine different selectors like that.

It looks like it's probably the postcss-css-variables plugin, which is used to support IE11 and below. In fact, there's an issue for unnecessarily splitting up comma-separated rules, which seems to be what's happening here. The CSS in style-editor.css

.has-background .has-link-color a,
.has-background.has-link-color a {
	color: var(--wp--style--color--link, var(--global--color-primary));
}

is transformed to this in ie-editor.css

.has-background .has-link-color a {
	color: #28303d;
}

.has-background.has-link-color a {
	color: #28303d;
}

They suggest using a minification plugin or postcss-merge-rules to merge these back together. … Since I'm here, I gave the merge-rules plugin a try and it works surprisingly well. I don't have my IE machine up at the moment, so I can't test the output, but it looks correct.

Just noting, this isn't the first issue we've had with postcss-css-variables - see #52158 where the postcss-discard-duplicates plugin was added to clean up another bug.

#3 @SergeyBiryukov
3 years ago

  • Milestone changed from Future Release to 5.9

#4 @Shaharyar10
3 years ago

  • Version set to trunk

Looking closer, assets/css/ie-editor.css has a lot more unnecessarily duplicated rules,

blockquote cite {
	font-weight: normal;
	letter-spacing: normal;
}

blockquote footer {
	font-weight: normal;
	letter-spacing: normal;
}

I would expect to see this instead:

blockquote cite,
blockquote footer {
	font-weight: normal;
	letter-spacing: normal;
}

also for this

blockquote.alignleft p {
	font-size: 1.125rem;
	max-width: inherit;
	width: inherit;
}

blockquote.alignright p {
	font-size: 1.125rem;
	max-width: inherit;
	width: inherit;
}

which should be like

blockquote.alignleft p,
blockquote.alignright p {
	font-size: 1.125rem;
	max-width: inherit;
	width: inherit;
}

Also there is another one

blockquote.alignleft cite {
	font-size: 1rem;
	letter-spacing: normal;
}

blockquote.alignleft footer {
	font-size: 1rem;
	letter-spacing: normal;
}

blockquote.alignright cite {
	font-size: 1rem;
	letter-spacing: normal;
}

blockquote.alignright footer {
	font-size: 1rem;
	letter-spacing: normal;
}

and it should be like

blockquote.alignleft cite,
blockquote.alignleft footer,
blockquote.alignright cite,
blockquote.alignright footer {
	font-size: 1rem;
	letter-spacing: normal;
}

@Shaharyar10
3 years ago

Created patch.

@Shital Patel
3 years ago

I added css in ie-editor.css and ie.css . please check and let me know if any changes.

#5 @Shital Patel
3 years ago

  • Keywords has-patch added; good-first-bug removed

#6 @Shital Patel
3 years ago

  • Owner set to Shital Patel
  • Status changed from new to accepted

#7 @Shital Patel
3 years ago

  • Keywords good-first-bug added

#8 in reply to: ↑ 2 @mukesh27
3 years ago

  • Owner Shital Patel deleted
  • Status changed from accepted to assigned

We already fix a similar issue before so I think we have to go ahead with the solution suggested below.

Replying to ryelle:

They suggest using a minification plugin or postcss-merge-rules to merge these back together. … Since I'm here, I gave the merge-rules plugin a try and it works surprisingly well. I don't have my IE machine up at the moment, so I can't test the output, but it looks correct.

Just noting, this isn't the first issue we've had with postcss-css-variables - see #52158 where the postcss-discard-duplicates plugin was added to clean up another bug.

@shital-patel @Shaharyar10 Thanks for the patch. IE related CSS is generated automatically in the build process.

This ticket was mentioned in Slack in #core-test by justinahinon. View the logs.


2 years ago

#10 @poena
2 years ago

I tried two things:
-The pull request as is
-Adding only the following changes to the theme (trunk):

Package.json, added {{{"postcss-merge-rules": "^5.0.2"}}}, to devDependencies.
(Deleted the lock file to regenerate it)

postcss.config.js added {{{require('postcss-merge-rules')}}} to plugins.

I saw no issues on npm install.
When I try to generate the updated CSS files, npm run start fails with:

assets/css/ie-editor.css
 1257:43  ✖  Unexpected empty block   block-no-empty

assets/css/ie.css
 2722:43  ✖  Unexpected empty block   block-no-empty
 4205:43  ✖  Unexpected empty block   block-no-empty

-There are no empty blocks on these lines.

If I only run the build:ie or build:ie-editor commands,
there are no error reports or warnings and most of the duplicate CSS is solved.

I am not sure how to solve the above problem with npm run start.

If it matters, my version of NPM is 6.14.15 (because Gutenberg is not compatible with higher and I use the same environment for testing both)

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

#11 @poena
2 years ago

As for the resulting CSS, I do not see any new issue when I test the theme in IE11.

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

#12 @ryelle
2 years ago

  • Keywords commit added; needs-testing removed

I just rebased the branch to get it up to date, and I'm not seeing any build errors. Since the built CSS looks good, I think this should get committed.

#13 @peterwilsoncc
2 years ago

I am seeing linting errors running npm run lint:scss but they seem unrelated to this change as they exist in trunk without the patch applied.

I agree this can be committed as the linting errors are pre-existing.

#14 @peterwilsoncc
2 years ago

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

In 52238:

Twenty Twenty-One: Combine duplicate CSS rules.

Use the postcss-merge-rules package when building TT1 stylesheets to reduce duplicate code in production CSS.

Props SergeyBiryukov, ryelle, Shaharyar10, shital-patel, mukesh27, poena.
Fixes #53605.

ryelle commented on PR #1475:


2 years ago
#15

committed in r52238.

Note: See TracTickets for help on using tickets.