25888 - inlines wrapping around floats only check top pixel of line for overlap (negative top margins or multiple floats)

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect, P3)

Description

[Daniel Lichtenberger]

Look at the submitted page, it contains a table (containing an image) aligned to
the left, and to the right a text paragraph with a negative margin-top (in this
case, of 7px). The first line of the text paragraph overlaps with the table,
making the text unreadable (I'm using the default 96 dpi setting). This HTML
code works ok with both Internet Explorer and Netscape Communicator, and
according to w3c.org, negative margins are allowed.
You might also want to take a look at the pre- and reviews at
http://www.topofgames.de, this problem occurs in all articles.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Right now the testcase in the URL field is not available (the document is
empty).

However, negative margins are allowed so that authors may suggest overlap.  It's
not a bug if they cause it, it's a feature.  There could be something wrong in
this case, but since I can't see the testcase and I don't see the problem on the
site mentioned, I can't tell.

Comment 2

[Daniel Lichtenberger]

I'm sorry, it seems the webserver was down yesterday. It should work now again.

Comment 3

[Marc Attinasi]

Taking over 1/3 of Pierre's NEW bugs to help reduce his doomage factor

Comment 4

[Pierre Saslawsky]

Reassigned back to me these bugs that shouldn't have left my list.

Comment 5

[Marc Attinasi]

It looks like there may be a layout bug here. The margin is not merely moving up 
by 7 pixels as the style rules says it should, it is also moving left by the 
width of the table.

Buster says this looks like a floater layout bug so I'll forward to him and then 
attach a simple testcase... 

Comment 6

[Marc Attinasi]

Attachment: simple testcase showing the incorrect wrapping of the text

Comment 7

[buster]

floaters in M15

Comment 8

[rickg]

moving all buster m15 bugs to m16.

Comment 9

[buster]

post-beta issue

Comment 10

[buster]

redistributing bugs across future milestones, sorry for the spam

Comment 11

[buster]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another 
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 

Comment 12

[Jesse Ruderman]

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

Comment 13

[Hixie (not reading bugmail)]

We need to examine this to work out exactly what is causing it and whether it
is wrong or not.

Comment 14

[Hixie (not reading bugmail)]

Yeah, this is a valid bug (simplified attachement coming up). Line boxes should
never overlap floats that are in their formatting context; in this case we are
seeing an element moved up using negative margins so that it's line boxes begin
higher than a float earlier in the flow, but we are not making the first line 
box wrap around the float.

Comment 15

[Hixie (not reading bugmail)]

Attachment: Test case - the text should not overlap the blue box

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think this has the same cause as bug 41412 -- see my 2001-09-17 13:56
comment on bug 86947.

Comment 17

[Kevin McCluskey (gone)]

Build reassigning Buster's bugs to Marc.

Comment 18

[Madhur Bhatia]

any progress with this bug ?

Comment 19

[Greg K.]

Confirmed using FizzillaCFM/2002070913. Setting All/All.

Comment 20

[Christopher Hoess (gone)]

->Layout: Floats

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 22328 [details]
Test case - the text should not overlap the blue box

This testcase doesn't show the bug anymore, but the original one does.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 26

[Hermann Schwab]

replacing http://stud3.tuwien.ac.at/~e9825754/mozilla/margin_top_overlap.html as it gives 404

Using URL from dupe Bug 300193 Text overlaps float
http://en.wikipedia.org/wiki/Hurricane_Dennis

Some en.wikipedia pages are showing this bug on Seamonkey, but not on Firefox2

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/20061104 SeaMonkey/1.1b

http://en.wikipedia.org/wiki/WP:RFAR
http://en.wikipedia.org/wiki/World_Bank_Group
http://en.wikipedia.org/wiki/International_Monetary_Fund
http://en.wikipedia.org/w/index.php?title=Oahu&oldid=58021284
http://en.wikipedia.org/wiki/Orangequat
http://en.wikipedia.org/wiki/Image:Layout_engine_usage_share.svg

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just checked in a bunch of reftests for this.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Given an easier-to-work-with space manager (or a replacement for it), this would be pretty simple to fix.  We'd just need to do the following:

* store a line available space height on the stack (maybe in nsBlockReflowState), and initialize it to zero in nsBlockFrame::ReflowInlineFrames inside the loop over bands (but outside the loop over no-pull redos).

* at the end of nsBlockFrame::DoReflowInlineFrames, check that the height the line got doesn't run into another band that's narrower than the band at its top.  (Note that we have to check all bands, not just the bottom one.)  If so, return LINE_REFLOW_REDO_NO_PULL and initialize the line available space height to the height of the line (after asserting that it's currently zero, maybe, although I'm not sure how this interacts with the other uses of NO_PULL).

* at the beginning of nsBlockFrame::DoReflowInlineFrames, initialize the available space rect based on the line available space height.

We'd need to check the interaction with the other causes of NO_PULL redoes, though.  The idea is that the line available space height would be zero for the first try of a line at a position and potentially increase (once) to a larger amount, but would never decrease until we move to the next band.

Comment 29

[Ray Kiddy]

Note that it appears that automated tests are in for this bug:

layout/reftests/bugs/25888-1l-notref.html
layout/reftests/bugs/25888-1l-ref.html
layout/reftests/bugs/25888-1l.html
layout/reftests/bugs/25888-1r-notref.html
layout/reftests/bugs/25888-1r-ref.html
layout/reftests/bugs/25888-1r.html
layout/reftests/bugs/25888-2l-ref.html
layout/reftests/bugs/25888-2l.html
layout/reftests/bugs/25888-2r-ref.html
layout/reftests/bugs/25888-2r.html
layout/reftests/bugs/25888-3l-ref.html
layout/reftests/bugs/25888-3l.html
layout/reftests/bugs/25888-3r-ref.html
layout/reftests/bugs/25888-3r.html

Right now, we have no tag to state whether a test is automated. The in-testsuite flag does not speak to this.

Comment 30

[Eli Friedman]

I can't tell if any of the testcases cover the following situation, so I thought I'd throw it out:
<div style="width: 200px; font-size: 0">
<div style="float:left; height: 10px; width: 50px"></div>
<div style="float:left; clear: left; height: 10px; width: 100px"></div>
<span style="display: inline-block; width: 10px; height:5px;"></span>
<span style="display: inline-block; width: 100px; height:15px;"></span>
</div>

Here, the second inline-block only fits below both floats; the issue here, though, is making sure that the first span is next to the first float, not the second.

Comment 34

[Stellan Klebom]

Attachment: testcase

Another testcase can be found at http://klebom.se/stellan/css/textboxclearing/

Comment 35

[Nick]

Attachment: test-case showing several related problems

See description at end of attachment.

Illustrates incorrect wrapping of lists and paragraphs around floated blocks.

Shows different possibilities when you hover on different parts, as per instructions.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: extend nsFloatManager to provide needed information

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: pre-patch 1: remove aState.IsImpactedByFloat() check in PrepareResizeReflow

I don't think this check should be needed anymore.  But if it is, it needs to be extended.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: fix this bug for inlines wrapping around floats

Full reftests pass with patches 1-3.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: fix this bug for blocks wrapping around floats

Full reftests pass with patches 1-4, but I'm still not happy with this patch (see comments within).

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A patch 5 is still needed to fix list item markers/bullets.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: stack explaining reason for hang

So (with the updated patches in my patch queue at http://hg.mozilla.org/users/dbaron_mozilla.com/patches/ ) I'm seeing a hang loading http://www.flightaware.com/ and I wrote what I think is in an equivalent testcase at 25888-2.html in the crashtests in the patches.

This stack explains why it's a problem:  it shows where my assumptions about mAvailSpaceRect not being updated during reflow are being violated.

I think the way to move forward here (which would also fix the issues that I have comments about regarding when mAvailSpaceRect is valid) is to pull mAvailSpaceRect out of nsBlockReflowState.  Instead, we should hold one avail space rect in nsBlockFrame::ReflowInlineFrames, one in nsBlockReflowState::FlowAndPlaceFloat, and one (or more) for bullet reflow (which might be a little more complicated, but could also be a little less efficient).  This would likely solve the "when is mAvailSpaceRect" valid, and also fix this hang (infinite loop in ReflowInlineFrames), which results from the things I'm trying to do with mAvailSpaceRect in PlaceLine/DoReflowInlineFrames/ReflowInlineFrames invalid.

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I've made a good bit of progress on the above in my patch queue.  However, I explicitly stopped dealing with floats inside the line, which means I'm no longer fixing the case in bug 345369.  I need to figure out how to deal with that.

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I locally reverted the change that I'd made that I expected made this not fix bug 345369 anymore.  Reverting it actually doesn't fix that bug... I haven't investigated that problem yet.

But it does reintroduce an infinite loop (broken by the spins == 1000 test) on layout/base/crashtests/266360-1.html .  And that points to a bigger problem, which is that our use of aAvailableWidth in nsBlockReflowState::AddFloat presumes this bug.  We actually do need to place floats there if they push below part of the line, but not all of it, since when we redo lines for "more floats", some of those additional floats could actually be in the line, and we don't want to skip placing them in the redo pass.  For example, given:

  <!-- narrow float above that intrudes only a few pixels into div below -->
  <div>
    <div id="B" style="float:left;clear:left;width:99%"></div>
    hello
  </div>

we should determine on the first pass that hello intersects the "B" div and that we need to reflow again, but on the second pass we still need to place the "B" div.

And if "B" is after "hello", it's even messier, since we then need to push "hello" on the second pass, which in turn pushes the float, which means we need a third pass to actually place "hello", but for that third pass we need to remember that we have to push the float that we pushed on the second pass below the line even though it looks like there was room for it.

So there's actually some messy interaction with bug 50630 here that I hadn't observed before.

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 355640 [details] [diff] [review]
pre-patch 1: remove aState.IsImpactedByFloat() check in PrepareResizeReflow

Removing this check should make the rest of this bug a good bit easier, and I think it's valid.  I may as well get this part out of my tree (and land it separately from the rest to allow separating regressions).

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: series of preparatory patches implementing comment 42

Here's a series of preparatory patches for this bug that I think I'd rather get out of my tree sooner rather than later.  This is in the format of the output of "hg out -p", but it's really a rather long sequence of patches that I'd rather keep separate (but I don't see a strong need to upload 8 separate attachments to bugzilla).

The point of these patches is described primarily in comment 42.  We already have a bit of existing confusion about when mAvailSpaceRect and mBandHasFloats are valid.  Since the patches for this bug will make their meaning more complicated (i.e., it relates to a good bit more current state than just a single vertical coordinate), this series of patches just moves mAvailSpaceRect out of nsBlockReflowState.  There's one patch to start things off, then one patch for each of the areas of code in which we track available space, and then one patch to remove the old bits of nsBlockReflowState.

Most of the patches are relatively straightforward, but the bullet one is not.  It also rewrites the hacky fix to bug 427370 in a better way that simultaneously fixes bug 428810.  The new fix involves the ability to compute available space using a saved space manager state, which is what we want in this case.  (I think this ability will also turn out useful for other patches in this bug, although I'm not entirely sure; see comment 44.)

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed pre-patch 1: http://hg.mozilla.org/mozilla-central/rev/98d7db017563

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+        aResult->mTop = kidPosition.mTop + offset;
+        aResult->mBaseline = kidPosition.mBaseline + offset;
+        aResult->mBottom = kidPosition.mBottom + offset;

Worth declaring an operator+ on LinePosition? Or a method "LinePosition MoveBy(nscoord)"? There are two or three places you could use it.

+    // Note: mAvailSpaceRect.x is relative to the content box and never

You mean floatAvailSpace.x?

 nsBlockReflowState::ComputeBlockAvailSpace(nsIFrame* aFrame,
                                            const nsStyleDisplay* aDisplay,
+                                           PRBool aBandHasFloats,
+                                           const nsRect& aFloatAvailableSpace,
                                            PRBool aBlockAvoidsFloats,
                                            nsRect& aResult)

As soon as we have more than one boolean parameter I reckon we should define a flags word. It's a little more efficient and much easier to read.

Do you think it might be worth defining a struct that contains an nsRect and a boolean to indicate whether there are any floats in the band, that GeGetFloatAvailableSpace and friends can return directly, and that we can pass around references to?

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #48)
> Do you think it might be worth defining a struct that contains an nsRect and a
> boolean to indicate whether there are any floats in the band, that
> GeGetFloatAvailableSpace and friends can return directly, and that we can pass
> around references to?

Yeah... I'd sort of been thinking that for a while.  So I went ahead and did it, but as a patch that applies on top of the other patches in the series.  (And thus I ignored the comment immediately before the one quoted, since that function now takes the new struct rather than having an additional boolean.)

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: series of preparatory patches implementing comment 42

This fixes the issues in comment 48 (except the ComputeBlockAvailSpace one, since that's no longer needed due to the change addressing the last comment).

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: series of preparatory patches implementing comment 42

A few things that I'd changed in the previous patch can still be just nsRect, actually.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 371363 [details] [diff] [review]
series of preparatory patches implementing comment 42

+    LinePosition operator+(nscoord aOffset) {

Make it const

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

preparatory patch series landed:
http://hg.mozilla.org/mozilla-central/rev/50ba649cfea2
http://hg.mozilla.org/mozilla-central/rev/4f86f4c54224
http://hg.mozilla.org/mozilla-central/rev/df0130484852
http://hg.mozilla.org/mozilla-central/rev/698d52722aeb
http://hg.mozilla.org/mozilla-central/rev/800e279fe24d
http://hg.mozilla.org/mozilla-central/rev/f8ac1e4ffe9c
http://hg.mozilla.org/mozilla-central/rev/8fdfeef7e076
http://hg.mozilla.org/mozilla-central/rev/f82ad9a10b7d
http://hg.mozilla.org/mozilla-central/rev/e5b1ab035426

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #43)
> I've made a good bit of progress on the above in my patch queue.  However, I
> explicitly stopped dealing with floats inside the line, which means I'm no
> longer fixing the case in bug 345369.  I need to figure out how to deal with
> that.

(In reply to comment #44)
> So I locally reverted the change that I'd made that I expected made this not
> fix bug 345369 anymore.  Reverting it actually doesn't fix that bug... I
> haven't investigated that problem yet.

I've been thinking that maybe I could un-revert that change, but then make certain types of line-redo get the available width for the call to AddFloat from a band rather than a position.  Or maybe it's more that I need to rethink the interaction between the different types of line-redo operations, e.g., by preserving the REDO_NO_PULL state (forceBreakInContent, etc.) when we get a REDO_MORE_FLOATS.

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: extend nsFloatManager to provide needed information

Comment 56

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: fix this bug for inlines wrapping around floats

This leaves a bunch of issues I was hoping to fix still unfixed, but does fix the most common cases.  I'd like to get this in soon and then come back to the other parts later.

(It leaves unfixed a bunch of issues dealing with a line wrapping around floats that are in the line itself.  Right now nsLineLayout::UpdateBand and its caller only handle those cases when such floats intersect the top of the line.  Getting all these issues really right would probably be easiest if we deferred float placement until we hit a break opportunity (unless we're already at one) and if we did vertical alignment and line height calculations as we reflowed the line rather than in a separate pass at the end.)

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 377450 [details] [diff] [review]
patch 3: fix this bug for inlines wrapping around floats

This seems OK.

Comment 58

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: pre-patch: remove InitFloat

I had another (more recent) preparatory patch lying around:  I don't see any need to have similar methods InitFloat and AddFloat.

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/5e76aa18541f
http://hg.mozilla.org/mozilla-central/rev/efc6ec4a3c35
http://hg.mozilla.org/mozilla-central/rev/be75e40365ce

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 179596 comment 26 has some thoughts relevant to fixing this for horizontal positioning of bullets (intended to be patch 5 in the series above, but which I've never started on).

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I landed the tests from patch 4 in
http://hg.mozilla.org/mozilla-central/rev/ec3bca6461a5

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm going to move the work of patch 4 to bug 538194 and patch 5 to bug 416732.

This bugs summary has always covered just what was already fixed by patches 1 through 3.  Thus I'm going to mark it as fixed, which it really has been for years.  (In milestone 1.9.2a1.)

Comment 65

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/c045b9b62a99
[fenix] Update Android Components version to 104.0.20220705143027. (https://github.com/mozilla-mobile/fenix/pull/25888)