Accordion Header, Panel: Add CSS for the default style#73032
Conversation
|
Size Change: +265 B (+0.01%) Total Size: 2.45 MB
ℹ️ View Unchanged
|
9e477dc to
1b5224c
Compare
|
Pinging @sabernhardt for visibility; I believe you have a deep understanding of the default theme. |
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @minaldiwan, @adityaanurag0219. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
mikachan
left a comment
There was a problem hiding this comment.
Thanks for working on this, @t-hamano! This is testing well for me.
A completely different approach might be to leave it entirely up to the theme developer to decide which CSS to apply. However, this means that the default themes themselves would need to be modified.
I prefer this option of including default styles with the block, and then theme developers can still override them in the same way if they wish to.
Here is TT1 before and after:
| Before | After |
|---|---|
![]() |
![]() |
| // high CSS specificity. Since it's impossible to predict this specificity, | ||
| // we reset the colors using `!important`. | ||
| background-color: inherit !important; | ||
| color: inherit !important; |
There was a problem hiding this comment.
Hm, could we try to cover to majority of cases and push themes that are "misbehaving" to update (or rely on its users to report issues to them)? It's not great to see !important here, we're making it harder for themes that don't do anything wrong to style accordions.
There was a problem hiding this comment.
I wonder if simply button.wp-block-accordion-heading__toggle would be enough.
There was a problem hiding this comment.
Secondly, why is it wrong to style a button as a button? Perhaps we are doing something wrong here? Why is the accordion block using buttons instead of <details>?
There was a problem hiding this comment.
Hm, could we try to cover to majority of cases and push themes that are "misbehaving" to update (or rely on its users to report issues to them)?
The main reason for this is that TT1 applies styles with a very high CSS specificity to the button element.
/* CSS Specificity: 0-3-1 */
button:not(:hover):not(:active):not(.has-background) {
background-color: var(--global--color-primary);
}
/* CSS Specificity: 0-3-1 */
button:not(:hover):not(:active):not(.has-text-color) {
color: var(--global--color-background);
}I didn't want to use !important, but without it, the accordion toggle button doesn't display correctly in TT1.
There was a problem hiding this comment.
Context: #21584 (comment). But I wonder if WP pushed more on this, it would be better everyone. 🤔 Things have improved over the last years and will continue to improve, especially if WP adopts it too
There was a problem hiding this comment.
button:not(:hover):not(:active):not(.has-background)
That's... super weird. Why is a background added like that instead of unsetting the background for the :not cases? What if we fixed TT1 instead of aggressively unsetting background etc. here?
There was a problem hiding this comment.
What if we fixed TT1 instead of aggressively unsetting background etc. here?
I think that approach is also possible.
There was a problem hiding this comment.
Do you think we should wait with this PR for a later RC then? Or should we just merge it now and adjust the styles later. Up to you :)
There was a problem hiding this comment.
For now, I think it's okay to merge this as is, in order to apply the minimum default styles. We can adjust the CSS specificity later based on consumers' feedback.
* Update accordion header defult styling * Accordion Header, Panel: Add CSS for the default style --------- Unlinked contributors: minaldiwan, adityaanurag0219. Co-authored-by: t-hamano <[email protected]> Co-authored-by: mikachan <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: theminaldiwan <[email protected]>
|
I just cherry-picked this PR to the wp/6.9 branch to get it included in the next release: a40b37a |


What?
This pull request adds default styles to the accordion header and accordion panel.
Why?
Many themes, especially classic themes, apply various CSS styles to heading and button elements. The Accordion Heading block is particularly susceptible to this because it consists of both heading and button elements. Therefore, an unnatural style may be applied to the accordion header in some cases.
How?
I tested all the default classic themes and added CSS to ensure the following specifications are met.
A completely different approach might be to leave it entirely up to the theme developer to decide which CSS to apply. However, this means that the default themes themselves would need to be modified.
Testing Instructions
Screenshots or screencast
The following describes the behavior using the TT1 theme. It should work similarly with other default classic themes as well.
cfac1132a963ebee199d409e2da18b29.mp4