Fix for link color in containers #34689
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oandregal
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
oandregal
requested review from
ryelle,
richtabor,
jorgefilipecosta,
getdave,
Mamaduka and
getsource
oandregal
added
[Feature] Design Tools
oandregal
commented
Sep 9, 2021
| @@ -38,7 +38,7 @@ function gutenberg_render_elements_support( $block_content, $block ) { | |||
| } | |||
| $link_color_declaration = esc_html( safecss_filter_attr( "color: $link_color" ) ); | |||
|
|
|||
| $style = "<style>.$class_name a{" . $link_color_declaration . " !important;}</style>\n"; | |||
There was a problem hiding this comment.
This is going to need changes in the tests. Will update them once we're certain this is the right direction.
There was a problem hiding this comment.
This is going to have 011 specificity.
I've looked at all current cases of core blocks and this change will still work well with global styles. The reason is that the current global styles generated for links have 011 or 002, depending on whether they are .wp-block-* a or element a. The element styles are loaded after the global styles, so the element styles win.
I was thinking that we could add the :not(.has-link-color) selector to the global styles we generate. Should any block have higher than 011 specificity this selector will prevent it from applying. However! This is also not complete: it won't work for some blocks that declare __experimentalSelector and target an inner element. Use case:
- some block has a wrapper such as
.wp-block-wrapper - for whatever reason, the block declares that the styles should match some inner element instead
__experimentalSelector: .wp-block-wrapper-inner .wp-block-wrapper-extra
Adding the negation in this situation would render .wp-block-wrapper-inner .wp-block-wrapper-extra:not(.has-link-color) { /* link color */ } which has higher specificity that the link color element and still targets the link in the block because the .has-link-color class lives in the wrapper.
|
Size Change: +32 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
getdave
reviewed
Sep 9, 2021
|
This fixes the issue in the patterns I flagged, so thanks!
Link color is still an experimental feature, so changing the behavior is to be expected, right? Or is this something that will need to be documented? I did manage to find one more edge case, where a nested link color is overridden by a link color on the parent container, but only on the frontend.
The group block has a white link color, but that individual paragraph uses a bright orange. In the editor, the In the editor… /* Before the group `div` */
.wp-elements-613a1a9b8f7d4 a {
color: var(--wp--preset--color--white);
}
/* Before the `p` */
.wp-elements-613a1a9b8f45f a {
color: #cf6100;
}Link is orange ✅ On the front end… /* After the `p` */
.wp-elements-613a1a9b8f45f a {
color: #cf6100;
}
/* After the group `div` */
.wp-elements-613a1a9b8f7d4 a {
color: var(--wp--preset--color--white);
}Link is white ❌ |
It needs to be documented. For themes that use the |
|
Now it works in all the scenarios I could think of 👍🏻 |
Mamaduka
reviewed
Sep 10, 2021
There was a problem hiding this comment.
@oandregal, I think the snapshot in PHP unit tests needs an update.
|
ok, this PR may work at a practical level. Theoretically, this could break in some situations #34689 (comment) What I don't like about this is that themes will need to fix some CSS and that they need to take care of not overriding user-provided styles. However, the current situation is also not ideal as blocks that use the I'm going to let this sit for a few days in case we can come with an alternative that fixes the issues and doesn't have the drawbacks. If we don't, I'll update the tests and merge this. cc @jorgefilipecosta @jasmussen in case you have thoughts. |
Themes will always be able to break things, whether for good or for bad. In this case, the removal of !important's feels worth any followup we might have to do. Thanks for the PR. |
jorgefilipecosta
added
the
Needs Dev Note
jorgefilipecosta
approved these changes
Sep 10, 2021
|
Thanks so much for your work on this @oandregal ! I'm guessing this applies to some of the comments about specific theme issues: |
|
Thanks all for the help. I'm going to fix the tests for this one and will merge it. |
oandregal
force-pushed
the
fix/link-color-element
branch
from
8d38f00
to
be65b1c
Compare
|
@oandregal FYI I tried to build a block theme over the weekend and ran into this |
The reason for this is nested content such as:
<group class="wp-element-1">
<p>Text with <link> and colors inherited from group</p>
<p class="wp-element-2">Text with <link> and own colors</p>
</group>
The selectors for the link are
- ".wp-element 1 a" => generated by the group
- ".wp-element-2 a" => generated by the paragraph
because they have the same specificity the latest will win.
We need the styles to respect the DOM order of the elements
that have them.
<style .wp-element-1>
<style .wp-element-2>
<group ...>...</group>
oandregal
force-pushed
the
fix/link-color-element
branch
from
be65b1c
to
7f697a9
Compare
|
Rebased to get #34776 in (a potential fix for a failing e2e test) |
DevnoteTo review/clarify/check when time comes to publish it: We've made some changes to the link color feature that affect its specificity and helped to fix some issues with links within container blocks:
Given these changes, the theme needs to make sure its styles play well with user choices. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #33437
When we landed support for elements (the link color) we made it so the generated class was this:
.wp-element-id a { color: color-value !important; }Notice the
!important. This was to make sure user-provided styles weren't overridden by theme-provided styles. However, the selector is too far-reaching and it's affecting blocks that use theabut, arguably, should not be considered "links" (such as button and social links).This PR removes the
!importantso the styles for the link color are:.wp-element-id a { color: color-value; }If we go ahead with this change, themes that add additional CSS for link colors need to make sure that user-provided styles are respected in every situation.
Testing
Blocks at the top-level
I tested this with the TT1-blocks:
Note that the TwentyTwentyOne theme needs to be updated to not override user styles (see the last paragraph, which should show the links in red):
Blocks within containers
I tested this with the TwentyTwentyOne theme:
Note that the TT1-blocks theme needs to be updated to make sure link color is not used in the button block:
Nested blocks
See #34689 (comment) for testing instructions.