Summary: | Snap offsets should update when style is programmatically changed | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Enhancement | CC: | bfulgham, cmarcelo, commit-queue, esprehn+autocc, glenn, jamesr, jonlee, kondapallykalyan, luiz, m.goleb+bugzilla, simon.fraser, tonikitoo, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=142590 | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 134283 | ||||||||||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2014-08-14 16:29:21 PDT
We'll want to take a look at RenderStyle::diff to address this issue. Created attachment 256183 [details]
Test pages for programmatic style updates in mainframe and overflow scroll snapping
Press the buttons on the top left to toggle element styles for scroll snapping.
Created attachment 256213 [details]
Patch
WIP patch -- still some other cases to test, such as removing repeat() on a scroll snap container when there are children with scroll snap coordinates. Created attachment 256276 [details]
Patch
Comment on attachment 256276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256276&action=review I have a few cleanup suggestions, but otherwise this looks good. r- to correct some minor readability issues. > Source/WebCore/rendering/RenderElement.cpp:1500 > + bool foundScrollSnapLayer = false; I don't think you need this variable. > Source/WebCore/rendering/RenderElement.cpp:1504 > + foundScrollSnapLayer = true; I think you could just return currentLayer in this case. > Source/WebCore/rendering/RenderElement.cpp:1513 > + return foundScrollSnapLayer ? currentLayer : nullptr; This could just be return nullptr. > Source/WebCore/rendering/RenderLayerModelObject.cpp:177 > + || oldStyle->scrollSnapDestination().height() != newStyle.scrollSnapDestination().height())) { This should be encapsulated in a method "static bool scrollSnapStyleIsEqual(oldStyle, newStyle)", or maybe add it to RenderStyle. Then this would just be "if (!scrollSnapStyleIsEqual(oldStyle, newStyle)" Comment on attachment 256276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256276&action=review Thanks for taking a look at my patch! I chatted with Simon today and learned that I should be traversing up the tree through RenderBlocks via containingBlock rather than through enclosing RenderLayers, so I refactored RenderElement::findEnclosingScrollSnapLayer to do so. I've made the changes you pointed out in the new implementation. I'll have a new patch up soon. >> Source/WebCore/rendering/RenderElement.cpp:1500 >> + bool foundScrollSnapLayer = false; > > I don't think you need this variable. Fixed! See overall comment. >> Source/WebCore/rendering/RenderElement.cpp:1504 >> + foundScrollSnapLayer = true; > > I think you could just return currentLayer in this case. Fixed! >> Source/WebCore/rendering/RenderElement.cpp:1513 >> + return foundScrollSnapLayer ? currentLayer : nullptr; > > This could just be return nullptr. Fixed! >> Source/WebCore/rendering/RenderLayerModelObject.cpp:177 >> + || oldStyle->scrollSnapDestination().height() != newStyle.scrollSnapDestination().height())) { > > This should be encapsulated in a method "static bool scrollSnapStyleIsEqual(oldStyle, newStyle)", or maybe add it to RenderStyle. Then this would just be "if (!scrollSnapStyleIsEqual(oldStyle, newStyle)" Fixed! Abstracted logic out to scrollSnapContainerRequiresUpdateForStyleUpdate(oldStyle, newStyle). Created attachment 256357 [details]
Patch
(In reply to comment #8) > Comment on attachment 256276 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256276&action=review > > Thanks for taking a look at my patch! I chatted with Simon today and learned > that I should be traversing up the tree through RenderBlocks via > containingBlock rather than through enclosing RenderLayers, so I refactored > RenderElement::findEnclosingScrollSnapLayer to do so. This needs to be covered by a layout test, like I mentioned: <div style="position: absolute"> <div style="overflow, snapping..."> <div style="scroll-snap-coordinate:...; position: absolute"> ... Created attachment 256383 [details]
Patch
> This needs to be covered by a layout test, like I mentioned:
>
> <div style="position: absolute">
> <div style="overflow, snapping...">
> <div style="scroll-snap-coordinate:...; position: absolute">
> ...
Thanks for the reminder. Changed the second container in the test case to test this case.
Comment on attachment 256383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256383&action=review > Source/WebCore/rendering/RenderElement.cpp:1509 > +RenderLayer* RenderElement::findEnclosingScrollSnapLayer() const > +{ > + RenderBlock* currentBlock = containingBlock(); > + while (currentBlock) { > + if (currentBlock->style().scrollSnapType() != ScrollSnapType::None) > + return currentBlock->layer(); > + > + if (currentBlock == currentBlock->containingBlock()) > + break; > + > + currentBlock = currentBlock->containingBlock(); > + } > + return nullptr; I don't think this should be returning RenderLayers; it should be returning RenderElements (or RenderBlocks). > LayoutTests/css3/scroll-snap/scroll-snap-style-changes.html:118 > + <body onload="setup();"> > + <div id="repeat-gallery" class="gallery"> > + <div class="horizontal hchild00"></div><div class="horizontal hchild01"></div><div class="horizontal hchild02"></div><div class="horizontal hchild03"></div><div class="horizontal hchild04"></div><div class="horizontal hchild05"></div> > + </div> > + <div id="coordinates-gallery" class="gallery"> > + <div class="snap-coord horizontal hchild00" id="child00"></div><div class="snap-coord horizontal hchild01" id="child01"></div><div class="snap-coord horizontal hchild02" id="child02"></div><div class="snap-coord horizontal hchild03" id="child03"></div><div class="snap-coord horizontal hchild04" id="child04"></div><div class="snap-coord horizontal hchild05" id="child05"></div> > + </div> > + <script src="../../resources/js-test-post.js"></script> > + </body> There's too much "gallery" in this test. Please put each <div> on its own line. I think this should be two separate tests. Comment on attachment 256383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256383&action=review >> Source/WebCore/rendering/RenderElement.cpp:1509 >> + return nullptr; > > I don't think this should be returning RenderLayers; it should be returning RenderElements (or RenderBlocks). Got it. Refactored to return a RenderBlock* >> LayoutTests/css3/scroll-snap/scroll-snap-style-changes.html:118 >> + </body> > > There's too much "gallery" in this test. Please put each <div> on its own line. I think this should be two separate tests. Got it. Split the test into two separate cases. Created attachment 256482 [details]
Patch
Comment on attachment 256482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256482&action=review > Source/WebCore/rendering/RenderElement.cpp:1495 > +// May return null if there is no such layer (e.g. setting a node to have scroll snap coordinates Comment refers to layers. > Source/WebCore/rendering/RenderLayerModelObject.cpp:132 > + return oldStyle.scrollSnapType() != newStyle.scrollSnapType() || oldStyle.scrollSnapPointsX() != newStyle.scrollSnapPointsX() > + || oldStyle.scrollSnapPointsY() != newStyle.scrollSnapPointsY() || oldStyle.scrollSnapDestination().width() != newStyle.scrollSnapDestination().width() > + || oldStyle.scrollSnapDestination().height() != newStyle.scrollSnapDestination().height(); Please put one condition per line. > Source/WebCore/rendering/RenderLayerModelObject.cpp:199 > + if (oldStyle && oldStyle->scrollSnapCoordinates() != newStyle.scrollSnapCoordinates()) { > + RenderBlock* scrollSnapBlock = enclosingBox().findEnclosingScrollSnapBlock(); > + if (scrollSnapBlock && scrollSnapBlock->layer()) { > + scrollSnapBlock->layer()->updateSnapOffsets(); > + scrollSnapBlock->layer()->updateScrollSnapState(); > + } I don't think this is enough. Style changes could cause a scroll-snap-coordinate node to get re-hsoted by a different scroller, and this code doesn't unregister it from the old one. The first comment suggests snap offsets are not updated when changed via JS and no layout occurs. However, I don't see the change working even if I trigger the layout afterwards; see e.g.: http://output.jsbin.com/micumar/11. This test case contains a couple of elements with height 100vh and a scroll event handler that disables CSS scroll snap after you scroll down more than 5000 pixels. Firefox reacts to the change while Safari/Edge still snap. Created attachment 261979 [details]
Patch
Comment on attachment 261979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261979&action=review > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:72 > + snapOffsetsAsFloat.append(roundToDevicePixel(offset, deviceScaleFactor, false)); Should be uncheckedAppend since we already did reserveInitialCapacity. > Source/WebCore/rendering/RenderElement.h:256 > - > + Please don’t land this change. > Source/WebCore/rendering/RenderLayerModelObject.cpp:134 > + || oldStyle.scrollSnapDestination().width() != newStyle.scrollSnapDestination().width() > + || oldStyle.scrollSnapDestination().height() != newStyle.scrollSnapDestination().height(); A size can be compared directly with !=; we don’t have to check width and height separately. Comment on attachment 261979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261979&action=review >> Source/WebCore/rendering/RenderElement.h:256 >> + > > Please don’t land this change. Sorry -- fixed. >> Source/WebCore/rendering/RenderLayerModelObject.cpp:134 >> + || oldStyle.scrollSnapDestination().height() != newStyle.scrollSnapDestination().height(); > > A size can be compared directly with !=; we don’t have to check width and height separately. Got it. I'll compare the sizes directly. Created attachment 261988 [details]
Patch
Comment on attachment 261988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261988&action=review > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:621 > + if (ScrollingStateFrameScrollingNode* node = downcast<ScrollingStateFrameScrollingNode>(m_scrollingStateTree->stateNodeForID(frameView.scrollLayerID()))) { I would use auto here instead of repeating the type name twice. > Source/WebCore/rendering/RenderLayerCompositor.cpp:4271 > + if (ScrollingCoordinator* scrollingCoordinator = this->scrollingCoordinator()) > + scrollingCoordinator->updateScrollSnapPropertiesWithFrameView(frameView); I would write: if (auto* coordinator = scrollingCoordinator()) coordinator->updateScrollSnapPropertiesWithFrameView(frameView); Comment on attachment 261988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261988&action=review Thanks for reviewing my patch! I'll land it soon. >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:621 >> + if (ScrollingStateFrameScrollingNode* node = downcast<ScrollingStateFrameScrollingNode>(m_scrollingStateTree->stateNodeForID(frameView.scrollLayerID()))) { > > I would use auto here instead of repeating the type name twice. I'll change this. >> Source/WebCore/rendering/RenderLayerCompositor.cpp:4271 >> + scrollingCoordinator->updateScrollSnapPropertiesWithFrameView(frameView); > > I would write: > > if (auto* coordinator = scrollingCoordinator()) > coordinator->updateScrollSnapPropertiesWithFrameView(frameView); Got it. I'll change this. Committed r190330: <http://trac.webkit.org/changeset/190330> |