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

Status: RESOLVED DUPLICATE of bug 1499443

(Core :: Layout: Block and Inline, defect)

Description

[Hixie (not reading bugmail)]

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)]

Attachment: Self explanatory testcase.

Comment 2

[Suresh Duddi (gone)]

Style team...

Comment 3

[Marc Attinasi]

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)]

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)]

Build reassigning Buster's bugs to Marc.

Comment 6

[Boris Zbarsky [:bzbarsky]]

.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I've noticed this before.

Comment 8

[Boris Zbarsky [:bzbarsky]]

*** Bug 225396 has been marked as a duplicate of this bug. ***

Comment 9

[Boris Zbarsky [:bzbarsky]]

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) (email my personal email if necessary)]

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)]

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) (email my personal email if necessary)]

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)]

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-4, no longer working on Mozilla)]

*** Bug 251553 has been marked as a duplicate of this bug. ***

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 259008 has been marked as a duplicate of this bug. ***

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 269322 has been marked as a duplicate of this bug. ***

Comment 17

[Gérard Talbot]

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-4, no longer working on Mozilla)]

also padding-right

Comment 19

[Steve England [:stevee]]

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) (email my personal email if necessary)]

Attachment: 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-4, no longer working on Mozilla)]

This is just fixing the inner padding aspects of this bug, not the content
margins aspects, right?

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

Attachment: 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-4, no longer working on Mozilla)]

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)]

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) (email my personal email if necessary)]

Comment on attachment 182143 [details] [diff] [review]
fix

fixes a layout regression I just introduced

Comment 27

[chris hofmann]

Comment on attachment 182143 [details] [diff] [review]
fix

a=chofmann

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in. I'll leave this open for the margin issue.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I checked in the wrong patch. I'll check in the right patch tomorrow.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

reopening because the content margin issue is still there.

Comment 32

[Steve England [:stevee]]

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]

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 [:bzbarsky]]

See comment 24.

Comment 35

[Gérard Talbot]

Reduced testcase:
http://www.gtalbot.org/BugzillaSection/PaddingRightNotRendered.html

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 38

[Foteos Macrides]

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-4, no longer working on Mozilla)]

... 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-4, no longer working on Mozilla)]

Er, I should say *will be* fixed.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

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-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

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-4, no longer working on Mozilla)]

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]

Attachment: 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]

Attachment: reftest-correct-spacing.html

Comment 52

[Ryan VanderMeulen [:RyanVM]]

This bug is 19 years old, tracking it as a regression doesn't really make sense at this point.

Comment 53

[Sylvestre Ledru [:Sylvestre]]

David, is there a chance that we fix it one day or should we just mark it as INACTIVE?
Thanks

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The padding and margin parts should probably be split into separate bugs, with details about the history of each, and then this one closed.

I think there's already at least one existing bug on the padding part. There probably isn't on the margin part -- and if other browsers uniformly match us on the treatment of margins then it's probably not worth having a bug.

But the padding half of this bug is pretty important and a common web developer complaint; the history here should be properly merged in to whatever bug we're using to track that, rather than losing the connection.

Comment 55

[Karl Dubost💡 :karlcow]

My usual candid bug archeologist. There was Bug 748518 which was closed in favor of Bug 1499443 opened by Daniel Holbert. And it seems to have similar patterns.

Comment 56

[Daniel Holbert [:dholbert] (mostly-away until Aug 19)]

Right - I think bug 1499443 was the "at least one existing bug on the padding part" that dbaron hinted at in comment 54.

And from quick testing with margins-on-things-inside-of-scrollable-areas, I'm not seeing any incompatibilities between us and Chrome there. (Firefox & chrome both do what you'd expect - they create more scrollable area for the margin)

So, let's just dupe this forward to bug 1499443, since that's where we've captured the CSSWG-approved spec-compliant way of addressing this.

Comment 57

[jonjohnjohnson]

(In reply to Daniel Holbert [:dholbert] from comment #56)

*** This bug has been marked as a duplicate of bug 1499443 ***

You reference https://github.com/w3c/csswg-drafts/issues/129#issuecomment-417525242 but @Fantasai leaves the issue open saying "this issue remains open to see what we can do to fix this in the default case (which is constrained by Web-compat). I suspect we might be able to make it work in the block axis, but not the inline one."

So filing this as a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1499443 still means gecko should have a separate filing for the default/general web compat of padding/scrolling?

Comment 58

[Daniel Holbert [:dholbert] (mostly-away until Aug 19)]

It's possible we'll sort that out as part of the work in bug 1499443. If we don't, we can open a followup (linked to this bug) as-needed.

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I suspect margins aren't that simple -- there might be interop on block-side in-flow margins, but the question is whether we lag other browsers for any of the other cases, e.g., inline-sides or out-of-flows.