Closed
Bug 325680
Opened 19 years ago
Closed 18 years ago
'width: auto' incorrectly shrink wraps on blocks inside a shrink-wrapped 'overflow'-non-'visible' div
Categories
(Core :: Layout: Block and Inline, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: matej, Assigned: roc)
References
Details
(Keywords: fixed1.8.1, Whiteboard: [reflow-refactor])
Attachments
(3 files, 2 obsolete files)
|
566 bytes,
text/html
|
Details | |
|
7.29 KB,
patch
|
dbaron
:
review-
dbaron
:
superreview-
|
Details | Diff | Splinter Review |
|
2.36 KB,
patch
|
roc
:
review+
roc
:
superreview+
roc
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
If we have containing block with position:absolute or position:fixed and it has overflow:hidden set, the calculated width of contained blocks is wrong (width:auto does not work for them).
Reproducible: Always
Steps to Reproduce:
Try following minimum test case:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">
<html>
<head>
<title>Test</title>
<style type="text/css">
#container {
position: absolute;
overflow: hidden;
background-color: silver;
}
p {
background-color: gray;
}
</style>
</head>
<body>
<div id="container">
<p>test block1</p>
<p>test block2 sdfsdfsd</p>
<p>test block3</p>
</div>
</body>
</html>
Actual Results:
<p> blocks have different widths (according to the width of their content)
Expected Results:
all <p> block should have the width of container div.
This problem started with Firefox 1.5, in Firefox 1.0.x it is correct.
|
||
Updated•19 years ago
|
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → 1.8 Branch
|
||
Comment 1•19 years ago
|
||
This changed between 1.8b2_2005042806 and 1.8b2_2005042906.
Yikes, this is a pretty serious regression. Maybe a little related to bug 309110, but not duplicate.
Status: UNCONFIRMED → NEW
Component: Layout → Layout: Block and Inline
Ever confirmed: true
QA Contact: layout → layout.block-and-inline
Summary: wrong width of blocks inside absolute or fixed positioned block with overflow:hidden → 'width: auto' incorrectly shrink wraps on blocks inside a shrink-wrapped 'overflow'-non-'auto' div
*** Bug 327144 has been marked as a duplicate of this bug. ***
Summary: 'width: auto' incorrectly shrink wraps on blocks inside a shrink-wrapped 'overflow'-non-'auto' div → 'width: auto' incorrectly shrink wraps on blocks inside a shrink-wrapped 'overflow'-non-'visible' div
Whiteboard: [reflow-refactor]
(In reply to comment #2) > Maybe a little related to bug 309110, but not duplicate. Actually, not really, now that I've read it.
|
Assignee | |
Comment 5•18 years ago
|
||
Assignee: nobody → roc
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 6•18 years ago
|
||
The problem is that nsBlockFrame doesn't reflow itself to account for the shrink-wrap width; it assumes its parent will do that. But its parent in this case is nsHTMLScrollFrame, which doesn't do that. This patch makes shrinkwrap-root nsHTMLScrollFrame reflow the scrolled content after determining its width. I bet there are weird testcases where reflowing again with the shrinkwrap width changes the content layout so that we suddenly need a scrollbar where we didn't before. Too bad. Certainly the new shrinkwrap approach on the reflow branch is the way to go.
Attachment #211958 -
Flags: superreview?(dbaron)
Attachment #211958 -
Flags: review?(dbaron)
nsBlockFrame has some logic to decide whether to reflow itself again -- could this be fixed more easily by fixing that logic? (I haven't looked yet, but wondering what your opinion is.)
|
|
Assignee | |
Comment 8•18 years ago
|
||
It could be fixed easily that way, but that could lead to pathological slowdown when you have deeply nested blocks with 'overflow' ... I think you could reflow the deepest blocks 2^depth times. Right?
Did you want this on the 1.8 branch?
|
|
Assignee | |
Comment 10•18 years ago
|
||
Yeah, 1.8.1 at least.
But what nsBlockFrame::ComputeFinalSize does is this:
// If we're shrink wrapping, then now that we know our final width we
// need to do horizontal alignment of the inline lines and make sure
// blocks are correctly sized and positioned. Any lines that need
// final adjustment will have been marked as dirty
if (aState.GetFlag(BRS_SHRINKWRAPWIDTH) && aState.GetFlag(BRS_NEEDRESIZEREFLOW)) {
// If the parent reflow state is also shrink wrap width, then
// we don't need to do this, because it will reflow us after it
// calculates the final width
const nsHTMLReflowState *prs = aReflowState.parentReflowState;
if (!prs || NS_SHRINKWRAPWIDTH != prs->mComputedWidth) {
why doesn't that kick in in this case?(Or is this one of the cases where we do shrink wrapping without NS_SHRINKWRAPWIDTH? Yay for having ten ways to do the same thing.)
sorry, without BRS_SHRINKWRAPWIDTH...
What do you think about this alternative? This works, and seems a bit less scary to me. Although I'm not a hundred percent convinced that the existing code is correct -- with nested absolutes, would we actually reflow the inner one when the outer one gets its second reflow for being shrink wrapped?
Attachment #225632 -
Flags: superreview?(roc)
Attachment #225632 -
Flags: review?(roc)
|
|
Assignee | |
Comment 15•18 years ago
|
||
(In reply to comment #11) > why doesn't that kick in in this case? Because NS_SHRINKWRAPWIDTH == prs->mComputedWidth .. the scrollframe does have shrinkwrap width.
|
|
Assignee | |
Comment 16•18 years ago
|
||
Your patch works as long as we don't wrap the block with anything inside the scrollframe. So it wouldn't work with columns inside a scrollframe. Maybe we don't care about this.
|
|
Assignee | |
Comment 17•18 years ago
|
||
I guess a more robust version of your patch could run up the prs chain to see if there's an unbroken chain of NS_SHRINKWRAPWIDTH == prs->mComputedWidth leading to another block.
|
|
Assignee | |
Comment 18•18 years ago
|
||
Comment on attachment 225632 [details] [diff] [review] alternative fix see comment #17
Attachment #225632 -
Flags: superreview?(roc)
Attachment #225632 -
Flags: superreview-
Attachment #225632 -
Flags: review?(roc)
Attachment #225632 -
Flags: review-
This does roughly what you suggested. (What scares me a bit about attachment 211958 [details] [diff] [review] is the potential interactions with the other multi-pass stuff that the scrollframe code does, as you mentioned in comment 6.)
Attachment #225632 -
Attachment is obsolete: true
Attachment #226219 -
Flags: superreview?(roc)
Attachment #226219 -
Flags: review?(roc)
|
|
Assignee | |
Comment 20•18 years ago
|
||
I think we should check NS_SHRINKWRAPWIDTH == prs->mComputedWidth on each of those intermediate reflow states.
Oh, right, I meant to do that...
Attachment #226219 -
Attachment is obsolete: true
Attachment #226224 -
Flags: superreview?(roc)
Attachment #226224 -
Flags: review?(roc)
Attachment #226219 -
Flags: superreview?(roc)
Attachment #226219 -
Flags: review?(roc)
|
|
Assignee | |
Updated•18 years ago
|
Attachment #226224 -
Flags: superreview?(roc)
Attachment #226224 -
Flags: superreview+
Attachment #226224 -
Flags: review?(roc)
Attachment #226224 -
Flags: review+
Attachment #226224 -
Flags: approval-branch-1.8.1?
Checked in to trunk. Do you think we want to get this on the 1.8 branch?
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Updated•18 years ago
|
Attachment #226224 -
Flags: approval-branch-1.8.1? → approval-branch-1.8.1+
Checked in to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
Comment on attachment 211958 [details] [diff] [review] fix minusing obsolete review request for patch that I didn't like
Attachment #211958 -
Flags: superreview?(dbaron)
Attachment #211958 -
Flags: superreview-
Attachment #211958 -
Flags: review?(dbaron)
Attachment #211958 -
Flags: review-
You need to log in
before you can comment on or make changes to this bug.
Description
•