181644 - Absolute positioned inline in Abs Pos block is slid up

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P3)

Description

[HARUNAGA Hirotoshi]

When absolute positioned block element contains only absolute
positioned inline elements, these inlines are slided up
even if {top: auto;}.

Testcase:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1362&action=view

No problem if Abs Pos block contains also normal inline element:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1369&action=view

Confirming with 2002111308/FreeBSD and 2002111908/Mac 9.2.2.

Comment 1

[Boris Zbarsky [:bzbarsky]]

layout

Comment 2

[Andrew Schultz]

regression between linux trunk builds 2002081508 and 2002081611
apparent regression from bug 44508

Comment 3

[Rick Ju]

take it

Comment 4

[HARUNAGA Hirotoshi]

Attachment: Another testcase

Same behavior when static <div> contains only Abs Pos inlines.

Comment 5

[Rick Ju]

i have found why.

it works with this patch:

Index: nsHTMLReflowState.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsHTMLReflowState.cpp,v
retrieving revision 1.150
diff -u -r1.150 nsHTMLReflowState.cpp
--- nsHTMLReflowState.cpp	24 Sep 2002 21:02:17 -0000	1.150
+++ nsHTMLReflowState.cpp	27 Nov 2002 08:02:45 -0000
@@ -856,6 +856,7 @@
     nsIFontMetrics*     metrics;
     const nsStyleFont*  font;
 
+#if 0
     frame->GetStyleData(eStyleStruct_Font, (const nsStyleStruct*&)font);
     aPresContext->GetMetricsFor(font->mFont, &metrics);
     if (metrics) {
@@ -867,6 +868,7 @@
       placeholderOffset.y -= ascent;
       NS_RELEASE(metrics);
     }
+#endif
     aHypotheticalBox.mTop = placeholderOffset.y;
 
     // To determine the left and right offsets we need to look at the block's
'direction'



the patch just remove the fontMetrics Stuff.

i didn't find why we need such a ascent offset. 
i am doing a regression test

Comment 6

[Boris Zbarsky [:bzbarsky]]

> i didn't find why we need such a ascent offset. 

Like the comment says, because the placeholder frame is positioned on the
baseline of the text (see nsPlaceholderFrame::Reflow()) while the top of the
positioned box should line up with the top of the text line....  The problem, it
seems, is that the placeholder is not positioned correctly when there is no
other text in the line.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, and forget the regression tests.  That change will break the second url in
comment 0 -- the one that works in current builds.

Comment 8

[Rick Ju]

how about if we only add that offset for not-replaced inline, whild don't add for 
replaced inline? 

Comment 9

[Boris Zbarsky [:bzbarsky]]

how would that help?

Comment 10

[Rick Ju]

i know more now, i guess.  pls ignore #8, i suggest.:)

bz gave the correct direction.

for a LineBox which contains only a placeholder, its ascent/descent should both
be  0. the placeholder's ascent/descent is 0 too.

so we should not use the placeholder's FontMetric to compute its ascent/descent! 
in fact, even when the placeholder has a static-positioned brother, we shouldn't
also! a placeholder's ascent/descent is always 0. 

then what we should use? the placeholder's origin, i guess. its origin should
always equal its parent LineBox's ascent. 

BTW, is possible change GetPlaceholderOffset's definition directly?

The patch should be sth like:



    854 #if 0
    855     // The y-offset is the baseline of where the text would be if it were
    856     // in the flow. We need the top position and not the baseline position
    857     nsIFontMetrics*     metrics;
    858     const nsStyleFont*  font;
    859
    860     frame->GetStyleData(eStyleStruct_Font, (const nsStyleStruct*&)font);
    861     aPresContext->GetMetricsFor(font->mFont, &metrics);
    862     if (metrics) {
    863       nscoord   ascent;
    864
    865       // Adjust the y-offset up by the font ascent. That will translate from
    866       // the baseline to the top of where the text would be
    867       metrics->GetMaxAscent(ascent);
    868       placeholderOffset.y -= ascent;
    869       NS_RELEASE(metrics);
    870     }
    871 #else
    872       nsPoint  offset;
    873       aPlaceholderFrame->GetOrigin(offset);
    874       placeholderOffset.y -= offset.y;
    875 #endif
    876


   

Comment 11

[EspeonEefi]

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

Comment 12

[Rick Ju]

Attachment: proposed patch

bz, can you r=?

Comment 13

[Hixie (not reading bugmail)]

This bug is... odd. Inlines (replaced or not) should become blocks when
positioned. So I don't see why the bug would be inline specific.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ian, the issue is that a positioned box without offsets set gets positioned
based on where it "would have been" in the flow.  For a block, that's easy --
the placeholder is in the right place.  For an inline, the placeholder is on the
baseline of the line the inline would have been in, but we want to position at
the top of the line instead.

I'll need to read a bunch of the line layout code to review this properly, Rick.
 It'll take me a while (like on the order of 2-3 weeks) to get to this review.

Comment 15

[Hixie (not reading bugmail)]

bz: Oh, I see what you mean. Yeah, that makes sense. I should make some
testcases for that I guess.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 108192 [details] [diff] [review]
proposed patch

Add a comment to the effect that we want to align our top with the top of the
linebox the placeholder lives in, ok?

It's not clear to me that this does the right thing when the placeholder is in
an inline box that was vertical-aligned to subscript or superscript or
something.

Comment 18

[Rick Ju]

//not know much about sub/super-scrit 
but here we are thinking the hypothetical Box's top is content-top of the inline
parent of the placeholder( which is only a rect with 0 height/ascent/descent )? 
if this is right, is that important where the inline parent is aligned? 

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 108192 [details] [diff] [review]
proposed patch

OK. Agreed that what we really want is the top of the inline box that would
have held the inline content.  Add a comment to that effect, please, and let's
run with this.

Comment 20

[Rick Ju]

ok. i will add the comment after kin sr=. 

sth like:

//use the top of thelinebox which the placeholder lives in as hypothetical box's
top, so adjust the y-offset up by the placeholder's origin

Comment 21

[Boris Zbarsky [:bzbarsky]]

Something like:

// Use the top of the inline box which the placeholder lives in as the
// hypothetical box's top.

would be perfect, imo.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 108192 [details] [diff] [review]
proposed patch

stealing from kin's sr-list. sr=roc+moz

Comment 23

[Jerry]

patch checked in.

Comment 24

[Rick Ju]

fixed

Comment 25

[Boris Zbarsky [:bzbarsky]]

This caused bug 187749

Comment 26

[Rick Ju]

reopen for regression #187749
will rewrite a patch

Comment 27

[Rick Ju]

Attachment: for discussion

can we say the hypothetical top is always >=0?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 110854 [details] [diff] [review]
for discussion

if nothing else, please use an nsCOMPtr for the nsIFontMetrics....

And add a comment explaining why there is this PR_MAX there now?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 110854 [details] [diff] [review]
for discussion

The more I think about this, the more it feels like a hack...

Comment 30

[Rick Ju]

sure. but in fact it comes from we using a FontMetrics to compute linebox top.
The later is a hack and now we just polish the hacking. :)

Comment 31

[Andrew Schultz]

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

Comment 32

[Boris Zbarsky [:bzbarsky]]

kin says that nsBlockFrame has a utility method to find the line box that
contains something.... perhaps we should use that here and just use the top of
the line box (since that's what we want, after all).

Comment 33

[kinmoz]

The method bz refers to above is nsBlockFrame::GetClosestLine(). It does a
binary search based on a point, and is currently used by selection.

Comment 34

[Rick Ju]

Attachment: for discussion

using the FindLineFor(...).  
the code is not polished yet
have passed the testcases for #181644, #187749, #188446 and #44508

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111384 [details] [diff] [review]
for discussion

This looks like a much better approach; I like it.  Looking over the code that
sets the mLeft/mRight (now that you have separated out the code for mTop) it
looks _identical_ for block and inline, except that 

aHypotheticalBox.mLeft = placeholderOffset.x;

is replaced by

aHypotheticalBox.mLeft = aBlockContentArea.left;

if dir is LTR and similarly for dir being RTL.	Would you mind merging those
two branches into a single chunk of code, with the conditioning being done
directly in those assignment lines?  Maybe something like:

aHypotheticalBox.mLeft =
 (NS_STYLE_DISPLAY_INLINE == mStyleDisplay->mOriginalDisplay) ?
  placeholderOffset.x : aBlockContentArea.left;

or a full if statement?  I think that not having identical code in two
different places would make this function much more readable...

> +  //First, determine the hypothetical box's mTop 

Space after "//" please.  And same for "//Second".

Comment 36

[Rick Ju]

Attachment: patch

except that the  851 if (aBlockFrame) {  make me not very comfortable.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111487 [details] [diff] [review]
patch

+    if (NS_STYLE_DISPLAY_INLINE == mStyleDisplay->mOriginalDisplay) {
+      // The placeholder represents the left edge of the hypothetical box

Shouldn't that comment be outside that if?  It belongs with the 
+  if (NS_STYLE_DIRECTION_LTR == blockVis->mDirection),
no?

with that change, r=bzbarsky.

What about the if (aBlockFrame) test bothers you?

Comment 38

[Rick Ju]

>with that change, r=bzbarsky.

that comment should only for _DISPLAY_INLINE so including it in that if() is
better? ( similiar case in +905 )

>What about the if (aBlockFrame) test bothers you?

no pointer check in line 843:

  842   const nsStyleVisibility* blockVis;
  843   aBlockFrame->GetStyleData(eStyleStruct_Visibility, (const
nsStyleStruct*&)blockVis);

Comment 39

[Boris Zbarsky [:bzbarsky]]

> That comment should only for _DISPLAY_INLINE 

Ah, true.  Then please move it accordingly in the RTL block.

> no pointer check in line 843

I suspect the test is bogus to start with; I do not see how aBlockFrame can
manage to be null (in the current impl; that may change if we allow the root to
be non-block).

Comment 40

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

Attachment: diff -w (no whitespace changes) of attachment 111487

Comment 41

[Boris Zbarsky [:bzbarsky]]

Since this patch actually fixes bug 187749, which is blocking1.3b+, requesting
blocking1.3b.... (and it would be nice to put patches in the right bugs instead
of reopening old ones... ;) )

Comment 42

[Asa Dotzler [:asa]]

OK.

Comment 43

[Rick Ju]

dbaron, what you mean by the last patch?  is there more work needed? time
running out...

Comment 44

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

Comment on attachment 111487 [details] [diff] [review]
patch

rs=dbaron

Comment 45

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

The last patch was just the result of applying your patch and doing a diff -w,
which shows only the non-whitespace changes.  It's no good for applying, but
it's easier to read when reviewing a patch like this.

Comment 46

[Asa Dotzler [:asa]]

Comment on attachment 111487 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 47

[Boris Zbarsky [:bzbarsky]]

checked in.

Comment 48

[Johnny Stenback (:jst)]

Um, this aint right, now the code at line 882 rightfully generates a using an
uninitialized variable warning, the code is as follows:

        nsPoint placeholderOffset;
        
        // Just use the placeholder's y-offset
        aHypotheticalBox.mTop = placeholderOffset.y;

There's a placeholderOffset defined in a broader scope here, should the
redefinition of placeholderOffset just be removed here, or is there more to it
than that?

REOPENING.

Comment 49

[Johnny Stenback (:jst)]

Attachment: Patch to remove redeclaration of placeholderOffset

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113726 [details] [diff] [review]
Patch to remove redeclaration of placeholderOffset

yeah, that just needs to go.

Comment 51

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

Comment on attachment 113726 [details] [diff] [review]
Patch to remove redeclaration of placeholderOffset

sr=dbaron too

Comment 52

[Johnny Stenback (:jst)]

Checked in. FIXED.