Bug 51071 - HTML5 <details> and <summary>: rendering
: HTML5 <details> and <summary>: rendering
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: HTML5
: 54260 54584
: 32934 56773 56967 57762
  Show dependency treegraph
 
Reported: 2010-12-14 16:20 PST by
Modified: 2011-04-04 10:18 PST (History)


Attachments
patch (4.52 KB, patch)
2010-12-14 16:36 PST, Luiz Agostini
kenneth: review-
Review Patch | Details | Formatted Diff | Diff
patch (4.66 KB, patch)
2010-12-15 02:04 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (27.20 KB, patch)
2011-02-02 09:05 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (27.23 KB, patch)
2011-02-02 10:23 PST, Luiz Agostini
hyatt: review-
Review Patch | Details | Formatted Diff | Diff
patch (28.16 KB, patch)
2011-02-03 16:41 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (44.66 KB, patch)
2011-02-08 10:07 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (54.00 KB, patch)
2011-02-08 12:53 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (53.38 KB, patch)
2011-02-08 13:15 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (55.57 KB, patch)
2011-02-09 13:57 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (55.62 KB, patch)
2011-02-10 06:28 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (55.70 KB, patch)
2011-02-10 12:47 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
sample html (5.03 KB, text/html)
2011-02-10 14:08 PST, Luiz Agostini
no flags Details
screen shot (80.87 KB, image/png)
2011-02-10 14:11 PST, Luiz Agostini
no flags Details
patch (33.03 KB, patch)
2011-02-10 20:17 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (86.91 KB, patch)
2011-02-15 15:39 PST, Luiz Agostini
hyatt: review-
Review Patch | Details | Formatted Diff | Diff
patch (103.28 KB, patch)
2011-02-17 16:07 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (196.38 KB, patch)
2011-02-28 09:25 PST, Luiz Agostini
hyatt: review-
Review Patch | Details | Formatted Diff | Diff
patch (198.40 KB, patch)
2011-02-28 12:41 PST, Luiz Agostini
no flags Review Patch | Details | Formatted Diff | Diff
patch (198.92 KB, patch)
2011-03-10 11:57 PST, Luiz Agostini
hyatt: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2010-12-14 16:36:04 PST -------
Created an attachment (id=76592) [details]
patch
------- Comment #2 From 2010-12-14 16:43:23 PST -------
Attachment 76592 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7113004
------- Comment #3 From 2010-12-14 17:11:05 PST -------
Attachment 76592 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7176003
------- Comment #4 From 2010-12-14 17:52:37 PST -------
Attachment 76592 [details] did not build on win:
Build output: http://queues.webkit.org/results/7152007
------- Comment #5 From 2010-12-15 00:05:34 PST -------
(From update of attachment 76592 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=76592&action=review

> WebCore/ChangeLog:5
> +        RenderDetails: the first <summary> element child of a <details> element

I do not understand this description. It is like something is missing.

Maybe: Render the first summary element who is a child of a details element? or similar.

> WebCore/rendering/RenderDetails.cpp:44
> +RenderBox* RenderDetails::findSummary() const

Maybe firstSummary()? It sounds from the change log like there can be more

> WebCore/rendering/RenderDetails.cpp:46
> +    for (RenderObject* summary = firstChild(); summary; summary = summary->nextSibling()) {

I guess this 'summary' is really potentialSummary? I guess calling it child instead would be better. What is used elsewhere?

> WebCore/rendering/RenderDetails.cpp:53
> +RenderObject* RenderDetails::layoutSpecialExcludedChild(bool relayoutChildren)

From the function name im not sure what exactly it does. It is the child that is special? or are you doing a special layout?

> WebCore/rendering/RenderDetails.cpp:56
> +    if (summary) {

an early return seems better
------- Comment #6 From 2010-12-15 01:55:46 PST -------
(In reply to comment #5)
> (From update of attachment 76592 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76592&action=review
> 
> > WebCore/rendering/RenderDetails.cpp:53
> > +RenderObject* RenderDetails::layoutSpecialExcludedChild(bool relayoutChildren)
> 
> From the function name im not sure what exactly it does. It is the child that is special? or are you doing a special layout?

Both. It is a special layout for a special child. It is an override of a virtual method of class RenderBlock.
------- Comment #7 From 2010-12-15 01:57:04 PST -------
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 76592 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=76592&action=review
> > 
> > > WebCore/rendering/RenderDetails.cpp:53
> > > +RenderObject* RenderDetails::layoutSpecialExcludedChild(bool relayoutChildren)
> > 
> > From the function name im not sure what exactly it does. It is the child that is special? or are you doing a special layout?
> 
> Both. It is a special layout for a special child. It is an override of a virtual method of class RenderBlock.

Oh ok :-) not much you can do then. Maybe add a short comment in the method to what it is doing.
------- Comment #8 From 2010-12-15 02:04:04 PST -------
Created an attachment (id=76638) [details]
patch
------- Comment #9 From 2010-12-15 02:08:48 PST -------
Attachment 76592 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7089009
------- Comment #10 From 2010-12-24 02:46:51 PST -------
Hi Hyatt and Simon, can one of you have a look at this one?
------- Comment #11 From 2011-01-27 23:39:14 PST -------
Are there any layout tests?
This looks to introduce new layout code, so we should have one(or more).
And I'm curious how <details> would work ;-)
------- Comment #12 From 2011-02-02 09:05:23 PST -------
Created an attachment (id=80917) [details]
patch
------- Comment #13 From 2011-02-02 09:16:09 PST -------
(In reply to comment #11)
> Are there any layout tests?
> This looks to introduce new layout code, so we should have one(or more).
> And I'm curious how <details> would work ;-)

Yes, we will need layout tests for sure! I will work on them soon. :)

I will upload an html file that I am using for testing.
------- Comment #14 From 2011-02-02 09:31:00 PST -------
Attachment 80917 [details] did not build on win:
Build output: http://queues.webkit.org/results/7685925
------- Comment #15 From 2011-02-02 10:05:56 PST -------
Attachment 80917 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7689213
------- Comment #16 From 2011-02-02 10:23:51 PST -------
Created an attachment (id=80928) [details]
patch

build fix
------- Comment #17 From 2011-02-02 11:36:30 PST -------
(From update of attachment 80928 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=80928&action=review

> Source/WebCore/rendering/RenderDetails.cpp:109
> +        m_interactiveArea = IntRect(m_mainSummary->logicalLeft(), m_mainSummary->logicalTop(), m_mainSummary->logicalWidth(), m_mainSummary->logicalHeight());

This seems hard-coded to horizontal text.

> Source/WebCore/rendering/RenderDetailsMarker.cpp:107
> +static Path getCannonicalPath(bool isOpen, bool isLtr)

This should be getCanonicalPath.  It also seems like this appearance may be hardcoded assuming a particular orientation (horizontal).  Have you tested this element in vertical text mode (and/or decided what it's going to do in that mode)?

> Source/WebCore/rendering/RenderObject.cpp:135
> +    if (node->hasTagName(summaryTag))
> +        return new (arena) RenderSummary(node);

Since this is tied directly to a specific DOM element, don't put this in the generic RenderObject creation method.  That's reserved only for objects that have display types.  Assuming there is a DOM subclass for HTMLSummaryElement, put the creation there.

> Source/WebCore/rendering/RenderSummary.cpp:86
> +RenderObject* RenderSummary::getParentOfFirstLineBox(RenderBlock* curr)

This looks like a duplication of the RenderListItem function, even down to the odd quirks mode behavior for lists.  I'm not sure why you'd want to propagate list item quirks into a brand new HTML5 element.
------- Comment #18 From 2011-02-03 11:43:07 PST -------
(In reply to comment #17)
> (From update of attachment 80928 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80928&action=review
> 
> > Source/WebCore/rendering/RenderDetails.cpp:109
> > +        m_interactiveArea = IntRect(m_mainSummary->logicalLeft(), m_mainSummary->logicalTop(), m_mainSummary->logicalWidth(), m_mainSummary->logicalHeight());
> 
> This seems hard-coded to horizontal text.

If I understood it right it is not. For example, this is the implementation of RenderBox::logicalLeft

    int logicalLeft() const { return style()->isHorizontalWritingMode() ? x() : y(); }

Am I wrong?

> 
> > Source/WebCore/rendering/RenderDetailsMarker.cpp:107
> > +static Path getCannonicalPath(bool isOpen, bool isLtr)
> 
> This should be getCanonicalPath.  It also seems like this appearance may be hardcoded assuming a particular orientation (horizontal).  Have you tested this element in vertical text mode (and/or decided what it's going to do in that mode)?

Yes, it is assuming horizontal orientation. I will fix it.

> 
> > Source/WebCore/rendering/RenderObject.cpp:135
> > +    if (node->hasTagName(summaryTag))
> > +        return new (arena) RenderSummary(node);
> 
> Since this is tied directly to a specific DOM element, don't put this in the generic RenderObject creation method.  That's reserved only for objects that have display types.  Assuming there is a DOM subclass for HTMLSummaryElement, put the creation there.

It would be nice to have a DOM subclass for HTMLSummaryElement but specification says that the interface is HTMLElement (http://www.w3.org/TR/html5/interactive-elements.html#the-summary-element). May I create the HTMLSummaryElement anyway?

> 
> > Source/WebCore/rendering/RenderSummary.cpp:86
> > +RenderObject* RenderSummary::getParentOfFirstLineBox(RenderBlock* curr)
> 
> This looks like a duplication of the RenderListItem function, even down to the odd quirks mode behavior for lists.  I'm not sure why you'd want to propagate list item quirks into a brand new HTML5 element.

Yes, I did duplicate some functions of RenderListItem keeping them as is for the duplication to be clear. I just made minor changes to getParentOfFirstLineBox but of course quirks mode should not be considered.
------- Comment #19 From 2011-02-03 16:41:48 PST -------
Created an attachment (id=81141) [details]
patch

I made some optimizations and prepared the way for creating own legend if there is no <summary> element.
I made some changes according to the review comments as well but did not cover all of them yet.

Next steps will probably be:

 - to create <details> own legend when there is no <summary> element.
 - to provide layout tests.
 - to fix pending layout issues.
------- Comment #20 From 2011-02-08 10:07:06 PST -------
Created an attachment (id=81657) [details]
patch
------- Comment #21 From 2011-02-08 10:12:33 PST -------
Attachment 81657 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7763149
------- Comment #22 From 2011-02-08 10:16:13 PST -------
Attachment 81657 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7763150
------- Comment #23 From 2011-02-08 10:44:13 PST -------
Attachment 81657 [details] did not build on win:
Build output: http://queues.webkit.org/results/7819162
------- Comment #24 From 2011-02-08 10:46:44 PST -------
Attachment 81657 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7797148
------- Comment #25 From 2011-02-08 11:47:32 PST -------
Attachment 81657 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7847145
------- Comment #26 From 2011-02-08 11:56:54 PST -------
Attachment 81657 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7847148
------- Comment #27 From 2011-02-08 12:53:10 PST -------
Created an attachment (id=81678) [details]
patch
------- Comment #28 From 2011-02-08 12:59:32 PST -------
Attachment 81678 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7819190
------- Comment #29 From 2011-02-08 13:15:06 PST -------
Created an attachment (id=81680) [details]
patch

build fix
------- Comment #30 From 2011-02-09 13:57:45 PST -------
Created an attachment (id=81865) [details]
patch
------- Comment #31 From 2011-02-09 14:03:57 PST -------
(In reply to comment #17)
> (From update of attachment 80928 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80928&action=review
> 
> > Source/WebCore/rendering/RenderDetails.cpp:109
> > +        m_interactiveArea = IntRect(m_mainSummary->logicalLeft(), m_mainSummary->logicalTop(), m_mainSummary->logicalWidth(), m_mainSummary->logicalHeight());
> 
> This seems hard-coded to horizontal text.

It should be ok in last patch.

> 
> > Source/WebCore/rendering/RenderDetailsMarker.cpp:107
> > +static Path getCannonicalPath(bool isOpen, bool isLtr)
> 
> This should be getCanonicalPath.  It also seems like this appearance may be hardcoded assuming a particular orientation (horizontal).  Have you tested this element in vertical text mode (and/or decided what it's going to do in that mode)?

It should be ok in last patch as well.

> 
> > Source/WebCore/rendering/RenderObject.cpp:135
> > +    if (node->hasTagName(summaryTag))
> > +        return new (arena) RenderSummary(node);
> 
> Since this is tied directly to a specific DOM element, don't put this in the generic RenderObject creation method.  That's reserved only for objects that have display types.  Assuming there is a DOM subclass for HTMLSummaryElement, put the creation there.

May I create a DOM subclass for HTMLSummaryElement? Specification says that the interface is HTMLElement (http://www.w3.org/TR/html5/interactive-elements.html#the-summary-element).
------- Comment #32 From 2011-02-09 14:38:11 PST -------
Attachment 81865 [details] did not build on win:
Build output: http://queues.webkit.org/results/7874059
------- Comment #33 From 2011-02-09 14:57:12 PST -------
Attachment 81865 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7869068
------- Comment #34 From 2011-02-09 17:33:35 PST -------
Attachment 81865 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7870080
------- Comment #35 From 2011-02-10 01:05:40 PST -------
Attachment 81865 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7884214
------- Comment #36 From 2011-02-10 06:28:03 PST -------
Created an attachment (id=81971) [details]
patch
------- Comment #37 From 2011-02-10 06:32:03 PST -------
Attachment 81971 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderDetailsMarker.cpp:100:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #38 From 2011-02-10 12:47:50 PST -------
Created an attachment (id=82030) [details]
patch
------- Comment #39 From 2011-02-10 14:08:06 PST -------
Created an attachment (id=82045) [details]
sample html
------- Comment #40 From 2011-02-10 14:11:22 PST -------
Created an attachment (id=82046) [details]
screen shot
------- Comment #41 From 2011-02-10 20:17:40 PST -------
Created an attachment (id=82092) [details]
patch

I removed the localization code from this patch. Now this bug depends on bug 54260.
------- Comment #42 From 2011-02-15 15:39:50 PST -------
Created an attachment (id=82536) [details]
patch

some layout tests
------- Comment #43 From 2011-02-16 10:48:59 PST -------
(From update of attachment 82536 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=82536&action=review

Thanks for adding all the writing mode logic.  This looks great.  Just a few comments.

> Source/WebCore/rendering/RenderDetails.cpp:179
> +    switch (style()->writingMode()) {
> +    case TopToBottomWritingMode:
> +    case LeftToRightWritingMode:
> +        break;
> +    case RightToLeftWritingMode: {
> +        m_interactiveArea.setX(width() - m_interactiveArea.x() - m_interactiveArea.width());
> +        break;
> +    }
> +    case BottomToTopWritingMode: {
> +        m_interactiveArea.setY(height() - m_interactiveArea.y() - m_interactiveArea.height());
> +        break;
> +    }
> +    }
> +}

The interactive area is supposed to be in the local coordinates of the RenderDetails.  I suspect you're just working around the fact that absoluteToLocal has not been patched to handle flipped blocks writing modes yet.  I would just yank this code, since once absoluteToLocal works correctly, you won't need it.

Note that if you could somehow get back to the hit test result, there are correct local mouse event coordinates in there for the renderer that was hit.  I'm not sure if that's possible from defaultEventHandler though.  At any rate, you have my blessing for this to be broken with flipped blocks writing modes, since absoluteToLocal does need to be patched eventually.

> Source/WebCore/rendering/RenderDetailsMarker.cpp:135
> +        switch (style()->direction()) {

We're trying to avoid querying direction like this nowadays.  Use style()->isLeftToRightDirection() instead.  It will cut down on the use of these LTR/RTL constants when we get around to renaming them.

> Source/WebCore/rendering/RenderDetailsMarker.cpp:140
> +        switch (style()->direction()) {

Here too.

> Source/WebCore/rendering/RenderDetailsMarker.cpp:145
> +        switch (style()->direction()) {

And here.

> Source/WebCore/rendering/RenderDetailsMarker.cpp:150
> +        switch (style()->direction()) {

And here.

> Source/WebCore/rendering/RenderObject.cpp:135
> +    if (node->hasTagName(summaryTag))
> +        return HTMLDetailsElement::createSummaryRenderer(arena, node);

Still don't like this.  It's ok to make an HTMLSummaryElement subclass without having any special DOM bindings for it.  I'd prefer you did that.  I think you'll find once you have that element that there will be other things you may want to put there in the future anwyay.
------- Comment #44 From 2011-02-17 16:07:20 PST -------
Created an attachment (id=82873) [details]
patch
------- Comment #45 From 2011-02-18 12:52:11 PST -------
(From update of attachment 82873 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=82873&action=review

> Source/WebCore/html/HTMLDetailsElement.cpp:68
> +    if (oldSummary != m_mainSummary && attached()) {
> +        detach();
> +        attach();
> +    }

Can you explain why this is necessary?  Detaching and re-attaching seems hacky to me.

> Source/WebCore/html/HTMLDetailsElement.cpp:92
> +        if (attached() && oldValue != m_isOpen) {
> +            detach();
> +            attach();
> +        }

Can you explain why this is necessary?  Detaching and re-attaching seems hacky to me.
------- Comment #46 From 2011-02-18 13:39:13 PST -------
(In reply to comment #45)
> (From update of attachment 82873 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82873&action=review
> 
> > Source/WebCore/html/HTMLDetailsElement.cpp:68
> > +    if (oldSummary != m_mainSummary && attached()) {
> > +        detach();
> > +        attach();
> > +    }
> 

If a <summary> element is added before all other <summary> elements in a <details> element it will become its main <summary>. The previous main <summary> element will have its rendering changed. It may be that the previous main <summary> will not even be rendered depending on the current value of the 'open' attribute of the <details>.

But it may be that, in this case, it could be optimized.

> Can you explain why this is necessary?  Detaching and re-attaching seems hacky to me.
> 
> > Source/WebCore/html/HTMLDetailsElement.cpp:92
> > +        if (attached() && oldValue != m_isOpen) {
> > +            detach();
> > +            attach();
> > +        }
> 
> Can you explain why this is necessary?  Detaching and re-attaching seems hacky to me.

If the open attribute of <details> is false then its children will not be rendered (except its main <summary>). When closed, no render is created to <details> children.
When open changes the <details> is re-attached to remove its children renderes if 'open' has changed to false, or to create children renderers if 'open' has changed to true.
------- Comment #47 From 2011-02-18 14:00:12 PST -------
(In reply to comment #46)
> (In reply to comment #45)
> > (From update of attachment 82873 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=82873&action=review
> > 
> > > Source/WebCore/html/HTMLDetailsElement.cpp:68
> > > +    if (oldSummary != m_mainSummary && attached()) {
> > > +        detach();
> > > +        attach();
> > > +    }
> > 
> 
> If a <summary> element is added before all other <summary> elements in a <details> element it will become its main <summary>. The previous main <summary> element will have its rendering changed. It may be that the previous main <summary> will not even be rendered depending on the current value of the 'open' attribute of the <details>.
> 

Yeah but it's still a RenderSummary in both cases, so I don't buy that you have to nuke the entire RenderDetails and all of its descendants just because a different summary became the main one.  It doesn't seem hard to shift what the main summary is, since the render object types aren't changing.

> But it may be that, in this case, it could be optimized.
> 
> > Can you explain why this is necessary?  Detaching and re-attaching seems hacky to me.
> > 
> > > Source/WebCore/html/HTMLDetailsElement.cpp:92
> > > +        if (attached() && oldValue != m_isOpen) {
> > > +            detach();
> > > +            attach();
> > > +        }
> > 
> > Can you explain why this is necessary?  Detaching and re-attaching seems hacky to me.
> 
> If the open attribute of <details> is false then its children will not be rendered (except its main <summary>). When closed, no render is created to <details> children.
> When open changes the <details> is re-attached to remove its children renderes if 'open' has changed to false, or to create children renderers if 'open' has changed to true.

Ok I buy this one.
------- Comment #48 From 2011-02-20 12:21:10 PST -------
As long as this patch isn’t landed, WebKit false positives on the feature test for native <details> support (without actually supporting it):

    'open' in document.createElement('details'); // true

See data:text/html,<details><summary>Foo</summary>Bar</details><script>console.log('open' in document.createElement('details'));</script>
------- Comment #49 From 2011-02-28 09:25:24 PST -------
Created an attachment (id=84073) [details]
patch
------- Comment #50 From 2011-02-28 10:30:00 PST -------
(From update of attachment 84073 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=84073&action=review

Comments below;

> Source/WebCore/html/HTMLDetailsElement.cpp:59
> +    for (Node* child = firstChild(); !m_mainSummary && child; child = child->nextSibling()) {
> +        if (child->hasTagName(summaryTag))
> +            m_mainSummary = child;
> +    }

Kind of a funny way to write this.  I'd just do a break rather than testing !m_mainSummary in the condition.

> Source/WebCore/html/HTMLDetailsElement.cpp:78
> +    if (!changedByParser) {
> +        Node* oldSummary = m_mainSummary;
> +        findMainSummary();
> +
> +        if (oldSummary != m_mainSummary && !m_isOpen && attached()) {
> +            if (oldSummary && oldSummary->attached())
> +                oldSummary->detach();
> +            if (m_mainSummary && childCountDelta < 0 && !m_mainSummary->renderer()) {
> +                m_mainSummary->detach();
> +                m_mainSummary->attach();
> +            }
> +        }
> +    }
> +}

Can you explain this case?  I'm having trouble understanding what's going on here.   In particular childCountDelta < 0?  I get why you have to detach the old summary since it's now inside the unrevealed content, but I don't understand the mainSummary case.

> Source/WebCore/html/HTMLDetailsElement.cpp:87
> +    if (attached() && m_mainSummary && !m_mainSummary->renderer()) {
> +        m_mainSummary->detach();
> +        m_mainSummary->attach();
> +    }

It feels like this has some overlap with the code in childrenChanged... maybe you should incorporate the attach/detach updating into findMainSummary?

> Source/WebCore/html/HTMLDetailsElement.cpp:122
> +    FloatPoint absolutePosition = renderDetails->absoluteToLocal(FloatPoint(mouseEvent->pageX() * factor, mouseEvent->pageY() * factor));

You should rename this.  The point is a local position not an absolute position..
------- Comment #51 From 2011-02-28 11:07:38 PST -------
(In reply to comment #50)
> (From update of attachment 84073 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84073&action=review
> 
> Comments below;
> 
> > Source/WebCore/html/HTMLDetailsElement.cpp:59
> > +    for (Node* child = firstChild(); !m_mainSummary && child; child = child->nextSibling()) {
> > +        if (child->hasTagName(summaryTag))
> > +            m_mainSummary = child;
> > +    }
> 
> Kind of a funny way to write this.  I'd just do a break rather than testing !m_mainSummary in the condition.

ok

> 
> > Source/WebCore/html/HTMLDetailsElement.cpp:78
> > +    if (!changedByParser) {
> > +        Node* oldSummary = m_mainSummary;
> > +        findMainSummary();
> > +
> > +        if (oldSummary != m_mainSummary && !m_isOpen && attached()) {
> > +            if (oldSummary && oldSummary->attached())
> > +                oldSummary->detach();
> > +            if (m_mainSummary && childCountDelta < 0 && !m_mainSummary->renderer()) {
> > +                m_mainSummary->detach();
> > +                m_mainSummary->attach();
> > +            }
> > +        }
> > +    }
> > +}
> 
> Can you explain this case?  I'm having trouble understanding what's going on here.   In particular childCountDelta < 0?  I get why you have to detach the old summary since it's now inside the unrevealed content, but I don't understand the mainSummary case.

If childCountDelta is less then zero and the main summary has changed it must be because previous main summary was removed. The new main summary was then inside the unrevealed content and needs to be reattached to create its renderer. If childCountDelta is not less then zero then a new <summary> was added and it will be attached without my help.

> 
> > Source/WebCore/html/HTMLDetailsElement.cpp:87
> > +    if (attached() && m_mainSummary && !m_mainSummary->renderer()) {
> > +        m_mainSummary->detach();
> > +        m_mainSummary->attach();
> > +    }
> 
> It feels like this has some overlap with the code in childrenChanged... maybe you should incorporate the attach/detach updating into findMainSummary?

It is similar to what is done in childrenChanged but not equal. I considered adding parameters to findMainSummary but at the end I found that it would more clear for each method to do what it needs alone.

> 
> > Source/WebCore/html/HTMLDetailsElement.cpp:122
> > +    FloatPoint absolutePosition = renderDetails->absoluteToLocal(FloatPoint(mouseEvent->pageX() * factor, mouseEvent->pageY() * factor));
> 
> You should rename this.  The point is a local position not an absolute position..

ok.
------- Comment #52 From 2011-02-28 12:41:10 PST -------
Created an attachment (id=84097) [details]
patch
------- Comment #53 From 2011-03-01 09:54:06 PST -------
Nice work, Luiz!

Shouldn’t the first <summary> element of every <details> element be keyboard accessible?
------- Comment #54 From 2011-03-01 12:13:32 PST -------
(In reply to comment #53)
> Nice work, Luiz!

Thanks.

> 
> Shouldn’t the first <summary> element of every <details> element be keyboard accessible?

Yes, I think it should. I will handle accessibility issues in future patches.
------- Comment #55 From 2011-03-10 11:57:27 PST -------
Created an attachment (id=85360) [details]
patch
------- Comment #56 From 2011-03-11 12:23:31 PST -------
(From update of attachment 85360 [details])
r=me
------- Comment #57 From 2011-03-14 10:39:32 PST -------
Committed r81035: <http://trac.webkit.org/changeset/81035>
------- Comment #58 From 2011-03-14 10:45:18 PST -------
http://trac.webkit.org/changeset/81035 might have broken Leopard Intel Release (Build)
------- Comment #59 From 2011-03-21 19:17:42 PST -------
This is causing crashes: https://bugs.webkit.org/show_bug.cgi?id=56773
------- Comment #60 From 2011-03-23 15:23:06 PST -------
This probably should've been implemented using the new shadow DOM plumbing. I'll try to convert.
------- Comment #61 From 2011-03-23 15:25:03 PST -------
(In reply to comment #60)
> This probably should've been implemented using the new shadow DOM plumbing. I'll try to convert.

Oh you implemented parts of ideas from XBL2? Can you give me the relavant commit revision to have a look? :-)
------- Comment #62 From 2011-03-23 15:29:24 PST -------
(In reply to comment #61)
> (In reply to comment #60)
> > This probably should've been implemented using the new shadow DOM plumbing. I'll try to convert.
> 
> Oh you implemented parts of ideas from XBL2? Can you give me the relavant commit revision to have a look? :-)

The trail starts here: https://bugs.webkit.org/showdependencytree.cgi?id=52962&hide_resolved=0
------- Comment #63 From 2011-03-23 15:30:33 PST -------
(In reply to comment #62)
> (In reply to comment #61)
> > (In reply to comment #60)
> > > This probably should've been implemented using the new shadow DOM plumbing. I'll try to convert.
> > 
> > Oh you implemented parts of ideas from XBL2? Can you give me the relavant commit revision to have a look? :-)
> 
> The trail starts here: https://bugs.webkit.org/showdependencytree.cgi?id=52962&hide_resolved=0

Thanks! Seems that you guys have been quite busy, nice work