Right-aligned images in CKEditor appear to the right of other fields

I thought for a second you were setting style="float:right" :) But you're just using the provided standard functionality it seems — good!

Could you provide a screenshot? I've not seen this happen myself.

Added clearfix class to every field div. I'm not sure about other complications, but it seems to be part of 'sensible defaults' to add a clearfix after every separate element.

I seem to have added the file twice.

I think we have to add a clearfix class to every div and it looks fine .

Fixed the tests, altered expected HTML ...

Removing accidentally committed whitespace from previous patch, sorry ^^

So… you say this is going to happen for any body field that contains some floated HTML elements, and that hence this is the only possible solution? Wow :(

Assigning to LewisNyman to get feedback.

I'm afraid it goes even deeper. It adds a clearfix class to every type of field, not just a body field. That's why I want feedback on this :-)

I'm afraid it goes even deeper. It adds a clearfix class to every type of field, not just a body field. That's why I want feedback on this :-)

Why would we need the class on every field? At most it would be on every field that could support wysiwyg? At the least this would need to check to contents of a field for floated elements, but that sounds like a considerable performance hit right Wim?

At most it would be on every field that could support wysiwyg?

Correct.

At the least this would need to check to contents of a field for floated elements, but that sounds like a considerable performance hit right Wim?

Indeed, that would not work. What is feasible, is to only do this for text field type formatters. That'd be better already.

But I was wondering if you see another solution, LewisNyman? How come this is not a problem in D7? It has to be, right? Does D7 already add a clearfix class to all fields? How is it fixed there?

I don't think it is fixed in D7. This is a rare use case, where there is little/no text and only an image in a wysiwyg. The text that wraps around the image would prevent the div from collapsing like this. Custom frontend themes probably deal with this edge case themselves from client bug reports.

I guess we need to add a clearfix class for textfields in Classy or just add CSS to the textfield class in Bartik.

This is a rare use case, where there is little/no text and only an image in a wysiwyg. The text that wraps around the image would prevent the div from collapsing like this.

Right.

An alternative would be to say that we don't support this, because it doesn't make sense to align an image if there is nothing to align it with/against?

Aligning left would make no sense but then aligning the image right does have a visual effect? Within the context of the page at least. I am still happy with the idea of not supporting this bug :-)

Asking webchick for feedback on whether she thinks Drupal should support the non-real-world-but-only-occurring-while-testing problem of aligning an image (relative to text) in a text field without text. Should we support that? Drupal 7 did not. But Drupal 7 didn't ship with that capability, of course. Nevertheless, I don't see why one would float an image relative to text while there is no text, and hence I don't see why we should support that. Seems LewisNyman thinks/wonders the same :)

in a text field without text. Should we support that?

I'm confused. Isn't this also an issue about a textfield with little text? Which certainly happens in the real world. For example, this screenshot based on HEAD:

Right, tumblr.com is an example of basically an entire social network consisting mainly of posts with only a tiny bit of text and an image/video. (see for example http://devopsreactions.tumblr.com/).

I can't really comment as to how we can best fix this, since CSS is something I last touched seriously in like 2005, but I do think it's a legit bug.

#20: good point.

#21: bad point, because that's exactly what I'm saying: those images aren't aligned… they're just image posts. There's no problem if there's just an image. It just doesn't make sense to align an image if there's nothing to align it against.

But IMHO #20 shows that we need to fix this.

So here's another attempt to fix this, doing what I said in #15, based on Lewis Nyman's feedback in #14:

At the least this would need to check to contents of a field for floated elements, but that sounds like a considerable performance hit right Wim?

Indeed, that would not work. What is feasible, is to only do this for text field type formatters. That'd be better already.

Apparently core/themes/bartik/templates/field--taxonomy-term-reference.html.twig already does this:

<div{{ attributes.addClass('clearfix') }}>

Hence every taxonomy term reference field has the clearfix class … but only in Bartik.

I was hopeful that our theme system + Twig's combined flexibility would allow us to add the clearfix class only to text field formatters… and it does :) See attached patch.

A really nice solution @WimLeers, it gets a +1 from me!

Looking good Wim, I think this code should go into Classy though?

#26: .clearfix lives in system.module.css, not in Classy, so I figured it doesn't belong in Classy? I defer that decision to you and other front-end people though :)

I, personally, wouldn't classify this as a bug. There are plenty of use-cases for having the image float against trailing fields. I do it all the time, depending on how the display should be structured (or rather how my client wants it. :/) To me this is more of an issue of do it one way or the other, but make sure everyone knows which way it is.

If the majority use case is to have trailing fields clear, that is fine. Wim's solution looks reasonable. It is better that the clearfix is there by default, because it is probably easier for someone to realize that they need to remove it, instead of realizing they need to add it to get the reverse result. And +1 to the fact that this is so much easier to do with the new theming system and everything in templates.

#26 Correct. These are field templates, so they don't go in Classy, yet. Once we finally get the field preprocess changes committed, we can move these templates to Classy with the others, or otherwise figure out what to do with them.

+++ b/core/modules/text/templates/field--text.html.twig
@@ -0,0 +1,20 @@
+{% set attributes = attributes.addClass('clearfix') %}

Does this actually work? I thought you could only manipulate what is within a block in the parent template. I also ask because we haven't yet modified the field template, but we are going to, and I'm wondering if the attribute manipulations in the parent template collide with the child. If it's kosher, I can think of a few other places we can do this.

#28: Interesting, thanks for sharing your perspective!

It's not clear to me if you're saying it should go in Classy or shouldn't? And yes, it works. :)

Where they are in the patch is fine. We may eventually move them to Classy, but that should be in a follow up or likely as part of the phase 2 banana stuff. #2348543: [meta] Consensus Banana Phase 2, transition templates to the starterkit theme

If that set truly works well with the parent/child relationship, that is a revelation (for me at least) and means I can simplify some of the other child templates. Not that I don't trust Wim's infinite knowledge ;-), but I want to play with that more thoroughly.

I don't have time to play with this right now, so don't assume you are waiting on me. If someone else thinks it should be RTBC, go ahead.

So it looks like I have +1 from a front-end POV, now let's get a +1 from a Twig POV, to fully address davidhernandez appropriate concerns.

(FYI: I'm not a Twig expert at all. I just googled and tried and found that this worked :))

Assigning to Fabianx to get Twig feedback.

I was also kinda surprised when I saw the change of context in the parent variables.

Blocks in twig are implemented in the compiled code as class functions (internally).

So you have a template, which is the base and then you have others that extend it. Extension is done via class based inheritance, IIRC.

So the template is always the same, but because it uses a extends relationship (so that other blocks can overwrite the blocks used), the code to change the context gets executed first and then the parent's code. So this is fine.

So while I am not sure, this is kinda 'public API behavior' the documentation states neither can nor can't do, so we should ensure we have test coverage for that case and then this should be good. (and we have)

How this will live in classy depends also on #2291449: Add Twig template inheritance based on the theme registry, enable adding Twig loaders but can certainly be follow-up.

I agree that its easier to remove things than to figure out this solution yourself, so Banana++.

All that said, and given the patch is fine, this is RTBC.

And thanks for making use of our new theme engine!

Hmmm... what happens when a theme overrides field.html.twig?

It will continue to work.

At the moment the theme will need to override all files, but with the theme registry loader it will automatically pick up the new one and extend from the right one in the theme.

Putting to fixed as there is a commit - unless you want to revert.

lol I didn't actually mean to commit this. Going mad. So if a theme overrides field.html.twig - they will also have to override the text module's templates - this seems a bit fragile.

The templates would have to be a full copy of field.html.twig or postpone this on #2291449: Add Twig template inheritance based on the theme registry, enable adding Twig loaders I *think* the template inheritance issue is close to being done though. ?

Yes, it is very close.

I played around with this some more, with multiple levels of child templates, and the manipulations work. (Thanks, Alex, for accidentally committing. Made it easier! :) ) Markup appears off limits, and rightly so without blocks, but variables are all fair game! Any attribute manipulation carries itself through the hierarchy of templates. Modification of other variables, like label also work. And I can set variables in the child templates and use them in the parent.

Is it sad how happy this makes me. :-D

Wim Leers++
Learning new things++

Is it sad how happy this makes me. :-D

Of course not! :-)