⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 276562 in webkit


Ignore:
Timestamp:
Apr 24, 2021, 7:12:07 PM (5 years ago)
Author:
timothy_horton@apple.com
Message:

Changing the source of a model element with clipping applied does not update the model
​https://bugs.webkit.org/show_bug.cgi?id=224917

Reviewed by Simon Fraser.

Source/WebCore:

Tests: model-element/model-element-contents-layer-updates-with-clipping.html

model-element/model-element-contents-layer-updates.html

Previously, a <model> with a contents clipping layer (e.g. border-radius)
would not reparent its contents layer in the right place when setContentsToModel
was called again (because the source changed), leaving the old model
contents layer in place.

  • platform/graphics/ca/GraphicsLayerCA.cpp:

(WebCore::GraphicsLayerCA::updateSublayerList):
Ensure that updateSublayerList always parents contentsLayer in one of its two homes:
under contentsClippingLayer, if it exists; otherwise, directly under the primary layer.

(WebCore::GraphicsLayerCA::setContentsToModel):
Drive-by fix a bug revealed by the tests for this patch: when swapping out the
contents layer in setContentsToModel, we also need to mark ContentsRectsChanged,
or the new contents layer will not get its bounds set during the subsequent flush.

(WebCore::GraphicsLayerCA::setContentsToPlatformLayer):
Remove special-case code that was added to fix this bug just for setContentsToPlatformLayer;
this case is now correctly handled for all contents layers by updateSublayerList.

(WebCore::GraphicsLayerCA::dumpInnerLayer const):

  • platform/graphics/GraphicsLayerClient.h:
  • platform/graphics/ca/PlatformCALayer.cpp:

(WebCore::PlatformCALayer::dumpAdditionalProperties):

  • platform/graphics/ca/PlatformCALayer.h:
  • testing/Internals.cpp:

(WebCore::toPlatformLayerTreeFlags):

  • testing/Internals.h:
  • testing/Internals.idl:

Add a bit to platformLayerTreeAsText() that makes PlatformCALayerRemoteModelHosting
dump the size of the model that it is hosting, which is used in the test for this bug.

Remove the IncludeOpacity bit since we can just always log opacity if it's not the default.

Source/WebKit:

  • WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemoteModelHosting.h:
  • WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemoteModelHosting.mm:

(WebKit::PlatformCALayerRemoteModelHosting::dumpAdditionalProperties):
Add a bit to platformLayerTreeAsText() that makes PlatformCALayerRemoteModelHosting
dump the size of the model that it is hosting, which is used in the test for this bug.

LayoutTests:

  • model-element/model-element-contents-layer-updates-expected.txt: Added.
  • model-element/model-element-contents-layer-updates-with-clipping-expected.txt: Added.
  • model-element/model-element-contents-layer-updates-with-clipping.html: Added.
  • model-element/model-element-contents-layer-updates.html: Added.
  • model-element/resources/cube.usdz: Added.
  • platform/ios-wk2/TestExpectations:
  • platform/mac/TestExpectations:

Add tests that ensure that adding a <model> with one source, then changing
it to another, correctly updates the content layer. Test this both
with and without clipping (the without-clipping case passed before this change,
with-clipping failed).

These tests only work on Cocoa ports with UI-side compositing enabled
because they depend on the PlatformCALayer subclass holding on to the model
data (and logging its size) in order to distinguish between the two models.

Location:
trunk
Files:
5 added
15 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r276558 r276562  
     12021-04-24  Tim Horton  <timothy_horton@apple.com>
     2
     3        Changing the source of a model element with clipping applied does not update the model
     4        https://bugs.webkit.org/show_bug.cgi?id=224917
     5
     6        Reviewed by Simon Fraser.
     7
     8        * model-element/model-element-contents-layer-updates-expected.txt: Added.
     9        * model-element/model-element-contents-layer-updates-with-clipping-expected.txt: Added.
     10        * model-element/model-element-contents-layer-updates-with-clipping.html: Added.
     11        * model-element/model-element-contents-layer-updates.html: Added.
     12        * model-element/resources/cube.usdz: Added.
     13        * platform/ios-wk2/TestExpectations:
     14        * platform/mac/TestExpectations:
     15        Add tests that ensure that adding a <model> with one source, then changing
     16        it to another, correctly updates the content layer. Test this both
     17        with and without clipping (the without-clipping case passed before this change,
     18        with-clipping failed).
     19
     20        These tests only work on Cocoa ports with UI-side compositing enabled
     21        because they depend on the PlatformCALayer subclass holding on to the model
     22        data (and logging its size) in order to distinguish between the two models.
     23
    1242021-04-24  Julian Gonzalez  <julian_a_gonzalez@apple.com>
    225

  • trunk/LayoutTests/model-element/model-element-graphics-layers-opacity.html

    r276327 r276562  
    2626    model.ready.then(value => {
    2727        if (window.testRunner)
    28             layers.innerText = window.internals.platformLayerTreeAsText(model, window.internals.PLATFORM_LAYER_TREE_INCLUDE_OPACITY);
     28            layers.innerText = window.internals.platformLayerTreeAsText(model);
    2929        model.remove();
    3030    }, reason => {

  • trunk/LayoutTests/platform/ios-wk2/TestExpectations

    r276391 r276562  
    4646fast/visual-viewport/rubberbanding-viewport-rects.html [ Pass ]
    4747fast/visual-viewport/rubberbanding-viewport-rects-header-footer.html  [ Pass ]
     48
     49model-element/model-element-contents-layer-updates-with-clipping.html [ Pass ]
     50model-element/model-element-contents-layer-updates.html [ Pass ]
    4851
    4952# Pending SDK changes, re-skip these tests that are un-skipped above.

  • trunk/LayoutTests/platform/mac/TestExpectations

    r276520 r276562  
    6767fast/dom/Window/slow-unload-handler.html
    6868fast/dom/Window/slow-unload-handler-only-frame-is-stopped.html
     69
     70# This test only works with UI-side compositing
     71model-element/model-element-contents-layer-updates-with-clipping.html [ Skip ]
     72model-element/model-element-contents-layer-updates.html [ Skip ]
    6973
    7074# Accessibility tests for notifications that don't exist or aren't needed on Mac OS X.

  • trunk/Source/WebCore/ChangeLog

    r276558 r276562  
     12021-04-24  Tim Horton  <timothy_horton@apple.com>
     2
     3        Changing the source of a model element with clipping applied does not update the model
     4        https://bugs.webkit.org/show_bug.cgi?id=224917
     5
     6        Reviewed by Simon Fraser.
     7
     8        Tests: model-element/model-element-contents-layer-updates-with-clipping.html
     9               model-element/model-element-contents-layer-updates.html
     10
     11        Previously, a <model> with a contents clipping layer (e.g. border-radius)
     12        would not reparent its contents layer in the right place when setContentsToModel
     13        was called again (because the source changed), leaving the old model
     14        contents layer in place.
     15       
     16        * platform/graphics/ca/GraphicsLayerCA.cpp:
     17        (WebCore::GraphicsLayerCA::updateSublayerList):
     18        Ensure that updateSublayerList always parents contentsLayer in one of its two homes:
     19        under contentsClippingLayer, if it exists; otherwise, directly under the primary layer.
     20
     21        (WebCore::GraphicsLayerCA::setContentsToModel):
     22        Drive-by fix a bug revealed by the tests for this patch: when swapping out the
     23        contents layer in setContentsToModel, we also need to mark ContentsRectsChanged,
     24        or the new contents layer will not get its bounds set during the subsequent flush.
     25
     26        (WebCore::GraphicsLayerCA::setContentsToPlatformLayer):
     27        Remove special-case code that was added to fix this bug just for setContentsToPlatformLayer;
     28        this case is now correctly handled for all contents layers by updateSublayerList.
     29
     30        (WebCore::GraphicsLayerCA::dumpInnerLayer const):
     31        * platform/graphics/GraphicsLayerClient.h:
     32        * platform/graphics/ca/PlatformCALayer.cpp:
     33        (WebCore::PlatformCALayer::dumpAdditionalProperties):
     34        * platform/graphics/ca/PlatformCALayer.h:
     35        * testing/Internals.cpp:
     36        (WebCore::toPlatformLayerTreeFlags):
     37        * testing/Internals.h:
     38        * testing/Internals.idl:
     39        Add a bit to platformLayerTreeAsText() that makes PlatformCALayerRemoteModelHosting
     40        dump the size of the model that it is hosting, which is used in the test for this bug.
     41
     42        Remove the IncludeOpacity bit since we can just always log opacity if it's not the default.
     43
    1442021-04-24  Julian Gonzalez  <julian_a_gonzalez@apple.com>
    245

  • trunk/Source/WebCore/platform/graphics/GraphicsLayerClient.h

    r276327 r276562  
    9191    Debug = 1 << 0,
    9292    IgnoreChildren = 1 << 1,
    93     IncludeOpacity = 1 << 2,
     93    IncludeModels = 1 << 2,
    9494};
    9595

  • trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp

    r276513 r276562  
    12411241        noteSublayersChanged();
    12421242
    1243     noteLayerPropertyChanged(OpacityChanged);
     1243    noteLayerPropertyChanged(ContentsRectsChanged | OpacityChanged);
    12441244}
    12451245#endif
    … …  
    12491249    if (m_contentsLayer && platformLayer == m_contentsLayer->platformLayer())
    12501250        return;
    1251 
    1252     if (m_contentsClippingLayer && m_contentsLayer)
    1253         m_contentsLayer->removeFromSuperlayer();
    12541251
    12551252    // FIXME: The passed in layer might be a raw layer or an externally created
    … …  
    12711268    m_contentsLayerPurpose = platformLayer ? purpose : ContentsLayerPurpose::None;
    12721269
    1273     if (m_contentsClippingLayer && m_contentsLayer)
    1274         m_contentsClippingLayer->appendSublayer(*m_contentsLayer);
    1275 
    12761270    noteSublayersChanged();
    12771271    noteLayerPropertyChanged(ContentsPlatformLayerChanged);
    … …  
    20312025    };
    20322026
     2027    auto appendContentsLayer = [&](PlatformCALayerList& list) {
     2028        if (m_contentsVisible && m_contentsLayer)
     2029            list.append(m_contentsLayer);
     2030    };
     2031
    20332032    auto appendClippingLayers = [&](PlatformCALayerList& list) {
    2034         if (!m_contentsVisible)
    2035             return;
    2036 
    2037         if (m_contentsClippingLayer) {
     2033        if (m_contentsVisible && m_contentsClippingLayer)
    20382034            list.append(m_contentsClippingLayer);
    2039             return;
    2040         }
    2041 
    2042         if (m_contentsLayer)
    2043             list.append(m_contentsLayer);
    20442035    };
    20452036
    … …  
    20482039            list.appendVector(*customSublayers);
    20492040
    2050         appendClippingLayers(list);
     2041        if (m_contentsClippingLayer)
     2042            appendClippingLayers(list);
     2043        else
     2044            appendContentsLayer(list);
    20512045    };
    20522046
    … …  
    20772071
    20782072    bool clippingLayerHostsChildren = m_contentsRectClipsDescendants && m_contentsClippingLayer;
    2079     if (clippingLayerHostsChildren) {
     2073    if (m_contentsClippingLayer) {
    20802074        PlatformCALayerList clippingChildren;
    2081         buildChildLayerList(clippingChildren);
     2075        if (clippingLayerHostsChildren)
     2076            buildChildLayerList(clippingChildren);
     2077        appendContentsLayer(clippingChildren);
    20822078        m_contentsClippingLayer->setSublayers(clippingChildren);
    20832079    }
    … …  
    39823978        ts << indent << "(bounds " << layer->bounds().width() << " " << layer->bounds().height() << ")\n";
    39833979       
    3984         if (flags.contains(PlatformLayerTreeAsTextFlags::IncludeOpacity))
     3980        if (layer->opacity() != 1)
    39853981            ts << indent << "(opacity " << layer->opacity() << ")\n";
    39863982
    39873983        if (layer->isHidden())
    39883984            ts << indent << "(hidden)\n";
     3985
     3986        layer->dumpAdditionalProperties(ts, flags);
    39893987
    39903988        if (!flags.contains(PlatformLayerTreeAsTextFlags::IgnoreChildren)) {

  • trunk/Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp

    r274033 r276562  
    199199}
    200200
     201void PlatformCALayer::dumpAdditionalProperties(TextStream&, OptionSet<PlatformLayerTreeAsTextFlags>)
     202{
     203}
     204
    201205TextStream& operator<<(TextStream& ts, PlatformCALayer::LayerType layerType)
    202206{

  • trunk/Source/WebCore/platform/graphics/ca/PlatformCALayer.h

    r276085 r276562  
    301301
    302302    void moveToLayerPool();
     303   
     304    virtual void dumpAdditionalProperties(TextStream&, OptionSet<PlatformLayerTreeAsTextFlags>);
    303305
    304306protected:

  • trunk/Source/WebCore/testing/Internals.cpp

    r276414 r276562  
    28622862    if (flags & Internals::PLATFORM_LAYER_TREE_IGNORES_CHILDREN)
    28632863        platformLayerTreeFlags.add(PlatformLayerTreeAsTextFlags::IgnoreChildren);
    2864     if (flags & Internals::PLATFORM_LAYER_TREE_INCLUDE_OPACITY)
    2865         platformLayerTreeFlags.add(PlatformLayerTreeAsTextFlags::IncludeOpacity);
     2864    if (flags & Internals::PLATFORM_LAYER_TREE_INCLUDE_MODELS)
     2865        platformLayerTreeFlags.add(PlatformLayerTreeAsTextFlags::IncludeModels);
    28662866    return platformLayerTreeFlags;
    28672867}

  • trunk/Source/WebCore/testing/Internals.h

    r276414 r276562  
    426426        PLATFORM_LAYER_TREE_DEBUG = 1,
    427427        PLATFORM_LAYER_TREE_IGNORES_CHILDREN = 2,
    428         PLATFORM_LAYER_TREE_INCLUDE_OPACITY = 4,
     428        PLATFORM_LAYER_TREE_INCLUDE_MODELS = 4,
    429429    };
    430430    ExceptionOr<String> platformLayerTreeAsText(Element&, unsigned short flags) const;

  • trunk/Source/WebCore/testing/Internals.idl

    r276414 r276562  
    505505    const unsigned short PLATFORM_LAYER_TREE_DEBUG = 1;
    506506    const unsigned short PLATFORM_LAYER_TREE_IGNORES_CHILDREN = 2;
    507     const unsigned short PLATFORM_LAYER_TREE_INCLUDE_OPACITY = 4;
     507    const unsigned short PLATFORM_LAYER_TREE_INCLUDE_MODELS = 4;
    508508    DOMString platformLayerTreeAsText(Element element, optional unsigned short flags = 0);
    509509

  • trunk/Source/WebKit/ChangeLog

    r276545 r276562  
     12021-04-24  Tim Horton  <timothy_horton@apple.com>
     2
     3        Changing the source of a model element with clipping applied does not update the model
     4        https://bugs.webkit.org/show_bug.cgi?id=224917
     5
     6        Reviewed by Simon Fraser.
     7
     8        * WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemoteModelHosting.h:
     9        * WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemoteModelHosting.mm:
     10        (WebKit::PlatformCALayerRemoteModelHosting::dumpAdditionalProperties):
     11        Add a bit to platformLayerTreeAsText() that makes PlatformCALayerRemoteModelHosting
     12        dump the size of the model that it is hosting, which is used in the test for this bug.
     13
    1142021-04-23  Kate Cheney  <katherine_cheney@apple.com>
    215

  • trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemoteModelHosting.h

    r274033 r276562  
    4343   
    4444    void populateCreationProperties(RemoteLayerTreeTransaction::LayerCreationProperties&, const RemoteLayerTreeContext&, WebCore::PlatformCALayer::LayerType) override;
     45   
     46    void dumpAdditionalProperties(TextStream&, OptionSet<PlatformLayerTreeAsTextFlags>) final;
    4547
    4648    Ref<Model> m_model;

  • trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemoteModelHosting.mm

    r274033 r276562  
    6767}
    6868
     69void PlatformCALayerRemoteModelHosting::dumpAdditionalProperties(TextStream& ts, OptionSet<PlatformLayerTreeAsTextFlags> flags)
     70{
     71    if (flags.contains(PlatformLayerTreeAsTextFlags::IncludeModels))
     72        ts << indent << "(model data size " << m_model->data()->size() << ")\n";
     73}
     74
    6975} // namespace WebKit
Note: See TracChangeset for help on using the changeset viewer.