Closed Bug 47710 Opened 24 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1499443
Tracking Flags:
Tracking Status
firefox-esr60 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fix-optional

People

(Reporter: ian, Unassigned)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: css2, testcase, Whiteboard: [webcompat] [Hixie-P4] hit during nsbeta2 standards compliance testing [nsbeta3-])

Attachments

(5 files)

Self explanatory testcase.
(deleted), text/html
Details
fix
(deleted), patch
dbaron
: review+
dbaron
: superreview+
chofmann
: approval1.8b2+
Details | Diff | Splinter Review
screenshot of testcase
(deleted), image/png
Details
reftest-incorrect-spacing.html
(deleted), text/html
Details
reftest-correct-spacing.html
(deleted), text/html
Details
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.
Attached file Self explanatory testcase. (deleted) —
Keywords: 4xp, correctness, css1, css2, nsbeta3, regression, testcase
QA Contact: petersen → py8ieh=bugzilla
Whiteboard: hit during nsbeta2 standards compliance testing
Style team...
Assignee: clayton → 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?)
Assignee: attinasi → buster
Whiteboard: hit during nsbeta2 standards compliance testing → hit during nsbeta2 standards compliance testing [nsbeta3-]
Target Milestone: --- → Future
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).
Blocks: 54661
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.
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.
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.
Attached patch fix (deleted) — — Splinter Review
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?
Attached image screenshot of testcase (deleted) —
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+
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?
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
Closed: 20 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.
The testcase WORKSFORME with Mozilla 1.8b2 build 2005070105 and Firefox Deer Park alpha 1 build 20050706. Is there still an issue?
Blocks: 313520
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.
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#
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
Attached file reftest-incorrect-spacing.html (deleted) —
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.
Attached file reftest-correct-spacing.html (deleted) —
Whiteboard: [Hixie-P4] hit during nsbeta2 standards compliance testing [nsbeta3-] → [webcompat] [Hixie-P4] hit during nsbeta2 standards compliance testing [nsbeta3-]

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

Keywords: regression

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

Status: REOPENED → RESOLVED
Closed: 20 years ago → 6 years ago
Flags: needinfo?(dbaron)
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---

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.

Flags: needinfo?(dbaron)

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.

Flags: needinfo?(dholbert)

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.

Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Flags: needinfo?(dholbert)
Resolution: --- → DUPLICATE

(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?

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.

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.

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

Attachment

General

Creator:
Created:
Updated:
Size: