Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 2884423003: Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by sunyunjia
Modified:
1 day, 17 hours ago
Reviewers:
majidvp, dtapuska
CC:
chromium-reviews, kenneth.christiansen, nasko+codewatch_chromium.org, eae+blinkwatch, apavlov+blink_chromium.org, kinuko+watch, rwlbuis, blink-reviews-css, jam, blink-reviews-dom_chromium.org, dglazkov+blink, darin-cc_chromium.org, blink-reviews, blink-reviews-api_chromium.org, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, blink-reviews-style_chromium.org, sof, darktears, piman+watch_chromium.org, blink-reviews-frames_chromium.org, dtapuska+chromiumwatch_chromium.org, cc-bugs_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use scroll-boundary-behavior to control overscroll-refresh/glow on android. Overscroll-navigation is a feature of Chrome. Users can pull verticallly to refresh the page, or swipe horizontally to browse history. Overscroll-glow is an effect indicating user that his scroll has reached the extend of the scroller. However, in some mobile web applications, navigation or glow does not make sense. This patch allows users to use a css attribute (scroll-boundary-behavior) to the root element in order to control overscroll-navigation and overscroll-glow. The plumbing and machinery have been implemented in both main thread and compositor thread. The scroll-boundary-behavior is bundled in did_overscroll_params and is passed through OverscrollControllerAndroid::Overscrolled(). Thus, we make the decision of whether to pull-to-refresh and whether to glow on Overscrolled() as opposed to immediately after ScrollUpdateAck. BUG=656801 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Patch Set 1 #

Patch Set 2 : Added missing files #

Total comments: 20

Patch Set 3 : Fixed the comments #

Total comments: 7

Patch Set 4 : update more tests #

Patch Set 5 : Update constructor #

Patch Set 6 : update more tests #

Patch Set 7 : Remove the test-expectations as they are still under flags #

Patch Set 8 : Update the test #

Total comments: 17

Patch Set 9 : rebase #

Patch Set 10 : Fixing comments #

Patch Set 11 : format #

Total comments: 12

Patch Set 12 : Fixing on android side #

Patch Set 13 : rebase #

Patch Set 14 : rebase #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+522 lines, -144 lines) Patch
M cc/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M cc/input/input_handler.h View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +4 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +10 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -1 line 0 comments Download
M cc/trees/layer_tree_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +8 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +7 lines, -0 lines 0 comments Download
M content/browser/android/overscroll_controller_android.h View 1 2 3 4 5 6 7 8 9 3 chunks +8 lines, -1 line 0 comments Download
M content/browser/android/overscroll_controller_android.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +61 lines, -20 lines 0 comments Download
A content/browser/android/overscroll_controller_android_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +167 lines, -0 lines 0 comments Download
M content/common/input_messages.h View 1 2 3 4 5 6 7 8 2 chunks +11 lines, -0 lines 0 comments Download
M content/renderer/gpu/render_widget_compositor.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/gpu/render_widget_compositor.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +6 lines, -0 lines 0 comments Download
M content/renderer/input/input_handler_wrapper.h View 1 2 3 4 5 6 7 8 1 chunk +6 lines, -4 lines 0 comments Download
M content/renderer/input/input_handler_wrapper.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -1 line 0 comments Download
M content/renderer/input/render_widget_input_handler.h View 1 2 3 4 5 6 7 8 2 chunks +6 lines, -5 lines 0 comments Download
M content/renderer/input/render_widget_input_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +4 lines, -1 line 0 comments Download
M content/renderer/render_view_impl.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -2 lines 0 comments Download
M content/renderer/render_widget.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M content/renderer/render_widget.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +3 lines, -2 lines 0 comments Download
M content/renderer/render_widget_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +4 lines, -2 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +19 lines, -1 line 1 comment Download
M third_party/WebKit/Source/core/exported/WebViewBase.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/EmptyClients.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/page/ChromeClient.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +5 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/page/ChromeClientImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/page/ChromeClientImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +13 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/page/scrolling/OverscrollController.h View 1 2 3 3 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/page/scrolling/OverscrollController.cpp View 1 2 3 1 chunk +11 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/page/scrolling/RootScrollerTest.cpp View 1 2 3 4 5 6 7 8 4 chunks +9 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/web/WebViewImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebViewImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp View 1 2 3 4 5 6 7 8 11 chunks +49 lines, -33 lines 0 comments Download
M third_party/WebKit/public/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/public/platform/WebLayerTreeView.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebViewClient.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/public/web/WebWidgetClient.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M ui/android/overscroll_glow.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -5 lines 0 comments Download
M ui/android/overscroll_glow.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -3 lines 0 comments Download
M ui/android/overscroll_refresh.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -5 lines 0 comments Download
M ui/android/overscroll_refresh.cc View 1 2 3 4 5 6 7 8 9 4 chunks +9 lines, -6 lines 0 comments Download
M ui/android/overscroll_refresh_unittest.cc View 1 2 3 4 8 chunks +9 lines, -9 lines 0 comments Download
M ui/events/blink/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M ui/events/blink/did_overscroll_params.h View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M ui/events/blink/input_handler_proxy.cc View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -1 line 0 comments Download
M ui/events/blink/input_handler_proxy_client.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M ui/events/blink/input_handler_proxy_unittest.cc View 1 2 3 4 5 6 7 8 4 chunks +19 lines, -20 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 79 (69 generated)
sunyunjia
Hi Majid, Here is the new patch that uses scroll-boundary-behavior to control pull-to-refresh on android. ...
2 months, 1 week ago (2017-05-17 01:02:18 UTC) #3
majidvp
I am not a CSS parser expert so I didn't review that part too closely. ...
2 months ago (2017-05-18 15:26:10 UTC) #4
sunyunjia
Hi Dave, Can you help look at this patch? We're uncertain about the css declarations, ...
2 months ago (2017-05-19 20:14:24 UTC) #8
dtapuska
https://codereview.chromium.org/2884423003/diff/40001/cc/ipc/cc_param_traits.h File cc/ipc/cc_param_traits.h (right): https://codereview.chromium.org/2884423003/diff/40001/cc/ipc/cc_param_traits.h#newcode11 cc/ipc/cc_param_traits.h:11: #include "cc/input/scroll_boundary_behavior.h" This is an odd include. Where is ...
2 months ago (2017-05-23 17:13:40 UTC) #9
sunyunjia
Hi Majid and Dave, I added some quite basic tests for overscroll_controller_android. Please take a ...
1 month, 1 week ago (2017-06-13 15:56:21 UTC) #38
majidvp
https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h File third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h (right): https://codereview.chromium.org/2884423003/diff/40001/third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h#newcode14 third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h:14: struct WebScrollBoundaryBehavior : public ScrollBoundaryBehavior { On 2017/06/13 15:56:21, ...
1 week, 3 days ago (2017-07-12 21:04:22 UTC) #45
sunyunjia
PTAL, thanks! https://codereview.chromium.org/2884423003/diff/220001/content/browser/android/overscroll_controller_android.cc File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2884423003/diff/220001/content/browser/android/overscroll_controller_android.cc#newcode121 content/browser/android/overscroll_controller_android.cc:121: refresh_effect_(CreateRefreshEffect(overscroll_refresh_handler)) { On 2017/07/12 21:04:20, majidvp wrote: ...
3 days, 15 hours ago (2017-07-19 20:44:12 UTC) #52
majidvp
https://codereview.chromium.org/2884423003/diff/220001/content/browser/android/overscroll_controller_android_unittest.cc File content/browser/android/overscroll_controller_android_unittest.cc (right): https://codereview.chromium.org/2884423003/diff/220001/content/browser/android/overscroll_controller_android_unittest.cc#newcode75 content/browser/android/overscroll_controller_android_unittest.cc:75: TEST_F(OverscrollControllerAndroidUnitTest, BasicNavigation) { On 2017/07/19 20:44:11, sunyunjia wrote: > ...
2 days, 23 hours ago (2017-07-20 13:26:55 UTC) #55
sunyunjia
PTAL, thanks! https://codereview.chromium.org/2884423003/diff/280001/content/browser/android/overscroll_controller_android.cc File content/browser/android/overscroll_controller_android.cc (right): https://codereview.chromium.org/2884423003/diff/280001/content/browser/android/overscroll_controller_android.cc#newcode117 content/browser/android/overscroll_controller_android.cc:117: OverscrollControllerAndroid( On 2017/07/20 13:26:55, majidvp wrote: > ...
1 day, 22 hours ago (2017-07-21 13:52:23 UTC) #68
majidvp
1 day, 21 hours ago (2017-07-21 15:24:12 UTC) #73
I think the CL description can be improved. Here are some comments:

> Overscroll-navigation is a feature of Chrome. Users can pull verticallly to
refresh the page, or swipe horizontally to browse history.
no need to hyphenate overscroll navigation. Also please mention that p2r is for
android and swipe is for desktop platform.


>  Overscroll-glow is
an effect indicating user that his scroll has reached the extend of the
scroller. 

s/effect/visual effect/  
s/extend/extent/
Also it is not that the scroll has reached the extent but that in fact it has
passed it.


> However, in some mobile web applications, navigation or glow does not
make sense. 

s/in/for
s


This patch allows users to use a css attribute
(scroll-boundary-behavior) to the root element in order to control
overscroll-navigation and overscroll-glow. 

s/to the root element/on the viewport defining element/

> The plumbing and machinery have been
implemented in both main thread and compositor thread.

Instead of this sentence I suggest describing exactly what has changed in main
and cc.


The scroll-boundary-behavior is bundled in did_overscroll_params and is passed
through OverscrollControllerAndroid::Overscrolled(). Thus, we make the decision
of whether to pull-to-refresh and whether to glow on Overscrolled() as opposed
to immediately after ScrollUpdateAck.


Also please link to specification:
https://wicg.github.io/scroll-boundary-behavior/

and any relevant design docs.

https://codereview.chromium.org/2884423003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2884423003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1982:
GetPage()->GetOverscrollController().SetScrollBoundaryBehavior(
If I understand this correctly, the current logic allows any document in
the page to control the overscroll navigation behavior of the page. Meaning that
if I have this css property on an iframe then it will prevent the navigation.
This is incorrect.

We should only allow the top level document to control this behavior. Which I
think means limiting this to when |Document::IsInMainFrame()| is true.

Also, this suggests to me that we are missing a few tests. For example there is
no test covering this. I suspect you can add tests to WebFrameTest to ensure we
receive the correct values according to the css property and whether this is the
main frame or not.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973