365191 - acid2 is broken (2006122706)

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20061227 Minefield/3.0a2pre ID:2006122706 [cairo]

see screenshot

works in 20061227_0057_0121_firefox-3.0a2pre.en-US.win32
fails in 20061227_0619_0645_firefox-3.0a2pre.en-US.win32

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1167209820&maxdate=1167229139

->Bug 243159

Comment 1

[Dão Gottwald [:dao]]

Attachment: another screenshot

This is what I get (and also got with 2006122604, so that could be another bug). When opened in a background tab, Acid2 passes. That is, I can't reproduce attachment 249815 [details].

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20061227 Minefield/3.0a2pre ID:2006122704 [cairo]

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> That is, I can't reproduce attachment 249815 [details].
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20061227
> Minefield/3.0a2pre ID:2006122704 [cairo]

Oops. Of course the nightly is not affected, because the patch for bug 243159 was checked in later that day.

Comment 3

[Bernd]

yes thats row 14 -> table pseudos

Comment 4

[Bill Gianopoulos [:WG9s]]

(In reply to comment #3)
> yes thats row 14 -> table pseudos
> 

Odd. Because it is row 13 (as defined on http://www.webstandards.org/action/acid2/guide/ ) that does not display correctly for me.

Comment 5

[Bernd]

Attachment: patch

Comment 6

[Bernd]

the key is to remove the table special handling at https://bugzilla.mozilla.org/attachment.cgi?id=249850&action=diff#mozilla/layout/base/nsCSSFrameConstructor.cpp_sec6
the rest are consequence of it.

Comment 7

[Bill Gianopoulos [:WG9s]]

(In reply to comment #6)
> the key is to remove the table special handling at
> https://bugzilla.mozilla.org/attachment.cgi?id=249850&action=diff#mozilla/layout/base/nsCSSFrameConstructor.cpp_sec6
> the rest are consequence of it.
> 
I can verify that this patch resolves the issue.  I especially like that the fix is to remove special case code rather than to add more.

Comment 8

[Bernd]

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

Boris I am not confident about the early return in ConstructFrameByDisplayType

Comment 9

[Bill Gianopoulos [:WG9s]]

Bad news!  Although acid 2 renders correctly with my build including this patch, my.yahoo.com does not. :-)

Comment 10

[Bill Gianopoulos [:WG9s]]

Attachment: test that fails with above patch installed

This is a reduced testcase of the issue with the my.yahoo.com page if the above patch is applied.

Comment 11

[Bernd]

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

-    rv = aState.AddChild(aNewOuterFrame, *frameItems, disp, aContent,
-                         outerStyleContext, parentFrame, aAllowOutOfFlow,
-                         aAllowOutOfFlow);
cannot be replaced by a single 

 if (NS_SUCCEEDED(rv) && !aHasPseudoParent) {
+        aFrameItems.AddChild(newFrame);
+      }

Comment 12

[Bernd]

fixed by backout of bug 243159

Comment 13

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20061229 Minefield/3.0a2pre ID:2006122905 [cairo]

VERIFIED

Comment 14

[Boris Zbarsky [:bzbarsky]]

Wait.  So why not use that last patch but use the right AddChild method?  Or is that the plan but you just plan to do more testing in bug 243159 first?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Also, need to add these testcases to a regression test suite of some sort.

Comment 16

[Bernd]

>Or is that the plan but you just plan to do more testing in bug 243159 first?
Boris, thats the plan, comment 10 made it obvious to me that the only description for what is going on here is: "chaltura". A patch of this scope has to pass rtest. I don't want to take the tree as hostage + I don't like to own bugs where every Mozilla fan boy is on CC.