Closed
Bug 926728
Opened 11 years ago
Closed 11 years ago
"ASSERTION: Should be starting from the first continuation" with sticky, table, float
Categories
(Core :: Layout: Positioned, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jruderman, Assigned: kip)
References
Details
(Keywords: assertion, testcase)
Attachments
(6 files, 7 obsolete files)
283 bytes,
text/html
|
Details | |
14.28 KB,
text/plain
|
Details | |
216 bytes,
text/html
|
Details | |
1.61 KB,
patch
|
Details | Diff | Splinter Review | |
291 bytes,
text/html
|
Details | |
2.55 KB,
patch
|
Details | Diff | Splinter Review |
With:
user_pref("layout.css.sticky.enabled", true);
###!!! ASSERTION: Should be starting from the first continuation: 'nsLayoutUtils::IsFirstContinuationOrSpecialSibling(aFrame)', file layout/generic/StickyScrollContainer.cpp, line 283
###!!! ASSERTION: Can't sticky position individual continuations: 'nsLayoutUtils::IsFirstContinuationOrSpecialSibling(aFrame)', file layout/generic/StickyScrollContainer.cpp, line 133
(Bug 918994 had similar symptoms, but is gone now.)
Reporter | ||
Comment 1•11 years ago
|
||
Comment 2•11 years ago
|
||
As of bug 828312 patches 9a/9b, if I understand correctly, RecomputePosition *should* always be passing a FirstContinuationOrSpecialSibling to StickyScrollContainer::PositionContinuations. So I suppose it's more properly a problem with that guarantee.
Reporter | ||
Comment 3•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → kgilbert
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8409920 -
Flags: review?(corey)
Assignee | ||
Comment 5•11 years ago
|
||
Comment on attachment 8409920 [details] [diff] [review]
V1 Fix for Bug 926728
Due to ib-splits or pseudo-elements, RestyleManager::ComputeStyleChangeFor will add multiple frames of a sticky element to the nsStyleChangeList, resulting in StickyScrollContainer::PositionContinuations being called on continuations or ib-split-siblings other than the first, triggering the assert. In this case, the first continuation or ib-split-sibling has already been passed into StickyScrollContainer::PositionContinuations, positioning all continuations and ib-split-siblings of the sticky already; additional calls to StickyScrollContainer::PositionContinuations for the frame can be skipped.
A condition that suppressed extraneous StickyScrollContainer::PositionContinuation calls in RestyleManager::RecomputePosition was removed in Bug 828312 Patch 9b; however this check was only checking for prior continuations and was not aware of ib-split-siblings.
This bug can be corrected by restoring condition within RestyleManager::RecomputePosition, implemented with nsLayoutUtils::IsFirstContinuationOrIBSplitSibling.
The existing comment within RestyleManager::RecomputePosition indicates that this is the expected behavior:
// Update sticky positioning for an entire element at once when
// RecomputePosition is called with the first continuation in a chain.
Assignee | ||
Comment 6•11 years ago
|
||
Fixed logic inversion.
Attachment #8409920 -
Attachment is obsolete: true
Attachment #8409920 -
Flags: review?(corey)
Attachment #8409942 -
Flags: review?(corey)
Assignee | ||
Comment 7•11 years ago
|
||
Comment on attachment 8409942 [details] [diff] [review]
V2 Fix for Bug 926728
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=9c63508e8231
Assignee | ||
Comment 8•11 years ago
|
||
Added crashtest to ensure that the "Should be starting from the first continuation" assertion is not fired when recalculating the position of a sticky with ib-split siblings.
Attachment #8410398 -
Flags: review?(corey)
Comment 9•11 years ago
|
||
That explanation makes sense. I'll try to look at the code before the end of the week.
Comment 10•11 years ago
|
||
Comment on attachment 8410398 [details] [diff] [review]
Crashtest for Bug 926728
Review of attachment 8410398 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/base/crashtests/crashtests.list
@@ +428,5 @@
> load 931460-1.html
> load 935765-1.html
> load 942690.html
> load 973390-1.html
> +load 926728.html
Inserting this before 931464.html would keep the file a little more sorted.
Attachment #8410398 -
Flags: review?(corey) → review+
Comment 11•11 years ago
|
||
(In reply to :kip (Kearwood Gilbert) from comment #5)
> Due to ib-splits or pseudo-elements, RestyleManager::ComputeStyleChangeFor
> will add multiple frames of a sticky element to the nsStyleChangeList
Weren't the patches in bug 828312 intended to change that? Or maybe they only addressed true continuations and not ib-splits? The position:relative code in RecomputePosition, for instance, also appears designed for the case of IsFirstContinuationOrIBSplitSibling(aFrame), so it seems like it would be preferable to avoid passing later continuations/ib-split-siblings into RecomputePosition at all.
Assignee | ||
Comment 12•11 years ago
|
||
Corrected order of entries in crashtests.list
Attachment #8410398 -
Attachment is obsolete: true
Attachment #8412705 -
Flags: review?(corey)
Updated•11 years ago
|
Attachment #8412705 -
Flags: review?(corey) → review+
Comment 13•11 years ago
|
||
Comment on attachment 8412705 [details] [diff] [review]
V2 Crashtest for Bug 926728,r=coreyf
> Bug 926728 - Added crashtest
No need for the extra request, but I noticed that this patch subject should be more descriptive ("crashtest for recalculating..." as you have in the body, perhaps?) and have r=corey.
Assignee | ||
Comment 14•11 years ago
|
||
I have stepped through RestyleManager::ComputeStyleChangeFor and saw that the logic there suppresses multiple continuations with the same style; however, it will always process the first continuation of all ib-split-siblings.
if GetPrevContinuationWithSameStyle can potentially return false for a continuation other than the first (which the logic in RestyleManager::ComputeStyleChangeFor appears to expect), then it is possible that multiple continuations within a single ib-split-sibling or non-ib-split continuation could be passed through.
Assignee | ||
Updated•11 years ago
|
Attachment #8412705 -
Attachment description: V2 Crashtest for Bug 926728 → V2 Crashtest for Bug 926728,r=coreyf
Assignee | ||
Comment 15•11 years ago
|
||
Updated description in patch
Attachment #8412705 -
Attachment is obsolete: true
Comment 16•11 years ago
|
||
Having browser RestyleManager.cpp some more, I see that multiple continuations and ib-split siblings of the same element can certainly be added to the change list. That makes sense when they have different styles (say, when ::first-line is involved).
But assuming (as the sticky code already does, actually) that continuations/ib-split siblings can't have different *positioning-related* styles, I wonder whether it would make any sense to skip setting nsChangeHint_RecomputePosition for anything but the first continuation?
Comment 17•11 years ago
|
||
> That makes sense when they have different styles (say, when ::first-line is involved).
which line of thinking leads me to a testcase that triggers the assertions without involving ib-splits. (But the current patch still fixes this one too.)
Comment 18•11 years ago
|
||
Comment on attachment 8409942 [details] [diff] [review]
V2 Fix for Bug 926728
In any case, I still suspect it would be nicer to place this check at some earlier stage. (At the very least, it should cover the relpos logic too, but as I noted it might better belong in the change hint handling.)
Attachment #8409942 -
Flags: review?(corey)
Assignee | ||
Comment 19•11 years ago
|
||
Removed redundant nested "if" block
Attachment #8409942 -
Attachment is obsolete: true
Attachment #8417504 -
Flags: review?(corey)
Comment 20•11 years ago
|
||
Comment on attachment 8417504 [details] [diff] [review]
V3 Fix for Bug 926728,r=corey
So since it sounds like change-hint refactoring could be a bigger project, this is a fine solution for now.
The other thing I was just thinking about is that you could consider adding a code comment to keep track of the fact that we probably shouldn't even be calling RecomputePosition in the situation where this test is false. A followup bug suffices too.
r=me regardless.
Attachment #8417504 -
Flags: review?(corey) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8417504 -
Attachment description: V3 Fix for Bug 926728 → V3 Fix for Bug 926728,r=coreyf
Assignee | ||
Updated•11 years ago
|
Attachment #8413919 -
Attachment description: V3 Crashtest for Bug 926728,r=coreyf → V3 Crashtest for Bug 926728,r=corey
Assignee | ||
Updated•11 years ago
|
Attachment #8417504 -
Attachment description: V3 Fix for Bug 926728,r=coreyf → V3 Fix for Bug 926728,r=corey
I actually think we should fix this one differently, by just updating the caller to get the first continuation or IB sibling in this case (i.e., patching the same code, but to get the first instead of condition on being first). I think this is a rare case, and it's a lot easier to guarantee that we're going to do the right thing both now and in the future if we just do the work twice in this edge case (of having a sticky inside a situation where styles change across continuations).
Comment 22•11 years ago
|
||
I suppose that would work fine too. Although it seems unlikely to me that we would ever *not* call RecomputePosition on the first.
Assignee | ||
Comment 23•11 years ago
|
||
Updated based on Comment 21
Rather than skipping the call to StickyScrollContainer::PositionContinuation for continuations other than the first, the first continuation of the first ib-split-sibling for the passed frame is used every time within RestyleManager::RecomputePosition.
Attachment #8417504 -
Attachment is obsolete: true
Attachment #8417652 -
Flags: review?(dbaron)
Assignee | ||
Comment 24•11 years ago
|
||
Comment on attachment 8417652 [details] [diff] [review]
V4 Fix for Bug 926728
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=6dbd7beb27eb
Comment 25•11 years ago
|
||
Comment on attachment 8417652 [details] [diff] [review]
V4 Fix for Bug 926728
This makes the comment a little inaccurate, and I guess the ComputeStickyOffsets call should probably be changed too? (I don't think there's any use storing offsets on aFrame when aFrame != firstFrame.)
Assignee | ||
Comment 26•11 years ago
|
||
Updated based on Comment 25, re-submitted to try:
https://tbpl.mozilla.org/?tree=Try&rev=a7c5a6a69926
Attachment #8417652 -
Attachment is obsolete: true
Attachment #8417652 -
Flags: review?(dbaron)
Attachment #8417663 -
Flags: review?(dbaron)
Comment on attachment 8417663 [details] [diff] [review]
V5 Fix for Bug 926728
I'd suggest calling the variable firstContinuation rather than firstFrame, and maybe expanding the comment to say that it's rare that the frame we already have isn't already the first continuation or IB sibling, but it can happen when we're inside of a situation where styles differ across continuations such as ::first-line or ::first-letter, and in those cases we will generally (but maybe not always) do the work twice
r=dbaron
Attachment #8417663 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 28•11 years ago
|
||
Updated with feedback from Comment 27
Attachment #8417663 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 29•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f4bd1dcbc0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77242ba4613
Flags: in-testsuite+
Keywords: checkin-needed
Comment 30•11 years ago
|
||
Follow-up to explicitly set the sticky enabled pref so this test will still run as intended after it merges to beta.
https://hg.mozilla.org/integration/mozilla-inbound/rev/88a8ca8a7f0c
Comment 31•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6f4bd1dcbc0f
https://hg.mozilla.org/mozilla-central/rev/c77242ba4613
https://hg.mozilla.org/mozilla-central/rev/88a8ca8a7f0c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•