Bug 84625 – Add ScrollAnimatorBlackBerry as an extension to ScrollAnimatorNone

• Component: Platform
• Status: RESOLVED FIXED

[Antonio Gomes]

Situation: we need to animate the scroll position adjustment for (say) virtual keyboard showing/hiding
Solution: Right now in the blackberry port we use ScrollableArea::ScrollAnimator::scroll() method.

Basically we:

1) calculate the scroll adjustment needed
2) set ScrollableArea::m_constrainsScrollingToContentEdge to 'false', so we can "overscroll" whenever needed in order to properly show the focused element
3) call main frame's ScrollableArea::scrollAnimator::scroll()
4) set ScrollableArea::m_constrainsScrollingToContentEdge back to 'true'

Problem: when we call (3), all the scroll animation is timer-based, so it happens asynchronously, whereas (4) happens right after (3) synchronously. This way when (3) is executing, we have already reset m_constrainsScrollingToContentEdge.

We need a way to tell ScrollableArea::ScrollAnimator::scroll() to execute something when it finishes animating, or to tell the caller when it finished.

[Antonio Gomes]

Created attachment 138847 [details]
patch v1

[Antonio Gomes]

Comment on attachment 138847 [details]
patch v1

will do it differently after talking to Andersca

[Antonio Gomes]

Created attachment 138929 [details]
patch v2

Adds ScrollAnimBB class, and add the customization to allow overscrolling while the animation runs in there (as per andersca suggestion)

[WebKit Review Bot]

Attachment 138929 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/platform/blackberry/ScrollAnimatorBlackBerry.cpp:6:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antonio Gomes]

Created attachment 138999 [details]
(r115347, r=andersca) patch v3

Fixed style and build system issues

[Antonio Gomes]

some context:

  1 <andersca>  tonikitoo: hmm, why do you need to overscroll when showing the keyboard? is it because the keyboard is on top of the web view somehow?
  2 <tonikitoo> andersca, exactly. when we show a Virtual Keyboard, we do not set the viewport size (trying to avoid layouts, and stuff)
  3 <andersca>  tonikitoo: do you disable scripts as well?
  4 <tonikitoo> we have a different way of controlling it
  5 <tonikitoo> andersca, not sure I followed the scripts part
  6 <andersca>  tonikitoo: does showing the keyboard prevent script execution
  7 <tonikitoo> andersca, no
  8 <andersca>  tonikitoo: so you could have a script that scrolls and wreaks havoc
  9 <tonikitoo> andersca, that script could exist, yes. And no, so far we are using the ScrollAnimNone, but in the future we will need a custom one
 10 <andersca>  tonikitoo: do you have a custom blackberry scroll animator?
 11 <andersca>  tonikitoo: so if you need a custom one then maybe you should just put the logic there
 12 <andersca>  tonikitoo: since it seems specialized
 13 <tonikitoo> andersca, I can add ScrollAnimBlackberry and inherit from ScrollAnimNone. but the base classes would still need a stub of the method right?
 14 <tonikitoo> agree that is seems like too specialized
 15 <andersca>  tonikitoo: then in your scroll function you could allow over scrolling and when you know the animation is finished you can disable it again
 16 <tonikitoo> andersca,  but for now I would rather still use scrollanimatornone::scroll
 17 <tonikitoo> andersca, maybe I should add animationFinished method instead
 18 <tonikitoo> and do the logic from there
 19 <andersca>  tonikitoo: just make your scroll animator inherit from scrollanimatornone :)
 20 <andersca>  tonikitoo: then you can add animationFinished to scrollanimatornone
 21 <tonikitoo> andersca, exactly, that is what I thought too
 22 <tonikitoo> andersca, do you think we would not need the setDisableConstrainsScrollingToContentEdgeWhileAnimating method then?
 23 <andersca>  tonikitoo: i don't think that would be needed, as long as you'd inherit from ScrollAnimatorNone
 24 <tonikitoo> andersca, but I want specific scroll animation runs to allow overscrolling
 25 <andersca>  tonikitoo: oh, i see
 26 <andersca>  hmm
 27 <tonikitoo> so if I do ScrollView::setContrainsScrollingToContentEdge(false); ScrollView::scrollAnimator()->scroll() ... I want to be able to toggle the overscroll flag OFF , or leave it as ON, depending on the case.
 28 <andersca>  tonikitoo: yeah i suppose
 29 <andersca>  it's still pretty specialized :/
 30 <andersca>  tonikitoo: where do you call scroll from
 31 <tonikitoo> andersca, so far it will be used for animating the scroll position adjustment as we show the virtual keyboard only.
 32 <tonikitoo> so it does not have other use cases
 33 <andersca>  tonikitoo: and where's that called from
 34 <tonikitoo> let me paste the code to you
 35 <andersca>  tonikitoo: couldn't you cast the scroll animator to your specific subclass and call your setter from there?
 36 <tonikitoo> andersca, it looks like this:
 37 <tonikitoo>  640             // In order to adjust the scroll position to ensure the focused input field is visible,
 38 <tonikitoo>  641             // we allow overscrolling. However this overscroll has to be strictly allowed towards the
 39 <tonikitoo>  642             // bottom of the page on the y axis only, where the virtual keyboard pops up from.
 40 <tonikitoo>  643             WebCore::IntPoint destinationScrollOffset = revealRect.location();
 41 <tonikitoo>  644             destinationScrollOffset.clampNegativeToZero();
 42 <tonikitoo>  645             WebCore::IntPoint maximumScrollPosition = WebCore::IntPoint(mainFrameView->contentsWidth() - actualScreenRect.width(), mainFrameView->contentsHeight() - actualScreenRect.heig     ht());
 43 <tonikitoo>  646             destinationScrollOffset = destinationScrollOffset.shrunkTo(maximumScrollPosition);
 44 <tonikitoo>  647
 45 <tonikitoo>  648             // Animate the scroll position adjustment.
 46 <tonikitoo>  649             WebCore::IntPoint initialScrollOffset = mainFrameView->scrollPosition();
 47 <tonikitoo>  650             mainFrameView->scrollAnimator()->setDisableConstrainsScrollingToContentEdgeWhileAnimating(true);
 48 <tonikitoo>  651             mainFrameView->scrollAnimator()->scroll(VerticalScrollbar, ScrollByPixel, 1, (destinationScrollOffset - initialScrollOffset).height());
 49 <tonikitoo> (sorry about the flood)
 50 <andersca>  tonikitoo: and this is inside blackberry specific code?
 51 <tonikitoo> andersca, yes
 52 <tonikitoo> I can subclass it, indeed.
 53 <andersca>  tonikitoo: just static_cast<ScrollAnimatorBlackberry*>(mainFrameView->scrollAnimator()) and then call disable on it
 54 <tonikitoo> was just trying to avoid that :-)
 55 <tonikitoo> andersca, sounds good. I can restrict it, sure
 56 <tonikitoo> andersca, thanks!

[Anders Carlsson]

Comment on attachment 138999 [details]
(r115347, r=andersca) patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=138999&action=review

> Source/WebCore/platform/ScrollAnimator.h:103
> +    virtual void animationWillStart() { }
> +    virtual void animationDidFinish() { }

I think you can just put these in ScrollAnimatorNone.

[Antonio Gomes]

Comment on attachment 138999 [details]
(r115347, r=andersca) patch v3

Committed: <http://trac.webkit.org/changeset/115347>