Bug 106643

Created attachment 182298 [details] Patch

Comment on attachment 182298 [details] Patch Attachment 182298 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15806300

Comment on attachment 182298 [details] Patch Attachment 182298 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15806301

Comment on attachment 182298 [details] Patch Attachment 182298 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15801385

Comment on attachment 182298 [details] Patch Attachment 182298 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15805365 New failing tests: accessibility/aria-checkbox-checked.html animations/3d/matrix-transform-type-animation.html http/tests/appcache/cyrillic-uri.html accessibility/anchor-linked-anonymous-block-crash.html animations/animation-add-events-in-handler.html http/tests/appcache/deferred-events-delete-while-raising.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html http/tests/appcache/deferred-events-delete-while-raising-timer.html accessibility/aria-describedby-on-input.html http/tests/appcache/crash-when-navigating-away-then-back.html animations/animation-border-overflow.html animations/animation-direction-alternate-reverse.html accessibility/accessibility-object-detached.html http/tests/appcache/abort-cache-ondownloading-manifest-404.html http/tests/appcache/credential-url.html http/tests/appcache/access-via-redirect.php animations/3d/change-transform-in-end-event.html accessibility/aria-controls-with-tabs.html http/tests/appcache/destroyed-frame.html animations/animation-controller-drt-api.html animations/3d/transform-perspective.html accessibility/accessibility-node-reparent.html http/tests/appcache/deferred-events.html animations/3d/state-at-end-event-transform.html animations/animation-direction-normal.html animations/3d/transform-origin-vs-functions.html accessibility/accessibility-node-memory-management.html animations/animation-css-rule-types.html accessibility/adjacent-continuations-cause-assertion-failure.html

Comment on attachment 182298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182298&action=review > Source/WebCore/css/StyleScopeResolver.cpp:219 > +#if ENABLE(SHADOW_DOM) > if (!ScopeContentDistribution::hasShadowElement(shadowRoot)) > break; > +#else > + break; > +#endif Factor into an inline function to reduce the number of ifdefs in this file?

Created attachment 182946 [details] Patch

Created attachment 182947 [details] Patch

Comment on attachment 182947 [details] Patch Attachment 182947 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15908331

Comment on attachment 182947 [details] Patch Attachment 182947 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15907268

Comment on attachment 182947 [details] Patch Attachment 182947 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15905347 New failing tests: fast/dom/shadow/composed-shadow-tree-walker.html editing/shadow/insertorderedlist-crash.html editing/shadow/selection-of-orphan-shadowroot.html fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html editing/shadow/select-contenteditable-shadowhost.html accessibility/corresponding-control-deleted-crash.html fast/dom/shadow/athost-atrules.html editing/shadow/doubleclick-on-meter-in-shadow-crash.html editing/shadow/adjusting-editing-boundary-with-table-in-shadow.html editing/shadow/breaking-editing-boundary-with-table.html fast/dom/shadow/compare-document-position.html fast/dom/shadow/composed-shadow-tree-walker-shadow-reprojection.html fast/css/style-scoped/style-scoped-apply-author-styles.html fast/dom/shadow/adopt-node-with-shadow-root.html editing/shadow/rightclick-on-meter-in-shadow-crash.html editing/shadow/shadow-selection-not-exported.html editing/shadow/contenteditable-propagation-at-shadow-boundary.html fast/block/block-remove-child-delete-line-box-crash.html fast/dom/shadow/access-key.html fast/css/style-scoped/style-scoped-in-shadow.html editing/shadow/bold-twice-in-shadow.html fast/dom/HTMLTemplateElement/cycles-in-shadow.html editing/shadow/breaking-editing-boundaries-2.html fast/css/style-scoped/style-scoped-with-important-rule.html editing/shadow/selection-of-shadowroot.html editing/shadow/execcommand-indent-in-shadow.html fast/css/style-scoped/style-scoped-nested.html editing/shadow/breaking-editing-boundaries.html editing/shadow/delete-characters-in-distributed-node-crash.html editing/shadow/compare-positions-in-nested-shadow.html

Comment on attachment 182947 [details] Patch Attachment 182947 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15905367 New failing tests: fast/dom/shadow/composed-shadow-tree-walker.html editing/shadow/insertorderedlist-crash.html editing/shadow/selection-of-orphan-shadowroot.html fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html editing/shadow/select-contenteditable-shadowhost.html accessibility/corresponding-control-deleted-crash.html fast/dom/shadow/athost-atrules.html editing/shadow/doubleclick-on-meter-in-shadow-crash.html editing/shadow/adjusting-editing-boundary-with-table-in-shadow.html editing/shadow/breaking-editing-boundary-with-table.html fast/dom/shadow/compare-document-position.html fast/dom/shadow/composed-shadow-tree-walker-shadow-reprojection.html fast/css/style-scoped/style-scoped-apply-author-styles.html fast/dom/shadow/adopt-node-with-shadow-root.html editing/shadow/rightclick-on-meter-in-shadow-crash.html editing/shadow/shadow-selection-not-exported.html editing/shadow/contenteditable-propagation-at-shadow-boundary.html fast/block/block-remove-child-delete-line-box-crash.html fast/dom/shadow/access-key.html fast/css/style-scoped/style-scoped-in-shadow.html editing/shadow/bold-twice-in-shadow.html fast/dom/HTMLTemplateElement/cycles-in-shadow.html editing/shadow/breaking-editing-boundaries-2.html fast/css/style-scoped/style-scoped-with-important-rule.html editing/shadow/selection-of-shadowroot.html editing/shadow/execcommand-indent-in-shadow.html fast/css/style-scoped/style-scoped-nested.html editing/shadow/breaking-editing-boundaries.html editing/shadow/delete-characters-in-distributed-node-crash.html editing/shadow/compare-positions-in-nested-shadow.html

Comment on attachment 182947 [details] Patch Attachment 182947 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15926548

Created attachment 183960 [details] Patch

This one is almost ready.

Comment on attachment 183960 [details] Patch Attachment 183960 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16040546

Comment on attachment 183960 [details] Patch Attachment 183960 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16038511 New failing tests: fast/dom/shadow/composed-shadow-tree-walker.html editing/shadow/insertorderedlist-crash.html editing/shadow/selection-of-orphan-shadowroot.html fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html editing/shadow/select-contenteditable-shadowhost.html accessibility/corresponding-control-deleted-crash.html fast/dom/click-method-on-html-element.html fast/dom/shadow/athost-atrules.html editing/shadow/doubleclick-on-meter-in-shadow-crash.html editing/shadow/adjusting-editing-boundary-with-table-in-shadow.html editing/shadow/breaking-editing-boundary-with-table.html fast/dom/shadow/compare-document-position.html fast/dom/shadow/composed-shadow-tree-walker-shadow-reprojection.html fast/css/style-scoped/style-scoped-apply-author-styles.html fast/dom/shadow/adopt-node-with-shadow-root.html editing/shadow/rightclick-on-meter-in-shadow-crash.html editing/shadow/shadow-selection-not-exported.html editing/shadow/contenteditable-propagation-at-shadow-boundary.html fast/css/style-scoped/style-scoped-with-important-rule.html fast/dom/shadow/access-key.html fast/css/style-scoped/style-scoped-in-shadow.html editing/shadow/bold-twice-in-shadow.html fast/dom/HTMLTemplateElement/cycles-in-shadow.html editing/shadow/breaking-editing-boundaries-2.html editing/shadow/selection-of-shadowroot.html editing/shadow/execcommand-indent-in-shadow.html fast/css/style-scoped/style-scoped-nested.html editing/shadow/breaking-editing-boundaries.html editing/shadow/delete-characters-in-distributed-node-crash.html editing/shadow/compare-positions-in-nested-shadow.html

Comment on attachment 183960 [details] Patch Attachment 183960 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16041592

Created attachment 184099 [details] Patch

Created attachment 184117 [details] Fixing Mac and Win build

Created attachment 184129 [details] Patch

Comment on attachment 184129 [details] Patch Attachment 184129 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16073128 New failing tests: fullscreen/full-screen-render-inline.html inspector/elements/shadow-root.html svg/custom/use-multiple-on-nested-disallowed-font.html

Comment on attachment 184129 [details] Patch Attachment 184129 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16063534 New failing tests: fullscreen/full-screen-render-inline.html inspector/elements/shadow-root.html svg/custom/use-multiple-on-nested-disallowed-font.html

Comment on attachment 184129 [details] Patch Attachment 184129 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16075310 New failing tests: svg/custom/use-multiple-on-nested-disallowed-font.html inspector/elements/shadow-root.html fullscreen/full-screen-render-inline.html

Created attachment 184437 [details] Patch

Created attachment 184454 [details] Patch

Attachment 184454 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fullscreen/full-screen-render-inline-expected.png', u'LayoutTests/fullscreen/full-screen-render-inline-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/mac/fast/html/details-nested-2-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/StyleScopeResolver.cpp', u'Source/WebCore/css/StyleScopeResolver.h', u'Source/WebCore/dom/AncestorChainWalker.cpp', u'Source/WebCore/dom/ComposedShadowTreeWalker.cpp', u'Source/WebCore/dom/ComposedShadowTreeWalker.h', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/ElementShadow.cpp', u'Source/WebCore/dom/ElementShadow.h', u'Source/WebCore/dom/NodeRenderingTraversal.cpp', u'Source/WebCore/dom/NodeRenderingTraversal.h', u'Source/WebCore/dom/ShadowRoot.cpp', u'Source/WebCore/dom/ShadowRoot.h', u'Source/WebCore/html/HTMLDetailsElement.cpp', u'Source/WebCore/html/HTMLDetailsElement.h', u'Source/WebCore/html/HTMLInputElement.cpp', u'Source/WebCore/html/shadow/ContentDistributor.cpp', u'Source/WebCore/html/shadow/ContentDistributor.h', u'Source/WebCore/html/shadow/ContentSelectorQuery.cpp', u'Source/WebCore/html/shadow/ContentSelectorQuery.h', u'Source/WebCore/html/shadow/HTMLContentElement.h', u'Source/WebCore/html/shadow/InsertionPoint.cpp', u'Source/WebCore/html/shadow/InsertionPoint.h', u'Source/WebCore/html/shadow/SelectRuleFeatureSet.cpp', u'Source/WebCore/html/shadow/SelectRuleFeatureSet.h', u'Source/WebCore/svg/SVGUseElement.cpp', u'Source/WebCore/svg/SVGUseElement.h', u'Source/WebCore/testing/Internals.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in']" exit_code: 1 LayoutTests/fullscreen/full-screen-render-inline-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

There seems to be a lot of confusing code and complexity here purely because of <details> elements Shadow DOM dependency. It might be better to move it off the Shadow DOM first.

Comment on attachment 184454 [details] Patch Attachment 184454 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16077694 New failing tests: fullscreen/full-screen-render-inline.html

(In reply to comment #28) > There seems to be a lot of confusing code and complexity here purely because of <details> elements Shadow DOM dependency. It might be better to move it off the Shadow DOM first. Shadow DOM is the right way to implement this. I don't understand why we would be forcing ourselves to churn this from Shadow DOM implementation to some one-off hack and back to it again?

Created attachment 184617 [details] Patch

This "cure" seems worse than the disease. This patch reads like an in-place fork of WebCore.

(In reply to comment #28) > There seems to be a lot of confusing code and complexity here purely because of <details> elements Shadow DOM dependency. It might be better to move it off the Shadow DOM first. Or we could just accept the fact that Shadow DOM is part of WebCore and skip all this make-work.

Created attachment 184665 [details] Patch

The last patch tried to make it clear which part is for non-SHADOW_DOM <details> implementations. I hope I could make it work even with SHADOW_DOM flag. But it's so hard - just like it is hard to make <input> work with shadow DOM.

Comment on attachment 184665 [details] Patch Attachment 184665 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16121136

Created attachment 184679 [details] Patch

> > Or we could just accept the fact that Shadow DOM is part of WebCore and skip all this make-work. Actually, this is what Antti did at Bug 103208. Probably I should go that way.

Created attachment 184698 [details] WIP

(In reply to comment #33) > Or we could just accept the fact that Shadow DOM is part of WebCore and skip all this make-work. No such decision has been made. Shadow DOM is currently strictly an experiment. If you want to make that argument please do it elsewhere.

Comment on attachment 184698 [details] WIP Attachment 184698 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16118294

Comment on attachment 184698 [details] WIP Attachment 184698 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16119266 New failing tests: fast/dom/shadow/composed-shadow-tree-walker.html editing/shadow/insertorderedlist-crash.html editing/shadow/selection-of-orphan-shadowroot.html fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html editing/shadow/select-contenteditable-shadowhost.html accessibility/corresponding-control-deleted-crash.html fast/dom/shadow/athost-atrules.html editing/shadow/doubleclick-on-meter-in-shadow-crash.html editing/shadow/adjusting-editing-boundary-with-table-in-shadow.html editing/shadow/breaking-editing-boundary-with-table.html fast/dom/HTMLTemplateElement/cycles-in-shadow.html fast/dom/shadow/composed-shadow-tree-walker-shadow-reprojection.html fast/css/style-scoped/style-scoped-apply-author-styles.html fast/dom/shadow/adopt-node-with-shadow-root.html editing/shadow/rightclick-on-meter-in-shadow-crash.html editing/shadow/shadow-selection-not-exported.html editing/shadow/contenteditable-propagation-at-shadow-boundary.html fast/css/style-scoped/style-scoped-with-important-rule.html fast/dom/shadow/access-key.html fast/css/style-scoped/style-scoped-in-shadow.html editing/shadow/bold-twice-in-shadow.html fast/dom/shadow/compare-document-position.html editing/shadow/breaking-editing-boundaries-2.html editing/shadow/selection-of-shadowroot.html editing/shadow/execcommand-indent-in-shadow.html fast/css/style-scoped/style-scoped-nested.html editing/shadow/breaking-editing-boundaries.html fast/dom/shadow/content-after-style.html editing/shadow/delete-characters-in-distributed-node-crash.html editing/shadow/compare-positions-in-nested-shadow.html

Comment on attachment 184698 [details] WIP Attachment 184698 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16120274

Comment on attachment 184698 [details] WIP Attachment 184698 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16120284

Comment on attachment 184698 [details] WIP Attachment 184698 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16123290

(In reply to comment #40) > (In reply to comment #33) > > Or we could just accept the fact that Shadow DOM is part of WebCore and skip all this make-work. > > No such decision has been made. Shadow DOM is currently strictly an experiment. If you want to make that argument please do it elsewhere. Can you help me understand this a bit more? I think you're saying that there's a process that takes a new feature in WebKit from some stage that called "experiment" to another stage where it's not. As far as I know, we don't have any process like that. We just announce a feature on webkit-dev, have a spirited discussion, and then, if everyone is content, we build it (http://www.webkit.org/coding/adding-features.html) -- which is what happened with Shadow DOM. Would you like me to start another discussion on webkit-dev to baptize Shadow DOM somehow? Should we have a modification to our typical process to clarify this?

We use feature flags for non-core (things that not everyone ships) and experimental (work-in-progress standards, things no one ships) features. This automatically prevents orthogonal features and the core from becoming dependent on them. Had this been done properly in the first place <details> could not depend on the Shadow DOM infrastructure now. If you want to argue that Shadow DOM is a core feature that should not be behind a feature flag then that is definitely a new discussion.

I think it's appropriate to use ENABLE macros to hide APIs from the web. However, I don't see any reason why we shouldn't use these facilities internally in the engine. The patch just reads like you're angry and want Shadow DOM to get off your lawn and you don't care about making a mess of WebCore in the process. For better or worse, we share this lawn and we should worth together to make it beautiful rather than working at cross purposes.

Comment on attachment 184698 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=184698&action=review > Source/WebCore/ChangeLog:14 > + Since <detail> and <summary> are relying on this mechanism, Nit: <detail> => <details> > Source/WebCore/ChangeLog:15 > + they are now disabled when ENABLE(SHADOW_DOM) is not given. Is this correct? Because in the patch it looks like ShadowTreeWalker is there to support details/summary when SHADOW_DOM isn’t enabled. > Source/WebCore/WebCore.exp.in:2531 > +__ZN7WebCore18ContentDistributor22ensureSelectFeatureSetEPNS_13ElementShadowE These symbols are kept in sorted order, I think; this should be sorted.

Comment on attachment 184698 [details] WIP We're removing Shadow DOM.