44508 - [ABS POS][INLINE]Wrong 'top:auto' calculation on absolute "inline"

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[bob_erek]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m17) Gecko/20000702
BuildID:
2000070208

In the attachment, the <SPAN> element with an absolute position and 'top:auto' is about
one line under the correct position.

Reproducible: Always
Steps to Reproduce:
1.Look the attachment


Actual Results:  The red
'--' is in front of the line after 'THIS' word.


Expected Results:  The red '--' should to
the left of 'THIS' word, at the same level. (see CSS2 specs at 9.8.4, it is the
last example)

The source code of the attachment: (it is the example of W3C)

   <P style="position:
relative; left: 10px; width: 200px;">
   I used two red hyphens to serve as a change bar. They
   will "float" to the left
of the line containing THIS
   <SPAN style="position: absolute; top: auto; left: -1em;
color: red;">--</SPAN>
   word. Here is some extra text to demonstrate the bug.</P>

Comment 1

[bob_erek]

Attachment: A test case for the bug

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Confirming bug, although I think this may be a duplicate.

I'm not 100% sure this is a bug.  The issue is I think that elements with 
'display:inline' and 'position:absolute' have their display property changed to 
block.  The question is whether the 'static-position' that 'auto' turns into 
refers to the position as if they had 'display: block' or as if they had 
'display: inline'.

Comment 3

[karnaze (gone)]

Reassigning to Buster.

Comment 4

[buster]

P3 bugs are getting moved to "future" milestone.  They will not be addressed for
NS6 RTM.

Comment 5

[Hixie (not reading bugmail)]

Upon managerial request, adding the "testcase" keyword to 84 open layout bugs that
do not have the "testcase" keyword and yet have an attachement with the word
"test" in the description field. Apologies for any mistakes.

Comment 6

[Peter 'Rufus' Nelson]

*** Bug 76924 has been marked as a duplicate of this bug. ***

Comment 7

[mozilla3eran]

In reply to David Baron's note:

According to http://www.w3.org/TR/REC-CSS2/visudet.html#abs-non-replaced-height :

   If 'top' has the value 'auto', replace it with the distance from the top edge
of the 
   containing block to the top margin edge of a hypothetical box that would have 
   been the first box of the element if its 'position' property had been 'static'.

So this is indeed a bug. 
Also note that the testcase given above appears in the CSS2 spec yet is
rendererd incorrectly.

Comment 8

[Kevin McCluskey (gone)]

Build reassigning Buster's bugs to Marc.

Comment 9

[Nicolás Lichtmaier]

Reproduced on Linux, marking as platform=ALL,os=ALL (also adding css2 keyword).

Comment 10

[Mats Palmgren (inactive)]

Attachment: Improved testcase

Comment 11

[Mats Palmgren (inactive)]

I have a patch that fixes this bug. It also fixes bug 53052 and bug 87712.
The problem is that "nsRuleNode::ComputeDisplayData" is resetting 'display' to
'block' for abs. pos. elements (to satisfy CSS2 9.7). This makes the
hypothetical box calculation (nsHTMLReflowState::CalculateHypotheticalBox)
always calulate it
as it were a block.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch

Comment 13

[Mats Palmgren (inactive)]

The patch saves the original 'display' value just before it is reset to 'block'
in nsRuleNode::ComputeDisplayData. The original value is then used to calculate
the hypothetical box. Also fixed a bug in CalculateHypotheticalBox() in the
case of inlines (this code hasn't been used until now) when the containing block
had NS_AUTOHEIGHT. I resorted to the same kind of code as is done for blocks
(take the viewport height).

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 93444 [details] [diff] [review]
Patch

The parts of the patch that are directly related to this bug look like a good
solution, but I'm a bit concerned about some of the other changes.

>     // Get the placeholder x-offset and y-offset in the coordinate
>     // space of the block frame that contains it
>-    GetPlaceholderOffset(aPlaceholderFrame, aBlockFrame, placeholderOffset);
>+    aPlaceholderFrame->GetOrigin(placeholderOffset);

This change (and the removal of GetPlaceholderOffset) seems wrong.  Consider
something like

<div>
  This is some text <span>that has an inline element
  within it that <img style="position: absolute;">
  has an absolutely</span> positioned element within
  it.
</div>

The following code looks like it needs the coordinate relative to the block. 
If that's not the case, then you need to change the comments.

>@@ -906,19 +866,22 @@
>     if (aBlockFrame) {
>       nsBlockFrame* blockFrame = NS_STATIC_CAST(nsBlockFrame*, aBlockFrame);
> 
>-      // We need the immediate child of the block frame, and that may not be
>-      // the placeholder frame
>-      nsIFrame *blockChild = FindImmediateChildOf(aBlockFrame, aPlaceholderFrame);
>-      nsBlockFrame::line_iterator lineBox = blockFrame->FindLineFor(blockChild);
>-      if (lineBox != blockFrame->end_lines()) {
>-        // The top of the hypothetical box is just below the line containing
>-        // the placeholder
>-        aHypotheticalBox.mTop = lineBox->mBounds.YMost();
>-      } else {
>+      // Find the YMost of the previous in flow frame
>+      nsIFrame *prevInFlow;
>+      aPlaceholderFrame->GetPrevInFlow(&prevInFlow);
>+      if (nsnull != prevInFlow) {
>+        nsBlockFrame::line_iterator lineBox = blockFrame->FindLineFor(prevInFlow);
>+        if (lineBox != blockFrame->end_lines()) {
>+          aHypotheticalBox.mTop = lineBox->mBounds.YMost();
>+        } else {
>+          prevInFlow = nsnull;
>+        }
>+      }
>+      if (nsnull == prevInFlow) {

Why did you change this (and remove FindImmediateChildOf)?  FindLineFor only
searches children (that could be better documented in nsBlockFrame.h, but it
can be discovered by following the implementation to nsLineBox::Contains and
nsLineBox::IndexOf), and the placeholder (see the example above) need not be a
child of the block.

>         nsPoint placeholderOffset;
>         
>         // Just use the placeholder's y-offset
>-        GetPlaceholderOffset(aPlaceholderFrame, aBlockFrame, placeholderOffset);
>+        aPlaceholderFrame->GetOrigin(placeholderOffset);

same as before.

>         aHypotheticalBox.mTop = placeholderOffset.y;
>       }
>     }

>-          if (NS_CSS_FRAME_TYPE_ABSOLUTE == type) {
>+          if ((NS_AUTOHEIGHT == aContainingBlockHeight)) {
>+            // Use the viewport height as the containing block height
>+            const nsHTMLReflowState* rs = cbrs->parentReflowState;
>+            while (rs->parentReflowState) {
>+              rs = rs->parentReflowState;
>+            }
>+            aContainingBlockHeight = rs->mComputedHeight;
>+          }
>+          else if (NS_CSS_FRAME_TYPE_ABSOLUTE == type) {

What led you to make this change?  It doesn't seem right, but I'm not sure what
you were trying to fix.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 93444 [details] [diff] [review]
Patch

The parts of the patch that are directly related to this bug look like a good
solution, but I'm a bit concerned about some of the other changes.

>     // Get the placeholder x-offset and y-offset in the coordinate
>     // space of the block frame that contains it
>-    GetPlaceholderOffset(aPlaceholderFrame, aBlockFrame, placeholderOffset);
>+    aPlaceholderFrame->GetOrigin(placeholderOffset);

This change (and the removal of GetPlaceholderOffset) seems wrong.  Consider
something like

<div>
  This is some text <span>that has an inline element
  within it that <img style="position: absolute;">
  has an absolutely</span> positioned element within
  it.
</div>

The following code looks like it needs the coordinate relative to the block. 
If that's not the case, then you need to change the comments.

>@@ -906,19 +866,22 @@
>     if (aBlockFrame) {
>       nsBlockFrame* blockFrame = NS_STATIC_CAST(nsBlockFrame*, aBlockFrame);
> 
>-      // We need the immediate child of the block frame, and that may not be
>-      // the placeholder frame
>-      nsIFrame *blockChild = FindImmediateChildOf(aBlockFrame, aPlaceholderFrame);
>-      nsBlockFrame::line_iterator lineBox = blockFrame->FindLineFor(blockChild);
>-      if (lineBox != blockFrame->end_lines()) {
>-        // The top of the hypothetical box is just below the line containing
>-        // the placeholder
>-        aHypotheticalBox.mTop = lineBox->mBounds.YMost();
>-      } else {
>+      // Find the YMost of the previous in flow frame
>+      nsIFrame *prevInFlow;
>+      aPlaceholderFrame->GetPrevInFlow(&prevInFlow);
>+      if (nsnull != prevInFlow) {
>+        nsBlockFrame::line_iterator lineBox = blockFrame->FindLineFor(prevInFlow);
>+        if (lineBox != blockFrame->end_lines()) {
>+          aHypotheticalBox.mTop = lineBox->mBounds.YMost();
>+        } else {
>+          prevInFlow = nsnull;
>+        }
>+      }
>+      if (nsnull == prevInFlow) {

Why did you change this (and remove FindImmediateChildOf)?  FindLineFor only
searches children (that could be better documented in nsBlockFrame.h, but it
can be discovered by following the implementation to nsLineBox::Contains and
nsLineBox::IndexOf), and the placeholder (see the example above) need not be a
child of the block.

>         nsPoint placeholderOffset;
>         
>         // Just use the placeholder's y-offset
>-        GetPlaceholderOffset(aPlaceholderFrame, aBlockFrame, placeholderOffset);
>+        aPlaceholderFrame->GetOrigin(placeholderOffset);

same as before.

>         aHypotheticalBox.mTop = placeholderOffset.y;
>       }
>     }

>-          if (NS_CSS_FRAME_TYPE_ABSOLUTE == type) {
>+          if ((NS_AUTOHEIGHT == aContainingBlockHeight)) {
>+            // Use the viewport height as the containing block height
>+            const nsHTMLReflowState* rs = cbrs->parentReflowState;
>+            while (rs->parentReflowState) {
>+              rs = rs->parentReflowState;
>+            }
>+            aContainingBlockHeight = rs->mComputedHeight;
>+          }
>+          else if (NS_CSS_FRAME_TYPE_ABSOLUTE == type) {

What led you to make this change?  It doesn't quite seem right, but I'm not
sure what the testcase is that you were trying to fix.	Does it affect our
conformance to 10.1 of CSS2?  Does it affect the way we compute percentage
heights?

Comment 16

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (from David's comment)

Comment 17

[Mats Palmgren (inactive)]

> This change (and the removal of GetPlaceholderOffset) seems wrong.  Consider
> something like ...


Yep, you are right, I was looking at a more complex testcase involving relative
positioned parents (from bug 53052). GetPlaceholderOffset() is back.
(see Testcase #2)


> Why did you change this (and remove FindImmediateChildOf)?  FindLineFor only
> searches children ...


I still think there is a bug in that code (bug 87712), but I rolled back this
part and will make that into a patch for bug 87712 instead.


> >-          if (NS_CSS_FRAME_TYPE_ABSOLUTE == type) {
> >+          if ((NS_AUTOHEIGHT == aContainingBlockHeight)) {
> >+            // Use the viewport height as the containing block height
> >+            const nsHTMLReflowState* rs = cbrs->parentReflowState;
> >+            while (rs->parentReflowState) {
> >+              rs = rs->parentReflowState;
> >+            }
> >+            aContainingBlockHeight = rs->mComputedHeight;
> >+          }
> >+          else if (NS_CSS_FRAME_TYPE_ABSOLUTE == type) {
> 
> What led you to make this change?  It doesn't seem right, but I'm not sure what
> you were trying to fix.


Well, I saw this assertion in InitAbsoluteConstraints():
###!!! ASSERTION: containing block height must be constrained:
 'containingBlockHeight != NS_AUTOHEIGHT', file nsHTMLReflowState.cpp, line 999

and I thought it was a result of the block->inline change, but it turns out
that this assertion is also in the original code.
I have removed this part as well (it can be handled in bug 53052 since it was
triggered with the testcase there).

I looked at all these three bugs at once since I fooled myself into believing
they were related. I now think they can be handled separately.

New (much simpler) patch coming up. FWIW, it also passes the block regression
tests (although positioned elements isn't very stressed in those tests).

Comment 18

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 19

[Mats Palmgren (inactive)]

*** Bug 142433 has been marked as a duplicate of this bug. ***

Comment 20

[Boris Zbarsky [:bzbarsky]]

Hm... could we:

1)  Initialize mOriginalDisplay in nsStyleDisplay::nsStyleDisplay() (preferably to
    some value that should never happen in real life, like "-1" or
    NS_STYLE_DISPLAY_NONE (dbaron?  Thoughts on that?)
2)  Assert that the value is not equal to said bogus value in
    nsHTMLReflowState.cpp
3)  Change nsStyleDisplay::nsStyleDisplay(const nsStyleDisplay& aSource) to
    properly copy mOriginalDisplay?

Other than that, looks good.  ;)

Comment 21

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Fixed Boris' three points above.

Comment 22

[Mats Palmgren (inactive)]

Attachment: Testcase #3, for testing 'display' values

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 93711 [details] [diff] [review]
Patch rev. 3

sr=bzbarsky

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 93711 [details] [diff] [review]
Patch rev. 3

r=dbaron

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you need someone to check this in for you?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually....

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... it looks like bzbarsky checked this in to the trunk, 2002-08-15 16:00 PDT.

Comment 28

[Mats Palmgren (inactive)]

Verified fixed in 2002-08-20-08 trunk Linux.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Likely bug 59788.
I can't reproduce bug 59788 after I can't reproduce this problem.

Comment 30

[Mats Palmgren (inactive)]

*** Bug 59788 has been marked as a duplicate of this bug. ***

Comment 31

[Boris Zbarsky [:bzbarsky]]

My apologies for not commenting in this bug when I checked in the patch...

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This exposed an underlying bug: bug 181644.