Summary: | HTML5 <details> and <summary> initial implementation | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Luiz Agostini <luiz> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Luiz Agostini <luiz> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, darin, gns, paulirish, peter, tkent, webkit.review.bot, xan.lopez, yael | ||||||||||||
Priority: | P2 | Keywords: | HTML5 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 32934 | ||||||||||||||
Attachments: |
|
Description
Luiz Agostini
2010-11-30 21:42:12 PST
Created attachment 75248 [details]
patch
Attachment 75248 [details] did not build on gtk: Build output: http://queues.webkit.org/results/6678001 Specification: http://www.w3.org/TR/html5/interactive-elements.html#the-details-element http://www.w3.org/TR/html5/interactive-elements.html#the-summary-element Created attachment 75427 [details]
patch
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. (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? Created attachment 75459 [details]
patch
Attachment 75459 [details] did not build on win: Build output: http://queues.webkit.org/results/6727025 Created attachment 75464 [details]
patch
Attachment 75464 [details] did not build on win: Build output: http://queues.webkit.org/results/6736030 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
Created attachment 75639 [details]
checking build systems before landing. please do not review.
Committed r73346: <http://trac.webkit.org/changeset/73346> |