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)
Firefox for Android Graveyard
Toolbar
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 | ||
Updated•8 years ago
|
Blocks: fennec-aboard-apz
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → rbarker
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8734542 -
Flags: review?(nchen)
Attachment #8734542 -
Flags: review?(botond)
Comment 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
(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 4•8 years ago
|
||
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+
Comment 5•8 years ago
|
||
(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().)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8734542 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
Address review comments carry forward r+ from :kats and :jchen
Attachment #8736380 -
Attachment is obsolete: true
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
carry forward r+ from :jchen and :kats.
Attachment #8736452 -
Attachment is obsolete: true
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)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(rbarker)
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/30d51057b6f5
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•