Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124973004:
ScrollableArea should update scrollAnimator with clamped position. (Closed)
|
Created:
4 years, 5 months ago by Eric Seckler Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews, Sami, szager1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScrollableArea should update scrollAnimator with clamped position.
Currently, ScrollableArea updates the scrollAnimator with the
argument to scrollPositionChanged(), which may lie outside the
min/max scroll position of the area. Instead, it should use the
clamped new scroll position obtained from the subclass.
BUG=626315
Patch Set 1 #Patch Set 2 : Use clamped but non-truncated position during ScrollableArea update. #Messages
Total messages: 29 (9 generated)
Description was changed from ========== ScrollableArea should update scrollAnimator with clamped position. Currently, ScrollableArea updates the scrollAnimator with the argument to scrollPositionChanged(), which may lie outside the min/max scroll position of the area. Instead, it should use the clamped new scroll position obtained from the subclass. BUG= ========== to ========== ScrollableArea should update scrollAnimator with clamped position. Currently, ScrollableArea updates the scrollAnimator with the argument to scrollPositionChanged(), which may lie outside the min/max scroll position of the area. Instead, it should use the clamped new scroll position obtained from the subclass. BUG=626315 ==========
eseckler@chromium.org changed reviewers: + bokan@chromium.org
Hi David, Discovered this while testing the scroll/scale override patch. (I was observing different currentPosition in the VisualViewport and its scrollAnimator after setScrollPosition() on the VisualViewport, which means that some of the userScroll() behavior in RootFrameViewport may distribute wrong scroll amounts between the viewports later.) We think this is a bug in ScrollableArea - Let us know if you can think of a legitimate reason for this behavior :) Thanks! Eric
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
Non-owner lgtm.
I've been puzzled by this choice in the past too. ScrollableArea should be the one source of truth. I can't think of what this might break but ScrollAnimator sits in a funny place between Blink and the compositor so it's often surprising. Lets see if anything breaks and we can work from there. lgtm.
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Turns out, we can't simply use the scrollPosition() of the subclass, because this may be truncated to integer precision above. And there are some layout tests that check that fractional scroll positions are accumulated correctly even in that case, for which the animator seems to be responsible in the truncated case. Thus, I tried simply clamping the non-truncated position to the limits again, before passing it to the animator. That seems to cause an issue with maintaining the correct scroll position in the animator of a PaintLayerScrollableArea when the page scale changes: e.g. fast/events/touch/gesture/touch-gesture-scroll-div-zoomed.html failes with the error below. I'm not sure where this stems from exactly, but it seems that the scroll animator thinks the currentScrollPosition of the area is different. Interestingly, PaintLayerScrollableArea::setScrollOffset() doesn't clamp the new position before storing it either, which is probably why this changes behavior. PASS wheelEventsOccurred is 0 second gesture PASS movingdiv.scrollTop is 32 -PASS movingdiv.scrollLeft is 80 +FAIL movingdiv.scrollLeft should be 80. Was 40. PASS wheelEventsOccurred is 0 scroll event 0+> [object HTMLDivElement] PASS scrollEventsOccurred is 1
On 2016/07/08 12:06:32, Eric Seckler wrote: > Thus, I tried simply clamping the non-truncated position to the limits again, > before passing it to the animator. That seems to cause an issue with maintaining > the correct scroll position in the animator of a PaintLayerScrollableArea when > the page scale changes: e.g. > fast/events/touch/gesture/touch-gesture-scroll-div-zoomed.html failes with the > error below. I'm not sure where this stems from exactly, but it seems that the > scroll animator thinks the currentScrollPosition of the area is different. > Interestingly, PaintLayerScrollableArea::setScrollOffset() doesn't clamp the new > position before storing it either, which is probably why this changes behavior. > > PASS wheelEventsOccurred is 0 > second gesture > PASS movingdiv.scrollTop is 32 > -PASS movingdiv.scrollLeft is 80 > +FAIL movingdiv.scrollLeft should be 80. Was 40. > PASS wheelEventsOccurred is 0 > scroll event 0+> [object HTMLDivElement] > PASS scrollEventsOccurred is 1 Sorry, it's page zoom and not page scale. Seems like this is where things go wrong: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... Maybe in this case, the PaintLayerScrollableArea would clamp to the old coordinate space rather then the one for the new zoom factor?
On 2016/07/08 13:53:49, Eric Seckler wrote: > On 2016/07/08 12:06:32, Eric Seckler wrote: > > Thus, I tried simply clamping the non-truncated position to the limits again, > > before passing it to the animator. That seems to cause an issue with > maintaining > > the correct scroll position in the animator of a PaintLayerScrollableArea when > > the page scale changes: e.g. > > fast/events/touch/gesture/touch-gesture-scroll-div-zoomed.html failes with the > > error below. I'm not sure where this stems from exactly, but it seems that the > > scroll animator thinks the currentScrollPosition of the area is different. > > Interestingly, PaintLayerScrollableArea::setScrollOffset() doesn't clamp the > new > > position before storing it either, which is probably why this changes > behavior. > > > > PASS wheelEventsOccurred is 0 > > second gesture > > PASS movingdiv.scrollTop is 32 > > -PASS movingdiv.scrollLeft is 80 > > +FAIL movingdiv.scrollLeft should be 80. Was 40. > > PASS wheelEventsOccurred is 0 > > scroll event 0+> [object HTMLDivElement] > > PASS scrollEventsOccurred is 1 > > Sorry, it's page zoom and not page scale. > > Seems like this is where things go wrong: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > Maybe in this case, the PaintLayerScrollableArea would clamp to the old > coordinate space rather then the one for the new zoom factor? Right, this is a pain for us now, and it's something I want to fix in the near term, but ScrollableAreas in Blink keep integer offsets, the ScrollAnimator keeps the fractional parts. That's why we can't simply use the ScrollableArea's position. That said, clamping the untruncated position should work. I looked into the failing test and I think I see what's going wrong. It looks like the contentsSize and visibleSize as used by maximumScrollPosition aren't updated when the zoom level changes. I added some stack tracing at some key points: When the zoom factor changes, LayoutBox::styleDidChange will update the scroll position since the scroll offset in ScrollableArea/PLScrollableArea is stored in DIPs (it needs to be multiplied by the CSS zoom). But you can see the maximumScrollPosition hasn't updated so if we clamp here it'll clamp to the old zoom level: SetScrollOffset[0x242b11d42bf0]: 100, 0 [1:1:0708/120156:373188110694:WARNING:PaintLayerScrollableArea.cpp(349)] MaximumScrollPosition: 50, 50 #0 0x7f2c6aabecbe base::debug::StackTrace::StackTrace() #1 0x7f2c62be56b6 blink::PaintLayerScrollableArea::setScrollOffset() #2 0x7f2c65f1668f blink::ScrollableArea::scrollPositionChanged() #3 0x7f2c65f0ff51 blink::ProgrammaticScrollAnimator::notifyPositionChanged() #4 0x7f2c65f0ffa3 blink::ProgrammaticScrollAnimator::scrollToOffsetWithoutAnimation() #5 0x7f2c65f16934 blink::ScrollableArea::programmaticScrollHelper() #6 0x7f2c65f164b5 blink::ScrollableArea::setScrollPosition() #7 0x7f2c62be6de1 blink::PaintLayerScrollableArea::scrollToPosition() #8 0x7f2c623d2091 blink::PaintLayerScrollableArea::scrollToOffset() #9 0x7f2c62d57aa2 blink::PaintLayerScrollableArea::scrollToXOffset() #10 0x7f2c62d3f8e7 blink::LayoutBox::styleDidChange() #11 0x7f2c62d00643 blink::LayoutBlock::styleDidChange() #12 0x7f2c62d1ccd2 blink::LayoutBlockFlow::styleDidChange() #13 0x7f2c62dd6733 blink::LayoutObject::setStyle() #14 0x7f2c62096fa4 blink::Element::recalcOwnStyle() #15 0x7f2c6209653b blink::Element::recalcStyle() #16 0x7f2c61ff1fbe blink::ContainerNode::recalcChildStyle() #17 0x7f2c62096673 blink::Element::recalcStyle() #18 0x7f2c61ff1fbe blink::ContainerNode::recalcChildStyle() #19 0x7f2c62096673 blink::Element::recalcStyle() #20 0x7f2c61ff1fbe blink::ContainerNode::recalcChildStyle() #21 0x7f2c62096673 blink::Element::recalcStyle() #22 0x7f2c62026fbc blink::Document::updateStyle() #23 0x7f2c6202353d blink::Document::updateStyleAndLayoutTree() #24 0x7f2c62028b0e blink::Document::updateStyleAndLayoutTreeIgnorePendingStylesheets() #25 0x7f2c6202853c blink::Document::updateStyleAndLayoutIgnorePendingStylesheets() #26 0x7f2c6297596f blink::LocalFrame::setPageAndTextZoomFactors() #27 0x7f2c629756c7 blink::LocalFrame::setPageZoomFactor() #28 0x00000095efd3 blink::Internals::setZoomFactor() The scroll dimensions (visible and content size) are updated at the end of *layout*: [1:1:0708/120156:373188279994:WARNING:PaintLayerScrollableArea.cpp(613)] UPDATE SCROLL DIMENSIONS #0 0x7f2c6aabecbe base::debug::StackTrace::StackTrace() #1 0x7f2c62be6c8b blink::PaintLayerScrollableArea::updateScrollDimensions() #2 0x7f2c62be6efc blink::PaintLayerScrollableArea::updateAfterLayout() #3 0x7f2c62d01664 blink::LayoutBlock::updateAfterLayout() #4 0x7f2c62d130e1 blink::LayoutBlockFlow::layoutBlock() #5 0x7f2c62d01700 blink::LayoutBlock::layout() #6 0x7f2c62cfb436 blink::LayoutObject::layoutIfNeeded() #7 0x7f2c62d02d9f blink::LayoutBlock::layoutPositionedObjects() #8 0x7f2c62d246a3 blink::LayoutBlockFlow::layoutBlockFlow() #9 0x7f2c62d13033 blink::LayoutBlockFlow::layoutBlock() #10 0x7f2c62d01700 blink::LayoutBlock::layout() #11 0x7f2c62d13de6 blink::LayoutBlockFlow::positionAndLayoutOnceIfNeeded() #12 0x7f2c62d14167 blink::LayoutBlockFlow::layoutBlockChild() #13 0x7f2c62d18898 blink::LayoutBlockFlow::layoutBlockChildren() #14 0x7f2c62d2421b blink::LayoutBlockFlow::layoutBlockFlow() #15 0x7f2c62d13033 blink::LayoutBlockFlow::layoutBlock() #16 0x7f2c62d01700 blink::LayoutBlock::layout() #17 0x7f2c62d13de6 blink::LayoutBlockFlow::positionAndLayoutOnceIfNeeded() #18 0x7f2c62d14167 blink::LayoutBlockFlow::layoutBlockChild() #19 0x7f2c62d18898 blink::LayoutBlockFlow::layoutBlockChildren() #20 0x7f2c62d2421b blink::LayoutBlockFlow::layoutBlockFlow() #21 0x7f2c62d13033 blink::LayoutBlockFlow::layoutBlock() #22 0x7f2c62d01700 blink::LayoutBlock::layout() #23 0x7f2c62d13de6 blink::LayoutBlockFlow::positionAndLayoutOnceIfNeeded() #24 0x7f2c62d14167 blink::LayoutBlockFlow::layoutBlockChild() #25 0x7f2c62d18898 blink::LayoutBlockFlow::layoutBlockChildren() #26 0x7f2c62d2421b blink::LayoutBlockFlow::layoutBlockFlow() #27 0x7f2c62d13033 blink::LayoutBlockFlow::layoutBlock() #28 0x7f2c62d01700 blink::LayoutBlock::layout() #29 0x7f2c62e3bd95 blink::LayoutView::layoutContent() #30 0x7f2c62e3c4c2 blink::LayoutView::layout() #31 0x7f2c6292d90c blink::layoutFromRootObject() #32 0x7f2c6292d4de blink::FrameView::performLayout() #33 0x7f2c6292b1cc blink::FrameView::layout() I think the scroll update needs to be moved to happen after layout as well, since we don't have the new scroll dimensions until after we do a layout. I think the style change will need to mark the scroll as needing a zoom update and the updateAfterLayout method needs to check that flag and update the scroll. +skobes@ who's the layout+scrolling expert and likely has much better insight. IMO, the scroll position should never be outside the bounds of the min/max.
Forgot to actually +cc skobes@
Yeah clamping is kind of a mess right now. Note that the PLSA::scrollToOffset API currently takes a ScrollOffsetClamping parameter, which can't be correctly implemented unless ScrollableArea permits unclamped scrolls. An immediate unclamped scroll with clamping after layout, does not seem terribly different from "marking" the PLSA as needing a clamped scroll after layout. I'm surprised that ScrollableArea and ScrollAnimator treat fractional offsets differently. I think tests should only expect fractional offsets when the FractionalScrollOffsets REF is enabled, in which case the ScrollableArea will store the fractional offset?
skobes@chromium.org changed reviewers: + skobes@chromium.org
+szager who has also fought with this stuff
On 2016/07/08 17:10:26, skobes wrote: > Yeah clamping is kind of a mess right now. > > Note that the PLSA::scrollToOffset API currently takes a ScrollOffsetClamping > parameter, which can't be correctly implemented unless ScrollableArea permits > unclamped scrolls. Hmm...that may throw a wrench in here. Although, I think what we really want here is to keep the ScrollAnimator in line with the ScrollableArea. Right now, clamping on the ScrollableArea doesn't clamp on the ScrollAnimator in scrollPositionChanged. I'm assuming ScrollAnimator eventually gets the right offset when we cycle through the process again but I think we could be more immediate about it. > An immediate unclamped scroll with clamping after layout, does not seem terribly > different from "marking" the PLSA as needing a clamped scroll after layout. I think the difference to me is that keeping the offset always clamped is much easier to reason about since it means ScrollableArea is internally consistent. For context, Eric is implementing a scroll/scale override in order to take screenshots with an arbitrary viewport size/location in headless chrome. We do this by setting the minimum/maximum positions to the desired location (see crrev.com/2096633002). The problem is that while the ScrollableArea respects these limits, it doesn't clamp the offset when passing it to the ScrollAnimator. RootFrameViewport::userScroll, I believe for reasons made obsolete with smooth scrolling on, currently uses the offsets in the ScrollAnimators rather than the ScrollableAreas so this causes some jumping. > I'm surprised that ScrollableArea and ScrollAnimator treat fractional offsets > differently. I think tests should only expect fractional offsets when the > FractionalScrollOffsets REF is enabled, in which case the ScrollableArea will > store the fractional offset? That's right. This has been a source of great frustration as the interactions can be subtle. I'd like to convert ScrollableArea to just use fractional offsets and truncate at the web API boundaries but I think it'd make sense to wait for the dsf-with-zoom refactoring work to complete.
On 2016/07/08 17:27:35, bokan wrote: > Hmm...that may throw a wrench in here. Although, I think what we really want > here is to keep the ScrollAnimator in line with the ScrollableArea. Right now, > clamping on the ScrollableArea doesn't clamp on the ScrollAnimator in > scrollPositionChanged. I'm assuming ScrollAnimator eventually gets the right > offset when we cycle through the process again but I think we could be more > immediate about it. +1 to making ScrollAnimator agree with ScrollableArea. :) > I think the difference to me is that keeping the offset always clamped is much > easier to reason about since it means ScrollableArea is internally consistent. Yes that invariant is nice, but the tradeoff of added dirty bits / plumbing deferred scrolls makes it (to me) not an obvious win. Certainly if we are going to always-clamp then we should kill ScrollOffsetClamping. > That's right. This has been a source of great frustration as the interactions > can be subtle. I'd like to convert ScrollableArea to just use fractional offsets > and truncate at the web API boundaries but I think it'd make sense to wait for > the dsf-with-zoom refactoring work to complete. sgtm
Thanks for looking into it, guys. Sounds like David's proposed rework of fractional offsets in ScrollableArea is the right way forward. In the meantime, would it make sense to revert back to patch #1 and modify breaking tests (so that they only assume fractional accumulation when FractionalScrollOffsets is enabled)? As an alternative short-term fix for my problem, I could overwrite setScrollPosition() in the VisualViewport to apply clamping before letting ScrollableArea handle the animator update. (This is what FrameView does.) Does that sound more reasonable? :)
On 2016/07/11 09:52:24, Eric Seckler wrote: > Thanks for looking into it, guys. Sounds like David's proposed rework > of fractional offsets in ScrollableArea is the right way forward. > > In the meantime, would it make sense to revert back to patch #1 and > modify breaking tests (so that they only assume fractional > accumulation when FractionalScrollOffsets is enabled)? No, FractionalScrollOffsets affects whether ScrollableArea stores the fractional part. ScrollAnimator should always store the fractional regardless of that flag and using patchset #1 would change that. > As an alternative short-term fix for my problem, I could overwrite > setScrollPosition() in the VisualViewport to apply clamping before > letting ScrollableArea handle the animator update. (This is what > FrameView does.) Does that sound more reasonable? :) Yas, if this works I think it's is a better approach for now. Please add a TODO explaining why the override is needed and link to the bug here.
On 2016/07/11 13:24:46, bokan wrote: > ScrollAnimator should always store the fractional regardless of that flag Um... why is this?
On 2016/07/11 14:47:31, skobes wrote: > On 2016/07/11 13:24:46, bokan wrote: > > ScrollAnimator should always store the fractional regardless of that flag > > Um... why is this? I thought it's so that we can scroll smoothly on the main thread while in hiDPI but it's been a while since I looked at this so maybe it's just incidental?
On 2016/07/11 15:07:26, bokan wrote: > On 2016/07/11 14:47:31, skobes wrote: > > On 2016/07/11 13:24:46, bokan wrote: > > > ScrollAnimator should always store the fractional regardless of that flag > > > > Um... why is this? > > I thought it's so that we can scroll smoothly on the main thread while in hiDPI > but it's been a while since I looked at this so maybe it's just incidental? Scroll offsets are sync'ed from blink -> cc by ScrollingCoordinator::scrollableAreaScrollLayerDidChange which just queries ScrollableArea::scrollPositionDouble(). So, I don't think MT-serviced smooth scrolls can set subpixel scroll offsets on the cc::Layer when FractionalScrollOffsets is off. But eseckler's proposal to clamp in VisualViewport::setScrollPosition sounds fine to me too.
> But eseckler's proposal to clamp in VisualViewport::setScrollPosition sounds > fine to me too. Great, I think clamping there is easier, since is doesn't seem to break existing behavior. Created a separate review for that change here: https://codereview.chromium.org/2138823002/ I'll update the bug and point it to the discussion here. |
