Last Comment Bug 47710 - Overflowed content does not take into account inner padding and content margins
: Overflowed content does not take into account inner padding and content margins
Status: REOPENED
[Hixie-P4] hit during nsbeta2 standar...
: css2, regression, testcase
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: -- normal with 15 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
http://www.hixie.ch/tests/evil/mixed/...
: 225396 251553 259008 269322 322881 (view as bug list)
Depends on: 240276
Blocks: 310083 313520 44192 54661 292371
  Show dependency treegraph
 
Reported: 2000-08-04 20:09 PDT by Hixie (not reading bugmail)
Modified: 2015-09-08 07:27 PDT (History)
27 users (show)
mbeltzner: blocking1.9.0.1-
samuel.sidler+old: wanted1.9.0.x-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Self explanatory testcase. (1.94 KB, text/html)
2000-08-04 20:11 PDT, Hixie (not reading bugmail)
no flags Details
fix (24.02 KB, patch)
2005-04-28 21:59 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
chofmann: approval1.8b2+
Details | Diff | Splinter Review
screenshot of testcase (64.25 KB, image/png)
2005-05-01 15:09 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
reftest-incorrect-spacing.html (256 bytes, text/html)
2014-07-02 14:24 PDT, Brandon Frohs
no flags Details
reftest-correct-spacing.html (273 bytes, text/html)
2014-07-02 14:25 PDT, Brandon Frohs
no flags Details

Description Hixie (not reading bugmail) 2000-08-04 20:09:39 PDT
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 6.0.17.2000080404.
   Originally seen on a recent Mac build.

BUG HISTORY:
   This is a regression. See bug 2751 and bug 13497.
Comment 1 Hixie (not reading bugmail) 2000-08-04 20:11:07 PDT
Created attachment 12403 [details]
Self explanatory testcase.
Comment 2 Suresh Duddi (gone) 2000-08-05 17:13:08 PDT
Style team...
Comment 3 Marc Attinasi 2000-08-07 11:49:47 PDT
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?)
Comment 4 Hixie (not reading bugmail) 2000-09-29 03:59:00 PDT
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).
Comment 5 Kevin McCluskey (gone) 2001-10-04 16:32:33 PDT
Build reassigning Buster's bugs to Marc.
Comment 6 Boris Zbarsky [:bz] 2003-03-18 18:42:14 PST
.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-03-19 12:45:13 PST
I've noticed this before.
Comment 8 Boris Zbarsky [:bz] 2003-11-11 22:00:26 PST
*** Bug 225396 has been marked as a duplicate of this bug. ***
Comment 9 Boris Zbarsky [:bz] 2004-05-01 13:36:42 PDT
So the problem is that mOverflowArea doesn't include margins (nor should it, per
documentation in nsHTMLReflowMetrics.h), right?
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-05-01 16:22:09 PDT
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.
Comment 11 Hixie (not reading bugmail) 2004-05-01 16:27:56 PDT
Exactly which case are you not sure about? I currently understand collapsing
margins pretty well and can probably explain any case you put forward.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-05-01 18:01:21 PDT
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?
Comment 13 Hixie (not reading bugmail) 2004-05-02 03:21:53 PDT
From CSS2.1, section 8.3.1:
# Vertical margins of elements with 'overflow' other than 'visible' do not 
# collapse with their in-flow children.
Comment 14 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-07-15 10:07:30 PDT
*** Bug 251553 has been marked as a duplicate of this bug. ***
Comment 15 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-09-13 08:17:35 PDT
*** Bug 259008 has been marked as a duplicate of this bug. ***
Comment 16 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-11-11 20:29:38 PST
*** Bug 269322 has been marked as a duplicate of this bug. ***
Comment 17 Gérard Talbot 2004-11-11 21:15:16 PST
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.
Comment 18 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-11-11 21:18:10 PST
also padding-right
Comment 19 Steve England [:stevee] 2005-04-28 18:42:00 PDT
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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-04-28 21:59:31 PDT
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.
Comment 21 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2005-04-29 15:59:37 PDT
This is just fixing the inner padding aspects of this bug, not the content
margins aspects, right?
Comment 22 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2005-04-29 16:00:29 PDT
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?
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-05-01 15:09:16 PDT
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.
Comment 24 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2005-05-01 15:17:53 PDT
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.
Comment 25 Hixie (not reading bugmail) 2005-05-01 15:20:37 PDT
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 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-05-01 17:07:22 PDT
Comment on attachment 182143 [details] [diff] [review]
fix

fixes a layout regression I just introduced
Comment 27 chris hofmann 2005-05-01 20:30:12 PDT
Comment on attachment 182143 [details] [diff] [review]
fix

a=chofmann
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-05-01 21:27:02 PDT
checked in. I'll leave this open for the margin issue.
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-05-01 21:55:53 PDT
I checked in the wrong patch. I'll check in the right patch tomorrow.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-05-02 16:02:26 PDT
checked in
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-05-02 16:02:49 PDT
reopening because the content margin issue is still there.
Comment 32 Steve England [:stevee] 2005-06-03 11:08:21 PDT
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.
Comment 33 Gérard Talbot 2005-07-07 00:46:57 PDT
The testcase WORKSFORME with Mozilla 1.8b2 build 2005070105 and Firefox Deer
Park alpha 1 build 20050706. Is there still an issue?

Comment 34 Boris Zbarsky [:bz] 2005-07-07 10:24:22 PDT
See comment 24.
Comment 35 Gérard Talbot 2006-01-31 01:20:59 PST
Reduced testcase:
http://www.gtalbot.org/BugzillaSection/PaddingRightNotRendered.html
Comment 36 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2006-05-30 15:18:19 PDT
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.
Comment 37 Boris Zbarsky [:bz] 2008-02-03 11:42:05 PST
*** Bug 322881 has been marked as a duplicate of this bug. ***
Comment 38 Foteos Macrides 2008-06-22 11:00:55 PDT
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#


Comment 39 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2008-12-26 09:01:16 PST
... and I think bug 458296 is fixing the vertical margin case.

So maybe this is fixed given comment 31, comment 36, and this?
Comment 40 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2008-12-26 09:01:49 PST
Er, I should say *will be* fixed.
Comment 41 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2008-12-26 09:02:37 PST
Actually, it isn't really, since that only considers the margins on children and not margins of their descendants.
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-27 02:41:19 PST
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?
Comment 43 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2008-12-27 07:06:58 PST
Yes, although the case I was thinking of was actually:

<div style="height: 10px">
  <div style="margin-bottom: 10px">
    lots of contents
  </div>
</div>
Comment 44 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2008-12-27 07:09:32 PST
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.
Comment 45 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-27 10:47:55 PST
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).
Comment 46 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2008-12-27 10:55:10 PST
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).
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-27 10:59:13 PST
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.
Comment 48 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2008-12-27 11:37:07 PST
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.
Comment 49 Brandon Frohs 2014-07-02 14:24:43 PDT
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.
Comment 50 Brandon Frohs 2014-07-02 14:25:31 PDT
Created attachment 8449740 [details]
reftest-correct-spacing.html

Note You need to log in before you can comment on or make changes to this bug.