1.94 KB, text/html
24.02 KB, patch
chris hofmann: approval1.8b2+
|Details | Diff | Splinter Review|
64.25 KB, image/png
256 bytes, text/html
273 bytes, text/html
TO REPRODUCE: View the attached testcase. ACTUAL RESULTS: The inner content stops at its bottom border, even though there remains some inner content margin and container padding to display. EXPECTED RESULTS: The scrollbars should allow access to all the content, even the whitespace. Currently, we are probably deciding that margins are not content and so we only let the user scroll to the lowest border, which means that there is no way of giving a margin to overflowed scrolled content. From the spec: # 10.6.3 Block-level, non-replaced elements in normal flow, and floating, # non-replaced elements # [...] # If [a block box] has block-level children, the height is the distance # between the top border-edge of the topmost block-level child box and the # bottom border-edge of the bottommost block-level child box. However, if the # element has a non-zero top padding and/or top border, then the content # starts at the top margin edge of the topmost child. Similarly, if the # element has a non-zero bottom padding and/or bottom border, then the # content ends at the bottom margin edge of the bottommost child. TESTED ON: Windows 2000 Commercial Build 126.96.36.1990080404. Originally seen on a recent Mac build. BUG HISTORY: This is a regression. See bug 2751 and bug 13497.
This looks like a layout issues, not style. Reassigning to buster and marking future since he will probably not get a chance to look at it any time soon due to his general doomage factor being high. Also, marking beta3- since there is no content lost and nothing crashes (rigorous criterion, eh?)
Seems we are drawing the scrollbars in a strange place too, which is fun. This is particularly visible with the bottom scrollbar on http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/htmlbodyrendering6.html ...(although that page has other errors).
Build reassigning Buster's bugs to Marc.
I've noticed this before.
*** Bug 225396 has been marked as a duplicate of this bug. ***
So the problem is that mOverflowArea doesn't include margins (nor should it, per documentation in nsHTMLReflowMetrics.h), right?
I'm not sure about how collapsing margins are supposed to work here. One thing I'm sure of is that the padding on the scrolled DIV is in the scrolled area at the top, left and right but not on the bottom. That is a bug for sure.
Exactly which case are you not sure about? I currently understand collapsing margins pretty well and can probably explain any case you put forward.
In the testcase, does the 2em margin-bottom on the inner DIV collapse with a margin on a subsequent DIV? If so, should that margin area be considered to be part of the inner DIV that overflows the outer DIV?
From CSS2.1, section 8.3.1: # Vertical margins of elements with 'overflow' other than 'visible' do not # collapse with their in-flow children.
*** Bug 251553 has been marked as a duplicate of this bug. ***
*** Bug 259008 has been marked as a duplicate of this bug. ***
*** Bug 269322 has been marked as a duplicate of this bug. ***
Would it be ok to change "inner padding" to "padding-bottom" in the summary to help duplicate searches? It seems to me that only padding-bottom is involved. My 2 cents.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050428 Firefox/1.0+ Since the landing of bug 240276 there is now the expected gap between the bottom of the div and the bottom of the containing scrollbox. However, the gap between the right of the div and the containing scrollbox now appears to be twice a big as it was previously.
Created attachment 182143 [details] [diff] [review] fix Yeah, the padding in this testcase is still wrong. The change to have the reflow state set zero padding for the scrollframe is wrong. That confuses everything because mComputedWidth/Height on the scrollframe ends up including the element's padding, but the nsGfxScrollFrame code keeps assuming it excludes the padding. This patch backs out that change and modifies nsHTMLScrollFrame::Reflow and friends to explicitly avoid using the padding stored in the reflow state.
This is just fixing the inner padding aspects of this bug, not the content margins aspects, right?
Er, actually, I could imagine it fixing both for the vertical case, but not for horizontal, where that's a lot more work. Perhaps it should be filed as a separate bug?
Created attachment 182358 [details] screenshot of testcase It all looks fine to me, 4em of whitespace on each side of the content's border-box. If I remove all the padding, I still see 2em of whitespace on each side. What problem are you expecting to see? Margin on the inner DIV gets included in the overflow area of the outer DIV (the scrolled frame), and margin on the outer DIV isn't a problem of course.
I see how it can fix the margins problem for a subset of the vertical case, but not for the general case. Consider: <div style="overflow: auto; height: ...; width: ..."> <div style="width: 130%; margin-right: 10%"> </div> </div> and: <div style="overflow: auto; height: 100px; width: ..."> <div style="height: 0"> <div style="height: 200px; margin-bottom: 50px"> </div> <div> </div> I think this bug is suggesting that we should scroll to show those margins. I don't think this patch does that, since it's a lot more work. I'm not sure if the spec requires it or what other browsers do.
I think the spec at least intends to require it; see comment 0. I haven't checked to see if we changed the spec in the last 5 years though.
Comment on attachment 182143 [details] [diff] [review] fix fixes a layout regression I just introduced
Comment on attachment 182143 [details] [diff] [review] fix a=chofmann
checked in. I'll leave this open for the margin issue.
I checked in the wrong patch. I'll check in the right patch tomorrow.
reopening because the content margin issue is still there.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050603 Firefox/1.0+ ID:2005060305 The testcase now WFM.
The testcase WORKSFORME with Mozilla 1.8b2 build 2005070105 and Firefox Deer Park alpha 1 build 20050706. Is there still an issue?
See comment 24.
Reduced testcase: http://www.gtalbot.org/BugzillaSection/PaddingRightNotRendered.html
So I think the content margin part of this is invalid, at least for the horizontal case (and preferably for both due to symmetry), because the CSS spec says that margin-right in an overconstrained case is ignored and becomes negative.
Note that the right padding also is missing for wide content which invokes the horizontal scroll bar See: http://www.macridesweb.com/oltest/BugFx_overflow_padding.html and: http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/56714cc038193d59?hl=en#
... and I think bug 458296 is fixing the vertical margin case. So maybe this is fixed given comment 31, comment 36, and this?
Er, I should say *will be* fixed.
Actually, it isn't really, since that only considers the margins on children and not margins of their descendants.
What are you referring to in comment #41? Bug 458296 ensures that the carried-out bottom margin of a scrolled block is added to the scrollable area. That includes the carried-out margins of block descendants if they collapse through to the bottom of the scrolled content. So given <div style="overflow:scroll;"> <div style="height:20px;">Hello</div> <div style="margin-bottom:100px;"></div> <div style="margin-bottom:-50px;"></div> </div> the height of the scrollable area will be 70px. Are you saying it should be 120px?
Yes, although the case I was thinking of was actually: <div style="height: 10px"> <div style="margin-bottom: 10px"> lots of contents </div> </div>
That said, I suspect other browsers don't do that, and as far as I can tell the spec doesn't actually define what to do here, so perhaps our behavior with bug 458296 is good enough.
The (In reply to comment #43) > <div style="height: 10px"> > <div style="margin-bottom: 10px"> > lots of contents > </div> > </div> The patch actually handles this case. Reflow of the outer div sets bottomEdgeOfChildren to include the outgoing bottom margin (because of the NS_UNCONSTRAINEDSIZE != aReflowState.ComputedHeight() test).
Sorry, I meant that there was a <div style="overflow:scroll"> ... </div> *outside* what I wrote in comment 43 (not that the overflow:scroll was on the outer div there).
That still works. nsBlockFrame::ComputeCombinedArea adjusts the overflow area to include bottomEdgeOfChildren for all blocks, so the height:10px div's overflow area includes the child's bottom margin, and the overflow area propagates up to the scrolled parent.
Ah, right. So I guess this will be fixed, except for a few very obscure direction-swapping cases, e.g.: <div style="direction:ltr; overflow:scroll"> <div style="direction:rtl"> <div style="position: relative; right: 50px; margin-right: 50px"> </div> </div> </div> that aren't covered by comment 36.
Created attachment 8449739 [details] reftest-incorrect-spacing.html Just came across this issue in production (at least I *think* it's the same one). I'm attaching a minimal html file that reproduces the problem, along with another file that can be used as a reference for what should(?) happen.