[webkit-reviews] review denied: [Bug 69210] CSS 2.1 failure: inline-box-002.htm fails : [Attachment 124600] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Robert Hogan <robert at webkit.org>'s
request for review:
Bug 69210: CSS 2.1 failure: inline-box-002.htm fails
https://bugs.webkit.org/show_bug.cgi?id=69210

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=124600&action=review


Getting closer.

> Source/WebCore/rendering/RenderInline.cpp:139
> +static void updateStyleOfDescendantBlocks(RenderObject* block, const
RenderStyle* newStyle)

I don't think "Descendant" is the best name here.
updateStyleForAnonymousBlockContinuations might be better.

> Source/WebCore/rendering/RenderInline.cpp:142
> +	   if (block->firstChild()->isRenderBlock()) {

A better way of asking this question would be "Is this block a continuation?"
There's a method for that!

if (block->isAnonymousBlockContinuation())

I'd also add a check to see whether or not the block's current style is already
relatively positioned. If it is, then there's no need to create a new style.

if (block->isAnonymousBlockContinuation() && block->style()->position() !=
newStyle->position())

> Source/WebCore/rendering/RenderInline.cpp:169
> +	   && (newStyle->position() == RelativePosition ||
(oldStyle->position() == RelativePosition && !hasRelPositionedAncestor(this))))
{

This check still doesn't seem good enough. If you lose relative positioning but
a descendant inline has relative positioning still, it seems like your check
here will still turn off relative positioning on the anonymous block
descendant. In other words, this case:

<span style="position:relative"><span style="position:relative">Hello world

If you turn off the outer one, it looks like your code will turn off relative
positioning, even though the inner one has relative positioning still. Your
hasRelPositionedAncestor check needs to start from the anonymous block
continuations and work its way up. It can't start from the current inline whose
style you happened to change.