Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two blocks with align attribute are not being cleared correctly. #13784

Closed
codetipi opened this issue Feb 8, 2019 · 20 comments
Closed

Two blocks with align attribute are not being cleared correctly. #13784

codetipi opened this issue Feb 8, 2019 · 20 comments
Labels
Needs Design Feedback Needs general design feedback.

Comments

@codetipi
Copy link

codetipi commented Feb 8, 2019

If you have two blocks that can be aligned (i.e. Image block), and the first block is aligned to the 'left', while the second is aligned to 'center' - The second block doesn't have clear: both; and therefore appears next to the floated left block.

Steps to reproduce the behavior:

  1. Add an Image block and align it to the left
  2. Duplicate the block and align this one to center

Expected behavior
The container of a block with an explicit center alignment should be full width and be cleared correctly, regardless of any blocks that exist before it.

@youknowriad
Copy link
Contributor

Why do you think the second block should clear the floats? Floats are used so text and other blocks can wrap floated blocks, if we clear automatically on the next block, we'll break this behavior.

@youknowriad youknowriad added the [Status] Needs More Info Follow-up required in order to be actionable. label Feb 8, 2019
@codetipi
Copy link
Author

This should only happen when the second block has an alignment option and specifically when it is set to center alignment, then it should clear the floats.

Official WordPress theme requirements mean this will always be the case in all themes. If you test this on TwentyNinteteen you'll see it in action too: Reproduce it there and then publish the post. If you then go to the frontend you'll see that the second centred block has the floats cleared, but inside Gutenberg this is not the case.

@youknowriad
Copy link
Contributor

youknowriad commented Feb 11, 2019

Got it thanks @codetipi I'm adding the "Needs Design Feedback" tag to get some feedback from designers.

cc @jasmussen

@youknowriad youknowriad added Needs Design Feedback Needs general design feedback. and removed [Status] Needs More Info Follow-up required in order to be actionable. labels Feb 11, 2019
@jasmussen
Copy link
Contributor

I'm inclined to suggest we should not change this in the vanilla style. As Riad noted, this is one of the quirks of CSS floats, so it's technically "accurate" behavior, even if (and I agree) it's not desirable behavior.

The reason I feel we should not address this is that we have received a great deal of feedback regarding CSS specificity (#11779). What this means is, the more CSS we add to the vanilla block styles, the harder it is for themes to override that CSS.

Given the fact that you can easily fix this yourself in the WordPress editor style, I'm tempted to suggest we should not address this in Gutenberg itself.

However that's just my opinion, and I'm going to CC a few others for additional thoughts: @m-e-h @joyously @kjellr

@kjellr
Copy link
Contributor

kjellr commented Feb 11, 2019

This is a tough one — I'm not totally sure of the right answer myself. On one hand, we should be as unstyled, plain vanilla as possible.

But on the other hand, if there's behavior that's expected (and enforced) for all themes in the theme directory, I'd like for that to be handled in the editor automatically. @codetipi do you have a reference link for this? I couldn't find a mention of it in the review docs.

@joyously
Copy link

Official WordPress theme requirements mean this will always be the case in all themes.

Not sure what you are referring to as to official theme requirements. Before the block editor, themes "should" style the standard classes of alignleft, aligncenter, alignright. How they defined them is up to the theme. The Classic Editor contains some small styles for those.

I personally think that the editor should not have very many styles for the classes that are in the content, and it should use the ones that the old editor did have, for consistency. Any additional styles should be in the theme.css that can be disabled by the theme.

It does make sense to me that aligncenter has clear:both, so that the block can be centered. I do this in my theme. Especially if the old editor had it, the new editor should, but only in the part of the style that can be removed. It's still a theme's choice for the back end and front end to match, isn't it?

@m-e-h
Copy link
Member

m-e-h commented Feb 11, 2019

Odd things definitely happen when you use Align left on an image block.
alignmentfloating

You have the problem mentioned here when you Align center the next block.
But also, regardless of the type of block following the floated image, the two sibling blocks seem to have their block space merged including the location of the block toolbar. Both block's toolbars are above the first, floated block.
alignmerge

If clearing something somewhere fixes it, I think it should be done in the editor CSS. But it's possible there's something else going on here.

@m-e-h
Copy link
Member

m-e-h commented Feb 11, 2019

Currently .editor-block-list__block[data-align="left"] is given a height: 0 in a style.scss somewhere. I'm sure there's a reason but fully understanding floats isn't something I can claim.

I do know if .editor-block-list__block[data-align="left"] is instead given the height of the image or it's contents, in my case above 356px, the issues mentioned here are resolved. That number would likely need to come from the JS.

@codetipi
Copy link
Author

codetipi commented Feb 11, 2019

@kjellr I think I was mistaken there as I can't find it in the official theme requirements either! However, because this is how TwentyNineteen is coded, I think most themes will be too, so for a consistent backend/frontend experience I think it'd be worth tweaking it.

I definitely agree with you on being as unstyled as possible, but we could fix it quite cleanly by having:

.editor-block-list__block[data-align="left"] + .editor-block-list__block[data-align="center"] { clear: both; }

That CSS would only clear the floats of a block that specifically has the alignment option set to 'center' and only when it comes directly after a left aligned block.

I've made a PR for it in case you guys think this is a good solution.

Thanks for the feedback on the issue :)

@joyously
Copy link

Why would it only be used after alignleft on an adjacent sibling?

@codetipi
Copy link
Author

@joyously Ah I forgot to include align right in my example code above - The actual PR clears floats for data-align="center" blocks if the predecesor has either data-align="left" or "right".

The reason to only apply it to a block in this specific circumstance is to try to keep the editor as unstyled as possible. We could make it simpler and more opinionated by just having:

.editor-block-list__block[data-align="center"] { clear: both; }

But from the comments above, I got the impression that there might be a reason this was not done already and may be too opinionated, is this the case? If not, I can tweak PR to be that way too.

@joyously
Copy link

Yes, just wondered why you are making it so specific since floats can be quite far reaching.
Even though MDN says "The rules for positioning and clearing of floats apply only to things within the same block formatting context.", there are so many rules there that the browser has to follow that it's often difficult to make sure your CSS has it all worked out. Floats have been known to affect what follows even if they aren't adjacent or siblings.
And how can you expect to ensure that something is centered if it has anything else on the same line?

@jasmussen
Copy link
Contributor

Thanks for all the feedback, and thanks again to codetipi for the initial PR. Some followup:

Currently .editor-block-list__block[data-align="left"] is given a height: 0 in a style.scss somewhere. I'm sure there's a reason but fully understanding floats isn't something I can claim.

This is sort of a complex one, but keep in mind this is intended only to be an editor style. The challenge is to get floats and wide images to coexist, and in this case the editor uses the approach shown in https://codepen.io/joen/pen/zLWvrW. Because there's additional markup in the editor not present on the front-end, the explicit height value is there.

Odd things definitely happen when you use Align left on an image block.

Isn't that the truth.

Like I mentioned previously, there's additional markup in the editor not present on the front-end, so without crazy javascript some of the borders that are painted outside the block will behave slightly weirdly. So what you see in that GIF you posted is actually "intentional" (because the 2nd image technically follows the float and should be on the right but is too wide to do that so it "clears" it), even though I do very much agree it's not ideal. I tried addressing that issue in #11357 but to truly work, we had to write some specific rules such as those suggested in #13784 (comment). That is — creating special cases for specific blocks following specific floats. But as Joy mentions, this has the potential to cause quite a few unintended consequences, so we ended up not creating such special cases. More context here: #11357 (comment)

To take it back to the core issue, it seems to me that the discussion centers around whether or not we change the following:

.editor-block-list__block[data-type="core/image"][data-align="center"] .wp-block-image {
    margin-left: auto;
    margin-right: auto;
}

which shows this:

screenshot 2019-02-12 at 10 45 36

... to this:

.editor-block-list__block[data-type="core/image"][data-align="center"] .wp-block-image {
    margin-left: auto;
    margin-right: auto;
    clear: both;
}

which results in this:

screenshot 2019-02-12 at 10 45 28

As mentioned, themes can do this themselves.

Looking at the above, I'm tempted to say, yes, it does make sense to add that clear rule. But again, no strong opinions.

@kjellr
Copy link
Contributor

kjellr commented Feb 12, 2019

^ Updating as described by @jasmussen describes sounds fine on my end too. It's fairly minimal, and makes floats a little more easy to handle as a user.

@m-e-h
Copy link
Member

m-e-h commented Feb 12, 2019

because the 2nd image technically follows the float and should be on the right but is too wide to do that

Ahh, that makes sense. Thanks for the explanation @jasmussen!

I say clear the 💩 out of that center aligned block.

@codetipi
Copy link
Author

Thanks for the input Jasmussen, I'm not 100% if that particular bit of CSS is enough though - that would only clear floats for centred image blocks and have the side effect of making the toolbar of the centred image block look a bit broken, as it would appear on top of the wrong block (it would appear above the previous floated block.) I'm developing Gutenberg blocks with alignments and that's how I stumbled upon this - so I do hope we can implement a more generalised fix.

I noticed that the wide/full alignments have already got clear: both, so with that and because TwentyNineteen clears aligncenter on the frontend, I think we should just make it more opinionated, as most well coded themes will also work this way on the frontend. So for better UX consistency, I've tweaked the PR to simply be:

.editor-block-list__layout .editor-block-list__block[data-align="center"] { clear: both; }

Does anyone know of any circumstances this wouldn't be good in Gutenberg?

@jasmussen
Copy link
Contributor

that would only clear floats for centred image blocks and have the side effect of making the toolbar of the centred image block look a bit broken, as it would appear on top of the wrong block (it would appear above the previous floated block.) I'm developing Gutenberg blocks with alignments and that's how I stumbled upon this - so I do hope we can implement a more generalised fix.

Yes, this is one of the issues that were discussed in #11357 (comment), and the markup for an image in the editor is not just the img tag or even limited to the wrapping figure. There is also additional markup that the editor needs to actually make the image resizable, captionable, editable and so on. All of this is outside the boundary of the content of the image, and adds to the complexity of the float situation.

Consider a left floated image placeholder, followed by an image placeholder:

screenshot 2019-02-13 at 09 38 04

In this case, the 2nd image placeholder is selected, and it looks as though it wraps the first one too. If we were to "fix" this (as was one of the goals with #11357), we would have to create a rule such as float + image { clear: both } (pseudo code). But we can't do that because we can't always be sure that an image following a float should always clear. It's somewhat more gray area with a centered image following a float, which is why it'd probably be okay to add the clearing I suggested. We might even do it using your suggested code, which would give us this:

screenshot 2019-02-13 at 09 40 54

We'd want to do some z-index stuff because as you can see the float has some of that going on, but that's fixable. But it would still be an edgecase that would make it inconsistent with how non-centered images behave. Or to frame it more simply: why would a centered image placeholder clear a float, but not a non-centered image placeholder?

I know this is not super intuitive. But I would posit that it's actually possible to make an argument that this is expected behavior considering how floats work. Consider this:

[left floated 70% wide image]
[50% wide image]

Float rules dictate that that the 2nd image, if it had been 30% or less wide, should place it self right next to the float, in this case giving us two images side by side. But because the 2nd image is too wide to fit there, it appears below the 1st image. But notably it doesn't clear it. It's technically still affected by the preceeding float. And the argument here is that by showing that the boundary starts right at the top, this is actually intended behavior. Yeah, a bit of a weak argument I know, but hopefully it gets the point across.


If we step back a little bit, perhaps it might be worth — separate from this discussion — to reconsider the 1px selected block border entirely.

What does it mean for a block to be "selected", and is there a different way to show this? Is there a technical implementation where we can paint the selected state of the block on the content itself as opposed to outside of it like we do now? I'm not sure there is, but if there was it might solve this and other related problems. If you have a great idea here, maybe it's codepen time!

@paaljoachim
Copy link
Contributor

I have added a feedback issue containing video.
Feedback: Video showing the use of multiple image blocks:
#15098

@MDWolinski
Copy link

In a discussion on this issue in #design today, wondering if perhaps a solution for this would be to create a block setting to clear floats. So in the case of the centered image, I can enable it to clear the float, that way different designs by users isn't forced one way or another.

Reference: #10299 (comment)

@mapk
Copy link
Contributor

mapk commented Oct 22, 2019

Alrighty, since we're working on a solution in issue #10299, let's just close this one.

@mapk mapk closed this as completed Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
9 participants