Bug 58591

Created attachment 89714 [details] Patch

I'd love to the feedback from Luiz. Although this change does keep basic behavior as is, but it isn't exactly same. Main behavioral change is: - marker position and size: Now the size is given by html.css. - The marker is now a child of summary. So details-position.html is rendered much differently. Although I tried to keep original, it's a bit different. I'm not sure if these differences matter.

Comment on attachment 89714 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89714&action=review > Source/WebCore/dom/Element.h:56 > +enum AttachShadowSubtree { > + AttachShadowSubtreeBeforeLightChildren, > + AttachShadowSubtreeAfterLightChildren > +}; Whoops. I may have broken all this with http://trac.webkit.org/changeset/83922.

Attachment 89714 [details] did not build on win: Build output: http://queues.webkit.org/results/8452023

> Whoops. I may have broken all this with http://trac.webkit.org/changeset/83922. Well, I'll take a look.

Created attachment 89814 [details] Patch

Created attachment 89815 [details] Updated to ToT

Created attachment 89816 [details] Windows build fix

Comment on attachment 89816 [details] Windows build fix View in context: https://bugs.webkit.org/attachment.cgi?id=89816&action=review overall, looks great. Except I think you need to really simplify the plumbing for rendering of children vs. shadow tree. We are getting rid of this soon anyway. > Source/WebCore/dom/Node.h:96 > +enum ShadowBoundaryType { > + ShadowBoundaryLight, > + ShadowBoundaryShadowExclusive, > + ShadowBoundaryShadowBeforeLight, > + ShadowBoundaryShadowAfterLight > +}; Why do we have so many types? These are all hacks that we will eventually remove, and we only use two of these types. Also, this needs better name. The enum describes how shadow nodes are renderered in relation to child nodes. So maybe ShadowRenderersPositions { BeforeChildren, AfterChildren }? > Source/WebCore/dom/Node.h:217 > + virtual ShadowBoundaryType shadowBoundaryType() const { return ShadowBoundaryLight; } I think this is backwards. We should just have a virtual method on a host element, saying whether or not children should be rendered (false by default). Then let HTMLSummaryElement set it to true. No need to tweak ShadowRoot at all. > Source/WebCore/html/HTMLSummaryElement.cpp:60 > +HTMLDetailsElement* HTMLSummaryElement::hostDetails() const should be detailsElement. > Source/WebCore/html/shadow/DetailsControlElements.cpp:51 > + if (result) > + result->setAnimatableStyle(style); Don't need this. Style is set just after createRenderer in Node::createRendererAndStyle. > Source/WebCore/html/shadow/DetailsControlElements.cpp:66 > +HTMLSummaryElement* DetailsMarkerControl::hostSummary() summaryElement. > Source/WebCore/html/shadow/DetailsControlElements.h:31 > +#ifndef DetailsControlElements_h Should just be DetailsMarkerControl. Don't follow the example of MediaControlElements -- it's an antipattern. All these classes should be split out into separate files.

Created attachment 89850 [details] Patch

Hi, Dimitri, thank you for reviewing! I update the patch. > > Source/WebCore/dom/Node.h:96 > > +enum ShadowBoundaryType { > > + ShadowBoundaryLight, > > + ShadowBoundaryShadowExclusive, > > + ShadowBoundaryShadowBeforeLight, > > + ShadowBoundaryShadowAfterLight > > +}; > > Why do we have so many types? These are all hacks that we will eventually remove, and we only use two of these types. > > Also, this needs better name. The enum describes how shadow nodes are renderered in relation to > child nodes. So maybe ShadowRenderersPositions { BeforeChildren, AfterChildren }? > > > Source/WebCore/dom/Node.h:217 > > + virtual ShadowBoundaryType shadowBoundaryType() const { return ShadowBoundaryLight; } > > I think this is backwards. We should just have a virtual method on a host element, saying whether or not children should be rendered (false by default). Then let HTMLSummaryElement set it to true. No need to tweak ShadowRoot at all. > Removed these and introduced Node::canHaveLightChildRendererWithShadow() for a replacement. > > Source/WebCore/html/HTMLSummaryElement.cpp:60 > > +HTMLDetailsElement* HTMLSummaryElement::hostDetails() const > > should be detailsElement. Renamed. > > > Source/WebCore/html/shadow/DetailsControlElements.cpp:51 > > + if (result) > > + result->setAnimatableStyle(style); > > Don't need this. Style is set just after createRenderer in Node::createRendererAndStyle. Removed. > > > Source/WebCore/html/shadow/DetailsControlElements.cpp:66 > > +HTMLSummaryElement* DetailsMarkerControl::hostSummary() > > summaryElement. Renamed. > > > Source/WebCore/html/shadow/DetailsControlElements.h:31 > > +#ifndef DetailsControlElements_h > > Should just be DetailsMarkerControl. Don't follow the example of MediaControlElements -- it's an antipattern. All these classes should be split out into separate files. Renamed the file.

Comment on attachment 89850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89850&action=review Ok with nits. > Source/WebCore/ChangeLog:10 > + Note thtat marker size is given via style for -webkit-details-marker pseudo class. thtat -> that > Source/WebCore/html/HTMLDetailsElement.cpp:93 > + if (oldSummary && oldSummary->parentNodeForRenderingAndStyle()) { > + oldSummary->detach(); > + oldSummary->attach(); > + } > + > + if (refreshRenderer == RefreshRendererAllowed) { > + m_mainSummary->detach(); > + m_mainSummary->attach(); > + } I cringe seeing this code, but I think we can tackle this later. > Source/WebCore/html/HTMLDetailsElement.cpp:145 > + setAttribute(openAttr, m_isOpen ? String() : String("")); use nullAtom and emptyAtom > Source/WebCore/html/HTMLSummaryElement.cpp:56 > + // Since HTMLDetailsElement::mainSummary() is possibly not ready at this time, we should make attach() lazy. We should always attach lazily. No need for this comment.

Committed r84039: <http://trac.webkit.org/changeset/84039>