Bug 80808 – absolutely-positioned element does not update width when container width changes

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

[Han]

An element that is absolutely positioned with all of top, left, and right so that it stretches all the way across a position:relative;display:inline container element doesn't change width even if the container width changes, e.g. due to appending elements, see http://jsfiddle.net/UgDQy/2/ .

Interestingly, if positioned with left and right but without top, it will update its width correctly, see http://jsfiddle.net/UgDQy/3/ .

[SravanKumar S(:sravan)]

Hi Han,

I would like to work on this bug, but i am finding it tough to create a .html out of it, as i am not much in to jquery :). can u please upload a test case(.html).

[Han]

Gladly, http://jsfiddle.net/UgDQy/4/ doesn't use jQuery, or if you prefer a single HTML page, http://fiddle.jshell.net/UgDQy/4/show/ .

[SravanKumar S(:sravan)]

I debugged this, and i found that layout for span element(with absolute positioning and top attribute) does'not happen because of following statements in void RenderBlock::layoutPositionedObjects(bool relayoutChildren)

if (relayoutChildren || (r->style()->hasStaticBlockPosition(isHorizontalWritingMode()) && r->parent() != this))
            r->setChildNeedsLayout(true, MarkOnlyThis);

1. In case of css "top: 0;" attribute being mentioned, the hasStaticBlockPosition function returns false as top(and/or bottom) is not Auto type, but rather fixed.

2. When "top: 0;"(and/or bottom) is not mentioned, it becomes auto and hence setChildNeedsLayout gets called, hence there is no issue.

Need to figure out why hasStaticBlockPosition is imposing topandBottom to be auto.

[Robert Hogan]

(In reply to comment #3)
> Need to figure out why hasStaticBlockPosition is imposing topandBottom to be auto.

It does this because it's only worried about cases where the block position of the positioned object is relative to a container - that's not the case here (because top: has been specified) so it's not the correct clause to expand. The fact that it fixes the test case is a happy accident.

Instead, the issue is that the position and width of the positioned object may need to update if either of left: (in the RTL case) or right: (in the LTR case) are not auto *and* are set to zero  and it has a relative positioned parent whose width has changed. So something like:

        if (r->style()->right()->isFixed() && !r->style()->right().value()) && r->parent() != this)
            r->setChildNeedsLayout(true, MarkOnlyThis);

should fix it in the simple horizontal mode LTR case. I'm not sure if it's worth checking whether the width of the object has changed before setting layout - this may be an unusual enough scenario to just layout whenever it's encountered.

I'm not saying this is definitely the right approach - but I think it's something along these lines. You will definitely need dhyatt to approve any changes in this function.

[SravanKumar S(:sravan)]

(In reply to comment #4)
> (In reply to comment #3)
> > Need to figure out why hasStaticBlockPosition is imposing topandBottom to be auto.
> 
> It does this because it's only worried about cases where the block position of the positioned object is relative to a container - that's not the case here (because top: has been specified) so it's not the correct clause to expand. The fact that it fixes the test case is a happy accident.
> 

Yes, i figured it out later on, and thanks a lot for confirmation and response.

> Instead, the issue is that the position and width of the positioned object may need to update if either of left: (in the RTL case) or right: (in the LTR case) are not auto *and* are set to zero  and it has a relative positioned parent whose width has changed. So something like:
> 

Yes, as u have mentioned above, it is on such particular conditions that layout should happen. But as of now when "top:0;" is not mentioned the layout still happens for positioned elements in the first if condition as it does not really bother about left/right being fixed. I will try and fix this too.

>         if (r->style()->right()->isFixed() && !r->style()->right().value()) && r->parent() != this)
>             r->setChildNeedsLayout(true, MarkOnlyThis);
> 
> should fix it in the simple horizontal mode LTR case. I'm not sure if it's worth checking whether the width of the object has changed before setting layout - this may be an unusual enough scenario to just layout whenever it's encountered.
> 
> I'm not saying this is definitely the right approach - but I think it's something along these lines. You will definitely need dhyatt to approve any changes in this function.

Thanks for the response and the pointers.

[SravanKumar S(:sravan)]

Created attachment 142995 [details]
Patch

[WebKit Review Bot]

Comment on attachment 142995 [details]
Patch

Attachment 142995 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12734586

[Early Warning System Bot]

Comment on attachment 142995 [details]
Patch

Attachment 142995 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12730604

[Early Warning System Bot]

Comment on attachment 142995 [details]
Patch

Attachment 142995 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12728917

[SravanKumar S(:sravan)]

Created attachment 143018 [details]
Patch

Patch after fixing compiler specific error.

[Pravin D]

Comment on attachment 143018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143018&action=review

> Source/WebCore/rendering/RenderBlock.cpp:2475
> +        if ((leftCheck || rightCheck) && r->parent() != this)

What happens vertical write mode ? Maybe instead of using leftCheck and rightCheck, these checks can be combined in a static function taking style as a parameter.

I hope its not rude of me jump out of nowhere n review this code. Just tot it might help.

[Robert Hogan]

(In reply to comment #11)
> (From update of attachment 143018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143018&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:2475
> > +        if ((leftCheck || rightCheck) && r->parent() != this)
> 
> What happens vertical write mode ? Maybe instead of using leftCheck and rightCheck, these checks can be combined in a static function taking style as a parameter.

Yup. I do suggest getting dhyatt's nod on the high-level approach before getting too far into the implementation. This is a performance-sensitive function so better to agree the approach first.

[SravanKumar S(:sravan)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 143018 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=143018&action=review
> > 
> > > Source/WebCore/rendering/RenderBlock.cpp:2475
> > > +        if ((leftCheck || rightCheck) && r->parent() != this)
> > 
> > What happens vertical write mode ? Maybe instead of using leftCheck and rightCheck, these checks can be combined in a static function taking style as a parameter.
> 
> Yup. I do suggest getting dhyatt's nod on the high-level approach before getting too far into the implementation. This is a performance-sensitive function so better to agree the approach first.

Sure, i will take care of Vertical writing mode too. But as robert suggest, its better if we know hyatt's view on the fix.

Hyatt,

Please provide with your valuable feedback on the approach.

[Julien Chaffraix]

Comment on attachment 143018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143018&action=review

> Source/WebCore/rendering/RenderBlock.cpp:2476
> +            r->setChildNeedsLayout(true, MarkOnlyThis);

I have strong doubts that this is the right fix. Could you explain why you believe we need this?

Looking at the issue, it's definitely not a repainting bug as selecting doesn't repaint the outline as it should be. Also the text is properly laid out, only the outline that is wrong. I would look into the bounding box / visual overflow from ".abs" and try to understand why it is wrongly computed in this case (we are likely not including the overflow from the new child but I don't know why). You can also track the paintOutline function and backtrack the values from there.

> LayoutTests/ChangeLog:12
> +        * compositing/absolute-position-change-width-with-inline-container-expected.html: Added.
> +        * compositing/absolute-position-change-width-with-inline-container.html: Added.

Why is the test in compositing/ when it involves 0 composition?

> LayoutTests/compositing/absolute-position-change-width-with-inline-container.html:26
> +        <p>When you click <button id="more">moooore</button>, the span.abs should expand with the container</p>
> +        <p><span id="container"><span class="abs">&nbsp;</span>Words, words, words</span>

This test passes for me locally on Chromium (haven't tested under DRT). Also can we have a test that doesn't involve a button that doesn't work? We also need some mention of the bug id, title and condition for passing.

[Han]

> the text is properly laid out, only the outline that is wrong.

To clarify, it's not just the outline, everything related to the width of the ".abs" is wrong, like .offsetWidth, and anything inside with text-align:center won't be re-centered (which is how I actually discovered the bug): http://fiddle.jshell.net/UgDQy/6/show/ or http://jsfiddle.net/UgDQy/6/

It might be worth noting that the text that's properly laid out is a sibling, not a descendant, of the ".abs", and is hence outside it.

[Robert Hogan]

Comment on attachment 143018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143018&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:2476
>> +            r->setChildNeedsLayout(true, MarkOnlyThis);
> 
> I have strong doubts that this is the right fix. Could you explain why you believe we need this?
> 
> Looking at the issue, it's definitely not a repainting bug as selecting doesn't repaint the outline as it should be. Also the text is properly laid out, only the outline that is wrong. I would look into the bounding box / visual overflow from ".abs" and try to understand why it is wrongly computed in this case (we are likely not including the overflow from the new child but I don't know why). You can also track the paintOutline function and backtrack the values from there.

See my comment on the problem above: ".. the issue is that the position and width of the positioned object may need to update if either of left: (in the RTL case) or right: (in the LTR case) are not auto *and* are set to zero  and it has a relative positioned parent whose width has changed."

layoutPositionedObjects() needs to detect cases where abs-positioned objects need a layout due to movement of their parents - as you can see from many of the comments in the function it's often a question of finding a good balance between 'definitely' and 'probably' needs layout. In LTR horizontal mode with a right of 0, the abs-positioned object needs layout if its relative positioned parent changes width, likewise in the RTL left:0 case. There isn't a good way at present of knowing if the parent has expanded - only layout will tell. I think the test can be tightened to include only elements with relpos parents too.

Note that the extra text doesn't belong to the abspositioned object - the outline doesn't expand because it's not getting a layout at all.

[Han]

I can still repro (with http://fiddle.jshell.net/UgDQy/4/show/ )  in Safari 8.0.8, but it appears to have been fixed in Chrome 48.

[Ahmad Saleem]

I am able to reproduce this bug in Safari 15.6 on macOS 12.5 using attached test case in URL field and outline does not expand when clicking on "mooore.." button and only way to fix is to click "fix" button while other browsers expand outline as soon as you add text via "mooore" button.

Just wanted to share updated testing results. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/97641970>

[EWS]

Committed 256315@main (c1324d73bd3c): <https://commits.webkit.org/256315@main>

Reviewed commits have been landed. Closing PR #6019 and removing active labels.