Bug 78223 – Alignment issue with positioned IMG tag

• Status: NEW
• Component: Layout and Rendering

[Sriram Neelakandan]

Created attachment 126278 [details]
bug reduction for render bug

Attached is a bug reduction html
Load this page You will notice the DRT will actually output this




</body></html>" 
layer at (0,0) size 793x536
  RenderView at (0,0) size 793x536
layer at (0,0) size 793x8
  RenderBlock {HTML} at (0,0) size 793x8
    RenderBody {BODY} at (8,8) size 777x0
layer at (0,0) size 104x104
  RenderBlock (positioned) {DIV} at (0,0) size 104x104 [border: (2px solid #FF0000)]
layer at (52,2) size 104x104
  RenderImage {IMG} at (52,2) size 104x104 [border: (2px solid #FFFF00)]


But now resize the window (QtTestBrowser) then the render will look like this

layer at (0,0) size 764x536
  RenderView at (0,0) size 764x536
layer at (0,0) size 764x8
  RenderBlock {HTML} at (0,0) size 764x8
    RenderBody {BODY} at (8,8) size 748x0
layer at (0,0) size 104x104
  RenderBlock (positioned) {DIV} at (0,0) size 104x104 [border: (2px solid #FF0000)]
layer at (2,2) size 104x104
  RenderImage {IMG} at (2,2) size 104x104 [border: (2px solid #FFFF00)]

Notice the position of IMG from (52,2) to (2,2)

[Sriram Neelakandan]

I did debug this one up untill some incorrect child->width() in 

WebCore::RenderBlock::startAlignedOffsetForLine() 
 Line 2767: logicalWidthForChild(child).. 
the child->width() is incorrectly setup

[Roland Steiner]

Can reproduce this on TOT WebKit as of r107214. It also reproduces in Chrome 16.0.912.77 (Official Build 118311) (WK 105360)

There are quite a few people on IRC reporting similar redrawing issues - that is, content not getting updated until the window is resized or some such (the positioning may be a separate issue to this). Bumping this to P1 accordingly.

[Alexander Pavlov (apavlov)]

http://trac.webkit.org/changeset/104183 seems to be the culprit, according to my git bisect'ion. Can someone rendering-proficient have a look?

[Sriram Neelakandan]

I re-created this issue on Webkit-Trunk-r116926  with QtTestBrowser on Linux/Qt/DFB Build

The issue is seen on Chrome-20/Windows as well.

Updating  platforms All/All

[Ken Buchanan]

apavlov: I don't think this is related to r104183; that change only affects relative positioned inlines, which aren't in the test case. In particular, a breakpoint on the changed code doesn't fire when the attached reduction is loaded.

There is something wrong with the RenderBox::m_frameRect for the image during the first layout, but I'm not familiar with this code to diagnose why.

[Pravin D]

hi Sriram , wanted to know if you are working on this bug or not... I want to work on this bug... please let me :)

[Sriram Neelakandan]

(In reply to comment #6)
> hi Sriram , wanted to know if you are working on this bug or not... I want to work on this bug... please let me :)

i am not actively working on this fix

[Pravin D]

Created attachment 145333 [details]
Patch

[Pravin D]

(In reply to comment #8)
> Created an attachment (id=145333) [details]
> Patch

About the issue:
<div>
  <img style="position:absolute" />
</div>

In the above code the Layout for the div starts, its non-positioned children are laid out first, then the lines for the div are created. Here the position of the inline positioned children are also calculated. Then after the lines are created all the inlines placed, the positioned objects are laid out.  

The issue is that when the position object is placed on the line, if it is the first child or its previous sibling is also positioned, then its width is taken in account for the calculation(position calculations). 

So while laying out the positioned objects, wrong inline position is taken. Either we might require another layout for the parent in this case or we can calculate the width of the positioned object, if its width is affecting its position on the line, before the lines are laid out for the parent.

The patch calculating the width approach.

[WebKit Review Bot]

Comment on attachment 145333 [details]
Patch

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

New failing tests:
fast/inline/left-right-center-inline-alignment-in-ltr-and-rtl-blocks.html
fast/css/bug4860-absolute-inline-child-inherits-alignment.html
fast/regions/positioned-objects-inline-static-in-regions.html
fast/regions/positioned-objects-inline-static-in-rtl-regions.html
fast/css/absolute-child-with-percent-height-inside-relative-parent.html

[WebKit Review Bot]

Created attachment 145379 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Daniel Bates]

Comment on attachment 145333 [details]
Patch

r- per comment 10. Please ensure that this patch doesn't regression any tests. If the differences in the test results after apply this patch are justified then you'll need to include rebased expected results as part of your patch.

[Pravin D]

(In reply to comment #12)
> (From update of attachment 145333 [details])
> r- per comment 10. Please ensure that this patch doesn't regression any tests. If the differences in the test results after apply this patch are justified then you'll need to include rebased expected results as part of your patch.

Thank you... I'll upload anther patch with the rebased results

[Pravin D]

Created attachment 146003 [details]
Patch

[Pravin D]

(In reply to comment #14)
> Created an attachment (id=146003) [details]
> Patch

This is patch version 2. Made the failing test cases Comment #10 that required rebasing into refTest. 

Also moved the dimension calculations for the positioned objects from RenderBlock::layoutInlineChildren() to RenderBlock::LineBreaker::skipLeadingWhitespace(), which is seems to be more specific and also reduces the number of OR and AND conditions

[WebKit Review Bot]

Comment on attachment 146003 [details]
Patch

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

New failing tests:
fast/regions/positioned-objects-inline-static-in-regions.html
fast/repaint/block-layout-inline-children-float-positioned.html
fast/regions/positioned-objects-inline-static-in-rtl-regions.html

[WebKit Review Bot]

Created attachment 146182 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Pravin D]

(In reply to comment #16)
> (From update of attachment 146003 [details])
> Attachment 146003 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12913074
> 
> New failing tests:
> fast/regions/positioned-objects-inline-static-in-regions.html
> fast/regions/positioned-objects-inline-static-in-rtl-regions.html

I'm not sure what is correct behavior for these test cases. David Hyatt needs to confirm the correct the behavior(tests added in Changeset 96452).

> fast/repaint/block-layout-inline-children-float-positioned.html
This failure is not a result of the current patch.

[Julien Chaffraix]

Comment on attachment 146003 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        However in case of positioned objects the layouting is differed until other children of the container are laid out.

s/differed/deferred/.

> Source/WebCore/ChangeLog:19
> +        TODO: Fix for vertical writing mode.

We don't put FIXME into ChangeLog as they get forgotten. This should be moved in the code (preferably fixed but a FIXME is OK for now). Btw, we don't use TODO in WebKit.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1811
> +static bool needsPreComputeDimensionsForPositionedObjects(RenderObject* o)

It's not really a pre-compute, it's a logical width computing.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1818
> +    if (!parent->isRenderView() && currentStyle->position() == AbsolutePosition && currentStyle->isOriginalDisplayInlineType() 

positioned objects cover both position: absolute and position: fixed. Why is it fine to ignore fixed positioned elements?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1927
> +            if (needsPreComputeDimensionsForPositionedObjects(object)) {
> +                if (object->parent()->style()->isHorizontalWritingMode())
> +                    toRenderBox(object)->computeLogicalWidth();
> +            }

It really looks like it's not at the right place. If other call paths are calling setStaticPositions before skipLeadingWhitespace is called, then we have the same bug. I don't know this code super well so my suggestion would be to look how you handle the other cases and how do you make sure the logical width is computed.

Btw, why do you think this is broken with vertical mode?

> LayoutTests/ChangeLog:6
> +        Added refTests for test case that required rebasing. 

That, in itself, is worth pushing to another bug. It's a good change orthogonal of this code change. That would shrink your change and reduce the test overhead.

[Levi Weintraub]

Comment on attachment 146003 [details]
Patch

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

>> Source/WebCore/ChangeLog:7
>> +        However in case of positioned objects the layouting is differed until other children of the container are laid out.
> 
> s/differed/deferred/.

s/layouting/layout/

> Source/WebCore/ChangeLog:18
> +        As only the width needs to be calculated for the issue, the patch using certain criteria (to reduce perfomance hit) forces only the width to be calculated before
> +        the lines are laid out. 
> +
> +        Criterias used:
> +        1) The object must have position:absolute, display:inline,inline-block and,
> +        2) Must have static inline positions and,
> +        3) Must have
> +            a) Text-align as center or,
> +            b) Text-align as left and text direction as rtl or,
> +            c) Text-align as right and text direction as ltr. 
> + 

How were these criteria determined? I'm also concerned about only handling position:absolute.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1811
>> +static bool needsPreComputeDimensionsForPositionedObjects(RenderObject* o)
> 
> It's not really a pre-compute, it's a logical width computing.

We don't need to compute dimensions, we need to compute logicalWidth.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1927
>> +            }
> 
> It really looks like it's not at the right place. If other call paths are calling setStaticPositions before skipLeadingWhitespace is called, then we have the same bug. I don't know this code super well so my suggestion would be to look how you handle the other cases and how do you make sure the logical width is computed.
> 
> Btw, why do you think this is broken with vertical mode?

Why not inside setStaticPositions?

[Pravin D]

(In reply to comment #19)
> (From update of attachment 146003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146003&action=review

> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1818
> > +    if (!parent->isRenderView() && currentStyle->position() == AbsolutePosition && currentStyle->isOriginalDisplayInlineType() 
> 
> positioned objects cover both position: absolute and position: fixed. Why is it fine to ignore fixed positioned elements?
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1927
> > +            if (needsPreComputeDimensionsForPositionedObjects(object)) {
> > +                if (object->parent()->style()->isHorizontalWritingMode())
> > +                    toRenderBox(object)->computeLogicalWidth();
> > +            }
> 
> It really looks like it's not at the right place. If other call paths are calling setStaticPositions before skipLeadingWhitespace is called, then we have the same bug. I don't know this code super well so my suggestion would be to look how you handle the other cases and how do you make sure the logical width is computed.
> 
My bad... I was trying to fix the issue this particular rather than the cause... I'll try to fix for other cases too.


> > LayoutTests/ChangeLog:6
> > +        Added refTests for test case that required rebasing. 
> 
> That, in itself, is worth pushing to another bug. It's a good change orthogonal of this code change. That would shrink your change and reduce the test overhead.
> 
Maybe I can create another bug for this...

[Pravin D]

(In reply to comment #20)
> (From update of attachment 146003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146003&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        As only the width needs to be calculated for the issue, the patch using certain criteria (to reduce perfomance hit) forces only the width to be calculated before
> > +        the lines are laid out. 
> > +
> > +        Criterias used:
> > +        1) The object must have position:absolute, display:inline,inline-block and,
> > +        2) Must have static inline positions and,
> > +        3) Must have
> > +            a) Text-align as center or,
> > +            b) Text-align as left and text direction as rtl or,
> > +            c) Text-align as right and text direction as ltr. 
> > + 
> 
> How were these criteria determined? I'm also concerned about only handling position:absolute.
> 

This has to do with how positioned objects are laid out and the cause for this issue.
In short, when we have leading positioned objects(inline) with static inline positions, then setStaticPositions() is called which in turn results in a call to startAlignedOffsetForLine(). startAlignedOffsetForLine() depends on width of the block in consideration. The issue is that at this point, layout for the positioned object is still pending. So the start offset got from startAlignedOffsetForLine() is wrong as the width of the block(positioned obj) is wrong. 



Criteria (1) and (2) are a result of the above mentioned flow. (3) is dependent on the implementation of updateLogicalWidthForAlignment() which is the function responsible for calculating the static inline offsets. The criteria were chosen to reduce the number of calls to width computation. 

> >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1811
> >> +static bool needsPreComputeDimensionsForPositionedObjects(RenderObject* o)
> > 
> > It's not really a pre-compute, it's a logical width computing.
> 
> We don't need to compute dimensions, we need to compute logicalWidth.
> 
> >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1927
> >> +            }
> > 
> > It really looks like it's not at the right place. If other call paths are calling setStaticPositions before skipLeadingWhitespace is called, then we have the same bug. I don't know this code super well so my suggestion would be to look how you handle the other cases and how do you make sure the logical width is computed.
> > 
> > Btw, why do you think this is broken with vertical mode?
> 
> Why not inside setStaticPositions?
In skipLeadingWhitespace() when floating objects are encountered, they are also laid out before being placed. On similar lines, when positioned objects are encountered we calculate the width , instead of full layout and place the objects. 


Anyways I guess I will be changing the fix altogether. I was trying to fix the issue in a very closed context to this bug... Ideally the fix has to be either in setStaticPositions() or in startAlignedOffsetForLine().

[Pravin D]

Created attachment 147292 [details]
Patch

[Pravin D]

(In reply to comment #23)
> Created an attachment (id=147292) [details]
> Patch

About the issue: 

<div>
Filler text or some tag(assume all inlines)
<img style="position:absolute;display:inline"> </span>
</div>

Here the Layout is called for the <div> .. Lines are created  for each node in the <div> and also the position of each of the node is determined/set. During the line creation if Layout is needed for any object , it is called. Once lines are created etc, layout for the positioned object contained in <div>  is called.

During the line creation RenderBlock::LineBreaker::nextLineBreak() is called. This function is responsible for deciding the contents of a line i.e the nodes that can be present on a given line. nextLineBreak() calls skipLeadingWhitespace() to skip spaces and to position any leading positioned/float objects. This uses setStaticPositions() to set the static positions for positioned objects.

The issue here is that setStaticPositions() uses startAlignedOffsetForLine() to get the line offset(static inline position) of the child(positioned object) for a given text alignment. startAlignedOffsetForLine() requires that the child's width is properly calculated before it is called, otherwise the offset returned would be incorrect. 

Patch:
As it is the responsibility of the caller to make sure that the width of the positioned child is properly set before calling startAlignedOffsetForLine(), we call compute the logical width in setStaticPositions(). 


startAlignedOffsetForLine() is called only when the container is RenderInline or child is inline. Also it is not entirely sensitive to the child's Logical width. There are some cases where child's logical width affect the aligned offset. These criteria are used in shouldComputeLogicalWidthForChild() to determine if we need to compute the logical width for the child at setStaticPositions() or not. 

Code removal from RenderBlock.cpp
It was added for the bug https://bugs.webkit.org/show_bug.cgi?id=4860
The bug seems to same as this one. The code seems to be redundant as we will be  setting the inline static position of the positioned object properly now.
Require Robert to give his opinion on the same.

[Robert Hogan]

At first glance this should be covered by https://bugs.webkit.org/show_bug.cgi?id=77754

[Pravin D]

(In reply to comment #25)
> At first glance this should be covered by https://bugs.webkit.org/show_bug.cgi?id=77754

sorry abt that... its Bud 77754 (REGRESSION bug)

[Robert Hogan]

Comment on attachment 147292 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:-2543
> -        // Adjust the static position of a center-aligned inline positioned object with a block child now that the child's width has been computed.
> -        if (!r->parent()->isRenderView() && r->parent()->isRenderBlock() && r->firstChild() && r->style()->position() == AbsolutePosition
> -            && r->style()->isOriginalDisplayInlineType() && (r->style()->textAlign() == CENTER || r->style()->textAlign() == WEBKIT_CENTER)) {
> -            RenderBlock* block = toRenderBlock(r->parent());
> -            LayoutUnit blockHeight = block->logicalHeight();
> -            block->setStaticInlinePositionForChild(r, blockHeight, block->startAlignedOffsetForLine(r, blockHeight, false));
> -        }
> -

Ah, spoke to soon! :) 

It's not clear from your Changelog which test cases currently failing in trunk this refactoring progresses. Can you make this explicit?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:885
> +    if (shouldComputeLogicalWidthForChild(child))
> +        child->computeLogicalWidth();

The downside here is that the width will now be computed twice for qualifying text-alignment/direction combinations. I went through the same thought process in bug 77754.

The only food for thought I can offer is:

- Can we avoid the second (subsequent) computation of the logical width?
- Why not amend the code you're reverting to cope with the cases that this patch fixes?

[Pravin D]

(In reply to comment #27)
> (From update of attachment 147292 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147292&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:-2543
> > -        // Adjust the static position of a center-aligned inline positioned object with a block child now that the child's width has been computed.
> > -        if (!r->parent()->isRenderView() && r->parent()->isRenderBlock() && r->firstChild() && r->style()->position() == AbsolutePosition
> > -            && r->style()->isOriginalDisplayInlineType() && (r->style()->textAlign() == CENTER || r->style()->textAlign() == WEBKIT_CENTER)) {
> > -            RenderBlock* block = toRenderBlock(r->parent());
> > -            LayoutUnit blockHeight = block->logicalHeight();
> > -            block->setStaticInlinePositionForChild(r, blockHeight, block->startAlignedOffsetForLine(r, blockHeight, false));
> > -        }
> > -
> 
> Ah, spoke to soon! :) 
> 
> It's not clear from your Changelog which test cases currently failing in trunk this refactoring progresses. Can you make this explicit?
> 
Will do it...

> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:885
> > +    if (shouldComputeLogicalWidthForChild(child))
> > +        child->computeLogicalWidth();
> 
> The downside here is that the width will now be computed twice for qualifying text-alignment/direction combinations. I went through the same thought process in bug 77754.
> 
> The only food for thought I can offer is:

> - Why not amend the code you're reverting to cope with the cases that this patch fixes?
> 
The cause of this issue is that startAlignedOffsetForLine() is called before the width is calculated properly. The caller in our case is setStaticPositions(). So anyone calling setStaticPositions() without proper logical width computed is bound to hit a similar issue.

Also currently skipTrailingWhitespace() and skipLeadingWhitespace() use setStaticPositions(). So if the reverting code is to be amended we have to check if the positioned object is a trailing node, leading node or a node proceeded by  white spaces which can be skipped(dependent on style again) in each of the lines(worst case all the lines).

It seems to become very specific here...

> 
> - Can we avoid the second (subsequent) computation of the logical width?
Not sure... need to check...

Thanks for your review comment :)

[WebKit Review Bot]

Comment on attachment 147292 [details]
Patch

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

New failing tests:
fast/inline/left-right-center-inline-alignment-in-ltr-and-rtl-blocks.html
fast/css/bug4860-absolute-inline-child-inherits-alignment.html
fast/repaint/block-layout-inline-children-float-positioned.html
fast/regions/positioned-objects-inline-static-in-regions.html
fast/regions/positioned-objects-inline-static-in-rtl-regions.html
fast/css/absolute-child-with-percent-height-inside-relative-parent.html

[WebKit Review Bot]

Created attachment 147421 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Julien Chaffraix]

Comment on attachment 147292 [details]
Patch

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

>>> Source/WebCore/rendering/RenderBlock.cpp:-2543
>>> -
>> 
>> Ah, spoke to soon! :) 
>> 
>> It's not clear from your Changelog which test cases currently failing in trunk this refactoring progresses. Can you make this explicit?
> 
> Will do it...

I am not convinced this is the right way. How about the rest of the code who assumes that the static inline position is set on RenderLayer?

>>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:885
>>> +        child->computeLogicalWidth();
>> 
>> The downside here is that the width will now be computed twice for qualifying text-alignment/direction combinations. I went through the same thought process in bug 77754.
>> 
>> The only food for thought I can offer is:
>> 
>> - Can we avoid the second (subsequent) computation of the logical width?
>> - Why not amend the code you're reverting to cope with the cases that this patch fixes?
> 
> 

I will add more food for thoughts: setStaticPosition is a setter that shouldn't start doing complex operations. Unless we decide to lazily compute the width, setters shouldn't do complex hidden operations. In this case, if we call setStaticPositions several times, then we recompute alos the width several times.

Levi proposed that but it feels wrong. As stated, I don't know this code a lot so maybe I am wrong but it itches when you propose to do that.

[Eric Seidel (no email)]

This bug is entirely a CSS 2.1 spec understanding question.  someone needs to understand exactly what the expected behavior here is from the CSS 2.1 spec.  I suspect that
http://www.w3.org/TR/CSS2/visudet.html#abs-replaced-width
is what applies here.  I don't understand FF's layout.  It seems to me that top/left are auto (default) and thus end up as 0 here, but I may not be fully understanding the effects of text-align: center yet:
http://www.w3.org/TR/CSS2/visuren.html#inline-formatting

Once we have an understanding of correct css 2.1 behavior here, then we can adjust our behavior if necessary.

[Tab Atkins Jr.]

(In reply to comment #32)
> This bug is entirely a CSS 2.1 spec understanding question.  someone needs to understand exactly what the expected behavior here is from the CSS 2.1 spec.  I suspect that
> http://www.w3.org/TR/CSS2/visudet.html#abs-replaced-width
> is what applies here.  I don't understand FF's layout.  It seems to me that top/left are auto (default) and thus end up as 0 here, but I may not be fully understanding the effects of text-align: center yet:
> http://www.w3.org/TR/CSS2/visuren.html#inline-formatting
> 
> Once we have an understanding of correct css 2.1 behavior here, then we can adjust our behavior if necessary.

Apologies for the delay in responding here.  I've had this tab open for two weeks. >_<

The correct behavior is what we do initially, where the yellow abspos is offset half the width of the red box.

This is difficult to follow in the 2.1 spec, because it hints at some abstractions that should be explicitly talked about, but most of the time you can get away with just imagining that abspos elements leave behind a 0x0 inline "placeholder" in the flow, and the auto position is just wherever that placeholder gets put.

In this case, because the outer block is text-align:center, the placeholder gets centered as well, so that explains the horizontal position of the abspos.

The vertical position is then explained by the fact that, since the only inline content is placeholders and collapsed white space, no real line box gets created, only a "phantom" line box that's 0px high and solely used to align placeholders.

[Pravin D]

(In reply to comment #33)
> (In reply to comment #32)
> > This bug is entirely a CSS 2.1 spec understanding question.  someone needs to understand exactly what the expected behavior here is from the CSS 2.1 spec.  I suspect that
> > http://www.w3.org/TR/CSS2/visudet.html#abs-replaced-width
> > is what applies here.  I don't understand FF's layout.  It seems to me that top/left are auto (default) and thus end up as 0 here, but I may not be fully understanding the effects of text-align: center yet:
> > http://www.w3.org/TR/CSS2/visuren.html#inline-formatting
> > 
> > Once we have an understanding of correct css 2.1 behavior here, then we can adjust our behavior if necessary.
> 
> Apologies for the delay in responding here.  I've had this tab open for two weeks. >_<
> 
> The correct behavior is what we do initially, where the yellow abspos is offset half the width of the red box.
> 
You mean to say that we are already rendering the abs element (yellow) correctly ? I'm a little confused sorry :(

> This is difficult to follow in the 2.1 spec, because it hints at some abstractions that should be explicitly talked about, but most of the time you can get away with just imagining that abspos elements leave behind a 0x0 inline "placeholder" in the flow, and the auto position is just wherever that placeholder gets put.
> 
> In this case, because the outer block is text-align:center, the placeholder gets centered as well, so that explains the horizontal position of the abspos.
> 
The issue that we imagine a 0x0 inline placeholder for the abs element. The behavior is different when overflow:scroll or auto is set on body.
If you see the test page has overflow:hidden; on body set and the frameView is layouted only once. If suppose we set overflow:scroll or auto, the frameView layouted twice. By the 2nd layout the size of the inner abs(yellow) is already known and centering happens properly...

> The vertical position is then explained by the fact that, since the only inline content is placeholders and collapsed white space, no real line box gets created, only a "phantom" line box that's 0px high and solely used to align placeholders.
> 
guess so...

[Pravin D]

(In reply to comment #33)
Forgot to mention... open the testcase, then start the webkit inspector... the see the behavior

[Tab Atkins Jr.]

(In reply to comment #34)
> (In reply to comment #33)
>> The correct behavior is what we do initially, where the yellow abspos is offset half the width of the red box.
>
> You mean to say that we are already rendering the abs element (yellow) correctly ? I'm a little confused sorry :(

Our initial rendering (where the yellow is half-offset from the red) is correct.  Our rendering after a resize (where the yellow and red overlap) is wrong.

Is there a reason why this bug is not marked as a duplicate of bug 77754?

[Pravin D]

(In reply to comment #37)
> Is there a reason why this bug is not marked as a duplicate of bug 77754?
> 

I guess the root cause for this issue and bug 77754 is similar...However the previous patch for bug 77754 does not fix the test case in this bug

The issue starts at the below function.... Putting it short... 

static void setStaticPositions(RenderBlock* block, RenderBox* child)
{
    // FIXME: The math here is actually not really right. It's a best-guess approximation that
    // will work for the common cases
    RenderObject* containerBlock = child->container();
    LayoutUnit blockHeight = block->logicalHeight();
    if (containerBlock->isRenderInline()) {
        // A relative positioned inline encloses us. In this case, we also have to determine our
        // position as though we were an inline. Set |staticInlinePosition| and |staticBlockPosition| on the relative positioned
        // inline so that we can obtain the value later.
        toRenderInline(containerBlock)->layer()->setStaticInlinePosition(block->startAlignedOffsetForLine(child, blockHeight, false));
        toRenderInline(containerBlock)->layer()->setStaticBlockPosition(blockHeight);
    }

    if (child->style()->isOriginalDisplayInlineType())
        block->setStaticInlinePositionForChild(child, blockHeight, block->startAlignedOffsetForLine(child, blockHeight, false));
    else
        block->setStaticInlinePositionForChild(child, blockHeight, block->startOffsetForContent(blockHeight));
    child->layer()->setStaticBlockPosition(blockHeight);
}

Basically when startAlignedOffsetForLine() expects that the elements(current) size is already calculated...

Previous bug 77754 patch was taking care of the case if (containerBlock->isRenderInline()) {} 

if (child->style()->isOriginalDisplayInlineType()){} was not patched... So I guess the bugs should not be duplicated(atleast that was the case when I started working on this bug)...

Another reason that these bugs should not be duplicated(atleast in my opinion) as the proper point for fixing the issue is a little complicated to arrive at...
three approaches I can think of...
1) A modification of the patch http://trac.webkit.org/changeset/113584/trunk/Source/WebCore/rendering/RenderBlock.cpp  

2) Modify startAlignedOffsetForLine()[Current patch on bug 77754]...
3) Do not delay the layout of all the positioned elements, depending on the conditions given in the spec...(atleast worth exploring)...


Maybe we can start a master bug which tries to solve the possible issues that might arise  when startAlignedOffsetForLine() is used with child element size is zero...
Please let me know your opinion...

[Anders Carlsson]

Comment on attachment 147292 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.