Bug 133522 – [New Multicolumn] Hit testing of composited content is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

This is because the pagination layers need to be temporarily set up just while hit testing.

[Dave Hyatt]

Created attachment 232490 [details]
Patch

[WebKit Commit Bot]

Attachment 232490 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.cpp:132:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderLayer.cpp:963:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Comment on attachment 232490 [details]
Patch

Never mind. This isn't quite right.

[Dave Hyatt]

Created attachment 232548 [details]
Patch

[Dave Hyatt]

Created attachment 232550 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 232550 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:955
>          m_enclosingPaginationLayer = this;
> +        m_enclosingLayerIsPaginatedAndComposited = isComposited();

Do you actually need to cache m_enclosingLayerIsPaginatedAndComposited? Is m_enclosingLayerIsPaginatedAndComposited ever different from m_enclosingPaginationLayer->isComposited()?

[Dave Hyatt]

(In reply to comment #6)
> (From update of attachment 232550 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232550&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:955
> >          m_enclosingPaginationLayer = this;
> > +        m_enclosingLayerIsPaginatedAndComposited = isComposited();
> 
> Do you actually need to cache m_enclosingLayerIsPaginatedAndComposited? Is m_enclosingLayerIsPaginatedAndComposited ever different from m_enclosingPaginationLayer->isComposited()?

Yes. An intermediate layer can be composited that sits in between the enclosing pagination layer and this layer.

[Dave Hyatt]

I'll get a test case going if this passes, but I actually suspect it was covered by existing test cases. We'll see what the bots say. :)

[Simon Fraser (smfr)]

Comment on attachment 232550 [details]
Patch

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

>>> Source/WebCore/rendering/RenderLayer.cpp:955
>>> +        m_enclosingLayerIsPaginatedAndComposited = isComposited();
>> 
>> Do you actually need to cache m_enclosingLayerIsPaginatedAndComposited? Is m_enclosingLayerIsPaginatedAndComposited ever different from m_enclosingPaginationLayer->isComposited()?
> 
> Yes. An intermediate layer can be composited that sits in between the enclosing pagination layer and this layer.

The name of m_enclosingLayerIsPaginatedAndComposited is a bit confusing then.

Is it worth caching this or could you just detect it on the fly by walking up the layer tree?

> Source/WebCore/rendering/RenderLayer.cpp:966
> +        m_enclosingPaginationLayer = parent()->enclosingPaginationLayer(IncludeCompositedPaginatedLayers);
> +        m_enclosingLayerIsPaginatedAndComposited = isComposited() ? true : parent()->enclosingLayerIsPaginatedAndComposited();
> +        if (parent()->hasTransform()) {
>              m_enclosingPaginationLayer = 0;

Why bother computing enclosingPaginationLayer() if you're just going to null it out? Also nullptr.

> Source/WebCore/rendering/RenderLayer.cpp:984
> +            m_enclosingPaginationLayer = containingBlock->layer()->enclosingPaginationLayer(IncludeCompositedPaginatedLayers);
> +            m_enclosingLayerIsPaginatedAndComposited = isComposited() ? true : containingBlock->layer()->enclosingLayerIsPaginatedAndComposited();
> +            if (containingBlock->layer()->hasTransform()) {
>                  m_enclosingPaginationLayer = 0;

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:5587
> +    const RenderLayer* paginationLayer = 0;

nullptr.

> Source/WebCore/rendering/RenderLayer.h:522
> +            return 0;

nullptr.

[Build Bot]

Comment on attachment 232550 [details]
Patch

Attachment 232550 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6040815583363072

New failing tests:
compositing/columns/hittest-composited-in-paginated.html

[Build Bot]

Created attachment 232563 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232550 [details]
Patch

Attachment 232550 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4926321163501568

New failing tests:
compositing/columns/hittest-composited-in-paginated.html

[Build Bot]

Created attachment 232567 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232550 [details]
Patch

Attachment 232550 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6202437551521792

New failing tests:
compositing/columns/hittest-composited-in-paginated.html

[Build Bot]

Created attachment 232576 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Dave Hyatt]

Created attachment 232621 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 232621 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.h:522
> +            return 0;

nullptr

[Dave Hyatt]

Fixed landed in r169651.