Bug 75306

Summary:

ShadowContentElement query should be able to have fallback element.

Product:

WebKit

Reporter:

Shinya Kawanaka <shinyak>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, dominicc, morrita, shinyak, webkit.review.bot

Priority:

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

Shinya Kawanaka

Reported 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.

Attachments
Patch (16.39 KB, patch) 2012-01-12 04:00 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (10.31 KB, patch) 2012-01-16 20:58 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (10.22 KB, patch) 2012-01-16 23:26 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (14.37 KB, patch) 2012-01-18 01:47 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (14.36 KB, patch) 2012-01-18 01:50 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (15.52 KB, patch) 2012-01-18 05:28 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (15.32 KB, patch) 2012-01-18 20:35 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Dominic Cooney

Comment 1 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?

Dimitri Glazkov (Google)

Comment 2 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.

Hajime Morrita

Comment 3 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.

Shinya Kawanaka

Comment 4 2012-01-12 04:00:50 PST

Created attachment 122208 [details] Patch

Shinya Kawanaka

Comment 5 2012-01-16 20:58:37 PST

Created attachment 122709 [details] Patch

Hajime Morrita

Comment 6 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?

Shinya Kawanaka

Comment 7 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.

Shinya Kawanaka

Comment 8 2012-01-16 23:26:52 PST

Created attachment 122718 [details] Patch

Hajime Morrita

Comment 9 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.

Shinya Kawanaka

Comment 10 2012-01-18 01:47:35 PST

Created attachment 122890 [details] Patch

Shinya Kawanaka

Comment 11 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.

Shinya Kawanaka

Comment 12 2012-01-18 01:50:18 PST

Created attachment 122891 [details] Patch

Shinya Kawanaka

Comment 13 2012-01-18 05:28:33 PST

Created attachment 122904 [details] Patch

Hajime Morrita

Comment 14 2012-01-18 18:04:43 PST

Comment on attachment 122904 [details] Patch Thanks for your hard work! Let's land.

Hajime Morrita

Comment 15 2012-01-18 18:05:21 PST

Comment on attachment 122904 [details] Patch Oops. ChangeLog has wrong diff. Could you update it?