Bug 9314 – Relative positioned block size doesnt update root layer size

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Sriram]

If a render block is relative positioned, the root layer doesnt get the correct size. As a result, if the browser window is too small, some of the content is clipped.


To run this test case, reduce the Safari browser window size and load the page. A scroll bar should appear allowing the user to see the entire content. 

The original webpage (espn.com)works because espn does browser sniffing and for Safari, it doesnt include the rel positioned div element.

The fix is most probably in RenderLayer::updateLayerPosition(). When the child layer position gets updated, the parent layer size needs to be updated.

The bug regarding content clipping exists in IE too, but works on Firefox.

[Sriram]

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

[Sriram]

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

[Dave Hyatt]

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.

[Antti Koivisto]

Created attachment 8802 [details]
refined test case

[Antti Koivisto]

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).

[Antti Koivisto]

Created attachment 8804 [details]
patch

updated patch, do relative for leftmostPosition() as well

[Dave Hyatt]

Comment on attachment 8804 [details]
patch

r=me

[Dave Hyatt]

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.

[Dave Hyatt]

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

[Dave Hyatt]

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.

[Dave Hyatt]

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.)

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[Antti Koivisto]

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.

[Dave Hyatt]

For this...

-    int bottom = RenderContainer::lowestPosition(includeOverflowInterior, includeSelf);
+    int bottom = includeSelf && m_width > 0 ? m_height : 0;

Won't calling the base class work?

[Dave Hyatt]

Comment on attachment 8831 [details]
updated patch

Ok, this is ready.  Just need test cases and results included and then I can +.

[Dave Hyatt]

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.

[Dave Hyatt]

Comment on attachment 8831 [details]
updated patch

Antti will commit tests to go with this, so marking r+.

[Antti Koivisto]

Commited. Overflow fix is still needed.

[David Kilzer (:ddkilzer)]

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

Committed as r14847.

[Timothy Hatcher]

Should this be open still for the other issues?

[Darin Adler]

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.

[Laszlo Gombos]

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