See upcoming testcase, I get two assertions with that testcase in my debug build, these are the warnings/assertions I get: WARNING: aFrame is already associated with a region, file c:/mozilla/mozilla/lay out/generic/nsSpaceManager.cpp, line 814 ###!!! ASSERTION: bad float placement: 'NS_SUCCEEDED(rv)', file c:/mozilla/mozil la/layout/generic/nsBlockReflowState.cpp, line 1005 WARNING: aFrame is already associated with a region, file c:/mozilla/mozilla/lay out/generic/nsSpaceManager.cpp, line 814 ###!!! ASSERTION: bad float placement: 'NS_SUCCEEDED(rv)', file c:/mozilla/mozil la/layout/generic/nsBlockReflowState.cpp, line 1005 ###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aN extSibling', file c:/mozilla/mozilla/layout/generic/nsIFrame.h, line 695 ###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aN extSibling', file c:/mozilla/mozilla/layout/generic/nsIFrame.h, line 695
Basically, we're calling AddRectRegion() twice on the same space manager with the same frame. That really shouldn't be happening, as I understand this code.
The problem is that we've got this unwrappable span containing a float. So we reflow the entire span, including the float, and then discover that the span doesn't fit and has to be pushed. But we don't remove the pushed float from mBelowCurrentLineFloats, so it gets placed after that line, and then it gets reflowed and placed again on the next line. In the testcase, you can see the space bogusly taken up by the float after the first line. In general when we split a line, if there are placed floats after the split, we need to unwind the float state, including removing any such floats from aState.mBelowCurrentLineFloats and the line's float cache, and removing any such floats from the space manager. Ick! Normally there aren't any placed floats because we will detect that text doesn't fit and split the line before we try to place floats after that text.
Perhaps we could remember "break here!" at the right content+offset, and then reflow the line again, forcing a break at the given position. We already allow redoing line reflow (to move lines down vertically when they don't fit next to floats), this would extend that. We could use that here to reflow the line again if split-off floats are detected, and avoid pulling up the split-off new line. We could also use this to fix the textframe mess that handles words containig multiple textframes, e.g., "<span>Hello Kit</span>ty". Currently the span textframe does nasty, complicated lookahead through the (not very well formed, because they haven't been reflowed yet) frame tree measuring text to see whether the complete word will fit. Instead, it could record "I can break here" at the space, just assume everything fits and return from Reflow(). Then if we find "ty" doesn't fit, we reflow the line again forcing it to break at the last remembered break point. Does this sound like a good idea? The textframe changes would not be wanted on branch, of course, but the rest could be branch-worthy if we find we need to fix this bug on branch. It's not clear that we do.
Created attachment 220079 [details] [diff] [review] fix This fixes it using the redo approach described above. The redo machinery here doesn't support breaking the line at an arbitary point like I want to do for textframes, that can be added later. It just reflows the current line again and preserves the current line break by refusing to pull frames up from the next line. This probably also fixes some bugs with the existing redo code not undoing floats properly.
Are the nsFloatCacheList/nsFloatCacheFreeList changes needed? Do we ever store nsFloatCacheFreeList objects as nsFloatCacheList objects? Should we prevent that from being possible instead of adding virtual functions? The idea of this patch is to place the text first, and then the float it contains, right? Or the opposite (which violates the CSS spec, IIRC)?
(In reply to comment #6) > Are the nsFloatCacheList/nsFloatCacheFreeList changes needed? I needed an implementation of DeleteAll for nsFloatCacheFreeList so I could call aState.mBelowCurrentLineFloats.DeleteAll(), so I refactored the code to create one. > Do we ever store > nsFloatCacheFreeList objects as nsFloatCacheList objects? I don't think so. > Should we prevent that from being possible instead of adding virtual > functions? You mean make the inheritance relationship private, or break it? I guess we could do that. Is that the way you want to go? > The idea of this patch is to place the text first, and then the float it > contains, right? Or the opposite (which violates the CSS spec, IIRC)? This patch is not supposed to change any layout behaviour except for the incorrect thing going on here. All that I'm trying to do here is guarantee that if a span is pushed to the following line, any floats in it also move to the following line, in particular floats that we may have already placed in the current line.
Seems like private inheritance (with a few inline forwarding functions) would probably be better than adding virtual functions.
OK, I'll do that.
There remains a bug in a case like this: <p><span style="white-space:pre"><div style="float:left; height:1em; width:90%">hello Hello hello</span> where we place the float, which fits, but then the rest of the span doesn't fit. We don't push the span because it's the first thing on the line, so it ends up overflowing the available width. What we really should do (I think) is convert the placed float to a below-current-line float so we end up with the span on one line and the float below it. I think I want to deal with that in another bug.
Created attachment 224819 [details] [diff] [review] fix updated with float list changes
Comment on attachment 224819 [details] [diff] [review] fix nsFloatCacheFreeList::DeleteAll should probably call nsFloatCacheList::DeleteAll and then set mTail to null (rather than being a separate implementation). Could you make what you implemented as nsFloatCacheList::Remove protected (and with another name), and have an inline version with void return (calling the protected method) for the public API? Returning the previous element is highly unintuitive. nsFloatCacheList::DeleteAll would probably be slightly better off if it iterated over a temporary variable rather than assigning to mHead since the compiler can optimize it better. (Just don't forget to make mHead null in the end.) You detached the comment about line layout allocation from the stack allocation itself. Could you move those back together? + // If floats have been placed that have been pushed to the new line, Fix by s/that have/whose placeholders have/ The names LINE_REFLOW_REDO and LINE_REFLOW_REDO_FOR_FLOAT confuse me a good bit. I think I'd prefer something like: LINE_REFLOW_REDO_FOR_FLOAT -> LINE_REFLOW_REDO_BELOW_FLOAT LINE_REFLOW_REDO -> LINE_REFLOW_REDO_NO_PULL (I'm not attached to these either, so feel free to improve.) This change would require renaming didRedoForFloat too. At first I was a little uncomfortable with popping the space manager for LINE_REFLOW_REDO_FOR_FLOAT too, but I suppose it makes sense and there's probably a similar nasty bug in that case. It seems like this patch may make it easier to fix bug 50630.
How about LINE_REFLOW_REDO_NEXT_BAND LINE_REFLOW_REDO_NO_PULL ?
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Did we want this in 1.8.1, or is it not needed there for some reason?
I backed this out because it regressed Tp on btek and probably on luna.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think this is a bit risky for 1.8.1 unless/until we get significant bake time. Furthermore, I think we should land the fix for bug 282173 (RemoveBuildFloatList) on the branches, and if we do that, then this bug should not be a crasher on the branches.
Created attachment 226095 [details] [diff] [review] fix This is the patch I had checked in, updated to trunk and with review comments addressed.
13 years ago
Attachment #224819 - Attachment is obsolete: true
Created attachment 226110 [details] [diff] [review] updated patch Hopefully this will improve things. I noticed that in many cases the new call to PushState/PopState on the space manager was forcing a dynamic allocation because we've already got one state pushed (e.g., while we do an unconstrained-width reflow of the line). This patch is the same as what I checked in except that I've altered PushState/PopState to save and restore to a state object, which can be stack allocated --- and is, everywhere --- so we completely eliminate dynamic allocations. It also simplifies bookkeeping and removes the need to call DiscardState explicitly.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago → 13 years ago
Resolution: --- → FIXED
This appears to have regressed btek again. I'll back it out after one more cycle.
After another cycle, it looks like this was just noise on btek, so I'm leaving it in. Yay!
Bug 342322 has a testcase that triggers the "Can only remove a singleton element" assertion added in this bug.
When I checked in the fix for 237085, I somehow backed this out :-( :-(. I'll check it in again
You need to log in before you can comment on or make changes to this bug.