Closed
Bug 423385
Opened 17 years ago
Closed 17 years ago
floating parent does not size correctly with floating child using negative margin
Categories
(Core :: Layout: Floats, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla1.9beta5
People
(Reporter: xtc4uall, Assigned: roc)
References
()
Details
(Keywords: regression, testcase)
Attachments
(6 files)
(deleted),
image/png
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
beltzner
:
approval1.9b5+
|
Details | Diff | Splinter Review |
after implementation of the reflow branch to trunk the rendering of http://www.vobis.com/shop.html seems to have regressed (see screenshot #1).
the site is valid, but has some css errors.
it works with Fx2 and Fx3-pre-a1 builds prior to reflow-branch-merging (like firefox-3.0a1.en-US.win32-2006-12-06-07).
Reporter | ||
Comment 1•17 years ago
|
||
thanks to polidobj for providing the testcase
Reporter | ||
Comment 2•17 years ago
|
||
screenshot of rendering the testcase with Fx3, latest WebKit, Opera 9.5alpha
(thanks to phiw13)
Comment 3•17 years ago
|
||
Note: without 'overflow:hidden' on the outside, wrapping div, the test case renders the same in Gecko 1.9 and WebKit.
Updated•17 years ago
|
Flags: blocking1.9?
Updated•17 years ago
|
Reporter | ||
Comment 4•17 years ago
|
||
Comment 5•17 years ago
|
||
I find it odd that the overflow style has an effect, given that the div should be shrink-wrapping anyway. I'd think the pref width (which includes the margins) would just be that 305px width even with overflow:visible....
Assignee | ||
Comment 6•17 years ago
|
||
This seems like something we should fix, and it shouldn't be that hard. Who wants it?
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Assignee | ||
Comment 7•17 years ago
|
||
So I guess what happens here is that nsIFrame::InlinePrefWidthData::ForceBreak accumulates the widths of all floats and then adds the result into the line width. So the large negative margin on the purple float "pulls back" the pref-width of the line to something fairly small, ensuring that the red float gets cut off.
IMHO it would make more sense and be more compatible (looking at that testcase rendering, thanks!) to just ignore floats that don't change the available space.
Assignee | ||
Comment 8•17 years ago
|
||
This fixes this testcase, making our rendering compatible with MSIE8 and Opera. And I think it makes sense. Running reftests now.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #311118 -
Flags: superreview?(dbaron)
Attachment #311118 -
Flags: review?(dbaron)
Assignee | ||
Comment 9•17 years ago
|
||
Reftests pass.
Assignee | ||
Updated•17 years ago
|
Whiteboard: [needs review]
Assignee | ||
Updated•17 years ago
|
Whiteboard: [needs review] → [needs review dbaron]
Comment 10•17 years ago
|
||
--> P2, this doesn't block the beta
Priority: P1 → P2
Target Milestone: --- → mozilla1.9
Comment 11•17 years ago
|
||
Mike, this could have some web-compat fallout, so if we _can_ take it for the beta, we really should...
Assignee | ||
Comment 12•17 years ago
|
||
Assignee | ||
Comment 13•17 years ago
|
||
I think we should get this into beta5 if we possibly can
Assignee | ||
Comment 14•17 years ago
|
||
I'll make this P1 again in the hope someone notices the above comments :-)
Priority: P2 → P1
Comment 15•17 years ago
|
||
We've noticed: waiting on dbaron's review ...
Comment 16•17 years ago
|
||
Comment on attachment 311118 [details] [diff] [review]
fix?
r+sr=dbaron
Attachment #311118 -
Flags: superreview?(dbaron)
Attachment #311118 -
Flags: superreview+
Attachment #311118 -
Flags: review?(dbaron)
Attachment #311118 -
Flags: review+
Comment 17•17 years ago
|
||
Comment on attachment 311300 [details] [diff] [review]
fix with reftest
a1.9b5=beltzner based on dbaron's review of above
Attachment #311300 -
Flags: approval1.9b5+
Updated•17 years ago
|
Whiteboard: [needs review dbaron]
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Comment 18•17 years ago
|
||
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v <-- nsFrame.cpp
new revision: 3.791; previous revision: 3.790
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/423385-1-ref.html,v
done
Checking in layout/reftests/bugs/423385-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/423385-1-ref.html,v <-- 423385-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/423385-1.html,v
done
Checking in layout/reftests/bugs/423385-1.html;
/cvsroot/mozilla/layout/reftests/bugs/423385-1.html,v <-- 423385-1.html
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list
new revision: 1.410; previous revision: 1.409
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: mozilla1.9 → mozilla1.9beta5
Comment 19•17 years ago
|
||
REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_centos5/mozilla/layout/reftests/pixel-rounding/border-height-5.html
REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_centos5/mozilla/layout/reftests/pixel-rounding/border-height-6.html
REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_centos5/mozilla/layout/reftests/pixel-rounding/border-width-5.html
REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_centos5/mozilla/layout/reftests/pixel-rounding/border-width-6.html
Failed on Linux and Win32... seems to pass on Mac, though. Backed out patch and the reftest.list addition.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 20•17 years ago
|
||
Broken reftests? Bad patch? At this point I'd like someone to reiterate the *need* to have this in the beta, as we're blocking on it, and we shipped 4 betas without it and didn't even relnote or get a lot of dupes.
Comment 21•17 years ago
|
||
(In reply to comment #19)
> REFTEST UNEXPECTED FAIL:
> file:///builds/slave/trunk_centos5/mozilla/layout/reftests/pixel-rounding/border-height-5.html
> REFTEST UNEXPECTED FAIL:
> file:///builds/slave/trunk_centos5/mozilla/layout/reftests/pixel-rounding/border-height-6.html
> REFTEST UNEXPECTED FAIL:
> file:///builds/slave/trunk_centos5/mozilla/layout/reftests/pixel-rounding/border-width-5.html
> REFTEST UNEXPECTED FAIL:
> file:///builds/slave/trunk_centos5/mozilla/layout/reftests/pixel-rounding/border-width-6.html
>
> Failed on Linux and Win32... seems to pass on Mac, though. Backed out patch and
> the reftest.list addition.
>
Aren't these due to bug 417178?
Updated•17 years ago
|
Keywords: checkin-needed
Comment 22•17 years ago
|
||
Relanded.
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v <-- nsFrame.cpp
new revision: 3.793; previous revision: 3.792
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list
new revision: 1.414; previous revision: 1.413
done
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Reporter | ||
Comment 23•17 years ago
|
||
with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032605 Firefox/3.0.0.0 ID:2008032605 testcase is rendered like MSIE8 beta1/Opera 9.5 build 9841 and http://www.vobis.com/shop.html looks fine too => VERIFIED FIXED
roc = my hero :D
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•