Closed Bug 393655 Opened 18 years ago Closed 18 years ago

{inc} Text in block overlaps image with margin-top or margin-bottom

Categories

(Core :: Layout: Block and Inline, defect, P1)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: MatsPalmgren_bugz, Assigned: dholbert)

References

(
URL
)

Details

(Keywords: regression, testcase, Whiteboard: [dbaron-1.9:RwCr])

Attachments

(7 files, 2 obsolete files)

Testcase #1
577 bytes, text/html
Details
testcase #2 (using data: uri for a 1px image)
662 bytes, text/html
Details
testcase #3 (using margin-top)
631 bytes, text/html
Details
reference #3 (added alt-text)
646 bytes, text/html
Details
testcase #4 (reduced, w/ dhtml)
265 bytes, text/html
Details
reftests (as patch)
3.15 KB, patch
roc
: review+
Details | Diff | Splinter Review
patch v3 (w/ more reftests)
5.91 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review
{inc} Text in block overlaps image with margin-bottom. STEPS TO REPRODUCE 1. load the attached testcase ACTUAL RESULT The text overlaps the image. Resizing the window slightly causes it to be displayed at the correct position. PLATFORMS AND BUILDS TESTED Bug occurs in Firefox 2007082304 trunk on Linux. It's a regression from bug 300030 (I searched its "depends on" list for a dupe but couldn't find one).
Attached file Testcase #1 —
If the text doesn't overlap on first load, try Shift+Reload. FYI, if I change the doctype to <!DOCTYPE HTML> or remove it then the bug does not occur. Or, if I add explicit width/height attributes on the <img> then the bug doesn't occur.
All/All, I see this on OS X builds. I've seen this for a couple of days now on and off on the front page of http://elpais.com.
OS: Linux → All
Hardware: PC → All
I see this daily, especially on slower sites, e.g. http://www.emimusic.jp/st/ It has high visibility and makes sites ugly, we shouldn't release Firefox 3 with this bug IMHO.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Whiteboard: [dbaron-1.9:RwCr]
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
It looks like we're doing something like this: During the body's first reflowDirtyLines call (before we've downloaded the image): a) Line 1 gets height = 17px (from font metrics) b) Line 2 gets y-position of 0 (despite line 1's height) *** Note: Line 1 overlaps Line 2 at this point. During a later reflowDirtyLines call (after we have the image data): c) Line 1 gets height = [image height + margin]. We keep track of how much it grew in deltaY, at nsBlockFrame.cpp:1911 deltaY = line->mBounds.YMost() - oldYMost; d) Line 2's y-position is incremented by deltaY, at nsBlockFrame::1921 SlideLine(aState, line, deltaY); So since the lines overlapped at (b), they still overlap at the end, because we shift the second line down exactly as much as line 1 grows. I'm guessing the problem is with the fact that the lines overlap after the first reflowDirtyLines, with either part (a) or part (b). That is... - at (a), we should give the first line a height of 0px (not 17px) in the absence of image data OR - at (b), we should give the second line a y-position of 17px so it doesn't overlap the first line.
At a guess, line 2 gets y-position 0 because PlaceLine for line 1 says that the line IsEmpty() and therefore mY should not advance. Is that right?
roc -- yup, you're right -- the code that does what you described is here: http://mxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#4021 4021 if (!aLine->CachedIsEmpty()) { 4022 // This line has some height. Therefore the application of the 4023 // previous-bottom-margin should stick. 4024 aState.mPrevBottomMargin.Zero(); 4025 newY = aLine->mBounds.YMost(); 4026 } 4027 else { 4028 // Don't let the previous-bottom-margin value affect the newY 4029 // coordinate (it was applied in ReflowInlineFrames speculatively) 4030 // since the line is empty. 4031 // We already called |ShouldApplyTopMargin|, and if we applied it 4032 // then BRS_APPLYTOPMARGIN is set. 4033 nscoord dy = aState.GetFlag(BRS_APPLYTOPMARGIN) 4034 ? -aState.mPrevBottomMargin.get() : 0; 4035 newY = aState.mY + dy; 4036 aLine->SlideBy(dy); // XXXldb Do we really want to do this? 4037 } On Shift-Reload: -- CachedIsEmpty returns true, because the most nested frame is an InlineFrame(img), which returns True by default. On normal Reload: -- CachedIsEmpty returns false, because the most nested frame is an ImageFrame(img), which returns False by default. (via the stub nsFrame::IsEmpty) (note: at line 4035, "newY = aState.mY + dy;", aState.mY and dy are both 0).
So to say it a slightly different way -- it seems that the basic issue is this: - When IsEmpty() / CachedIsEmpty() is true, we don't increment mY with that line's height in PlaceLines - When a new element within that line is substituted for another one, we increment mY for the subsequent lines by [newElement.YMost - oldElement.YMost] And this strategy breaks when IsEmpty() changes from false to true during the substitution.
Maybe somewhere within nsCSSFrameConstructor::RecreateFramesForContent, we need to have something like this: if (the affected line's IsEmpty() has changed) { mark the following line as dirty; } where "the affected line" is the line in the nearest ancestor block that contains the recreated frames.
(In reply to comment #11) > It's probably better to try to fix this logic in ReflowDirtyLines: > http://mxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#1869 > Actually, cancel comment #10 -- I'm looking more into the difference between how we reflow in the with-margin case vs. without-margin case now. Before the image downloads, and just after the nested InlineFrames have all been reflowed, the first line has height 0 in the without-margin case but it has height 17px in the with-margin case (regardless of the margin size). I don't think that difference makes sense, and that's got to be the root of our problem here. (let me know if that difference is expected for some reason, though)
Ok -- the difference is due to the "zeroEffectiveSpanBox" quirk, in nsLineLayout::VerticalAlignFrames. http://mxr.mozilla.org/seamonkey/source/layout/generic/nsLineLayout.cpp#1655 When there's no margin (the "normal" case), we set the zeroEffectiveSpanBox flag to true. But when we add a margin, we leave it at false. This flag affects how we behave at various places, in particular, it controls this clause: http://mxr.mozilla.org/seamonkey/source/layout/generic/nsLineLayout.cpp#2114 That clause is where we shrink the nsInlineFrames mRects back down from 1140 (17 px) to 0 height, and we're hurting in the test case because we're skipping that clause.
Note the comment right above where zeroEffectiveSpanBox is set: 1658 // XXXldb This should probably just use nsIFrame::IsSelfEmpty, assuming that 1659 // it agrees with this code. (If it doesn't agree, it probably should.) This bug's testcase one instance where IsSelfEmpty does *not* agree with this code. That is, spanFramePFD->mFrame->IsSelfEmpty() == PR_TRUE zeroEffectiveSpanBox == PR_FALSE (where spanFramePFD->mFrame is the innermost nsInlineFrame for the image, before the image data has been downloaded)
(In reply to comment #13) > Ok -- the difference is due to the "zeroEffectiveSpanBox" quirk, in > nsLineLayout::VerticalAlignFrames. ... although that's probably not the whole story, since: - the zeroEffectiveSpanBox code seems to have been around and relatively untouched since long before Firefox 2.0, and this bug's testcases work in Firefox 2.0 - an alternate testcase with margin-right instead of margin-bottom doesn't trigger the bug, even though it should still disable zeroEffectiveSpanBox
(In reply to comment #15) > and this bug's testcases work in > Firefox 2.0 Ah: On Branch, the image gets a nsImageFrame from the start. (whereas Trunk starts it out with an nsInlineFrame -- see comment #8) So that's why Branch works here.
This testcase shows that margin-top causes the problem, too -- not just margin-bottom.
Attached patch patch v1 (obsolete) — — Splinter Review
This patch extends "IsSelfEmpty" to count top and bottom margin/padding as being non-empty, rather than just sideways margins. This fixes the problem mentioned in Comment #7 and Comment #8 with the line returning IsEmpty() and not incrementing mY.
Comment on attachment 286741 [details] [diff] [review] patch v1 This looks fine but could you check for browser and spec compatibility? In particular you're changing what happens in a testcase like this: <div style="margin-top:100px; margin-bottom:100px;"><span style="padding-top:1px;"></span></div> Before this patch, we'll collapse the top and bottom margins of the DIV together because it's empty. After this patch, we won't. Which sounds fine to me but interop matters.
Attachment #286741 - Flags: superreview+
Attachment #286741 - Flags: review+
(In reply to comment #19) > (From update of attachment 286741 [details] [diff] [review]) > This looks fine but could you check for browser and spec compatibility? Hrm -- this patch does make us diverge from IE on a testcase like the one you described. See http://people.mozilla.com/~dholbert/tests/393655/breaktests/break1.html With this patch, we draw a green bar for the inner div, with 100px padding on either side. (as compared to unpatched trunk == IE == no green bar, 100px padding total)
Comment on attachment 286741 [details] [diff] [review] patch v1 Obsoleting patch, as it'd cause a regression. (see prev 2 posts)
Attachment #286741 - Attachment is obsolete: true
Summary: {inc} Text in block overlaps image with margin-bottom → {inc} Text in block overlaps image with margin-top or margin-bottom
Same as testcase #3, but with alt-text added. The alt text makes "IsEmpty()" return false while we're loading the image, and so the bug doesn't reproduce in this case.
You should be able to create a DHTML testcase which just inserts a character into a span to switch it from empty to non-empty. That'd be simpler
Simple DHTML testcase, per roc's suggestion.
IE7, Branch, Konqueror, and Opera all render testcase #4 as: a b Trunk renders it with the a and b superimposed.
Attached patch patch v2 (obsolete) — — Splinter Review
This patch extends the reach of the "maybeWasEmpty" boolean in nsBlockFrame::reflowDirtyLines to handle the EmptyLine --> NonEmptyLine cases that cause this bug. Previously, "maybeWasEmpty" just indicated whether the line used to have zero height. Now it also indicates whether the line used to *effectively* have zero height -- i.e. whether the subsequent line had the same y-position.
Attachment #286851 - Flags: superreview?(roc)
Attachment #286851 - Flags: review?(roc)
2 reftests, built from DHTML testcase. One uses margin-top and the other uses margin-bottom. (also moved a nearby out-of-order line in bugs/reftest.list into its correct position)
Attachment #286853 - Flags: review?(roc)
In nsBlockFrame::ReflowDirtyLines, "PropagateFloatDamage" isn't marking the body's second line as dirty, when it should. If we do mark it as dirty, the page lays out correctly.
+ PRBool maybeWasEmpty = (oldY == oldYMost || + oldY == line.next()->mBounds.y); Can't we just use the second condition instead?
Reftests landed.
Flags: in-testsuite+
(In reply to comment #29) > + PRBool maybeWasEmpty = (oldY == oldYMost || > + oldY == line.next()->mBounds.y); > > Can't we just use the second condition instead? Yes -- I'll post a new patch to that effect shortly. Removing the first condition will mean we'll be slightly lying about "maybeWasEmpty" in some cases. But we'll still do the right thing in those cases, *and* we'll be faster, so it'll be ok. More Detailed Explanation: The only situation in which we'd satisfy the first condition but not the second condition would be if we were already honoring a margin on the empty line, at the time when it changes from empty to nonEmpty. For example, suppose we start with this: FOO<div style="margin-bottom: 50px"></div><div>BAR</div> In this case, we'd honor the styled div's margin, even though it's empty, because there's content both above and below it. Now, if we dynamically insert text into the styled div, we'll end up running across the "maybeWasEmpty" check. Technically, the div *was* empty, but this new patch won't set maybeWasEmpty to be true. That's ok, though, because the margins were already being honored. So we're ok just calculating deltaY and shifting the next lines by deltaY, as normal. This is a (minor) speedup in such cases, too, because marking the next line's previous margin dirty (as we previously would've) requires that we reflow that line. After this patch, we'll just shift the next line, without needing to reflow it.
This implements roc's suggestion from comment 29. Includes 3 additional reftests, which demonstrate that this change is OK, using examples like the one described in Comment 31. These new tests all pass both before and after this patch.
Attachment #286851 - Attachment is obsolete: true
Attachment #286851 - Flags: superreview?(roc)
Attachment #286851 - Flags: review?(roc)
Attachment #288602 - Flags: superreview?(roc)
Attachment #288602 - Flags: review?(roc)
BTW -- the only difference between the new tests is that 393655-3-ref uses margin-bottom, 393655-4-ref uses margin-top, and 393655-5-ref uses both top and bottom.
Attachment #288602 - Flags: superreview?(roc)
Attachment #288602 - Flags: superreview+
Attachment #288602 - Flags: review?(roc)
Attachment #288602 - Flags: review+
checkin needed?
Nope, I'll check it in myself as soon as tree reopens.
patch v3 checked in. Checking in generic/nsBlockFrame.cpp; /cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v <-- nsBlockFrame.cpp new revision: 3.889; previous revision: 3.888 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/393655-3-ref.html,v done Checking in reftests/bugs/393655-3-ref.html; /cvsroot/mozilla/layout/reftests/bugs/393655-3-ref.html,v <-- 393655-3-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/393655-3.html,v done Checking in reftests/bugs/393655-3.html; /cvsroot/mozilla/layout/reftests/bugs/393655-3.html,v <-- 393655-3.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/393655-4-ref.html,v done Checking in reftests/bugs/393655-4-ref.html; /cvsroot/mozilla/layout/reftests/bugs/393655-4-ref.html,v <-- 393655-4-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/393655-4.html,v done Checking in reftests/bugs/393655-4.html; /cvsroot/mozilla/layout/reftests/bugs/393655-4.html,v <-- 393655-4.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/393655-5-ref.html,v done Checking in reftests/bugs/393655-5-ref.html; /cvsroot/mozilla/layout/reftests/bugs/393655-5-ref.html,v <-- 393655-5-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/393655-5.html,v done Checking in reftests/bugs/393655-5.html; /cvsroot/mozilla/layout/reftests/bugs/393655-5.html,v <-- 393655-5.html initial revision: 1.1 done Checking in reftests/bugs/reftest.list; /cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list new revision: 1.209; previous revision: 1.208 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Depends on: 405517
(In reply to comment #31) > (In reply to comment #29) > > + PRBool maybeWasEmpty = (oldY == oldYMost || > > + oldY == line.next()->mBounds.y); > > > > Can't we just use the second condition instead? > > Yes -- I'll post a new patch to that effect shortly. We may need the first (original) condition after all -- I think its removal may have caused bug 405517.
Nevermind, we still shouldn't need that first condition. Bug 405517 was caused by a different issue which wasn't important when we had the "oldY == oldYMost" condition, but became visible when that condition was removed. That issue's been fixed -- see the bug page for details.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: