[webkit-reviews] review denied: [Bug 51071] RenderDetails: layout the first <summary> element child of a <details> element : [Attachment 82536] patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Luiz Agostini <luiz at webkit.org>'s
request for review:
Bug 51071: RenderDetails: layout the first <summary> element child of a
<details> element
https://bugs.webkit.org/show_bug.cgi?id=51071

Attachment 82536: patch
https://bugs.webkit.org/attachment.cgi?id=82536&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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.