WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
219073
Make scroll-padding independent of scroll-snap and have it affect scrollIntoView
https://bugs.webkit.org/show_bug.cgi?id=219073
Summary
Make scroll-padding independent of scroll-snap and have it affect scrollIntoView
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.
Top of Page
Format For Printing
XML
Clone This Bug