398797 - www.download.com has scrambled text

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100512 Minefield/3.0a9pre ID:2007100512

repro:
open url
look at the middle-top part

result:
text on top of other text

this works fin in FF 2.0.0.7

Comment 1

[Peter van der Woude [:Peter6]]

Attachment: screenshot

Comment 2

[Peter van der Woude [:Peter6]]

regressionwindow:
works in 20070125_0340_firefox-3.0a2pre.en-US.win32
fails in 20070127_1545_firefox-3.0a2pre.en-US.win32

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-01-25+03%3A40%3A00+&maxdate=2007-01-27+15%3A45%3A00+&cvsroot=%2Fcvsroot

Comment 3

[Martijn Wargers (dead)]

Attachment: unminimized testcase

This is where I stopped minimizing.
When I remove the display: inline-block; rule from this unminized testcase, trunk looks the same as branch.
So I suspect this regressed when bug 9458 got fixed. Peter already gave an indication for that, he said on irc it regressed at/around 2007-01-27 (which is when bug 9458 got fixed).

So this bug is likely a tech evangelism issue.

Comment 4

[j.j.]

Attachment: reduced testcase

Opera 9.5b, Safari 3b(win) and IE7 match on this testcase, current trunk is different.

j.j.

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

This probably should live somewhere in layout...

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase2

Jj, thanks for that testcase, I already wondered why it displayed fine in Opera. I guess I should have taken a closer look at that.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase3

The problem is that we aren't letting an inline-block with the properties
   position: relative;
   display: inline-block; 
be a containing block for absolute content, when we should.

However, we do (correctly) let it be a containing block if we remove the "display: inline-block", or if we change "relative" to "absolute".

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch v1

This seems to fix it... not sure if it's exactly correct, or if there's anything more we need to add for this.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch v2

This patch
1. Adds IsBlockInside() calls, to clean up code and add support for inline-block relatively-positioned frames
2. Calls NS_NewFloatingItemWrapperFrame to provide  NS_BLOCK_SPACE_MGR and NS_BLOCK_MARGIN_ROOT flags for these frames.

Note: #2 is admittedly a bit hack-ish, since we're working with relatively-positioned frames and not floats, but it seemed cleaner to me than the alternative ways for getting those flags in there.  Those alternatives being...
  a) adding a "flags" argument to NS_NewRelativeItemWrapperFrame
  b) casting newFrame as a nsBlockFrame so I could call SetFlags() on it.

Do let me know if these (or other) alternatives are preferable to calling NS_NewFloatingItemWrapperFrame, though.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 285300 [details] [diff] [review]
patch v2

>Index: nsCSSFrameConstructor.cpp
>+        newFrame = NS_NewFloatingItemWrapperFrame(mPresShell, aStyleContext);

I'd prefer either of the other alternatives to this.  Adding an arg to NS_NewRelativeItemWrapperFrame is probably cleanest, but if you want to use a direct SetFlags call that's fine too.  There's no need to cast newFrame before calling SetFlags(), by the way.

r+sr=bzbarsky with that changed.

Add reftests for this when you land, if possible?

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: patch v3

Ok, this version adds an arg to NS_NewRelativeItemWrapperFrame.

Also including 4 reftests, which assert that relative + absolute-positioned divs are containers for absolute content, whether they're "display: inline-block" or not.  

Of these tests, only test "398797-1a.html" (relative + inline-block) failed before this patch, whereas tests b, c, and d all passed. I'm including b/c/d for thoroughness, though.

(These tests all pass post-patching, of course.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 285401 [details] [diff] [review]
patch v3

s/red/yellow/ throughout (because red means fail, usually) and looks great.

Comment 13

[Daniel Holbert [:dholbert]]

patch v3 checked in, with s/red/yellow in the reftests.

Checking in base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1420; previous revision: 1.1419
done
Checking in generic/nsHTMLParts.h;
/cvsroot/mozilla/layout/generic/nsHTMLParts.h,v  <--  nsHTMLParts.h
new revision: 3.149; previous revision: 3.148
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/398797-1-ref.html,v
done
Checking in reftests/bugs/398797-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/398797-1-ref.html,v  <--  398797-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/398797-1a.html,v
done
Checking in reftests/bugs/398797-1a.html;
/cvsroot/mozilla/layout/reftests/bugs/398797-1a.html,v  <--  398797-1a.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/398797-1b.html,v
done
Checking in reftests/bugs/398797-1b.html;
/cvsroot/mozilla/layout/reftests/bugs/398797-1b.html,v  <--  398797-1b.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/398797-1c.html,v
done
Checking in reftests/bugs/398797-1c.html;
/cvsroot/mozilla/layout/reftests/bugs/398797-1c.html,v  <--  398797-1c.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/398797-1d.html,v
done
Checking in reftests/bugs/398797-1d.html;
/cvsroot/mozilla/layout/reftests/bugs/398797-1d.html,v  <--  398797-1d.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/398797-style.css,v
done
Checking in reftests/bugs/398797-style.css;
/cvsroot/mozilla/layout/reftests/bugs/398797-style.css,v  <--  398797-style.css
initial revision: 1.1
done
Checking in reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.188; previous revision: 1.187
done

Comment 14

[Daniel Holbert [:dholbert]]

Reftest fails on mac for some reason -- the reference case doesn't have a large enough yellow background rectangle on the "bar" div.

Marked reftest as being random on cocoa for now, to fix tinderbox orangeness.  I'm investigating, and I'll either fix the issue or file a new bug for it.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to comment #14)
> Reftest fails on mac for some reason

Windows too, actually.

> I'm investigating, and I'll either fix the issue or file a new bug for it.

Done -- filed bug 402918.

In the meantime, I set "float: left" on the reference case's <span>, and that works around the problem.  Added a comment to explain this and to point to bug 402918.

Comment 16

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112805 Minefield/3.0b2pre, download.com site renders fine.