Fix sticky position in classic themes with appearance tools support #56743
Conversation
tellthemachines
requested review from
spacedmonkey and
ellatrix
as code owners
|
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
|
Size Change: +854 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I tested this on the theme "Go".
All worked fine in terms of selecting sticky and applying to a group block in the editor. However the theme adds overflow: hidden; to .site-content which means the sticky position doesn't work. :/
I tested in the Astra theme and it worked correctly, so perhaps an example of the challenges we'll have and something we'll need to communicate to theme developers.
With classic themes particularly, there's always the possibility of custom theme styles overriding core functionality. Core can't anticipate or even less provide workarounds for these cases; the best it can do is ensure that new or current features such as sticky position don't conflict with legacy theme support features such as the wrappers around wide and full aligned blocks that were causing problems here. Enabling support for appearance tools is an opt-in for classic themes, so themes that do want to opt in should ensure their styles work with the new features. |
tellthemachines
added
[Type] Enhancement
There was a problem hiding this comment.
This is working quite nicely for me, too! Similar to Andy, I noticed that some themes (e.g. TwentyTwenty) also output overflow: hidden on the site content, breaking the sticky positioning rules.
I agree that this sort of thing is part of what we'll need to communicate in terms of classic themes opting in to appearance tools (i.e. here's what your theme needs to do to be compatible). Further down the track, if it seems like sticky position support is too frustrating for classic themes to deal with as they'd need to not do particular overflow rules or have z-indexes that conflict, we could always re-evaluate whether we want to include this particular support for classic themes. Not a blocker for this PR, though!
The main thing I noticed while reviewing is that I'm not sure if we need to include the themeSupportsAppearanceTools flag, as the position support already implicitly checks that the theme supports sticky positioning before outputting the is-position-sticky classname, since it looks to see if position.sticky is supported before generating classnames, so we might be able to simplify things a bit there?
| @@ -172,7 +179,11 @@ function BlockListBlock( { | |||
| if ( isAligned ) { | |||
| blockEdit = ( | |||
| <div | |||
| className="wp-block" | |||
| className={ `wp-block${ | |||
| themeSupportsAppearanceTools && isSticky | |||
There was a problem hiding this comment.
Do we need the themeSupportsAppearanceTools check? The is-position-sticky classname will only be output if the theme supports sticky position, so I think the position support effectively already does this check in:
gutenberg/packages/block-editor/src/hooks/position.js
Lines 194 to 197 in 4e26178
Unless I'm missing something of course!
There was a problem hiding this comment.
Ooooh right that simplifies things a bit!
| @@ -172,7 +179,11 @@ function BlockListBlock( { | |||
| if ( isAligned ) { | |||
| blockEdit = ( | |||
| <div | |||
| className="wp-block" | |||
| className={ `wp-block${ | |||
There was a problem hiding this comment.
Tiny nit: rather than concatenating manually, would it be worth calling classnames( 'wp-block', className ) instead?
There was a problem hiding this comment.
Ah well spotted! Yes it would 😄
| @@ -221,7 +232,7 @@ function BlockListBlock( { | |||
| isTemporarilyEditingAsBlocks, | |||
| }, | |||
| dataAlign && themeSupportsLayout && `align${ dataAlign }`, | |||
| className | |||
| ! ( dataAlign && isSticky ) && className | |||
There was a problem hiding this comment.
Probably not an issue, but if there are other block supports that generate a className (like element colors for link colors) then these classnames will also be moved up to the align wrapper if the block has been set to sticky? I think this is most likely fine, though, as we're already moving those classnames that need to act on the inner wrapper down to the inner wrapper to handle block gap, so it's all working nicely for me locally.
I.e. I can both set to sticky and set a custom block gap 👍
There was a problem hiding this comment.
Hmmm I tried a few supports in testing and the others didn't seem to be included in the className variable. I'll check again. Also now I'm wondering if we can leverage classname swapping to fix the issue with background color overlapping background image too:
There was a problem hiding this comment.
So the only other classname being transferred to the alignment wrapper is wp-elements-x, which isn't problematic because its styles don't target direct descendants. If this did become a problem later on we could perhaps filter out the position classnames, but best avoid adding extra logic if it isn't needed.
What I did notice is that in the editor, when testing with a single group block on the page. position and layout classnames were identical wp-container-0. In practice that can't cause problems because applying sticky position to the inner group container won't do anything, and neither will the block spacing rules override margins set on the block. To be on the safe side though, we might want to consider changing the layout class in the editor to match the front end wp-container-core-group-layout-x (and possibly a similar specificity could be applied to the position class at some point). I'd do that as a separate PR though.
There was a problem hiding this comment.
Update: the background styling in the screenshot above is specific to TT1, so I guess it's fine to leave as is. Imo it's not worth the overhead of trying to attach the background image to the inner container, because it's pretty easy to fix this in the UI.
There was a problem hiding this comment.
Sounds good. The less we need to mutate for the classic themes case the better, if possible. We can always continue exploring in follow-ups if need be, too!
|
Flaky tests detected in 3981184. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7096342627
|
What?
Fixes one of the items in #56131: Extra wrapper on wide/full aligned blocks interferes with sticky position styles
Sticky position works in relation to its parent container, so when its classnames are added to a block wrapped in a legacy
data-alignwrapper, it doesn't work. This fixes the issue by detecting the sticky classes and adding them to the align wrapper instead.Testing Instructions
add_theme_support( 'appearance-tools' );into the themes's setup function.