Bug 9314

Created attachment 8715 [details] Reduced test case for the espn rel postioning issue

Created attachment 8718 [details] This fix resizes the parent layer after children have been positioned

Comment on attachment 8718 [details] This fix resizes the parent layer after children have been positioned This change would increase the sizes of layers even when the child is out of flow. That is incorrect and will lead to layers being way larger than they need to be. The correct place to patch would be the rightmost/lowest/leftmostPosition methods that are used to determine your scrollable area.

Created attachment 8802 [details] refined test case

Created attachment 8803 [details] patch - Take relative positioning into account in leftmost/rightmost/lowestPosition() - Ignore zero width/height elements when calculating extremes. This matches firefox behavior (and prevent this patch from breaking any layout tests).

Created attachment 8804 [details] patch updated patch, do relative for leftmostPosition() as well

Comment on attachment 8804 [details] patch r=me

Comment on attachment 8804 [details] patch Actually hold up. I think there's a better way to do this. There are more mistakes than just the scroll position being wrong. Basically the overflowWidth/Height/Left/Top members of RenderBlock are all wrong. When these values are computed, they don't account for relative positioning. This means we incorrectly think relpositioned objects might overflow (when they don't) and vice versa. Rather than patching leftmost/rightmost/lowestPosition, I think we should patch the computation of m_overflowLeft/Top/Width/Height to factor in relative position offsets when examining children.

This overflow-patching approach would also deal correctly with relative-positioned elements other than RenderFlows (like images).

cc'ing mitz. We basically need test cases for relative positioned elements that cause overflow (incorrectly) in the untranslated position but not in their translated position and vice versa. We should have tests where the relative positioned objects are both inline and block and flows vs. replaced content. We'll need to make sure that if we change the definition of overflow to account for relative positioning that the incremental repaint code still works properly when repainting the relpositioned child layers as a result of ancestor movement.

Even if we don't patch overflowLeft/Top/Width/Height, the leftmost/Rightmost/LowestPosition calls have to be patched in basically all the places they grab kids and look at their x/y positions, and I think it's more important to fix this case where we don't show scrollbars when we should then to get all the edge cases right. (The case where we show scrollbars when we shouldn't seems much more unlikely to occur in practice.) So you could adjust your patch to apply the relativePositionOffset at the floating objects walk (have to account for floating objects like images being relatively positioned) in RenderBlock.cpp and the child walks in RenderFlow.cpp. I would r+ a patch that is very similar to what you posted as long as it accounts for non-RenderFlow relative-positioned objects. (Patch the places in leftmost/rightmost/lowestPosition that fetch children, rather than the RenderFlow return chokepoint.)

Created attachment 8805 [details] Test case that needs to be addressed via leftmost/Rightmost/lowestPosition changes. Test case that shows the same problem (not showing scrollbars when we need to) when an image is relative positioned.

Created attachment 8806 [details] Second test case that uses a floating image instead of an in-flow image. Here's the same example, but with a floating image.

Created attachment 8807 [details] Test Case that shows the opposite problem (does not have to be fixed to land). Here's the reason that overflowWidth/Height/Left/Top have to be patched. In this case we show scrollbars when we didn't need to. Even if we patch left/right/lowest position, as long as overflowWidth/Height/LEft/Top are wrong, this bug will persist. Note, though, that this bug is much less severe and in fact Firefox gets it wrong too. It's unclear to me whether it even *is* wrong, since maybe you are supposed to go ahead and leave "space" for the spot in which the element participated untranslated in the layout of the object.

Created attachment 8831 [details] updated patch Moved the relative position correction to renderbox so it gets applied to all elements. I also split relativePositionOffset() to x and y coordinats since thats how it is mostly used.

For this... - int bottom = RenderContainer::lowestPosition(includeOverflowInterior, includeSelf); + int bottom = includeSelf && m_width > 0 ? m_height : 0; Won't calling the base class work?

Comment on attachment 8831 [details] updated patch Ok, this is ready. Just need test cases and results included and then I can +.

Created attachment 8832 [details] Another test case that is broken. Here's another test case that would be fixed if overflowWidth/Height were patched. No need to fix it before landing, but getting it attached for future work on this bug.

Comment on attachment 8831 [details] updated patch Antti will commit tests to go with this, so marking r+.

Commited. Overflow fix is still needed.

(In reply to comment #20) > Commited. Overflow fix is still needed. Committed as r14847.

Should this be open still for the other issues?

Comment on attachment 8831 [details] updated patch Clearing the review flag so this won't show up as a patch that needs to be landed.

Changing the status to FIXED as I belie the fix has been landed a while ago.