The specification [1] reads: These offsets reduce the region of the scrollport that is considered “viewable” for scrolling operations: they have no effect on layout, on the scroll origin or initial position, or on whether or not an element is considered actually visible, but should affect whether an element or the caret is considered scrolled into view (e.g. for targetting or focusing operations), and reduce the amount of scrolling for paging operations (such as using the PgUp and PgDn keys or triggering equivalent operations from the scrollbar) so that within the optimal viewing region of the scrollport the user sees a continuous stream of content. This bug is specifically for tracking the implementation of scroll-padding's effect on paging operations. 1. https://drafts.csswg.org/css-scroll-snap-1/#propdef-scroll-padding
<rdar://problem/71747786>
Created attachment 417817 [details] Patch
Created attachment 417826 [details] Patch
Created attachment 417829 [details] Patch
Comment on attachment 417829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417829&action=review > Source/WebCore/rendering/RenderLayerModelObject.cpp:176 > + if (oldStyle && oldStyle->scrollPadding() != newStyle.scrollPadding()) { Does a change to scrollPadding only trigger this code? What does RenderStyle::diff() return when only scroll padding changes?
(In reply to Simon Fraser (smfr) from comment #5) > Comment on attachment 417829 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417829&action=review > > > Source/WebCore/rendering/RenderLayerModelObject.cpp:176 > > + if (oldStyle && oldStyle->scrollPadding() != newStyle.scrollPadding()) { > > Does a change to scrollPadding only trigger this code? What does > RenderStyle::diff() return when only scroll padding changes? It does trigger when only changing scrollPadding (verified by changing this property in the console and with the inspector). RenderStyle:diff() between the old and new styles returns StyleDifference::Equal here.
Created attachment 417953 [details] Patch
(In reply to Martin Robinson from comment #6) > (In reply to Simon Fraser (smfr) from comment #5) > > Comment on attachment 417829 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=417829&action=review > > > > > Source/WebCore/rendering/RenderLayerModelObject.cpp:176 > > > + if (oldStyle && oldStyle->scrollPadding() != newStyle.scrollPadding()) { > > > > Does a change to scrollPadding only trigger this code? What does > > RenderStyle::diff() return when only scroll padding changes? > > It does trigger when only changing scrollPadding (verified by changing this > property in the console and with the inspector). RenderStyle:diff() between > the old and new styles returns StyleDifference::Equal here. How does that work then? Maybe inspector triggers some additional style recalcs/layouts that make this work. Try a testcase that changes scroll padding on a timer.
(In reply to Simon Fraser (smfr) from comment #8) > How does that work then? Maybe inspector triggers some additional style > recalcs/layouts that make this work. > > Try a testcase that changes scroll padding on a timer. That's a really good question. One of the tests already does modify scroll-padding multiple times using CSSOM, which is the main reason that I realized I needed this new bit of code here. Tomorrow I'll see if I can figure out why this is triggered.
(In reply to Martin Robinson from comment #9) > (In reply to Simon Fraser (smfr) from comment #8) > > > How does that work then? Maybe inspector triggers some additional style > > recalcs/layouts that make this work. > > > > Try a testcase that changes scroll padding on a timer. > > That's a really good question. One of the tests already does modify > scroll-padding multiple times using CSSOM, which is the main reason that I > realized I needed this new bit of code here. Tomorrow I'll see if I can > figure out why this is triggered. Here are the results of my investigation. This is the call stack leading up to RenderLayerModelObject::styleDidChange(...): 1 0x5099732c5 WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 2 0x509948afa WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 3 0x509963d59 WebCore::RenderBlockFlow::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 4 0x5099a8ffd WebCore::RenderElement::setStyle(WebCore::RenderStyle&&, WebCore::StyleDifference) 5 0x509b56434 WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdates const&) 6 0x509b55690 WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) 7 0x509b5519b WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) 8 0x509046a89 WebCore::Document::updateRenderTree(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) 9 0x509046e06 WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) 10 0x509047277 WebCore::Document::updateStyleIfNeeded() 11 0x5096f9f96 WebCore::ThreadTimers::sharedTimerFiredInternal() 12 0x50972082f WebCore::timerFired(__CFRunLoopTimer*, void*) 13 0x7fff2043090d __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ 14 0x7fff204303e8 __CFRunLoopDoTimer 15 0x7fff2042ff42 __CFRunLoopDoTimers 16 0x7fff2041657f __CFRunLoopRun 17 0x7fff204156ce CFRunLoopRunSpecific 18 0x7fff211a2fa1 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] 19 0x7fff21231384 -[NSRunLoop(NSRunLoop) run] 20 0x7fff2006c3dd _xpc_objc_main 21 0x7fff2006be65 _xpc_copy_xpcservice_dictionary 22 0x10997e2e4 WebKit::XPCServiceMain(int, char const**) 23 0x7fff2033a621 start The style difference is calculated in RenderElement::setStyle(...): StyleDifference diff = StyleDifference::Equal; OptionSet<StyleDifferenceContextSensitiveProperty> contextSensitiveProperties; if (m_hasInitializedStyle) diff = m_style.diff(style, contextSensitiveProperties); diff = std::max(diff, minimalStyleDifference); diff = adjustStyleDifference(diff, contextSensitiveProperties); Style::loadPendingResources(style, document(), element()); auto didRepaint = repaintBeforeStyleChange(diff, m_style, style); styleWillChange(diff, style); It seems that even if the difference is calculated as StyleDifference::Equal, which it is in this case, styleWillChange(...) is still called and we still reach RenderLayerModelObject styleDidChange(). The method itself does not consult the StyleDifference, but relies of checking the difference of certain properties between the old and new style to update the RenderLayer or FrameView. In that respect, my change does not work differently, but I'm not sure if the pre-existing approach is correct or not.
I think it's working through luck. Ideally we'd be able to optimize away styleWill/DidChange if the diff is Equal
(In reply to Simon Fraser (smfr) from comment #11) > I think it's working through luck. Ideally we'd be able to optimize away > styleWill/DidChange if the diff is Equal Could it be that there is currently no kind of StyleDifference that captures this sort of change? Changing the padding shouldn't require a full relayout or a repaint. Instead it should just modify what happens when a scrolling operation happens in the future.
You're right, our style difference flags are too coarse-grained.
commit-queue failed to commit attachment 417953 [details] to WebKit repository.
Created attachment 418259 [details] Patch
Committed r271788: <https://trac.webkit.org/changeset/271788> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418259 [details].