List Block: Fix numbering style to be applied correctly #53301
Conversation
t-hamano
force-pushed
the
fix/list-number-type
branch
from
724e38c to
377f799
Compare
t-hamano
added
[Type] Bug
|
Size Change: +160 B (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
|
This tested well for me in the editor, but I noticed there is a similar issue in the front end with some non-block themes, eg. TT1. In this case the themes own style is overriding the user styles assigned in the editor: 
I wonder if we should be doing something to try and give the styles for this a higher specificity in the frontend 🤔 This could be a follow up PR if we do. |
Would it hurt to use |
I guess it depends on the selectors a theme might be using - this is why the other user applied global styles use a |
|
Thanks for the review, @ramonjd, @glendaviesnz. I am a bit confused about applying this style to the front end as well. At the very least, this is because the markup must be updated for this attribute alone as follows: Before <ul type="a">
<li>List Item</li>
<li>List Item</li>
</ul>After <ul type="a" class="is-type-a">
<li>List Item</li>
<li>List Item</li>
</ul>On the back end, However, the classic theme may need a good deal of updating to properly apply these styles on the front end. There may be some burden on the theme developer and the default theme regarding updates, but I personally think it is acceptable for now 🤔 What do you think? |
| const blockProps = useBlockProps( { | ||
| ...( Platform.isNative && { style } ), | ||
| className: classnames( { | ||
| [ `is-type-${ type }` ]: type && type !== '1', |
There was a problem hiding this comment.
Would love if we can find a solution without adding extra classes.
There was a problem hiding this comment.
Is there no way we can tweak the reset styles that we have?
|
So it seems that gutenberg/lib/client-assets.php Line 348 in 284455b |
|
Unintended CSS loading into the iframe should be fixed, but since the editor may not be an iframe (meta box is enabled, or v2 blocks are included, etc.), I think we need to address this issue, assuming that WP-Admin's |
|
And the funny thing is, we actually had to add the reset styles for lists because it was breaking list margins: So the reset styles both fix and break list styles at the same time! Ideally we should just make it work without the reset styles. |
|
And in addition, we should fix the reset styles so that they're not too aggressive. Looking into it. |
Yes, but is there no way to fix the reset styles? |
Sorry, this was my mistake. I see that WP-Admin enqueues only
BTW, I have learned that the |
|
I've added a commit that adds reset styles without the classes 72512d7 |
Yes, that a good option |
|
Also, this has been an issue pre 6.3 |
Oh, I didn't see this comment! Removing my commit 😅 |
ellatrix
force-pushed
the
fix/list-number-type
branch
from
72512d7 to
377f799
Compare
|
Yes, I do not understand this behavior 😅 I think I'll switch to an inline style implementation.
|
|
Since this is not a regression, maybe we should indeed rewrite the feature to use CSS styling. |
|
Have you seen https://caniuse.com/css-case-insensitive? Apparently you create a case sensitive selector: |
t-hamano
force-pushed
the
fix/list-number-type
branch
from
377f799 to
8e2c68a
Compare
|
Based on our previous discussions, I have taken the following approach:
|
|
@ellatrix What are your thoughts on whether this PR should ship to WP6.4? We may need to continue discussing what list types to support, but personally, I think it's okay to inherit the existing list types and merge this PR that fixes the bug. |
|
I don't think it's a serious issue and we're introducing a block deprecation. Why not switch immediately to CSS styling? |
|
Ok, now let's merge 👍 |
|
@t-hamano ? I think we shouldn't introduce a new deprecation if we're planning to change it immediately again? Why not switch immediately to CSS styling? |
|
@ellatrix Sorry, I may have misunderstood something. However, as noted in this comment, this PR has been updated to use inline CSS styles. am I missing something? |
| ordered && type !== 'decimal' ? type : undefined, | ||
| }, | ||
| } ) } | ||
| > |
There was a problem hiding this comment.
Why are we changing the save function if we're only needing to fix something in the admin?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, sorry, I misunderstood and thought this was the previous fix.
There was a problem hiding this comment.
How were these styles injected into the frontend before this? Why did we decide to use inline styles now?
There was a problem hiding this comment.
Because the type attribute is deprecated.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok Thanks for the details. 👍
|
Oh ok, I didn't realise it had been rewritten. I'll review |
|
Sorry, in that case it seems fine! |
|
Thanks! If you find any issues with this PR in the future, I will be happy to address them 👍 |
| const blockProps = useBlockProps( { | ||
| ...( Platform.isNative && { style } ), |
There was a problem hiding this comment.
Not sure why this was ignored for native. Does that have any consequences for the new inline style?
There was a problem hiding this comment.
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.
Fixes: #52826
Fixes: #54384
Update: This PR has changed its approach based on the discussion. For the latest implementation details, please check the comments here.
What?
This PR adds explicit selectors and styles to ensure that numbering styles are applied correctly under certain conditions.
Why?
From what I have found in this comment, this problem is because the WP-Admin
common.cssandreset.cssapplied to the editor are overriding the styles. I also found that these styles are also loaded into the iframe editor in the case of the classic theme.However, if, for example, custom fields are enabled or a block with apiVersion 2 is present, the editor is not iframed.
As a result, the WP-Admin reset style is applied even in the case of a block theme, which causes problems.
How?
This issue should be approached on the assumption that a WP-Admin reset style exists.
I first wrote the following selector:
However, it did not take into account whether the value was uppercase or lowercase, as seen in the screenshot below (confirmed in Chrome and FireFox)
Therefore, I explicitly generated class names like
is-type-${type}and applied styles to them.Testing Instructions