Overflowed content does not take into account inner padding and content margins

REOPENED
Unassigned

Status

()

Core
Layout: Block and Inline
REOPENED
17 years ago
2 years ago

People

(Reporter: Hixie (not reading bugmail), Unassigned)

Tracking

(Blocks: 1 bug, {css2, regression, testcase})

Trunk
css2, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.1 -
wanted1.9.0.x -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P4] hit during nsbeta2 standards compliance testing [nsbeta3-], URL)

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
Created attachment 12403 [details]
Self explanatory testcase.
(Reporter)

Updated

17 years ago
Keywords: 4xp, correctness, css1, css2, nsbeta3, regression, testcase
QA Contact: petersen → py8ieh=bugzilla
Whiteboard: hit during nsbeta2 standards compliance testing

Comment 2

17 years ago
Style team...
Assignee: clayton → attinasi

Comment 3

17 years ago
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?)
Assignee: attinasi → buster
Whiteboard: hit during nsbeta2 standards compliance testing → hit during nsbeta2 standards compliance testing [nsbeta3-]
Target Milestone: --- → Future
(Reporter)

Comment 4

17 years ago
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).
(Reporter)

Updated

17 years ago
Blocks: 54661
(Reporter)

Updated

16 years ago
Whiteboard: hit during nsbeta2 standards compliance testing [nsbeta3-] → [Hixie-P4] hit during nsbeta2 standards compliance testing [nsbeta3-]
Build reassigning Buster's bugs to Marc.
Assignee: buster → attinasi
.
Assignee: attinasi → block-and-inline
Component: Layout → Layout: Block & Inline
Priority: P3 → --
Target Milestone: Future → ---
I've noticed this before.
Assignee: block-and-inline → roc+moz
*** Bug 225396 has been marked as a duplicate of this bug. ***
Blocks: 44192
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.
(Reporter)

Comment 11

13 years ago
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?
(Reporter)

Comment 13

13 years ago
From CSS2.1, section 8.3.1:
# Vertical margins of elements with 'overflow' other than 'visible' do not 
# collapse with their in-flow children.

Updated

13 years ago
Depends on: 240276
*** 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. ***

Comment 17

13 years ago
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.
also padding-right
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.
Attachment #182143 - Flags: superreview?(dbaron)
Attachment #182143 - Flags: review?(dbaron)
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.
Attachment #182143 - Flags: superreview?(dbaron)
Attachment #182143 - Flags: superreview+
Attachment #182143 - Flags: review?(dbaron)
Attachment #182143 - Flags: review+
(Reporter)

Comment 25

12 years ago
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
Attachment #182143 - Flags: approval1.8b2?
Blocks: 292371

Comment 27

12 years ago
Comment on attachment 182143 [details] [diff] [review]
fix

a=chofmann
Attachment #182143 - Flags: approval1.8b2? → approval1.8b2+
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.
checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
reopening because the content margin issue is still there.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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

12 years ago
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.

Updated

12 years ago
Blocks: 313520

Comment 35

12 years ago
Reduced testcase:
http://www.gtalbot.org/BugzillaSection/PaddingRightNotRendered.html
Blocks: 310083
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.
Duplicate of this bug: 322881

Comment 38

9 years ago
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#


Updated

9 years ago
Flags: blocking1.9.0.1?
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-
Flags: wanted1.9.0.x? → wanted1.9.0.x-
... 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.
QA Contact: ian → layout.block-and-inline
Assignee: roc → nobody

Comment 49

3 years ago
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

3 years ago
Created attachment 8449740 [details]
reftest-correct-spacing.html

Updated

2 years ago
See Also: → bug 1202671
You need to log in before you can comment on or make changes to this bug.