Bug 219073

Summary:

Make scroll-padding independent of scroll-snap and have it affect scrollIntoView

Product:

WebKit

Reporter:

Martin Robinson <mrobinson>

Component:

CSS

Assignee:

Martin Robinson <mrobinson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

changseok, diane.ko, esprehn+autocc, ews-watchlist, fred.wang, glenn, gyuyoung.kim, joepeck, kondapallykalyan, macpherson, menard, pdr, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=219498

Bug Depends on:

Bug Blocks:

179379

Attachments:

Description	Flags
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none

Martin Robinson

Reported 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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Martin Robinson

Comment 1 2020-11-18 03:43:17 PST

Created attachment 414436 [details] Patch

Martin Robinson

Comment 2 2020-11-18 06:29:29 PST

Created attachment 414445 [details] Patch

Simon Fraser (smfr)

Comment 3 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.

Martin Robinson

Comment 4 2020-11-19 00:55:09 PST

Created attachment 414547 [details] Patch

Martin Robinson

Comment 5 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.

Martin Robinson

Comment 6 2020-11-19 05:47:54 PST

Comment on attachment 414547 [details] Patch Landing as the failures are unrelated to this change.

EWS

Comment 7 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].

Radar WebKit Bug Importer

Comment 8 2020-11-19 06:01:17 PST

<rdar://problem/71584492>

Note You need to log in before you can comment on or make changes to this bug.