Closed Bug 550325 Opened 14 years ago Closed 14 years ago

"ASSERTION: We should have padding here" with abs pos, overflow:scroll

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(4 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: We should have padding here! (out of memory?): 'hasPadding', file /Users/jruderman/central/layout/generic/nsFrame.cpp, line 695

This assertion (in its current form) was added by bug 542361.
blocking2.0: --- → ?
blocking2.0: ? → final+
Priority: -- → P2
I'll take a look...
Assignee: nobody → matspal
OS: Mac OS X → All
Hardware: x86 → All
Attached file Testcase #2
Adding an "X" to show that the left position is wrong.
The problem is that the UsedPaddingProperty is missing, that causes the
assertion and the layout error.  nsGfxScrollFrame explicitly sets it:
http://hg.mozilla.org/mozilla-central/annotate/9b79a3d0c554/layout/generic/nsGfxScrollFrame.cpp#l495
but this is immediately removed by nsHTMLReflowState::InitConstraints
http://hg.mozilla.org/mozilla-central/annotate/9b79a3d0c554/layout/generic/nsHTMLReflowState.cpp#l1656
which then never sets it up again, since in nsCSSOffsetState::InitOffsets
http://hg.mozilla.org/mozilla-central/annotate/9b79a3d0c554/layout/generic/nsHTMLReflowState.cpp#l1893
we take the first 'else' which just assigns 'mComputedPadding' --
it's ComputePadding() in the last 'else' that sets it up (when needed):
http://hg.mozilla.org/mozilla-central/annotate/9b79a3d0c554/layout/generic/nsHTMLReflowState.cpp#l2190
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Let's handle these properties in InitOffsets() instead of in multiple places.
(It's called from InitConstraints() unconditionally, and only from there.)
This also allows us to optimize the Delete+Set in case the property
exists (avoiding delete+new and extra property search).

The bug fix is that when the aPadding argument is non-null we set the
property if the padding style is width dependent.

I'm also slightly changing ComputePadding() for some table elements;
it used to compute a %-padding and set the property to that value and
then set mComputedPadding to zero.  This seems like a bug, I think the
property value should always be the same as the computed value.

Passed TryServer unit tests (some known orange).
Attachment #488862 - Flags: review?(roc)
Attached patch Patch rev. 2Splinter Review
> property value should always be the same as the computed value

We should also fix that for border-collapse tables and scrollbars
which set mComputedPadding to zero at the end of InitOffsets:
http://hg.mozilla.org/mozilla-central/annotate/9b79a3d0c554/layout/generic/nsHTMLReflowState.cpp#l1934

Updating UsedPaddingProperty at the end InitOffsets should fix that.
Attachment #488862 - Attachment is obsolete: true
Attachment #488871 - Flags: review?(roc)
Attachment #488862 - Flags: review?(roc)
Comment on attachment 488871 [details] [diff] [review]
Patch rev. 2

Split needProp into needMarginProp and needPaddingProp.
Attachment #488871 - Flags: review?(roc) → review+
Attached patch Patch rev. 3Splinter Review
Nit fixed.  Also, I moved local variable 'presContext' a bit earlier
to eliminate a 'frame->PresContext()'.  Trivial change, so not going
to ask for a new review.
http://hg.mozilla.org/mozilla-central/rev/7f16577a2671
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Bug 611532 has another testcase that triggers the assertion.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: