Bug 58591 – RenderDetailsMarker should belong to shadow element.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

This is part of Bug 56967.
At first, We are going to give a shadow element to RenderDetailsMarker.

[Hajime Morrita]

Created attachment 89714 [details]
Patch

[Hajime Morrita]

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.

[Dimitri Glazkov (Google)]

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.

[Build Bot]

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

[Hajime Morrita]

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

[Hajime Morrita]

Created attachment 89814 [details]
Patch

[Hajime Morrita]

Created attachment 89815 [details]
Updated to ToT

[Hajime Morrita]

Created attachment 89816 [details]
Windows build fix

[Dimitri Glazkov (Google)]

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.

[Hajime Morrita]

Created attachment 89850 [details]
Patch

[Hajime Morrita]

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.

[Dimitri Glazkov (Google)]

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.

[Hajime Morrita]

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