Skip to content
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.

Sign up for GitHub

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

List Block: Fix numbering style to be applied correctly #53301

Merged
merged 9 commits into from
Sep 14, 2023
Merged

List Block: Fix numbering style to be applied correctly #53301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
34593a3
List Block: Fix numbering style to be applied correctly
t-hamano Aug 3, 2023
974ef4e
Use inline style instead of type attributes
t-hamano Aug 5, 2023
8e2c68a
Add deprecation
t-hamano Aug 5, 2023
5be90bc
Try to fix unit test
t-hamano Aug 5, 2023
bcfd325
Try to fix unit test
t-hamano Aug 6, 2023
1885959
Try to fix raw-handling test
t-hamano Aug 6, 2023
3e66d7d
Fix fixture files
t-hamano Aug 6, 2023
b57f19a
refactoring
t-hamano Aug 6, 2023
c3bbf51
Merge branch 'trunk' into fix/list-number-type
t-hamano Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 84 additions & 3 deletions packages/block-library/src/list/deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
/**
* WordPress dependencies
*/
import { RichText, useBlockProps } from '@wordpress/block-editor';
import { RichText, InnerBlocks, useBlockProps } from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import migrateFontFamily from '../utils/migrate-font-family';
import { migrateToListV2 } from './utils';
import { migrateToListV2, migrateTypeToInlineStyle } from './utils';

const v0 = {
attributes: {
Expand Down Expand Up @@ -138,6 +138,87 @@ const v1 = {
migrate: migrateToListV2,
};

// In #53301 changed to use the inline style instead of type attribute.
const v2 = {
attributes: {
ordered: {
type: 'boolean',
default: false,
__experimentalRole: 'content',
},
values: {
type: 'string',
source: 'html',
selector: 'ol,ul',
multiline: 'li',
__unstableMultilineWrapperTags: [ 'ol', 'ul' ],
default: '',
__experimentalRole: 'content',
},
type: {
type: 'string',
},
start: {
type: 'number',
},
reversed: {
type: 'boolean',
},
placeholder: {
type: 'string',
},
},
supports: {
anchor: true,
className: false,
typography: {
fontSize: true,
lineHeight: true,
__experimentalFontFamily: true,
__experimentalFontWeight: true,
__experimentalFontStyle: true,
__experimentalTextTransform: true,
__experimentalTextDecoration: true,
__experimentalLetterSpacing: true,
__experimentalDefaultControls: {
fontSize: true,
},
},
color: {
gradients: true,
link: true,
__experimentalDefaultControls: {
background: true,
text: true,
},
},
spacing: {
margin: true,
padding: true,
__experimentalDefaultControls: {
margin: false,
padding: false,
},
},
__unstablePasteTextInline: true,
__experimentalSelector: 'ol,ul',
__experimentalSlashInserter: true,
},
isEligible( { type } ) {
return !! type;
},
save( { attributes } ) {
const { ordered, type, reversed, start } = attributes;
const TagName = ordered ? 'ol' : 'ul';
return (
<TagName { ...useBlockProps.save( { type, reversed, start } ) }>
<InnerBlocks.Content />
</TagName>
);
},
migrate: migrateTypeToInlineStyle,
};

/**
* New deprecations need to be placed first
* for them to have higher priority.
Expand All @@ -146,4 +227,4 @@ const v1 = {
*
* See block-deprecation.md
*/
export default [ v1, v0 ];
export default [ v2, v1, v0 ];
9 changes: 6 additions & 3 deletions packages/block-library/src/list/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,14 @@ function IndentUI( { clientId } ) {
}

export default function Edit( { attributes, setAttributes, clientId, style } ) {
const { ordered, type, reversed, start } = attributes;
const blockProps = useBlockProps( {
...( Platform.isNative && { style } ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this was ignored for native. Does that have any consequences for the new inline style?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this line mean that the Edit component's style prop is used only on native platforms? This line appears to have originally been changed in #42711, which changed the List block to use nested blocks.

https://github.com/WordPress/gutenberg/pull/42711/files#diff-99bb126c574fb4790a60e7b6cf372c8a8bce3114f8a9740d0f92e47e74238724R133

style: {
...( Platform.isNative && style ),
listStyleType: ordered && type !== 'decimal' ? type : undefined,
},
} );

const innerBlocksProps = useInnerBlocksProps( blockProps, {
allowedBlocks: [ 'core/list-item' ],
template: TEMPLATE,
Expand All @@ -140,7 +145,6 @@ export default function Edit( { attributes, setAttributes, clientId, style } ) {
__experimentalCaptureToolbars: true,
} );
useMigrateOnLoad( attributes, clientId );
const { ordered, type, reversed, start } = attributes;

const controls = (
<BlockControls group="block">
Expand Down Expand Up @@ -172,7 +176,6 @@ export default function Edit( { attributes, setAttributes, clientId, style } ) {
ordered={ ordered }
reversed={ reversed }
start={ start }
type={ type }
{ ...innerBlocksProps }
/>
{ controls }
Expand Down
25 changes: 20 additions & 5 deletions packages/block-library/src/list/ordered-list-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,26 @@ const OrderedListSettings = ( { setAttributes, reversed, start, type } ) => (
__nextHasNoMarginBottom
label={ __( 'Numbering style' ) }
options={ [
{ value: '1', label: __( 'Numbers' ) },
{ value: 'A', label: __( 'Uppercase letters' ) },
{ value: 'a', label: __( 'Lowercase letters' ) },
{ value: 'I', label: __( 'Uppercase Roman numerals' ) },
{ value: 'i', label: __( 'Lowercase Roman numerals' ) },
{
label: __( 'Numbers' ),
value: 'decimal',
},
{
label: __( 'Uppercase letters' ),
value: 'upper-alpha',
},
{
label: __( 'Lowercase letters' ),
value: 'lower-alpha',
},
{
label: __( 'Uppercase Roman numerals' ),
value: 'upper-roman',
},
{
label: __( 'Lowercase Roman numerals' ),
value: 'lower-roman',
},
] }
value={ type }
onChange={ ( newValue ) => setAttributes( { type: newValue } ) }
Expand Down
11 changes: 10 additions & 1 deletion packages/block-library/src/list/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,16 @@ export default function save( { attributes } ) {
const { ordered, type, reversed, start } = attributes;
const TagName = ordered ? 'ol' : 'ul';
return (
<TagName { ...useBlockProps.save( { type, reversed, start } ) }>
<TagName
{ ...useBlockProps.save( {
reversed,
start,
style: {
listStyleType:
ordered && type !== 'decimal' ? type : undefined,
},
} ) }
>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we changing the save function if we're only needing to fix something in the admin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR settled on the solution of using the inline style (list-style-type) because the type attribute of the list would be overridden on the editor side. Shouldn't the save function side also be updated to match the editor side?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, I misunderstood and thought this was the previous fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How were these styles injected into the frontend before this? Why did we decide to use inline styles now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the type attribute is deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another reason is that the type attribute is overwritten by CSS styles (see). We also tried overriding it with a CSS style using the type attribute as a selector, but there was also the problem that uppercase and lowercase letters were not distinguished (see).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok Thanks for the details. 👍

<InnerBlocks.Content />
</TagName>
);
Expand Down
23 changes: 22 additions & 1 deletion packages/block-library/src/list/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,23 @@
*/
import { createBlock, rawHandler } from '@wordpress/blocks';

const LIST_STYLES = {
A: 'upper-alpha',
a: 'lower-alpha',
I: 'upper-roman',
i: 'lower-roman',
};

export function createListBlockFromDOMElement( listElement ) {
const type = listElement.getAttribute( 'type' );
const listAttributes = {
ordered: 'OL' === listElement.tagName,
anchor: listElement.id === '' ? undefined : listElement.id,
start: listElement.getAttribute( 'start' )
? parseInt( listElement.getAttribute( 'start' ), 10 )
: undefined,
reversed: listElement.hasAttribute( 'reversed' ) ? true : undefined,
type: listElement.getAttribute( 'type' ) ?? undefined,
type: type && LIST_STYLES[ type ] ? LIST_STYLES[ type ] : undefined,
};

const innerBlocks = Array.from( listElement.children ).map(
Expand Down Expand Up @@ -78,3 +86,16 @@ export function migrateToListV2( attributes ) {
listBlock.innerBlocks,
];
}

export function migrateTypeToInlineStyle( attributes ) {
const { type } = attributes;

if ( type && LIST_STYLES[ type ] ) {
return {
...attributes,
type: LIST_STYLES[ type ],
};
}

return attributes;
}
12 changes: 6 additions & 6 deletions test/integration/__snapshots__/blocks-raw-handling.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ exports[`Blocks raw handling pasteHandler google-docs-table 1`] = `"<br><br><br>

exports[`Blocks raw handling pasteHandler google-docs-table-with-colspan 1`] = `"<br><br>Test colspan<br><br>"`;

exports[`Blocks raw handling pasteHandler google-docs-table-with-rowspan 1`] = `"<br><br>Test rowspan<br><br>"`;

exports[`Blocks raw handling pasteHandler google-docs-table-with-comments 1`] = `"<br><br><br><br>One<br>Two<br>Three<br>1<br>2<br>3<br>I<br>II<br>III"`;

exports[`Blocks raw handling pasteHandler google-docs-table-with-rowspan 1`] = `"<br><br>Test rowspan<br><br>"`;

exports[`Blocks raw handling pasteHandler google-docs-with-comments 1`] = `"This is a <strong>title</strong><br><br>This is a <em>heading</em><br><br>Formatting test: <strong>bold</strong>, <em>italic</em>, <a href="https://w.org/">link</a>, <s>strikethrough</s>, <sup>superscript</sup>, <sub>subscript</sub>, <strong><em>nested</em></strong>.<br><br>A<br>Bulleted<br>Indented<br>List<br><br>One<br>Two<br>Three<br><br><br><br><br>One<br>Two<br>Three<br>1<br>2<br>3<br>I<br>II<br>III<br><br><br><br><br><br>An image:<br><br><img src="https://lh4.googleusercontent.com/ID" width="544" height="184"><br><br>"`;

exports[`Blocks raw handling pasteHandler gutenberg 1`] = `"Test"`;
Expand Down Expand Up @@ -174,10 +174,10 @@ exports[`rawHandler should convert a caption shortcode with link 1`] = `
`;

exports[`rawHandler should convert a list with attributes 1`] = `
"<!-- wp:list {"ordered":true,"type":"i","start":2,"reversed":true} -->
<ol type="i" reversed start="2"><!-- wp:list-item -->
<li>1<!-- wp:list {"ordered":true,"type":"i","start":2,"reversed":true} -->
<ol type="i" reversed start="2"><!-- wp:list-item -->
"<!-- wp:list {"ordered":true,"type":"lower-roman","start":2,"reversed":true} -->
<ol reversed start="2" style="list-style-type:lower-roman"><!-- wp:list-item -->
<li>1<!-- wp:list {"ordered":true,"type":"lower-roman","start":2,"reversed":true} -->
<ol reversed start="2" style="list-style-type:lower-roman"><!-- wp:list-item -->
<li>1</li>
<!-- /wp:list-item --></ol>
<!-- /wp:list --></li>
Expand Down
0 ...ures/blocks/core__list__ol-with-type.html → ...res/blocks/core__list__deprecated-v2.html
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion ...ures/blocks/core__list__ol-with-type.json → ...res/blocks/core__list__deprecated-v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"attributes": {
"ordered": true,
"values": "",
"type": "A"
"type": "upper-alpha"
},
"innerBlocks": [
{
Expand Down
0 ...ocks/core__list__ol-with-type.parsed.json → ...cks/core__list__deprecated-v2.parsed.json
View file
Open in desktop
File renamed without changes.
5 changes: 5 additions & 0 deletions test/integration/fixtures/blocks/core__list__deprecated-v2.serialized.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<!-- wp:list {"ordered":true,"type":"upper-alpha"} -->
<ol style="list-style-type:upper-alpha"><!-- wp:list-item -->
<li>Item 1</li>
<!-- /wp:list-item --></ol>
<!-- /wp:list -->
5 changes: 0 additions & 5 deletions test/integration/fixtures/blocks/core__list__ol-with-type.serialized.html
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions test/integration/fixtures/documents/ms-word-out.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ <h2 class="wp-block-heading">This is a heading level 2</h2>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->

<!-- wp:list {"ordered":true,"type":"1"} -->
<ol type="1"><!-- wp:list-item -->
<!-- wp:list {"ordered":true} -->
<ol><!-- wp:list-item -->
<li>One</li>
<!-- /wp:list-item -->

Expand Down
Loading