Bug 90079

Summary: [CSS Regions] It should be possible to specify ::BEFORE/AFTER as regions
Product: WebKit Reporter: Andrei Bucur <abucur>
Component: WebCore Misc.Assignee: Andrei Bucur <abucur>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, eric, esprehn, hyatt, stearns, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Andrei Bucur 2012-06-27 08:56:06 PDT
Currently, the implementation assumes the RenderRegions have a direct node attached to them. This is not true in the case of  pseudo-elements renderers.
Comment 1 Andrei Bucur 2012-06-28 07:10:02 PDT
Created attachment 149947 [details]
Patch
Comment 2 Andrei Bucur 2012-08-30 02:20:33 PDT
Created attachment 161429 [details]
Patch
Comment 3 Dave Hyatt 2012-08-30 10:50:34 PDT
Comment on attachment 161429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161429&action=review

Just one question about this one.

> Source/WebCore/rendering/RenderRegion.h:50
> +    // Regions can't have any children.
> +    virtual bool isChildAllowed(RenderObject*, RenderStyle*) const { return false; }

This is weird to me. Eventually we want to turn RenderRegions into RenderBlocks so that they can have ::before and ::after children. In other words, we do want to support children on RenderRegions eventually, so why do we turn that off?
Comment 4 Andrei Bucur 2012-08-30 11:27:32 PDT
(In reply to comment #3)
> (From update of attachment 161429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161429&action=review
> 
> Just one question about this one.
> 
> > Source/WebCore/rendering/RenderRegion.h:50
> > +    // Regions can't have any children.
> > +    virtual bool isChildAllowed(RenderObject*, RenderStyle*) const { return false; }
> 
> This is weird to me. Eventually we want to turn RenderRegions into RenderBlocks so that they can have ::before and ::after children. In other words, we do want to support children on RenderRegions eventually, so why do we turn that off?

That comment was supposed to be read more like "FIXME PLEASE: Regions can't have any children because they are RenderReplaced and every time in RenderObjectChildList someone tries to add a child on generatedContentContainer an ASSERT will trigger... (make me RenderBlock!)"
Comment 5 Dave Hyatt 2012-08-30 11:29:44 PDT
Ah ok, got it. Go ahead and put in a FIXME then and put up a new patch and I will r+ and cq+.
Comment 6 Andrei Bucur 2012-08-30 11:31:53 PDT
(In reply to comment #5)
> Ah ok, got it. Go ahead and put in a FIXME then and put up a new patch and I will r+ and cq+.

Tomorrow, bed time now.
Comment 7 Andrei Bucur 2012-08-31 06:01:49 PDT
Created attachment 161683 [details]
Patch
Comment 8 Andrei Bucur 2012-08-31 06:07:58 PDT
Comment on attachment 161683 [details]
Patch

The patch is a little different. I've realised it's pointless to override isChildAllowed() now and I think this is a better way to do it. Basically if the pseudo-element is a region, the content values are ignored. I think this is inline with the expected behaviour as well. A pseudo-element with flow-from but no content values should still be displayed.
Comment 9 Dave Hyatt 2012-08-31 08:44:53 PDT
Comment on attachment 161683 [details]
Patch

r=me
Comment 10 WebKit Review Bot 2012-08-31 08:49:45 PDT
Comment on attachment 161683 [details]
Patch

Clearing flags on attachment: 161683

Committed r127269: <http://trac.webkit.org/changeset/127269>
Comment 11 WebKit Review Bot 2012-08-31 08:49:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Andrei Bucur 2012-09-18 08:20:24 PDT
*** Bug 92125 has been marked as a duplicate of this bug. ***
Comment 13 Mihai Balan 2012-10-01 01:12:42 PDT
*** Bug 75587 has been marked as a duplicate of this bug. ***