18754 - [FLOAT] problems with HRs around floats

Status: VERIFIED FIXED

Description

[Neil Marshall]

I have an image (The first thing inside of the body tag) setup like this:
<img src="myimage.gif" width="185" height="591" align="left">

Then I have numerous <p></p>'s and a horizontal rule.  Now it comes up fine, but
when I resize the image, most of the text flows around it, but there are some
lines of text which do not.

For example, this line flows around it perfectly
<p><font face="Arial, Helvetica, sans-serif"><b>November 11, 1999</b></font></p>
while this one
<p><font face="Arial, Helvetica, sans-serif"><b>October 30, 1999</b></font></p>
places itself right on top of the image.  I personally don't see much of a
difference in the 2

<hr align="center">
The horizontal rule, when I resize the page, also ignores the image and centers
itself on the page, as opposed to in the room that it has been given.

Build: 1999111208

Comment 1

[slipszi]

The text is rendered fine for me (build 1999-12-12-08 on Win98), but the problem
with the HR tag remains. I'll attach a simplified test case for this.

Comment 2

[slipszi]

Created attachment 3436 [details]
Test case

Comment 3

[slipszi]

*** Bug 18559 has been marked as a duplicate of this bug. ***

Comment 4

[slipszi]

*** Bug 20466 has been marked as a duplicate of this bug. ***

Comment 5

[slipszi]

*** Bug 21499 has been marked as a duplicate of this bug. ***

Comment 6

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Bug 18559 has references to historical bugs.

Comment 7

[leger]

Updating to default Layout Assignee...kipp no longer with us :-(

Comment 8

[troy]

Why are you re-reassing layout bugs? Do NOT touch layout bugs.

The bugs are assigned to Kipp so they can stay neatly organized until we have a
new owner for the block/inline code.

Comment 9

[buster]

mass moving all Kipp's pre-beta bugs to M15.  Nisheeth and I will
prioritize these and selectively move high-priority bugs into M13 and M14.

Comment 10

[ekrock's old account (dead)]

Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!

Comment 11

[Joe McCabe]

This behavior is apparant with any <foo align=foo><hr> elements. See
http://www.dzm.com/ to see the <hr> overlap a table that is align=right.

Comment 12

[buster]

mine! mine mine mine!  all mine!  whoo-hoo!

Comment 13

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

http://www.w3.org/Style/CSS/Test/current/sec414.htm shows some interesting
behavior of HR's around floats.

Comment 14

[rickg]

moving all buster m15 bugs to m16.

Comment 15

[buster]

*** Bug 34921 has been marked as a duplicate of this bug. ***

Comment 16

[Sean Richardson]

*** Bug 35788 has been marked as a duplicate of this bug. ***

Comment 17

[buster]

mass-moving all remaining M16 bugs to M17

Comment 18

[karnaze (gone)]

Adding nsbeta2 keyword since there are a lot of duplicates.

Comment 19

[karnaze (gone)]

Correcting mistake and reassigning back to buster.

Comment 20

[Peter Trudelle]

[nsbeta2-]

Comment 21

[Joe McCabe]

This bad <HR> behavior appears to be gone in the M16 build for Linux.

Comment 22

[buster]

*** Bug 34892 has been marked as a duplicate of this bug. ***

Comment 23

[Milan Kopacka]

The code attached by Silvester Erdeg -
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=3436
is in M16 still incorrect.

So is the code I submitted as #20466

M16 for Linux, Build ID: 2000061311

Mozilla/5.0 (X11; U; Linux 2.2.17pre9 i686; en-US; m16) Gecko/20000613

Comment 24

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

I think we should get this sorted out for beta3.  It's got a lot of dups.

Comment 25

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

*** Bug 22563 has been marked as a duplicate of this bug. ***

Comment 26

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

*** Bug 25839 has been marked as a duplicate of this bug. ***

Comment 27

[Mats Palmgren (:mats)]

*** Bug 46625 has been marked as a duplicate of this bug. ***

Comment 28

[Aaron Kaluszka]

OS field should be changed to 'All' I think..

Another page with a lot of floating HR occurances:
http://kontek.net/pp/

Comment 29

[Marc Attinasi]

Approving for beta3 based on number of dups and the fact that HR use is so 
common.

Comment 30

[buster]

mine

Comment 31

[leger]

Hopefully, can be fixed soon. Moving to P2.  Adding [PDTP2]

Comment 32

[buster]

I am soooooo close on this one!

Comment 33

[buster]

This is a mess.  HR is a block element. In Nav4 and IE5, however, it acts like 
an inline element when it is positioned next to a float.  That is, it is 
sized and positioned taking full account of the floater geometry.  This is in 
direct contradition to the CSS2 spec, which says block elements are laid out as 
if floaters did not exist. (someone could quote the spec here for me!)

But, a ton of pages make use of this behavior.

So, I suggest a quirks mode fix.  See if you can shoot any holes in this idea.  
It's truly unsavory, but it's pretty low risk and preliminary testing looks 
hopeful.

The basic idea is to make HR inline, rather than block, in quirks mode.  Use 
generated content to insert newlines before and after the HR, giving it the 
behavior of a block element.  Finally, add some magic to make it align like a 
block even though it's an inline.

The fix has 3 parts:
1) Implement generated content on HR's (in fact, on all leaf nodes.)  Generated 
content is currently impleneted on containers, but not leaf nodes.  This is 
contrary to the CSS2 spec, and I can't find a good reason for the omission.  
It's a separate issue about whether we want to advertise support for this 
feature in Netscape 6, I'll submit a separate bug on that. 

2) In quirks.css, make HR display:inline.  Add HR:before and HR:after rules with 
content("/A") and whitespace:pre.  Also make HR box-sizing:border-box; since the 
edge of an HR is drawn as a border, though really it's the content isn't it?  
Finally, make HR a "replaced" element (I don't see anything in the CSS spec that 
says HR's can't be considered "replaced" elements!  dbaron?)

3) Add code to nsLineLayout::HorizontalAlignFrames() that detects the situation 
of an HR on a line (we know it must be the only thing on the line), and tinker 
with the alignment attribute to get left, right, and center based on the style 
context for the HR element.

Feedback welcome.  Patches to be attached soon (maybe not until tomorrow am.)

Comment 34

[buster]

I have the fix as described below.  The question for dbaron, ian, and others 
interested in CSS2 compliance is:  should the be quirks mode only, or should it 
behave this way in strict mode as well?

Comment 35

[Hixie (not reading bugmail)]

*** Bug 37589 has been marked as a duplicate of this bug. ***

Comment 36

[Hixie (not reading bugmail)]

I guess it's going to have to be strict mode too, although I really don't like
saying that.

Can you make a new value for the 'display' property which triggers this magic
code? -moz-hr or something? I really would hate to know what will happen if 
someone says 
   HR { display: block; }
...or
   HR:before { content: ' / '; } 
   HR:after { content: ''; }
   HR { display: inline; border; none; margin: 0; padding: 0; }
...or whatever.

Comment 37

[buster]

Created attachment 13841 [details] [diff] [review]
proposed fix (still needs some extra commenting and minor cleanup)

Comment 38

[buster]

yeah, I know what you mean.  that's why I'd argue for it to be quirks mode only.
in standard mode, the user can get what they're looking for by other means using 
standard (non-icky) css boxes.  For example, you could stack left-floating 
boxes...
<style>
div {
  float:left; height:100px;
}
<div style="width:100px; border: 1px solid black;">left div</div>
<div>auto-width div<hr width=100%></div>

there must be a better way....

Comment 39

[Hixie (not reading bugmail)]

How possible would it be to use a new value on 'display'?

Comment 40

[buster]

fix checked in 9/11/00
r=karnaze
Ian: a new value of display would be possible, given more time.  But that's time 
I don't have this week.  If you feel strongly, a separate bug should be opened 
on that solution.

Comment 41

[Chris Petersen]

Marking verified in the Sept 12th build.

Comment 42

[Chris Waterson]

I'm looking into bug 81776, which appears to have been caused by the fixes for
this bug. Why were these changes to nsLineLayout required?

> PRUint32 textAlign = mTextAlign;
> // here is where we do special adjustments for HR's 
> // see bug 18754
> if (!InStrictMode()) {
>   if (psd->mFirstFrame && psd->mFirstFrame->mFrame)
>     {
>       nsCOMPtr<nsIAtom> frameType;
>       psd->mFirstFrame->mFrame->GetFrameType(getter_AddRefs(frameType));
>       if (nsLayoutAtoms::hrFrame == frameType.get()) {
>         // get the alignment from the HR frame
>         {
>           const nsStyleSpacing* spacing;
>           psd->mFirstFrame->mFrame->GetStyleData(eStyleStruct_Spacing,
>                                          (const nsStyleStruct*&)spacing);
>           textAlign = NS_STYLE_TEXT_ALIGN_CENTER;
>           nsStyleCoord zero(nscoord(0));
>           nsStyleCoord temp;
>           if ((eStyleUnit_Coord==spacing->mMargin.GetLeftUnit()) &&
>               (zero==spacing->mMargin.GetLeft(temp)))
>             {
>               textAlign = NS_STYLE_TEXT_ALIGN_LEFT;
>             }
>           else if ((eStyleUnit_Coord==spacing->mMargin.GetRightUnit()) &&
>                    (zero==spacing->mMargin.GetRight(temp))) {
>               textAlign = NS_STYLE_TEXT_ALIGN_RIGHT;
>             }
>         }
>     }
> }

Comment 43

[Aaron Kaluszka]

I believe there was a time when neither of these bugs existed because I have a 
site affected by them, and it was okay for a while.. I don't think bug 81776 
came from the patch for this one.