Closed Bug 1257269 Opened 8 years ago Closed 8 years ago

Panning up in a scrollable element should not hide the toolbar

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

Details

Attachments

(1 file, 3 obsolete files)

Currently in Fennec, when the user pans up in a scrollable element, the toolbar will first hide before the element is scrolled. This causes the element to move under the users finger. To prevent the element from shifting, the toolbar should not hide unless it is the root content that is being scrolled. For security reasons, scrolling down will always show the toolbar if it is hidden regardless of what content is being scrolled.
Assignee: nobody → rbarker
Attachment #8734542 - Flags: review?(nchen)
Attachment #8734542 - Flags: review?(botond)
Comment on attachment 8734542 [details] [diff] [review]
0001-Bug-1257269-Panning-up-in-a-scrollable-element-should-not-hide-the-toolbar-in-Fennec-r-16032415-9182b4e.patch

Review of attachment 8734542 [details] [diff] [review]:
-----------------------------------------------------------------

JNI bits LGTM.

::: mobile/android/base/java/org/mozilla/gecko/gfx/NativePanZoomController.java
@@ +324,5 @@
>          }
>      }
>  
> +    @WrapForJNI(allowMultithread = true)
> +    private void scrollingRootContent() {

Instead of redirecting to UI thread in Java, I think you can use | APZThreadUtils::RunOnControllerThread | in native code? The overhead should be lower that way.
Attachment #8734542 - Flags: review?(nchen) → review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> Comment on attachment 8734542 [details] [diff] [review]
> 0001-Bug-1257269-Panning-up-in-a-scrollable-element-should-not-hide-the-
> toolbar-in-Fennec-r-16032415-9182b4e.patch
> 
> Review of attachment 8734542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> JNI bits LGTM.
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/gfx/NativePanZoomController.java
> @@ +324,5 @@
> >          }
> >      }
> >  
> > +    @WrapForJNI(allowMultithread = true)
> > +    private void scrollingRootContent() {
> 
> Instead of redirecting to UI thread in Java, I think you can use |
> APZThreadUtils::RunOnControllerThread | in native code? The overhead should
> be lower that way.

I figured since it was only once at the start of a scroll, it wouldn't really add much overhead, but I can change it.
Comment on attachment 8734542 [details] [diff] [review]
0001-Bug-1257269-Panning-up-in-a-scrollable-element-should-not-hide-the-toolbar-in-Fennec-r-16032415-9182b4e.patch

Review of attachment 8734542 [details] [diff] [review]:
-----------------------------------------------------------------

Summarizing feedback given on IRC:

  - The trigger for setting the "scrolling root content" flag (APZC::AttemptScroll()) and the
    trigger for clearing it (DynamicToolbarAnimator.onInterceptTouchEvent()) are very far
    away and in different places in the code. They also don't match very precisely, because
    AttemptScroll() can be called for input methods other than touch, which could leave the
    flag stuck in the "set" state.

    I think a better place to clear the flag would be when AndroidContentController processes
    an APZStateChange::TransformEnd notification.

  - It would be nice to de-couple this behaviour from the "allow immediate handoff" pref.
    In addition to avoiding the coupling of multiple behaviours to a single pref, that would
    avoid having to listen to the pref if DynamicToolbarAnimator.

    I think we can do this by implementing the first bullet point in bug 1257264 comment 6
    ("track InputBlockState::mScrolledApzc regardless of the value of gfxPrefs::
    APZAllowImmediateHandoff()), and then moving the code in AttemptScroll() that sends the
    message outside of the "if (!gfxPrefs::APZAllowImmediateHandoff())" block.

The final review should probably be done by Kats, as there may be subtleties related to the dynamic toolbar implementation that I'm not aware of.
Attachment #8734542 - Flags: review?(botond) → feedback+
(In reply to Botond Ballo [:botond] from comment #4)
>     I think a better place to clear the flag would be when AndroidContentController processes
>     an APZStateChange::TransformEnd notification.

(That's still fairly far away from AttemptScroll(), but I can't think of anything better. When processing the APZStateChange::TransformBegin we don't know whether the APZC that will actually scroll is the root content (and in immediate-handoff mode, we could start scrolling the root content in the middle of the input block), and there isn't really a "stop scrolling" counterpart to AttemptScroll().)
Depends on: 1260588
Comment on attachment 8736380 [details] [diff] [review]
0001-Bug-1257269-Panning-up-in-a-scrollable-element-should-not-hide-the-toolbar-r-16033010-5fc4b09.patch

:botond asked :kats do final review. Updated patch to reflect :botond's comments. Carry forward r+ from :jchen
Attachment #8736380 - Flags: review?(bugmail.mozilla)
Comment on attachment 8736380 [details] [diff] [review]
0001-Bug-1257269-Panning-up-in-a-scrollable-element-should-not-hide-the-toolbar-r-16033010-5fc4b09.patch

Review of attachment 8736380 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed but if you change the condition where mScrollingRootContent is used I'd like to see an updated patch.

::: gfx/layers/apz/public/GeckoContentController.h
@@ +152,5 @@
>    virtual void NotifyFlushComplete() = 0;
>  
>    virtual void UpdateOverscrollVelocity(const float aX, const float aY) {}
> +  virtual void UpdateOverscrollOffset(const float aX, const float aY) {}
> +  virtual void ScrollingRootContent(const bool isRootContent) {}

Rename it to SetScrollingRootContent, here and everywhere that you have functions called [Ss]crollingRootContent.

::: mobile/android/base/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java
@@ +94,5 @@
>       * behaviour. */
>      private PointF mTouchStart;
>      private float mLastTouch;
>  
> +    private boolean mScrollingRootContent = false;

Drop the | = false| bit and add a comment above this indicating what it's for.

@@ +97,5 @@
>  
> +    private boolean mScrollingRootContent = false;
> +
> +    public void scrollingRootContent(boolean isRootContent) {
> +        mScrollingRootContent = isRootContent;

Move this down in the file - at least below the constructor.

@@ +354,5 @@
>              // to cause translation.
>              boolean inBetween = (mToolbarTranslation != 0 && mToolbarTranslation != mMaxTranslation);
>              boolean reachedThreshold = -aTouchTravelDistance >= exposeThreshold;
>              boolean atBottomOfPage = aMetrics.viewportRectBottom() >= aMetrics.pageRectBottom;
> +            if (mScrollingRootContent && (inBetween || (reachedThreshold && !atBottomOfPage))) {

So this condition change means that if you are scrolling downwards on a subframe and reverse direction when the toolbar is half-visible, it will stay half-visible until you release your finger. Is that the intended effect? If so, I would prefer a

// Never reduce visibility of the toolbar while scrolling
// subframes
if (!mScrollingRootContent) {
  return 0;
}

near the top of this block. If the intention is something else then the code will need to change a bit more.
Attachment #8736380 - Flags: review?(bugmail.mozilla) → review+
Address review comments carry forward r+ from :kats and :jchen
Attachment #8736380 - Attachment is obsolete: true
Comment on attachment 8736452 [details] [diff] [review]
0001-Bug-1257269-Panning-up-in-a-scrollable-element-should-not-hide-the-toolbar-r-16033013-389e767.patch

Review of attachment 8736452 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java
@@ +355,5 @@
>              // to cause translation.
>              boolean inBetween = (mToolbarTranslation != 0 && mToolbarTranslation != mMaxTranslation);
>              boolean reachedThreshold = -aTouchTravelDistance >= exposeThreshold;
>              boolean atBottomOfPage = aMetrics.viewportRectBottom() >= aMetrics.pageRectBottom;
> +            if (inBetween || (mScrollingRootContent && reachedThreshold && !atBottomOfPage)) {

Please update the comment block above as well.
I backed this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed189d26cad66521781f127ebefcc6a93b4d33f because it was making the merge from mozilla-central back to inbound difficult. Feel free to reland this as you see fit.
Flags: needinfo?(rbarker)
Flags: needinfo?(rbarker)
https://hg.mozilla.org/mozilla-central/rev/30d51057b6f5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.