Bug 135964 – Snap offsets should update when style is programmatically changed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Wenson Hsieh]

When style is changed programmatically (i.e. through JavaScript) snap offsets should be properly updated. Currently, this only happens upon layout (see RenderLayer::updateSnapOffsets) and would therefore not respond correctly to changes in style that do not require layout.

[Wenson Hsieh]

We'll want to take a look at RenderStyle::diff to address this issue.

[Radar WebKit Bug Importer]

<rdar://problem/18162411>

[Wenson Hsieh]

Created attachment 256183 [details]
Test pages for programmatic style updates in mainframe and overflow scroll snapping

Press the buttons on the top left to toggle element styles for scroll snapping.

[Wenson Hsieh]

Created attachment 256213 [details]
Patch

[Wenson Hsieh]

WIP patch -- still some other cases to test, such as removing repeat() on a scroll snap container when there are children with scroll snap coordinates.

[Wenson Hsieh]

Created attachment 256276 [details]
Patch

[Brent Fulgham]

Comment on attachment 256276 [details]
Patch

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

I have a few cleanup suggestions, but otherwise this looks good. r- to correct some minor readability issues.

> Source/WebCore/rendering/RenderElement.cpp:1500
> +    bool foundScrollSnapLayer = false;

I don't think you need this variable.

> Source/WebCore/rendering/RenderElement.cpp:1504
> +            foundScrollSnapLayer = true;

I think you could just return currentLayer in this case.

> Source/WebCore/rendering/RenderElement.cpp:1513
> +    return foundScrollSnapLayer ? currentLayer : nullptr;

This could just be return nullptr.

> Source/WebCore/rendering/RenderLayerModelObject.cpp:177
> +        || oldStyle->scrollSnapDestination().height() != newStyle.scrollSnapDestination().height())) {

This should be encapsulated in a method "static bool scrollSnapStyleIsEqual(oldStyle, newStyle)", or maybe add it to RenderStyle. Then this would just be "if (!scrollSnapStyleIsEqual(oldStyle, newStyle)"

[Wenson Hsieh]

Comment on attachment 256276 [details]
Patch

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

Thanks for taking a look at my patch! I chatted with Simon today and learned that I should be traversing up the tree through RenderBlocks via containingBlock rather than through enclosing RenderLayers, so I refactored RenderElement::findEnclosingScrollSnapLayer to do so. I've made the changes you pointed out in the new implementation. I'll have a new patch up soon.

>> Source/WebCore/rendering/RenderElement.cpp:1500
>> +    bool foundScrollSnapLayer = false;
> 
> I don't think you need this variable.

Fixed! See overall comment.

>> Source/WebCore/rendering/RenderElement.cpp:1504
>> +            foundScrollSnapLayer = true;
> 
> I think you could just return currentLayer in this case.

Fixed!

>> Source/WebCore/rendering/RenderElement.cpp:1513
>> +    return foundScrollSnapLayer ? currentLayer : nullptr;
> 
> This could just be return nullptr.

Fixed!

>> Source/WebCore/rendering/RenderLayerModelObject.cpp:177
>> +        || oldStyle->scrollSnapDestination().height() != newStyle.scrollSnapDestination().height())) {
> 
> This should be encapsulated in a method "static bool scrollSnapStyleIsEqual(oldStyle, newStyle)", or maybe add it to RenderStyle. Then this would just be "if (!scrollSnapStyleIsEqual(oldStyle, newStyle)"

Fixed! Abstracted logic out to scrollSnapContainerRequiresUpdateForStyleUpdate(oldStyle, newStyle).

[Wenson Hsieh]

Created attachment 256357 [details]
Patch

[Simon Fraser (smfr)]

(In reply to comment #8)
> Comment on attachment 256276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256276&action=review
> 
> Thanks for taking a look at my patch! I chatted with Simon today and learned
> that I should be traversing up the tree through RenderBlocks via
> containingBlock rather than through enclosing RenderLayers, so I refactored
> RenderElement::findEnclosingScrollSnapLayer to do so.

This needs to be covered by a layout test, like I mentioned:

<div style="position: absolute">
  <div style="overflow, snapping...">
    <div style="scroll-snap-coordinate:...; position: absolute">
...

[Wenson Hsieh]

Created attachment 256383 [details]
Patch

[Wenson Hsieh]

> This needs to be covered by a layout test, like I mentioned:
> 
> <div style="position: absolute">
>   <div style="overflow, snapping...">
>     <div style="scroll-snap-coordinate:...; position: absolute">
> ...

Thanks for the reminder. Changed the second container in the test case to test this case.

[Simon Fraser (smfr)]

Comment on attachment 256383 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1509
> +RenderLayer* RenderElement::findEnclosingScrollSnapLayer() const
> +{
> +    RenderBlock* currentBlock = containingBlock();
> +    while (currentBlock) {
> +        if (currentBlock->style().scrollSnapType() != ScrollSnapType::None)
> +            return currentBlock->layer();
> +        
> +        if (currentBlock == currentBlock->containingBlock())
> +            break;
> +
> +        currentBlock = currentBlock->containingBlock();
> +    }
> +    return nullptr;

I don't think this should be returning RenderLayers; it should be returning RenderElements (or RenderBlocks).

> LayoutTests/css3/scroll-snap/scroll-snap-style-changes.html:118
> +    <body onload="setup();">
> +        <div id="repeat-gallery" class="gallery">
> +            <div class="horizontal hchild00"></div><div class="horizontal hchild01"></div><div class="horizontal hchild02"></div><div class="horizontal hchild03"></div><div class="horizontal hchild04"></div><div class="horizontal hchild05"></div>
> +        </div>
> +        <div id="coordinates-gallery" class="gallery">
> +            <div class="snap-coord horizontal hchild00" id="child00"></div><div class="snap-coord horizontal hchild01" id="child01"></div><div class="snap-coord horizontal hchild02" id="child02"></div><div class="snap-coord horizontal hchild03" id="child03"></div><div class="snap-coord horizontal hchild04" id="child04"></div><div class="snap-coord horizontal hchild05" id="child05"></div>
> +        </div>
> +        <script src="../../resources/js-test-post.js"></script>
> +    </body>

There's too much "gallery" in this test. Please put each <div> on its own line. I think this should be two separate tests.

[Wenson Hsieh]

Comment on attachment 256383 [details]
Patch

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

>> Source/WebCore/rendering/RenderElement.cpp:1509
>> +    return nullptr;
> 
> I don't think this should be returning RenderLayers; it should be returning RenderElements (or RenderBlocks).

Got it. Refactored to return a RenderBlock*

>> LayoutTests/css3/scroll-snap/scroll-snap-style-changes.html:118
>> +    </body>
> 
> There's too much "gallery" in this test. Please put each <div> on its own line. I think this should be two separate tests.

Got it. Split the test into two separate cases.

[Wenson Hsieh]

Created attachment 256482 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 256482 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1495
> +// May return null if there is no such layer (e.g. setting a node to have scroll snap coordinates

Comment refers to layers.

> Source/WebCore/rendering/RenderLayerModelObject.cpp:132
> +    return oldStyle.scrollSnapType() != newStyle.scrollSnapType() || oldStyle.scrollSnapPointsX() != newStyle.scrollSnapPointsX()
> +        || oldStyle.scrollSnapPointsY() != newStyle.scrollSnapPointsY() || oldStyle.scrollSnapDestination().width() != newStyle.scrollSnapDestination().width()
> +        || oldStyle.scrollSnapDestination().height() != newStyle.scrollSnapDestination().height();

Please put one condition per line.

> Source/WebCore/rendering/RenderLayerModelObject.cpp:199
> +    if (oldStyle && oldStyle->scrollSnapCoordinates() != newStyle.scrollSnapCoordinates()) {
> +        RenderBlock* scrollSnapBlock = enclosingBox().findEnclosingScrollSnapBlock();
> +        if (scrollSnapBlock && scrollSnapBlock->layer()) {
> +            scrollSnapBlock->layer()->updateSnapOffsets();
> +            scrollSnapBlock->layer()->updateScrollSnapState();
> +        }

I don't think this is enough. Style changes could cause a scroll-snap-coordinate node to get re-hsoted by a different scroller, and this code doesn't unregister it from the old one.

[Michał Gołębiowski-Owczarek]

The first comment suggests snap offsets are not updated when changed via JS and no layout occurs. However, I don't see the change working even if I trigger the layout afterwards; see e.g.: http://output.jsbin.com/micumar/11. This test case contains a couple of elements with height 100vh and a scroll event handler that disables CSS scroll snap after you scroll down more than 5000 pixels. Firefox reacts to the change while Safari/Edge still snap.

[Wenson Hsieh]

Created attachment 261979 [details]
Patch

[Darin Adler]

Comment on attachment 261979 [details]
Patch

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

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:72
> +            snapOffsetsAsFloat.append(roundToDevicePixel(offset, deviceScaleFactor, false));

Should be uncheckedAppend since we already did reserveInitialCapacity.

> Source/WebCore/rendering/RenderElement.h:256
> -
> +    

Please don’t land this change.

> Source/WebCore/rendering/RenderLayerModelObject.cpp:134
> +        || oldStyle.scrollSnapDestination().width() != newStyle.scrollSnapDestination().width()
> +        || oldStyle.scrollSnapDestination().height() != newStyle.scrollSnapDestination().height();

A size can be compared directly with !=; we don’t have to check width and height separately.

[Wenson Hsieh]

Comment on attachment 261979 [details]
Patch

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

>> Source/WebCore/rendering/RenderElement.h:256
>> +    
> 
> Please don’t land this change.

Sorry -- fixed.

>> Source/WebCore/rendering/RenderLayerModelObject.cpp:134
>> +        || oldStyle.scrollSnapDestination().height() != newStyle.scrollSnapDestination().height();
> 
> A size can be compared directly with !=; we don’t have to check width and height separately.

Got it. I'll compare the sizes directly.

[Wenson Hsieh]

Created attachment 261988 [details]
Patch

[Darin Adler]

Comment on attachment 261988 [details]
Patch

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

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:621
> +    if (ScrollingStateFrameScrollingNode* node = downcast<ScrollingStateFrameScrollingNode>(m_scrollingStateTree->stateNodeForID(frameView.scrollLayerID()))) {

I would use auto here instead of repeating the type name twice.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:4271
> +    if (ScrollingCoordinator* scrollingCoordinator = this->scrollingCoordinator())
> +        scrollingCoordinator->updateScrollSnapPropertiesWithFrameView(frameView);

I would write:

    if (auto* coordinator = scrollingCoordinator())
        coordinator->updateScrollSnapPropertiesWithFrameView(frameView);

[Wenson Hsieh]

Comment on attachment 261988 [details]
Patch

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

Thanks for reviewing my patch! I'll land it soon.

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:621
>> +    if (ScrollingStateFrameScrollingNode* node = downcast<ScrollingStateFrameScrollingNode>(m_scrollingStateTree->stateNodeForID(frameView.scrollLayerID()))) {
> 
> I would use auto here instead of repeating the type name twice.

I'll change this.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:4271
>> +        scrollingCoordinator->updateScrollSnapPropertiesWithFrameView(frameView);
> 
> I would write:
> 
>     if (auto* coordinator = scrollingCoordinator())
>         coordinator->updateScrollSnapPropertiesWithFrameView(frameView);

Got it. I'll change this.

[Wenson Hsieh]

Committed r190330: <http://trac.webkit.org/changeset/190330>