Bug 145843 – CSS Scroll Snap - support snapping to nested elements

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

[Majid Valipour]

According to the current Snap Points Spec draft[1] an scrolling container may snap to snap coordinates that are defined on any of its descendant. Here is the relevant language from the spec:
"The scroll-snap-coordinate property is used to define the x and y coordinate within the element which will align with the *nearest ancestor* scroll container’s snap-destination for the respective axis."

The current Webkit implementation only snaps to snap points defined on immediate children. Lots of interesting layouts outside simple image galleries require in-between layers between scroller and the actual items to provide structure. Firefox implementation of snap points support snapping to arbitrary descendant as opposed to only immediate children.

Here is an simple repro: http://majido.github.io/snap-points/tests/element-snap-coordinates.html
Elements are indented according to their nesting level. Webkit nightly only snaps to black elements (i.e. immediate children) while Firefox snaps to all elements.

[Simon Fraser (smfr)]

There was css-wg discussion around this, and the resolution was to bring "elements" back:
https://lists.w3.org/Archives/Public/www-style/2015May/0282.html

[Majid Valipour]

Thanks for the link.

If I understand that correctly the 'elements' makes it so that the scrolling container has to be explicit when it is interested in capturing the snap points from descending children. This makes a sense.

However, it is still the case that the snap points are allowed to be defined by any descendants (and not just immediate children). That means the following should still snap:


#snap-container {
  scroll-snap-points-y: elements;
  scroll-snap-type: mandatory;
}

#snap-element {
  scroll-snap-coordinates: 50% 50%;
}

<div id='snap-container'>
   <div>
      <p id='snap-element'></p> 
   <div>
</div>

[Simon Fraser (smfr)]

Yes, you're correct. We should fix this!

[Radar WebKit Bug Importer]

<rdar://problem/21339581>

[Brent Fulgham]

Created attachment 254858 [details]
Patch

[Brent Fulgham]

Comment on attachment 254858 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Tested by css3/scroll-snap/nested-elements.html

This test is not part of this patch. I'm debugging the test separately.

[Brent Fulgham]

Created attachment 254863 [details]
Patch v2 (Includes Test)

[Brent Fulgham]

(In reply to comment #0)
> 
> Here is an simple repro:
> http://majido.github.io/snap-points/tests/element-snap-coordinates.html
> Elements are indented according to their nesting level. Webkit nightly only
> snaps to black elements (i.e. immediate children) while Firefox snaps to all
> elements.

Thanks for the great test case! I've incorporated it into the tests now.

[Simon Fraser (smfr)]

Comment on attachment 254863 [details]
Patch v2 (Includes Test)

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

> Source/WebCore/dom/Document.cpp:6693
> +void Document::addElementToScrollSnapContainer(Element* scrollSnapElement, Element* scrollSnapContainer)
> +{
> +    auto& containerElements = m_scrollSnapElementsForContainers.add(scrollSnapContainer, HashSet<Element*>()).iterator->value;
> +    containerElements.add(scrollSnapElement);
> +}
> +
> +const HashSet<Element*>* Document::elementsForScrollSnapContainer(Element* scrollSnapContainer)
> +{
> +    auto containerElementsIterator = m_scrollSnapElementsForContainers.find(scrollSnapContainer);
> +    if (containerElementsIterator == m_scrollSnapElementsForContainers.end())
> +        return nullptr;
> +
> +    return &containerElementsIterator->value;
> +}
> +
> +void Document::removeElementFromScrollSnapContainer(Element* scrollSnapElement, Element* scrollSnapContainer)
> +{
> +    auto containerElementsIterator = m_scrollSnapElementsForContainers.find(scrollSnapContainer);
> +    if (containerElementsIterator == m_scrollSnapElementsForContainers.end())
> +        return;
> +
> +    if (containerElementsIterator->value.isEmpty())
> +        return;
> +
> +    containerElementsIterator->value.remove(scrollSnapElement);
> +
> +    if (containerElementsIterator->value.isEmpty())
> +        m_scrollSnapElementsForContainers.remove(scrollSnapContainer);
> +}

I'm a bit concerned here that the only thing that removes these raw element pointers from this map are when renderers are removed from the tree.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:63
> +        LayoutRect viewSize = box->contentBoxRect();
> +        LayoutUnit viewWidth = viewSize.width();
> +        LayoutUnit viewHeight = viewSize.height();
> +        FloatPoint position = box->localToContainerPoint(FloatPoint(), parent.renderBox());

We have to think about containing block here. What happens with:

<div style="position:absolute">
  <div style="overflow:scroll">
   <div style="position: absolute; scroll-snap-coordinate: 10px 10px;>

Here the containing block of the inner div is the outer div, skipping overflow, and the children don't scroll.

> Source/WebCore/rendering/RenderElement.cpp:126
> +    // Find the first node with a valid scrollable area starting with the current
> +    // node and traversing its parents (or shadow hosts).
> +    for (Element* candidate = node; candidate; candidate = candidate->parentElement()) {
> +        RenderBox* box = candidate->renderBox();
> +        if (box && box->layer())
> +            return candidate;
> +    }
> +    
> +    return nullptr;

This isn't correct. You need to find the ancestor with overflow, since there are other reasons a renderer has a layer (e.g. opacity: 0.5). You should add a testcase for this.

> Source/WebCore/rendering/RenderElement.cpp:165
> +        if (!m_style->scrollSnapCoordinates().isEmpty()) {
> +            Element* scrollSnapElement = element();
> +            Element* scrollSnapContainer = findEnclosingScrollableContainer(scrollSnapElement);
> +            
> +            if (scrollSnapElement && scrollSnapContainer)
> +                document().removeElementFromScrollSnapContainer(scrollSnapElement, scrollSnapContainer);
> +        }

I think you should do this in RenderElement::willBeRemovedFromTree()

[Brent Fulgham]

Comment on attachment 254863 [details]
Patch v2 (Includes Test)

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

>> Source/WebCore/dom/Document.cpp:6693
>> +}
> 
> I'm a bit concerned here that the only thing that removes these raw element pointers from this map are when renderers are removed from the tree.

Can you suggest any other likely points where I could do this work? We also remove these pointers when the style changes such that it no longer has scroll coordinates.

Or are you suggesting that they be captured as Ref or RefPtr?

>> Source/WebCore/rendering/RenderElement.cpp:126
>> +    return nullptr;
> 
> This isn't correct. You need to find the ancestor with overflow, since there are other reasons a renderer has a layer (e.g. opacity: 0.5). You should add a testcase for this.

OK!

>> Source/WebCore/rendering/RenderElement.cpp:165
>> +        }
> 
> I think you should do this in RenderElement::willBeRemovedFromTree()

OK!

[Brent Fulgham]

Created attachment 254894 [details]
Patch

[Brent Fulgham]

Created attachment 254900 [details]
Patch

[Brent Fulgham]

Created attachment 254901 [details]
Patch v3 (Correct typo)

[Brent Fulgham]

Comment on attachment 254900 [details]
Patch

This had a horrible typo in it!

[Simon Fraser (smfr)]

Comment on attachment 254900 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6665
> +void Document::addElementToScrollSnapContainer(const RenderElement* scrollSnapElement, const RenderElement* scrollSnapContainer)

The names are wrong; you're not adding Elements any more.

Since this now tracks renderers, I think it should move to RenderView.

> Source/WebCore/rendering/RenderElement.cpp:914
> +#if ENABLE(CSS_SCROLL_SNAP)
> +    if (!newStyle.scrollSnapCoordinates().isEmpty() || (oldStyle && !oldStyle->scrollSnapCoordinates().isEmpty())) {
> +        if (newStyle.scrollSnapCoordinates().isEmpty())
> +            document().addElementNeedingScrollSnapContainerUnregistration(this);
> +        else
> +            document().addElementNeedingScrollSnapContainerRegistration(this);
> +    }
> +#endif

You can't do it like this. RenderElement is not ref-counted, so nothing keeps it alive.

> Source/WebCore/rendering/RenderElement.cpp:1781
> +const RenderElement* 10100() const

10100!

> LayoutTests/ChangeLog:12
> +        * css3/scroll-snap/nested-elements-expected.txt: Added.
> +        * css3/scroll-snap/nested-elements.html: Added.
> +        * css3/scroll-snap/scroll-snap-offsets-expected.txt: Updated to
> +          account for 50%/50% scroll-snap-coordinates.

I would like to see tests that:

1. programmatically change scroll-snap-coordinate style
2. programmatically change overflow:scroll on ancestors of scroll-snap-coordinate elements
3. programmatically remove DOM nodes which are scrolling containers, and have scroll-snap-coordinates

[Brent Fulgham]

(In reply to comment #15)
> > Source/WebCore/rendering/RenderElement.cpp:914
> > +#if ENABLE(CSS_SCROLL_SNAP)
> > +    if (!newStyle.scrollSnapCoordinates().isEmpty() || (oldStyle && !oldStyle->scrollSnapCoordinates().isEmpty())) {
> > +        if (newStyle.scrollSnapCoordinates().isEmpty())
> > +            document().addElementNeedingScrollSnapContainerUnregistration(this);
> > +        else
> > +            document().addElementNeedingScrollSnapContainerRegistration(this);
> > +    }
> > +#endif
> 
> You can't do it like this. RenderElement is not ref-counted, so nothing
> keeps it alive.

Do you mean these should be held as Elements instead? Or is there a way to keep RenderElements alive?

[Simon Fraser (smfr)]

> Do you mean these should be held as Elements instead? Or is there a way to
> keep RenderElements alive?

I think you should just keep a HashSet of RenderElements with scroll-snap-coordinate.

[Brent Fulgham]

(In reply to comment #15)

> I would like to see tests that:
> 
> 1. programmatically change scroll-snap-coordinate style
> 2. programmatically change overflow:scroll on ancestors of
> scroll-snap-coordinate elements
> 3. programmatically remove DOM nodes which are scrolling containers, and
> have scroll-snap-coordinates

Also:

4. Test multiple scroll-snap-coordinates on a single element. (List of coordinates).

[Brent Fulgham]

Created attachment 254911 [details]
Patch

[Darin Adler]

Comment on attachment 254911 [details]
Patch

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

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:50
> +    RenderView* renderView = scrollContainer->frame().view()->renderView();
> +    if (!renderView)
> +        return;

This seems strange. RenderElement has a function view() that returns a RenderView&. Why would we instead go all the way “around the horn” to the FrameView to get it? Is this possibly a different RenderView? Do we have a case where the RenderView is null? Test cases covering it?

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:52
> +    Vector<const RenderElement*> elements;

I suggest Vector<const RenderBox*> instead. The type check should go in the first loop instead of the second loop.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:53
> +    for (const auto* element : renderView->renderElementsWithScrollSnapCoordinates()) {

I suggest auto* instead of const auto*.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:61
> +        ASSERT(child);

This assertion is not needed. We just built this vector above and we didn’t put any nulls in it. Also, the assert below also does the appropriate type check.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:63
> +        const RenderBox* box = downcast<RenderBox>(child);

I suggest auto* instead of const RenderBox*. But also, I suggest a reference instead.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:65
> +        if (!box)
> +            continue;

Not needed. The code that guarantees this is non-null is really close above. And downcast will not return null if it’s the wrong type, so this doesn’t help for that either.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:67
> +        const auto& scrollSnapCoordinates = box->style().scrollSnapCoordinates();

I think just auto& should do.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:73
> +        LayoutUnit viewWidth = viewSize.width();
> +        LayoutUnit viewHeight = viewSize.height();

Existing code, but not sure these locals make the code easier to read.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:76
> +        LayoutUnit left = position.x();
> +        LayoutUnit top = position.y();

Existing code, but not sure these locals make the code easier to read.

> Source/WebCore/rendering/RenderElement.cpp:909
> +        ASSERT(frame().view());

I’m not sure this assertion is valuable. If there’s any reason to doubt this is true, then we need code to handle it, not an assertion, and if not, then I think it’s overkill.

> Source/WebCore/rendering/RenderElement.cpp:910
> +        if (RenderView* renderView = frame().view()->renderView()) {

Same comment as above about going a long way to get to the RenderView.

> Source/WebCore/rendering/RenderElement.cpp:1076
> +        ASSERT(frame().view());

Same comment as above about the assertion.

> Source/WebCore/rendering/RenderElement.cpp:1077
> +        if (RenderView* renderView = frame().view()->renderView())

Same comment as above about going a long way to get to the RenderView.

> Source/WebCore/rendering/RenderElement.cpp:1796
> +    for (const RenderElement* candidate = this; candidate; candidate = candidate->parent()) {
> +        if (!is<RenderBox>(*candidate))
> +            continue;
> +        
> +        const RenderBox* box = downcast<RenderBox>(candidate);
> +        if (box && box->hasOverflowClip())
> +            return candidate;
> +    }
> +    
> +    return nullptr;

The modern way to write this is more like this:

    for (auto& box : lineageOfType<RenderBox>(*this)) {
        if (box->hasOverflowClip())
            return box;
    }
    return nullptr;

> Source/WebCore/rendering/RenderElement.h:211
> +    const RenderElement* findEnclosingScrollableContainer() const;

Return type should be RenderBox*, not RenderElement*.

I also think this function should probably go on RenderBox rather than RenderElement.

> Source/WebCore/rendering/RenderView.h:250
> +    void registerRenderElementWithScrollSnapCoordinates(const RenderElement*);
> +    void unregisterRenderElementWithScrollSnapCoordinates(const RenderElement*);

These should take RenderBox&; can’t be null and always need to be boxes. Also, I don’t think we need the word “render” in this function name.

It doesn’t seem smart to keep elements that are not boxes registered; we know we don’t need to do anything with them!

> Source/WebCore/rendering/RenderView.h:251
> +    const HashSet<const RenderElement*>& renderElementsWithScrollSnapCoordinates() { return m_renderElementsWithScrollSnapCoordinates; }

Seems a bit strange to expose a const& to the HashSet. I wonder if there’s a cleaner way to do this. The current client only iterates this to make a vector. Maybe we could change this to return a vector, or to take a function and call it on each element. I guess it’s OK like this, but seems a tiny bit inflexible to tie it to HashSet specifically.

Maybe this should take a RenderElement* and return a vector of the elements in that scroll container.

> Source/WebCore/rendering/RenderView.h:377
> +    HashSet<const RenderElement*> m_renderElementsWithScrollSnapCoordinates;

I suggest RenderBox instead of RenderElement. Also, I am not convinced that having these be const is valuable. I don’t think the member name needs the word “render” in it.

> WebKit.xcworkspace/xcshareddata/xcschemes/All Source (target WebProcess).xcscheme:157
> +      <AdditionalOptions>
> +      </AdditionalOptions>

Intentionally included in patch, or by accident?

> WebKit.xcworkspace/xcshareddata/xcschemes/All Source (target WebProcess).xcscheme:170
> +      debugServiceExtension = "internal"
>        allowLocationSimulation = "YES">
> -      <BuildableProductRunnable>
> +      <BuildableProductRunnable
> +         runnableDebuggingMode = "0">

Intentionally included in patch, or by accident?

[Brent Fulgham]

Comment on attachment 254911 [details]
Patch

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

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:50
>> +        return;
> 
> This seems strange. RenderElement has a function view() that returns a RenderView&. Why would we instead go all the way “around the horn” to the FrameView to get it? Is this possibly a different RenderView? Do we have a case where the RenderView is null? Test cases covering it?

Nope. I just didn't notice the view() method in RenderObject and thought this was the right way. Thank you for pointing this out!

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:53
>> +    for (const auto* element : renderView->renderElementsWithScrollSnapCoordinates()) {
> 
> I suggest auto* instead of const auto*.

Will do!

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:61
>> +        ASSERT(child);
> 
> This assertion is not needed. We just built this vector above and we didn’t put any nulls in it. Also, the assert below also does the appropriate type check.

Ok!

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:63
>> +        const RenderBox* box = downcast<RenderBox>(child);
> 
> I suggest auto* instead of const RenderBox*. But also, I suggest a reference instead.

OK!

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:65
>> +            continue;
> 
> Not needed. The code that guarantees this is non-null is really close above. And downcast will not return null if it’s the wrong type, so this doesn’t help for that either.

OK!

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:67
>> +        const auto& scrollSnapCoordinates = box->style().scrollSnapCoordinates();
> 
> I think just auto& should do.

OK!

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:73
>> +        LayoutUnit viewHeight = viewSize.height();
> 
> Existing code, but not sure these locals make the code easier to read.

I'll get rid of them.

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:76
>> +        LayoutUnit top = position.y();
> 
> Existing code, but not sure these locals make the code easier to read.

I'll get rid of them.

>> Source/WebCore/rendering/RenderElement.cpp:909
>> +        ASSERT(frame().view());
> 
> I’m not sure this assertion is valuable. If there’s any reason to doubt this is true, then we need code to handle it, not an assertion, and if not, then I think it’s overkill.

OK!

>> Source/WebCore/rendering/RenderElement.cpp:910
>> +        if (RenderView* renderView = frame().view()->renderView()) {
> 
> Same comment as above about going a long way to get to the RenderView.

OK!

>> Source/WebCore/rendering/RenderElement.cpp:1076
>> +        ASSERT(frame().view());
> 
> Same comment as above about the assertion.

OK!

>> Source/WebCore/rendering/RenderElement.cpp:1077
>> +        if (RenderView* renderView = frame().view()->renderView())
> 
> Same comment as above about going a long way to get to the RenderView.

OK!

>> Source/WebCore/rendering/RenderElement.cpp:1796
>> +    return nullptr;
> 
> The modern way to write this is more like this:
> 
>     for (auto& box : lineageOfType<RenderBox>(*this)) {
>         if (box->hasOverflowClip())
>             return box;
>     }
>     return nullptr;

That's much nicer. I'll revise to use this syntax.

>> Source/WebCore/rendering/RenderElement.h:211
>> +    const RenderElement* findEnclosingScrollableContainer() const;
> 
> Return type should be RenderBox*, not RenderElement*.
> 
> I also think this function should probably go on RenderBox rather than RenderElement.

OK.

>> Source/WebCore/rendering/RenderView.h:250
>> +    void unregisterRenderElementWithScrollSnapCoordinates(const RenderElement*);
> 
> These should take RenderBox&; can’t be null and always need to be boxes. Also, I don’t think we need the word “render” in this function name.
> 
> It doesn’t seem smart to keep elements that are not boxes registered; we know we don’t need to do anything with them!

Sure, that seems smart. I'll revise appropriately.

>> Source/WebCore/rendering/RenderView.h:251
>> +    const HashSet<const RenderElement*>& renderElementsWithScrollSnapCoordinates() { return m_renderElementsWithScrollSnapCoordinates; }
> 
> Seems a bit strange to expose a const& to the HashSet. I wonder if there’s a cleaner way to do this. The current client only iterates this to make a vector. Maybe we could change this to return a vector, or to take a function and call it on each element. I guess it’s OK like this, but seems a tiny bit inflexible to tie it to HashSet specifically.
> 
> Maybe this should take a RenderElement* and return a vector of the elements in that scroll container.

I like this because it avoids building and copying information here. I guess I could pass in the relevant scroll container and do the logic here, but that would move a bunch of scroll-snap-specific stuff into this file.

I had an earlier version of this patch that stored individual HashSets for each scroll container, but we ended up abandoning that because the point where we identify these scroll-snap-coordinate elements is before layout has happened, so we can't reliably find the relevant containing element.

>> Source/WebCore/rendering/RenderView.h:377
>> +    HashSet<const RenderElement*> m_renderElementsWithScrollSnapCoordinates;
> 
> I suggest RenderBox instead of RenderElement. Also, I am not convinced that having these be const is valuable. I don’t think the member name needs the word “render” in it.

OK!

>> WebKit.xcworkspace/xcshareddata/xcschemes/All Source (target WebProcess).xcscheme:157
>> +      </AdditionalOptions>
> 
> Intentionally included in patch, or by accident?

Not intentional.

>> WebKit.xcworkspace/xcshareddata/xcschemes/All Source (target WebProcess).xcscheme:170
>> +         runnableDebuggingMode = "0">
> 
> Intentionally included in patch, or by accident?

Not intentional!

[Brent Fulgham]

Committed r185606: <http://trac.webkit.org/changeset/185606>