While debugging bug 159363 I learned the follwing ... During an unconstrained reflow, FlowAndPlaceFloaters() positions all right floaters at |mAvailSpaceRect.x|. It also prevents right floaters from being added to the space manager, and instead tries to keep track of the areas the right floaters would occupy in |mRightFloaterCombinedRect|. The idea is that this would allow any content *after* the right floater to flow unconstrained and un-impacted by any right floaters, and that after all the content for a given line is flowed, that mRightFloaterCombinedRect could simply be added with the combined area for the line to produce an area that was roughly equivalent to the space needed to fit everything. Unfortunately placing right floaters at |mAvailSpaceRect.x| makes the calculation of the area needed by right floaters inaccurate because it allows adjacent right floaters to be stacked on top of each other, or it's placement impacted by an left floaters, yielding an invalid mRightFloaterCombinedRect. This has the effect of calculating line combined widths that are either too small or too large. The proposed fix for bug 159363 (attachment 99589 [details] [diff] [review]) makes the sizing a bit more consistent with the test case I will add to this bug ... but it's only consistent because all the floaters are lined up at the unconstrained right edge. A possible way to fix this problem, is to modify FlowAndPlaceFloaters to position right floaters |mRightFloaterCombinedRect.x - region.width|.
Created attachment 99889 [details] Test Case This is a testcase I've been debugging with, if you load this you'll notice how differently some of the tables (containing left and right floaters) are sized, though they really should be the same.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.2beta
Works beautifully now. Small question: there's a nice TC here, would it be worth it to turn it into a reftest and include it in the test suite, or is everything it covers already well tested by existing tests?
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WORKSFORME
Maybe, but I wouldn't consider it a high priority. It's hard to know if it's well-covered by existing tests.
You need to log in before you can comment on or make changes to this bug.