Bug 219073 - Make scroll-padding independent of scroll-snap and have it affect scrollIntoView
Summary: Make scroll-padding independent of scroll-snap and have it affect scrollIntoView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords: InRadar
Depends on:
Blocks: 179379
  Show dependency treegraph
 
Reported: 2020-11-18 02:12 PST by Martin Robinson
Modified: 2020-11-19 06:01 PST (History)
14 users (show)

See Also:


Attachments
Patch (37.48 KB, patch)
2020-11-18 03:43 PST, Martin Robinson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (38.38 KB, patch)
2020-11-18 06:29 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (37.10 KB, patch)
2020-11-19 00:55 PST, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2020-11-18 02:12:19 PST
The scroll-snap specification [1] says that scroll-padding should be applied independently of scroll-snap and that it should affect scrolling operations.

1. https://drafts.csswg.org/css-scroll-snap-1/#propdef-scroll-padding
Comment 1 Martin Robinson 2020-11-18 03:43:17 PST
Created attachment 414436 [details]
Patch
Comment 2 Martin Robinson 2020-11-18 06:29:29 PST
Created attachment 414445 [details]
Patch
Comment 3 Simon Fraser (smfr) 2020-11-18 10:34:39 PST
Comment on attachment 414445 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:601
> +static void expandScrollRectToVisibleTargetRectToIncludeScrollPadding(RenderBox* renderBox, const LayoutRect& viewRect, LayoutRect* targetRect)

targetRect should be a LayoutRect&

> Source/WebCore/rendering/RenderLayer.cpp:2825
> +AnimatedScroll RenderLayer::shouldUseAnimatedScrollForScrollRectToVisible(Element* element, ScrollBehavior behavior)
> +{
> +    bool useAnimatedScrolling = !renderer().frame().eventHandler().autoscrollInProgress()
> +        && element && useSmoothScrolling(behavior, element);
> +    return useAnimatedScrolling ? AnimatedScroll::Yes : AnimatedScroll::No;
> +}

This could just be a lambda function in scrollRectToVisible()

> Source/WebCore/rendering/RenderLayer.cpp:2858
> +            AnimatedScroll animated = this->shouldUseAnimatedScrollForScrollRectToVisible(box->element(), options.behavior);

auto animated

> Source/WebCore/rendering/RenderLayer.cpp:2890
> +                AnimatedScroll animated = this->shouldUseAnimatedScrollForScrollRectToVisible(element, options.behavior);

auto animated

> Source/WebCore/rendering/RenderLayerModelObject.cpp:125
> +    return oldStyle.scrollPadding() != newStyle.scrollPadding() || oldStyle.scrollSnapType() != newStyle.scrollSnapType();

This should not all be inside the #if ENABLE(CSS_SCROLL_SNAP) right?

> Source/WebCore/rendering/style/RenderStyle.cpp:2498
> +const ScrollSnapType& RenderStyle::scrollSnapType() const

Just return by value. It's an enum.

> Source/WebCore/rendering/style/RenderStyle.cpp:2503
> +const ScrollSnapAlign& RenderStyle::scrollSnapAlign() const

Return by value.

> Source/WebCore/rendering/style/RenderStyle.cpp:2508
> +void RenderStyle::setScrollSnapType(const ScrollSnapType& type)

Argument should be a ScrollSnapType

> Source/WebCore/rendering/style/RenderStyle.h:736
> +    const ScrollSnapType& scrollSnapType() const;
>      const ScrollSnapAlign& scrollSnapAlign() const;

Return by value.

> Source/WebCore/rendering/style/RenderStyle.h:1276
> +    void setScrollSnapType(const ScrollSnapType&);
>      void setScrollSnapAlign(const ScrollSnapAlign&);

Pass values not references.
Comment 4 Martin Robinson 2020-11-19 00:55:09 PST
Created attachment 414547 [details]
Patch
Comment 5 Martin Robinson 2020-11-19 00:58:52 PST
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 414445 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414445&action=review

Thanks for the review. I've applied all your suggestions, apart from one:

> > Source/WebCore/rendering/RenderLayerModelObject.cpp:125
> > +    return oldStyle.scrollPadding() != newStyle.scrollPadding() || oldStyle.scrollSnapType() != newStyle.scrollSnapType();
> 
> This should not all be inside the #if ENABLE(CSS_SCROLL_SNAP) right?

This helper function is still only used by the scroll snap code. It's possible that this should be moved to the callsite in a cleanup change.
Comment 6 Martin Robinson 2020-11-19 05:47:54 PST
Comment on attachment 414547 [details]
Patch

Landing as the failures are unrelated to this change.
Comment 7 EWS 2020-11-19 06:00:20 PST
Committed r270023: <https://trac.webkit.org/changeset/270023>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414547 [details].
Comment 8 Radar WebKit Bug Importer 2020-11-19 06:01:17 PST
<rdar://problem/71584492>