Closed Bug 380842 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9alpha5

People

(Reporter: phiw2, Assigned: bzbarsky)

References

Details

(4 keywords)

Attachments

(3 files, 1 obsolete file)

Attached file 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.
Summary: [regression] table-cell before/after set to display:block show as inline → [regression] table-cell :before/:after pseudo-element set to display:block show as display:inline
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.
Blocks: 323656
Component: Layout → Style System (CSS)
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.12?
OS: Mac OS X → All
QA Contact: layout → style-system
Hardware: Macintosh → All
Attached patch Trunk patch (obsolete) — Splinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #265007 - Flags: superreview?(dbaron)
Attachment #265007 - Flags: review?(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
Attachment #265007 - Flags: superreview?(dbaron)
Attachment #265007 - Flags: superreview+
Attachment #265007 - Flags: review?(dbaron)
Attachment #265007 - Flags: review+
Attachment #265007 - Attachment is obsolete: true
Attachment #265010 - Flags: approval1.8.1.5?
Attachment #265010 - Flags: approval1.8.1.4?
Attachment #265010 - Flags: approval1.8.0.13?
Attachment #265010 - Flags: approval1.8.0.12?
Checked in on trunk.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha5
(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...
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.
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.
Flags: in-testsuite?
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Attachment #265010 - Flags: approval1.8.1.4?
Attachment #265010 - Flags: approval1.8.0.12?
Comment on attachment 265010 [details] [diff] [review]
Merged to branch version

Approved for 1.8.1.5, a=dveditz for release-drivers
Attachment #265010 - Flags: approval1.8.1.5? → approval1.8.1.5+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
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 on attachment 265010 [details] [diff] [review]
Merged to branch version

approved for 1.8.0.13, a=dveditz
Attachment #265010 - Flags: approval1.8.0.13? → approval1.8.0.13+
Checked in on both branches.  Thanks for the approvals!
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.