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.
Comment | File | Size | Author |
---|---|---|---|
#24 | protect_aligned_images_in_text_fields-2358529-24.patch | 3.84 KB | Wim Leers |
Comments
Comment #1
Wim LeersI 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.
Comment #2
brylie CreditAttribution: brylie commentedComment #3
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedAdded 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.
Comment #4
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedI seem to have added the file twice.
Comment #7
brahmjeet789 CreditAttribution: brahmjeet789 commentedI think we have to add a clearfix class to every div and it looks fine .
Comment #8
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedFixed the tests, altered expected HTML ...
Comment #9
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedRemoving accidentally committed whitespace from previous patch, sorry ^^
Comment #12
Wim LeersSo… 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.
Comment #13
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedI'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 :-)
Comment #14
LewisNymanWhy 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?
Comment #15
Wim LeersCorrect.
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?Comment #16
LewisNymanI 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.
Comment #17
Wim LeersRight.
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?
Comment #18
LewisNymanAligning 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 :-)
Comment #19
Wim LeersAsking 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 :)
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedI'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:
Comment #21
webchickRight, 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.
Comment #22
Wim Leers#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:
Apparently
core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
already does this: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 totext
field formatters… and it does :) See attached patch.Comment #24
Wim LeersComment #25
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedA really nice solution @WimLeers, it gets a +1 from me!
Comment #26
LewisNymanLooking good Wim, I think this code should go into Classy though?
Comment #27
Wim Leers#26:
.clearfix
lives insystem.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 :)Comment #28
davidhernandezI, 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.
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.
Comment #29
Wim Leers#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. :)
Comment #30
davidhernandezWhere 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.
Comment #31
Wim LeersSo 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.
Comment #32
Fabianx CreditAttribution: Fabianx commentedI 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!
Comment #33
alexpottHmmm... what happens when a theme overrides field.html.twig?
Comment #35
Fabianx CreditAttribution: Fabianx commentedIt 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.
Comment #36
alexpottlol 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.
Comment #37
davidhernandezThe 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. ?
Comment #38
Fabianx CreditAttribution: Fabianx commentedYes, it is very close.
Comment #39
davidhernandezI 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++
Comment #40
Wim LeersOf course not! :-)
Comment #42
brahmjeet789 CreditAttribution: brahmjeet789 as a volunteer and at gai Technologies Pvt Ltd commented