Use DIV instead of BR to clear text align after inline images

I understand the concern... but if I DIDN'T have the br clear in there someone else would be on here complaining about how img_assist is messing up their layouts. It's basically a lose-lose situation for me as the developer. This is now in a theme file so you can easily override it. Otherwise, submit a patch that makes it work different way and I'll take a look at it. Sorry to be blunt, but you need to remember that Drupal and contributed modules like img_assist are free. If you don't like them, you can change them. A patch for this issue would be welcome, but I'm not sure how to best change this (nor do I have time right now).

An additional negative side effect to the <br class=\"clear-both\" /> is that it seems to be triggering the 'read more' even if there isn't any more to read. Now I have some pretty complicated theme logic so it may just be affecting me.

That's how I found this thread — trying to root out all of these unneeded 'read more' links, and this looks like the culprit. Anyone else notice this behavior?

I hate to say it (because he appears to be kind of a jerkoff) but, the more I think about this, the more I agree with jpetso.

This kind of thing is really the responsibility of the theme's css. Let the theme designer's worry about clearing their own floats.

I am having the same problem, though it is not clear that it is due to img_assist. I have turned it off, left it on, and in all cases, the
appears at the end of the text portion of every field I create within a node.

So:

Title

....and so on.

Any ideas how to get rid of this b/c it is somewhat of a problem. In the CSS, I simply set br.clear to 'display:none' but this doesn't help in those cases where it might be necessary.

Could something else be causing this?
I am running drupal 4.7...

Thanks,
Maria

All,

I can see benshell's point of view from a developers angle and appreciate the theme angle too. However, it doesn't have to be a lose-lose situation.

The above patch adds a checkbox to the "Other properties" that can disable the <br class="clear"/> tagging. The default is as per the module, include it. But it allows those that don't want it to switch it off.

Maybe this is an acceptable trade off all would be happy with?

best regards
--AjK

I am happy to try this, but I don't know how to run a patch. Can you tell me what to do?
Thanks,
Maria

@Maria: HOWTO: Apply patches.

I changed it to append a div instead and it seems to do what both of you want without the complexity of adding another preference that nobody will understand.

after applying the patch by binford it stills seems to trigger the "read more" for me on the front page. Once i click on read more, the rest of the text would show. does anyone else have this problem?

I'm really glad I found this thread. I was banging my head against a wall and never thought of img_assist. The patch in #10 is working for me and removes the large space between the teaser and the "read more" link.

BUT

When I use the following filters, everything is fine.
- Line Break COnverter
- Inline Images

However, If I use this set of filters, I get the breaks again.
- HTML filter
- Line Break COnverter
- Inline Images

Could the HTML filter be doing the same thing?

I applied the checkbox patch (#7) rather than the div patch (#10) as I don't want a stray clearing div appearing after various field outputs. So far so good. I don't see a br.class appearing after my field output.

Just out of curiousity, what was the purpose of the br.clear?

Thanks also to Sun for the patch howto.

Maria

Why don't you disable this line break with your theme CSS?

br.clear {
    display: none;
}

If you do not want to have this line break and are not able to remove it from code this is the preferred way to disable it. The purpose of this align-cleaning line break is simple: img_assist inserts an inline image into a node which is maybe aligned left or right. If there is few text next to the image the following links (comments, attachments, login/register, etc.) are displayed below the image and not below the text. (drupal.css has to be included in your theme to have the appropriate style rules for br.clear.)

Wow, this just used about 6 hours of my life. Thanks for the CSS trick, it seems to do the trick.

Really, I'm not sure my grief was caused by img_assist, since I did a grep for the code, found
in img_assist.module, added some test characters, but the modification did not appear in the code on my front page. Theoretically could be some weird caching, but I suspect my problems were not caused by img_assist. There is another thread here which places the blame on filter.module. I don't understand the code inside that module, so I don't know whether it is at fault.

But in short: no module should be doing this. Whether img_assist, filter, or something else. Why would anything add code to areas that it should not affect? It is confusing; far more confusing than a picture which requires a bit of manual tweaking. At least we can see the image is messed up, and know where to start fixing it.

have the same prob atm. as im not handling with pictures yet - if i set display:none;

will the clear thing still work?

sorry for that question. i am glad i found this discussion!

I doubt that the additional <br class="clear" /> has been removed from img_assist since it's usable for many sites.
But CSS always works.

I second using span instead of br.

AFAIK, this has been sourced out into a theme function.

Yes, it's a theme, and fairly easy to override.
But wouldn't it be better to have the default option be something less intrusive than a line break?

Guys, it's been one year since my CSS trick and noone was able to create this simple patch?

Please thoroughly test the attached patch.

Looks good. Only we'll have to add the span.clear class to img_assist.css, can't find that anywhere. (The br worked automatically, a span needs to have clear: both.) Or does Drupal core provide .clear?

please have a look at this:

http://drupal.org/node/64292#clear

Yes, I know about .clear-block, but that would require that the whole node content would be wrapped in a new div:

<div class="clear-block">[node content]</div>

Maybe that's preferred, but I feel that maybe a single <span /> at the end of the node content is less intrusive. Thoughts?

I committed this with a small change to the css. Garland uses a similar technique so I used its version of span.clear.

@zoo33: Thanks for comitting. However, please adhere to Drupal's CVS commit message guideline.

I thought that's what I did.

#62472 Use SPAN instead of BR to clear text align after inline images
  Patch by jpetso and sun

Is it the order in which the information is given that bothers you, or is it the fact that I took a shortcut and used the title as a description?

Don't get me wrong, I did not want to offend you.
The CVS log is parsed automatically by different systems.
Your commit message should have read:

#62472 by jpetso, sun: Use SPAN instead of BR to clear text align after inline images.

That is what the CVS commit message guideline is basically about.

I was only aware of the automatic linking of issue numbers. I'm all for following conventions though, so thanks for pointing that out. I'll definitely do that in the future.

Re-opening this issue - two reasons:

1. Neither <span /> nor <span></span> is XHTML-valid.
2. If we change that code now, let's change the class name, too. class="clear" is too ambigious and already used as a global style in many themes. We should stick to class="image-clear" or the like.

Since code has already changed in 5.x-dev, this issue needs a bit more love and attention than usual, thus setting the status to critical.

What would you say we should do wrt xhtml validity? Use a surrounding <div> instead?

'<div class="clear-block">' . $content . '</div>'

I'm a little reluctant to this, if too many modules do this we'll end up with loads of nested divs.

IMHO, a surrounding class="clear-block" is not an option, because the clear-block style coming from defaults.css will break custom layouts. Overriding that style (namely .clear-block:after) is too difficult.

AFAIS, converting that span to a div seems to be the only acceptible option.

Please test attached patch, especially regarding XHTML validation.

Another one: Image Assist inserts this code even if there is no image in the post at all. That's wrong and should be fixed within this issue. Thus, I've also altered img_assist_filter() to only invoke theme_img_assist_filter() if at least one replacement has been performed.

Better title.

just to confirm, the current <span class="clear" /> method is really undesirable and should be changed quickly, to if no other solution presents itself.

The current method caused me a major headache all weekend, as - as part of a wholesale upgrade to Drupal 5.2 - this module caused most of my site to be illegible in internet explorer, under any theme. Everything after that span clear tag was rendered in a font so small you could barely see it, let alone read it! You'll find my agonised wail here - http://drupal.org/node/173795

Disabling modules one by one eventually uncovered img_assist as the culprit, and changing the span tag as above has now cured the problem for me.

Sorry, messed up my code tags there - the change should be from <span class="clear" /> to <span class="clear"></span>. Or something better/more compliant if possible, but make sure it works in IE first!

Wow, why do problems always surface only after a commit... OK, so let's fix this quickly.

@sun: I tested these four lines in W3C's XHTML validator:

<span />
<div />
<span></span>
<div></div>

None of them gave any errors. Are you sure an empty div is better than a span? I would prefer <span></span> unless it's invalid of course.

Nice job with the $processed check! Please add a $processed = FALSE before the loop so that we don't through unnecessary warnings. (Or I can just do it before I commit.)

Firefox' HTML validator yields a "Trimming empty <span>" warning for each instance of <span class="image-clear"></span>. Additionally, there is a visible gap between the node content and the following output.
However, an empty div container works. Successfully tested in IE6.

Added the missing declaration of $processed.

Fair enough. Committed!