When inserting an image into the rich text editor with a "float: right" property, the image floats to the right of subsequent fields. This would normally be fixed with a 'clear' declaration on subsequent elements.

This float behavior may be confusing for new users. Please provide a 'sensible default' for floated images to be constrained within their parent element, and to display above subsequent elements.

Steps to reproduce:
1) Add a new content, of type "Article"
2) Insert an image into the body, via the rich text button
3) Select "Right" under the align section
4) Add a tag to the article
5) Save

When the article renders, the image will be floating to the right of the tag field.
Image floating right

Comments

Wim Leers’s picture

Title: Images with "float:right" in ckeditor appear to the right of other fields » Right-aligned images in CKEditor appear to the right of other fields
Version: 8.1.x-dev » 8.0.x-dev
Status: Active » Postponed (maintainer needs more info)
Issue tags: +CSS, +Novice, +css-novice

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.

brylie’s picture

Issue summary: View changes
FileSize
22.72 KB
Tom Verhaeghe’s picture

Assigned: Unassigned » Tom Verhaeghe
Status: Postponed (maintainer needs more info) » Needs review
FileSize
486 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,208 pass(es), 3 fail(s), and 0 exception(s). View
486 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,206 pass(es), 3 fail(s), and 0 exception(s). View

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.

Tom Verhaeghe’s picture

I seem to have added the file twice.

The last submitted patch, 3: right_aligned_images_in-2358529-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: right_aligned_images_in-2358529-3.patch, failed testing.

brahmjeet789’s picture

FileSize
60.32 KB

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

Tom Verhaeghe’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
2.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,106 pass(es), 1 fail(s), and 0 exception(s). View

Fixed the tests, altered expected HTML ...

Tom Verhaeghe’s picture

FileSize
857 bytes
2.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,112 pass(es). View

Removing accidentally committed whitespace from previous patch, sorry ^^

The last submitted patch, 8: right_aligned_images_in-2358529-8.patch, failed testing.

Wim Leers’s picture

Assigned: Tom Verhaeghe » LewisNyman

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.

Tom Verhaeghe’s picture

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 :-)

LewisNyman’s picture

Issue tags: -css-novice +frontend

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?

Wim Leers’s picture

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?

LewisNyman’s picture

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.

Wim Leers’s picture

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?

LewisNyman’s picture

Assigned: LewisNyman » Unassigned

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 :-)

Wim Leers’s picture

Assigned: Unassigned » webchick

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 :)

effulgentsia’s picture

FileSize
34.88 KB

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:

webchick’s picture

Assigned: webchick » Unassigned

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.

Wim Leers’s picture

Issue tags: -Novice +Twig
FileSize
2.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,397 pass(es), 1 fail(s), and 0 exception(s). View

#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.

Status: Needs review » Needs work

The last submitted patch, 22: protect_aligned_images_in_text_fields-2358529-22.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,740 pass(es). View
976 bytes
Tom Verhaeghe’s picture

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

LewisNyman’s picture

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

Wim Leers’s picture

#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 :)

davidhernandez’s picture

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.

Wim Leers’s picture

#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. :)

davidhernandez’s picture

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 Classy 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.

Wim Leers’s picture

Assigned: Unassigned » Fabianx

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.

Fabianx’s picture

Assigned: Fabianx » Unassigned
Status: Needs review » Reviewed & tested by the community

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!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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

  • alexpott committed d46dedd on 8.0.x
    Issue #2358529 by Tom Verhaeghe, Wim Leers, brylie, brahmjeet789,...
Fabianx’s picture

Status: Needs review » Fixed

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.

alexpott’s picture

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.

davidhernandez’s picture

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. ?

Fabianx’s picture

Yes, it is very close.

davidhernandez’s picture

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++

Wim Leers’s picture

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

Of course not! :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

brahmjeet789’s picture