Bug 75306

Summary: ShadowContentElement query should be able to have fallback element.
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, morrita, shinyak, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 75302    
Bug Blocks: 75930, 76262    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Shinya Kawanaka 2011-12-28 05:34:00 PST
In the case issue 75302 is landed, we would like to have fallback element when query does not matches any elements.
Currently ShadowContentElement is not allowed to have children, but it can be used for fallback elements.
Comment 1 Dominic Cooney 2012-01-04 18:41:21 PST
When is the right time to move ShadowContentElement out of dom/ and into html/(somewhere) and make it a subtype of HTMLElement instead of Element?
Comment 2 Dimitri Glazkov (Google) 2012-01-04 19:31:18 PST
(In reply to comment #1)
> When is the right time to move ShadowContentElement out of dom/ and into html/(somewhere) and make it a subtype of HTMLElement instead of Element?

I think we can do this now, right? There's no visible effect at the moment.
Comment 3 Hajime Morrita 2012-01-05 03:00:24 PST
(In reply to comment #2)
> (In reply to comment #1)
> > When is the right time to move ShadowContentElement out of dom/ and into html/(somewhere) and make it a subtype of HTMLElement instead of Element?
> 
> I think we can do this now, right? There's no visible effect at the moment.
No, there is no visible effect. So we can turn this into a subclass of HTMLElement.
BTW apparently the comment was jumped from some other bug, it looks.
Comment 4 Shinya Kawanaka 2012-01-12 04:00:50 PST
Created attachment 122208 [details]
Patch
Comment 5 Shinya Kawanaka 2012-01-16 20:58:37 PST
Created attachment 122709 [details]
Patch
Comment 6 Hajime Morrita 2012-01-16 21:32:07 PST
Comment on attachment 122709 [details]
Patch

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

Could you test some nested cases?

> Source/WebCore/ChangeLog:24
> +          Changed the order of calculation a bit.

Please explain the change concretely.

> Source/WebCore/dom/NodeRenderingContext.cpp:86
> +            if (!shadowContentElement->inclusions() || !shadowContentElement->inclusions()->first()) {

Can inclusions() be null? Is it OK to take the case as a fallback?
Comment 7 Shinya Kawanaka 2012-01-16 23:26:30 PST
(In reply to comment #6)
> (From update of attachment 122709 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122709&action=review
> 
> Could you test some nested cases?
> 
> > Source/WebCore/ChangeLog:24
> > +          Changed the order of calculation a bit.
> 
> Please explain the change concretely.

Done.

> 
> > Source/WebCore/dom/NodeRenderingContext.cpp:86
> > +            if (!shadowContentElement->inclusions() || !shadowContentElement->inclusions()->first()) {
> 
> Can inclusions() be null? Is it OK to take the case as a fallback?

Ah, inclusions() won't be null.
Comment 8 Shinya Kawanaka 2012-01-16 23:26:52 PST
Created attachment 122718 [details]
Patch
Comment 9 Hajime Morrita 2012-01-17 17:43:50 PST
Comment on attachment 122718 [details]
Patch

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

Basically looks fine. Could you add some more tests?

> Source/WebCore/dom/NodeRenderingContext.cpp:85
> +            if (!shadowContentElement->inclusions()->first()) {

Could you make this conditional a method on ShadowContentElement? hasInclusion() or something?

> LayoutTests/fast/dom/shadow/shadow-contents-fallback.html:1
> +<!DOCTYPE html>

Oops, I meant to talk about content inside shadow inside content.
Such pattern should hit tricky flattening path.
Comment 10 Shinya Kawanaka 2012-01-18 01:47:35 PST
Created attachment 122890 [details]
Patch
Comment 11 Shinya Kawanaka 2012-01-18 01:48:07 PST
(In reply to comment #9)
> (From update of attachment 122718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122718&action=review
> 
> Basically looks fine. Could you add some more tests?
> 
> > Source/WebCore/dom/NodeRenderingContext.cpp:85
> > +            if (!shadowContentElement->inclusions()->first()) {
> 
> Could you make this conditional a method on ShadowContentElement? hasInclusion() or something?

Done.

> > LayoutTests/fast/dom/shadow/shadow-contents-fallback.html:1
> > +<!DOCTYPE html>
> 
> Oops, I meant to talk about content inside shadow inside content.
> Such pattern should hit tricky flattening path.

I've added a few more tests about it.
Comment 12 Shinya Kawanaka 2012-01-18 01:50:18 PST
Created attachment 122891 [details]
Patch
Comment 13 Shinya Kawanaka 2012-01-18 05:28:33 PST
Created attachment 122904 [details]
Patch
Comment 14 Hajime Morrita 2012-01-18 18:04:43 PST
Comment on attachment 122904 [details]
Patch

Thanks for your hard work! Let's land.
Comment 15 Hajime Morrita 2012-01-18 18:05:21 PST
Comment on attachment 122904 [details]
Patch

Oops. ChangeLog has wrong diff. Could you update it?
Comment 16 Shinya Kawanaka 2012-01-18 20:35:42 PST
Created attachment 123061 [details]
Patch
Comment 17 WebKit Review Bot 2012-01-18 23:06:12 PST
Comment on attachment 123061 [details]
Patch

Clearing flags on attachment: 123061

Committed r105387: <http://trac.webkit.org/changeset/105387>
Comment 18 WebKit Review Bot 2012-01-18 23:06:18 PST
All reviewed patches have been landed.  Closing bug.