380842 - [regression] table-cell :before/:after pseudo-element set to display:block show as display:inline

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[philippe (part-time)]

Attachment: test case

td:before or td:after {display:block} shows as inline box instead of block box
In the test case the text '[before]' and '[after]' should show on their own line.
works correctly for paragraphs etc. Fails for table-cell (td).

On trunk this regressed between 20070415-04 (OK) and 20070416-04 (fails)
Branch 1.8.1.4 pre also fails the test.
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.4pre) Gecko/20070515 BonEcho/2.0.0.4pre ID:2007051505
Firefox 2.0.0.3 works correctly

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-04-15+04%3A00%3A00&maxdate=2007-04-16+04%3A00%3A00&cvsroot=%2Fcvsroot

suspicion: bug 323656, as this landed on branch as well.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yeah, this is indeed a regression from bug 323656 and a problem on both branches...

I realize it's really late in the cycle, and this correctness regression might not be quite enough to stop ship.  But if something else comes up to make us respin, we should take this too.  And we should definitely take this next cycle.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk patch

Comment 3

[David Baron :dbaron:]

Comment on attachment 265007 [details] [diff] [review]
Trunk patch

>       // XXX IsBlockInside?  (except for the marker bit)
>       if (parentDisplay->IsBlockLevel() ||
>-          parentDisplay->mDisplay == NS_STYLE_DISPLAY_INLINE_BLOCK) {
>+          parentDisplay->mDisplay == NS_STYLE_DISPLAY_INLINE_BLOCK ||
>+          parentDisplay->mDisplay == NS_STYLE_DISPLAY_TABLE_CELL) {

I think you want to add TABLE_CAPTION in addition to TABLE_CELL, no?

(I'd suggest using IsBlockInside like the comment says, but really this code should be gradually moving towards if(PR_TRUE), since the restrictions aren't in CSS2.1 anymore.  So we may as well keep TABLE in there now that we've supported it.)

>Index: layout/reftests/bugs/380842-1.html

Could you also test at least one of the cases where display:block before and after pseudos *are* supposed to change to inlines?

r+sr=dbaron with that

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk patch updated to comments

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to branch version

Comment 6

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.

Comment 7

[David Baron :dbaron:]

(In reply to comment #3)
> >Index: layout/reftests/bugs/380842-1.html
> 
> Could you also test at least one of the cases where display:block before and
> after pseudos *are* supposed to change to inlines?

Actually, since the spec doesn't support doing this anymore, maybe we shouldn't test it (since it's actually incorrect).  Sorry to keep changing my mind here...

Comment 8

[Boris Zbarsky [:bzbarsky]]

I think it's worth testing until we actually decide to change our impl, because changing the rule node code without changing the frame constructor to do {ib} splits in this situation will probably lead to problems that we want to catch, and this test might do that, I think.

Comment 9

[:Gijs (he/him)]

I'd just like to note that this regression broke ChatZilla's faces motifs
(http://chatzilla.hacksrus.com/faces.pl). I realize that we're an obscure
usecase and all, but yeah... It would be wonderful if bz's patch was shipped on
branch as well.

Comment 10

[Jeff Walden [:Waldo]]

That's what I get for flipping the flag before skimming tinderbox checkins:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/reftests/bugs&command=DIFF_FRAMESET&file=reftest.list&rev1=1.66&rev2=1.67&root=/cvsroot

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 265010 [details] [diff] [review]
Merged to branch version

Approved for 1.8.1.5, a=dveditz for release-drivers

Comment 12

[Boris Zbarsky [:bzbarsky]]

I think we should also fix this on the 1.8.0 branch; this is a pretty serious layout regression, and just leaving branch like this is very suboptimal...  Who'd be doing approvals for that branch?  Would it make sense to approve bugs applicable to both branches for both?

Holding off on checking this in pending answers, because landing on both branches at once is a lot less time-consuming than doing it separately...

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 265010 [details] [diff] [review]
Merged to branch version

approved for 1.8.0.13, a=dveditz

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in on both branches.  Thanks for the approvals!

Comment 15

[Adam Guthrie]

Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5pre) Gecko/20070709 BonEcho/2.0.0.5pre and the reftest.

FWIW, I filed bug 387600 for the discrepancy between the reference renderings I see between trunk and branch.