-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Paragraph: Add e2e tests for computed block styles #61932
base: trunk
Are you sure you want to change the base?
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: 0 B Total Size: 1.76 MB ℹ️ View Unchanged
|
/** | ||
* Deletes all post revisions using the REST API. | ||
* | ||
* @param {} this RequestUtils. | ||
* @param {string|number} parentId Post attributes. | ||
*/ | ||
async function getThemeGlobalStylesRevisions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this from another file, but it seems the description is wrong, so I'll update it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably one of my copy-paste doozies, thanks for updating.
return themeGlobalStylesId; | ||
} | ||
|
||
async function updateGlobalStyles( this: RequestUtils, styles: Object ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to add doc blocks.
Hopefully I don't have to add types for the global styles object 🙏
If there's a type somewhere else that can be imported that would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked in core-data, and it's mostly using any
or Record< string, Object >
as return values for selectors 😄 .
I did create an interface for GlobalStylesRevisions, I guess it would also be helpful to have one for global styles.
This comment is zero helpful I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I didn't know those existed. Could be useful for the utils if we can import the types from there. I'd feel better about adding a global styles type in that location.
// fontStyle, fontWeight, fontSize. | ||
await expect( block ).toHaveCSS( | ||
'font', | ||
'100 11px / 12.1px Times' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the test is failing, probably because on linux it evaluates a different font name (Times vs Times New Roman).
I'll break this out into multiple assertions for each property to fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @talldan 👍
The general approach LGTM. It's easy to follow and looks easy to spin up new tests for other blocks or edge cases, which should help increase adoption and provide greater confidence in our style system.
I can definitely see these sorts of tests catching a few regressions if we can get sufficient coverage.
Approach aside, there are a couple of tweaks I think we can make for the paragraph block specific tests which I've left as inline comments.
Thanks for getting the ball rolling on style tests!
); | ||
} ); | ||
|
||
await test.step( 'Block instance styles', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the paragraph block, it might be good to test the padding against a couple of paragraph block instances. That way we can confirm:
- When a background is set on the instance, the padding is overridden as per the
p.has-background
style - When the instance has background and padding styles, the instance padding styles take precedence
This test would then also cover the change to p.has-background
specificity in #61638.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, there was already one assertion for the first case in the colors tests, but I moved them to the 'dimensions' section and added an extra assertion for the instance padding overriding the has-background
padding.
In case it helps, below is a quick diff relating to the suggestions I left in my review above: Suggested tweaksdiff --git a/test/e2e/specs/editor/block-computed-styles/paragraph-styles.spec.js b/test/e2e/specs/editor/block-computed-styles/paragraph-styles.spec.js
index 29bfa5dadc..512d38916f 100644
--- a/test/e2e/specs/editor/block-computed-styles/paragraph-styles.spec.js
+++ b/test/e2e/specs/editor/block-computed-styles/paragraph-styles.spec.js
@@ -365,38 +365,85 @@ test.describe( 'Paragraph computed styles', () => {
} );
await test.step( 'Block instance styles', async () => {
- const instanceStyles = {
- spacing: {
- margin: {
- top: '11px',
- bottom: '12px',
- },
- padding: {
- top: '13px',
- right: '14px',
- bottom: '15px',
- left: '16px',
- },
- },
+ const marginStyles = {
+ top: '11px',
+ right: '12px',
+ bottom: '17px',
+ left: '18px',
+ };
+ const paddingStyles = {
+ top: '13px',
+ right: '14px',
+ bottom: '15px',
+ left: '16px',
};
await admin.createNewPost();
+
+ // Paragraph is wrapped in a group block with a default layout so
+ // that left/right margins will not be overridden by the layout.
+ await editor.insertBlock( {
+ name: 'core/group',
+ layout: { type: 'default' },
+ innerBlocks: [
+ {
+ name: 'core/paragraph',
+ attributes: {
+ content:
+ 'Hello <a href="https://wordpress.org">world</a>',
+ backgroundColor: 'primary',
+ style: {
+ spacing: { margin: marginStyles },
+ },
+ },
+ },
+ ],
+ } );
await editor.insertBlock( {
name: 'core/paragraph',
attributes: {
content: 'Hello <a href="https://wordpress.org">world</a>',
- style: instanceStyles,
+ style: {
+ color: { background: '#2d2d2d' },
+ spacing: {
+ padding: paddingStyles,
+ },
+ },
},
} );
- const block = editor.canvas.getByRole( 'document', {
+ const paragraphs = editor.canvas.getByRole( 'document', {
name: 'Block: Paragraph',
} );
- // Left/right margin is set to auto by alignments, there's no reliable way to test this.
- await expect( block ).toHaveCSS( 'margin-top', '11px' );
- await expect( block ).toHaveCSS( 'margin-bottom', '12px' );
- await expect( block ).toHaveCSS( 'padding', '13px 14px 15px 16px' );
+ await expect( paragraphs.first() ).toHaveCSS(
+ 'margin-top',
+ '11px'
+ );
+ await expect( paragraphs.first() ).toHaveCSS(
+ 'margin-right',
+ '12px'
+ );
+ await expect( paragraphs.first() ).toHaveCSS(
+ 'margin-bottom',
+ '17px'
+ );
+ await expect( paragraphs.first() ).toHaveCSS(
+ 'margin-left',
+ '18px'
+ );
+ // p.has-background padding is 1.25em 2.375. At 16px font size padding should be 20px 38px.
+ await expect( paragraphs.first() ).toHaveCSS(
+ 'padding',
+ '20px 38px'
+ );
+
+ // Block instance padding styles should override the default padding
+ // when background is applied.
+ await expect( paragraphs.nth( 1 ) ).toHaveCSS(
+ 'padding',
+ '13px 14px 15px 16px'
+ );
} );
} );
} );
I've tested these against this branch and everything passes. After cherry picking this PR's commits to #61638, they also failed at the expected styles. |
// Note: This is different to what I reproduce locally. | ||
// Bottom margin should be 0px for the last block. | ||
await expect( nestedParagraph3 ).toHaveCSS( | ||
'margin', | ||
'1px 2px 3px 4px' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only oddity I'm seeing, when I tested this manually the bottom margin was 0px
for this paragraph 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can reproduce this now. It seems like the last-child
style doesn't get applied straight away. Steps to reproduce:
- Set some non-zero random margin values for the paragraph block type in global styles
- Create a group with two paragraphs
- Select the group and toggle 'Inner blocks use content width'
- Without selecting it, inspect the styles of the last paragraph, observe that it has the margin you specified in global styles
- Now select and deselect the last paragraph
- Inspect the styles again, observe that there's now a new style for the
last-child
that sets bottom margin to zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think the issue is that the group block is selected, and that means a block-list-appender
is inserted at the end of the group block. The appender is the last-child
so it receives the :last-child
margin and not the last paragraph.
It's very noticeable when the global styles bottom margin is set to a huge value.
I'll make a separate issue for it. In the editor, the :last-child
style should I guess only be applied to blocks.
For the test, I'll make it deselect blocks before making the assertion.
edit: I see @aaronrobertshaw beat me to it (#61846) 😄
a10f171
to
5297d6b
Compare
E2E tests now passing. I'm ignoring the 'Compressed Size' failure, which isn't relevant to this PR anyway. 😄 The big omission from this PR is testing the frontend styles. I'd like to spend a bit more time try to find a nice way to do that. |
getCurrentThemeGlobalStylesPostId, | ||
getThemeGlobalStylesRevisions, | ||
updateGlobalStyles, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@talldan, do you think these utils will be helpful in other tests? The best practice is to avoid overloading global utils in favor of local utils - https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/e2e/README.md#dont-overload-test-utils-inline-simple-utils.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two of them were already present, I just moved them to a different file.
updateGlobalStyles
is the only new one, which I think will be very useful, as I plan to add some more of these tests for different blocks and combinations of blocks.
It might also be useful for clearing global styles changes as part of the test cleanup, though perhaps we also want to clear revisions when doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateGlobalStyles
is potentially helpful for testing revisions, see: https://github.com/WordPress/gutenberg/blob/add/e2e-tests-for-block-styles/test/e2e/specs/site-editor/user-global-styles-revisions.spec.js#L304 👍🏻
6db5e93
to
fe2af4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! There's a test failure though. Any clue?
const globalStylesURL = | ||
currentTheme?._links?.[ 'wp:user-global-styles' ]?.[ 0 ]?.href; | ||
if ( globalStylesURL ) { | ||
themeGlobalStylesId = globalStylesURL?.split( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should we throw an error to indicate there's no post id found? Maybe undefined
or null
would be better than an empty string too.
* set styles. Styles that you want to keep should be merged in manually. | ||
* | ||
* @param {RequestUtils} this RequestUtils. | ||
* @param {Object} styles A theme.json compatible object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The jsdoc type isn't needed if we're using typescript, it's annoying to keep them in sync anyway 😅.
fe2af4e
to
c8b7294
Compare
What?
One of the big gaps in our automated test coverage is that we lack assertions that the generated styles for blocks are correct.
The block editor's styling system is a layered complex system with global element styles, global styles for block types, block instance styles, block style variations, and probably more, all of which have an order of precedence. It's often too easy to cause a regression as this as the generated CSS can depend upon specificity for the style rules, as well as the ordering for the CSS.
At the moment this PR only adds a test for the paragraph block, but once the approach is agreed upon it should be easier to replicate these tests across different block types, and also add some combinations of blocks for layouts and section styling.
How?
The tests are structured like this:
test.describe
)test
for each type of style (e.g. color, typography, spacing)test.step
for each layer of style starting from the top (usually global element styles), and working down to the bottom (usually block instance styles) and for each step checking that it overrides the previous layer.I've tried to keep the assertions as simple as possible, they just use
toHaveCss
which runsgetComputedStyle
in the background.A new
updateGlobalStyles
util is added, so the test doesn't have to tinker with the UI. I think there need to be separate e2e tests for the global styles UI, as that seems like something else where there's little coverage.