Bug 58591 - RenderDetailsMarker should belong to shadow element.
Summary: RenderDetailsMarker should belong to shadow element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 56967
  Show dependency treegraph
 
Reported: 2011-04-14 15:56 PDT by Hajime Morrita
Modified: 2011-04-15 14:57 PDT (History)
5 users (show)

See Also:


Attachments
Patch (654.58 KB, patch)
2011-04-14 18:38 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (657.03 KB, patch)
2011-04-15 11:07 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Updated to ToT (657.05 KB, patch)
2011-04-15 11:08 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Windows build fix (657.71 KB, patch)
2011-04-15 11:11 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (653.04 KB, patch)
2011-04-15 14:09 PDT, Hajime Morrita
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-04-14 15:56:00 PDT
This is part of Bug 56967.
At first, We are going to give a shadow element to RenderDetailsMarker.
Comment 1 Hajime Morrita 2011-04-14 18:38:45 PDT
Created attachment 89714 [details]
Patch
Comment 2 Hajime Morrita 2011-04-14 18:45:30 PDT
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 3 Dimitri Glazkov (Google) 2011-04-14 20:22:07 PDT
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.
Comment 4 Build Bot 2011-04-14 20:44:23 PDT
Attachment 89714 [details] did not build on win:
Build output: http://queues.webkit.org/results/8452023
Comment 5 Hajime Morrita 2011-04-15 09:34:02 PDT
> Whoops. I may have broken all this with http://trac.webkit.org/changeset/83922.
Well, I'll take a look.
Comment 6 Hajime Morrita 2011-04-15 11:07:18 PDT
Created attachment 89814 [details]
Patch
Comment 7 Hajime Morrita 2011-04-15 11:08:36 PDT
Created attachment 89815 [details]
Updated to ToT
Comment 8 Hajime Morrita 2011-04-15 11:11:38 PDT
Created attachment 89816 [details]
Windows build fix
Comment 9 Dimitri Glazkov (Google) 2011-04-15 11:40:21 PDT
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.
Comment 10 Hajime Morrita 2011-04-15 14:09:04 PDT
Created attachment 89850 [details]
Patch
Comment 11 Hajime Morrita 2011-04-15 14:11:04 PDT
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 12 Dimitri Glazkov (Google) 2011-04-15 14:18:49 PDT
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.
Comment 13 Hajime Morrita 2011-04-15 14:57:02 PDT
Committed r84039: <http://trac.webkit.org/changeset/84039>