Bug 50309 - HTML5 <details> and <summary> initial implementation
Summary: HTML5 <details> and <summary> initial implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Luiz Agostini
URL:
Keywords: HTML5
Depends on:
Blocks: 32934
  Show dependency treegraph
 
Reported: 2010-11-30 21:42 PST by Luiz Agostini
Modified: 2010-12-05 21:11 PST (History)
9 users (show)

See Also:


Attachments
patch (65.94 KB, patch)
2010-11-30 21:50 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch (65.90 KB, patch)
2010-12-02 15:52 PST, Luiz Agostini
darin: review-
Details | Formatted Diff | Diff
patch (48.28 KB, patch)
2010-12-02 20:40 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch (48.82 KB, patch)
2010-12-02 21:49 PST, Luiz Agostini
darin: review+
Details | Formatted Diff | Diff
checking build systems before landing. please do not review. (49.70 KB, patch)
2010-12-05 15:59 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luiz Agostini 2010-11-30 21:42:12 PST
HTML5 <details> and <summary> elements initial implementation. 

The main objective is to add the files for html elements and renderers, and to get rid of build system issues in future patches.
Comment 1 Luiz Agostini 2010-11-30 21:50:07 PST
Created attachment 75248 [details]
patch
Comment 2 WebKit Review Bot 2010-11-30 22:33:08 PST
Attachment 75248 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6678001
Comment 4 Luiz Agostini 2010-12-02 15:52:52 PST
Created attachment 75427 [details]
patch
Comment 5 Darin Adler 2010-12-02 17:48:55 PST
Comment on attachment 75427 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75427&action=review

I don’t want to see these renderer classes added until we are sure we need them. Are you certain you will need all three?

> WebCore/html/HTMLDetailsElement.cpp:55
> +bool HTMLDetailsElement::open() const
> +{
> +    return !getAttribute(openAttr).isNull();
> +}
> +
> +void HTMLDetailsElement::setOpen(bool value)
> +{
> +    setAttribute(openAttr, value ? "" : 0);
> +}

These functions are not needed.

> WebCore/html/HTMLDetailsElement.h:31
> +    static PassRefPtr<HTMLDetailsElement> create(Document* document);

We should not add this function unless we have a callsite that needs it.

> WebCore/html/HTMLDetailsElement.h:34
> +    bool open() const;
> +    void setOpen(bool);

These functions are not needed.

> WebCore/html/HTMLDetailsElement.idl:23
> +        attribute boolean open;

This should use [Reflect] so we don’t need open and canOpen functions in the C++ class.

> WebCore/html/HTMLTagNames.in:118
> -summary interfaceName=HTMLElement
> +summary

This change is incorrect. See the HTML5 specification. It specifically says the DOM interface uses HTMLElement. The class HTMLSummaryElement is not needed.
Comment 6 Luiz Agostini 2010-12-02 19:07:08 PST
(In reply to comment #5)
> (From update of attachment 75427 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75427&action=review
> 
> I don’t want to see these renderer classes added until we are sure we need them. Are you certain you will need all three?

The plan is to RenderDetails to use RenderBlock::layoutSpecialExcludedChild() to set the position of its first RenderSummary child. And this first RenderSummary creates a RenderDetailsMarker in a way similar to what is done by RenderListItem.

What do you think?
Comment 7 Luiz Agostini 2010-12-02 20:40:08 PST
Created attachment 75459 [details]
patch
Comment 8 Build Bot 2010-12-02 21:31:41 PST
Attachment 75459 [details] did not build on win:
Build output: http://queues.webkit.org/results/6727025
Comment 9 Luiz Agostini 2010-12-02 21:49:10 PST
Created attachment 75464 [details]
patch
Comment 10 Build Bot 2010-12-02 22:39:14 PST
Attachment 75464 [details] did not build on win:
Build output: http://queues.webkit.org/results/6736030
Comment 11 Darin Adler 2010-12-03 09:42:19 PST
Comment on attachment 75464 [details]
patch

I’m still not sure I understand if this will require all three unique renderer classes. At the moment we are just checking them in as dead code which seems a little weak.

r=me, but please find out why the Windows build is failing and resolve that before checking in
Comment 12 Luiz Agostini 2010-12-05 15:59:33 PST
Created attachment 75639 [details]
checking build systems before landing. please do not review.
Comment 13 Luiz Agostini 2010-12-05 17:15:59 PST
Committed r73346: <http://trac.webkit.org/changeset/73346>