Bug 112024 – Sticky positioning is broken for table rows

• Component: CSS
• Status: RESOLVED FIXED

[Jon Rimmer]

position: -webkit-sticky is not working for tr elements. It is still working for at least some others. See attached testcase.

Testing info...

Google Chrome: 27.0.1436.0 (Official Build 187216) canary
OS: Windows 
WebKit:	537.33 (@145278)
JavaScript: V8 3.17.9

[Jon Rimmer]

Created attachment 192475 [details]
Simple testcase

[Viatcheslav Ostapenko]

Created attachment 222471 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 222471 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3093
> +        const RenderBlock* cb = containingBlock->isRenderBlock() ? toRenderBlock(containingBlock) : containingBlock->containingBlock();

It's pretty confusing that a parameter called "containingBlock" is not actually the containingBlock. Should we make the parameter a RenderBlock* and fix the callers?

[Viatcheslav Ostapenko]

Comment on attachment 222471 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:3093
>> +        const RenderBlock* cb = containingBlock->isRenderBlock() ? toRenderBlock(containingBlock) : containingBlock->containingBlock();
> 
> It's pretty confusing that a parameter called "containingBlock" is not actually the containingBlock. Should we make the parameter a RenderBlock* and fix the callers?

It appears to be pretty big change, because there are other methods (like containingBlockLogicalWidthForPositioned) that have the same problem and they call containingBlockLogicalHeightForPositioned .
Can it be done in another patch?

[Simon Fraser (smfr)]

Comment on attachment 222471 [details]
Patch

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

>>> Source/WebCore/rendering/RenderBox.cpp:3093
>>> +        const RenderBlock* cb = containingBlock->isRenderBlock() ? toRenderBlock(containingBlock) : containingBlock->containingBlock();
>> 
>> It's pretty confusing that a parameter called "containingBlock" is not actually the containingBlock. Should we make the parameter a RenderBlock* and fix the callers?
> 
> It appears to be pretty big change, because there are other methods (like containingBlockLogicalWidthForPositioned) that have the same problem and they call containingBlockLogicalHeightForPositioned .
> Can it be done in another patch?

OK. Also in some rare cases containingBlock() can return nil; not sure if we need to handle that here.

[Viatcheslav Ostapenko]

(In reply to comment #5)
> (From update of attachment 222471 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222471&action=review
> 
> >>> Source/WebCore/rendering/RenderBox.cpp:3093
> >>> +        const RenderBlock* cb = containingBlock->isRenderBlock() ? toRenderBlock(containingBlock) : containingBlock->containingBlock();
> >> 
> >> It's pretty confusing that a parameter called "containingBlock" is not actually the containingBlock. Should we make the parameter a RenderBlock* and fix the callers?
> > 
> > It appears to be pretty big change, because there are other methods (like containingBlockLogicalWidthForPositioned) that have the same problem and they call containingBlockLogicalHeightForPositioned .
> > Can it be done in another patch?
> 
> OK. Also in some rare cases containingBlock() can return nil; not sure if we need to handle that here.

I have put ASSERT there and didn't manage to make it fire.

Also, callers for containingBlockLogicalWidthForPositioned and containingBlockLogicalHeightForPositioned have this comment:

// We don't use containingBlock(), since we may be positioned by an enclosing
// relative positioned inline.

Checking what I can do about this.

[WebKit Commit Bot]

Comment on attachment 222471 [details]
Patch

Clearing flags on attachment: 222471

Committed r162960: <http://trac.webkit.org/changeset/162960>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.