WIP: lower specificity to max width on post template #39014
Closed
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
|
Size Change: +59 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
MaggieCabrera
requested review from
youknowriad,
scruffian,
jasmussen and
jffng
|
I have an alternative solution to this one over at #39016 and I don't know which one is preferred, please any input is welcome. |
8 tasks
|
Looking at this closely it seems like we could preserve the current selectors if we could change the loading order of the styles. The post-content styles are loading before the layout ones and that's part of why this bug is happening. I'm not sure how to change that order or even if that should be done. In case that's not an option, I think this fix is the best way to go about this bug. |
|
@jorgefilipecosta this bug was introduced by this change: WordPress/wordpress-develop@43a8631 It seems to me that we should try to preserve the change from that commit, which means we probably need to look at a fix like the one @MaggieCabrera suggests. |
|
I prefer this PR to #39016, mainly because of having to add the |
|
I still haven't fully understood what the problem is. I have a feeling it has to do with the layout engine not being aware of the wrapping tag that you can select under "Advanced", and so if we land any fixes in the mean time it'd be important to not forget that, if the root cause is actually different. I tried recreating the issue in a codepen, and of course that clarifies that the 100% max width is meant for the container, while the If it should, it'd still be good to replicate this rather nuanced issue in a simpler codepen so we can have some confidence in the fix and cause. |
|
This is the best I could manage with codepen: https://codepen.io/onemaggie/pen/VwrBKqw If you swap the order of the selectors, it works for both cases. I'm using this markup on emptytheme to highlight how the case with the extra tag has the issue but without it hasn't: |
|
That's an excellent codepen, thank you. From just that codepen alone, my question is: should the |
Either that or the "container" class move to the inner element, yes |
|
Right, that seems to be the issue. Do you have any sense of whether it would be possible to fix this (move the classes) at all? |
No idea, I'm going to investigate! |
|
I found another way to trigger this bug while testing Skatepark: https://skateparkdemo.wordpress.com/?s=a If you have a group block with inherit layout and a query loop inside, the same problem with the order of the styles affects the max width of the post content blocks. This only happens in the frontend:
|
|
Indeed, that's a good testable example. To summarize for my own sake, what it boils down to is that the
Does that sound about right? If yes, then if we could decide on 1 or 2, it could probably unblock this PR. |
|
I think has to be 1 unless we build a lot of extra things to stop child blocks from also enabling the default layout, which would in turn break a lot of themes. |
|
Isn't the loading order the same issue as the one raised here? #38750 (comment) (so a regression of that PR) |
|
Is this PR relevant here? #39164 |
|
@jasmussen yes 100%. That PR fixes this issue. Thanks for connecting the dots! There is a regression in #38750 and the block support styles are added too early and get overwritten in many situations. |
8 tasks
|
Yes, the problem is fixed, thank you all for helping investigate the issue! |
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.
Description
When we lowered specificity to the alignment CSS they started following behind other block rules that affect the same properties. In this case the max-width of the post content block styles is more specific than the new layout rules in some cases (in my testing, this shows up on query blocks that have layout inherit and a tagName attribute).
I'm not sure if the correct fix is to lower specificity to the offending CSS or raising it up to the layout styles and then fixing it a different way for alignleft and right. I'm going to keep tinkering but I leave this here for others to chime in.
Testing Instructions
To test this, use this block markup on empty theme, use it as index.html:
The post content shows as 100% width without the fix.
Screenshots
Before:
After:
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).