third_party/WebKit/Source/core/input/ScrollManager.cpp - Issue 2884423003: Use scroll-boundary-behavior to control overscroll-refresh/glow on android.

Side by Side Diff: third_party/WebKit/Source/core/input/ScrollManager.cpp

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

Patch Set: Update input_messages with the order in the enum. Created 3 years, 4 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« no previous file with comments | « third_party/WebKit/Source/core/exported/WebViewImpl.cpp ('k') | third_party/WebKit/Source/core/loader/EmptyClients.h » ('j') | ui/android/overscroll_refresh.h » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 // Copyright 2016 The Chromium Authors. All rights reserved.	1 // Copyright 2016 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "core/input/ScrollManager.h"	5 #include "core/input/ScrollManager.h"

6	6

7 #include <memory>	7 #include <memory>

8 #include "core/dom/DOMNodeIds.h"	8 #include "core/dom/DOMNodeIds.h"

9 #include "core/dom/NodeComputedStyle.h"	9 #include "core/dom/NodeComputedStyle.h"

10 #include "core/events/GestureEvent.h"	10 #include "core/events/GestureEvent.h"

(...skipping 118 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
129 cur_element = document_element;	129 cur_element = document_element;

130 }	130 }

131	131

132 if (cur_element) {	132 if (cur_element) {

133 if (CanScroll(scroll_state, *cur_element))	133 if (CanScroll(scroll_state, *cur_element))

134 scroll_chain.push_front(DOMNodeIds::IdForNode(cur_element));	134 scroll_chain.push_front(DOMNodeIds::IdForNode(cur_element));

135 if (IsViewportScrollingElement(*cur_element) \|\|	135 if (IsViewportScrollingElement(*cur_element) \|\|

136 cur_element == document_element)	136 cur_element == document_element)

137 break;	137 break;

138	138

139 if (!CanPropagate(scroll_state, *cur_element))	139 if (!CanPropagate(scroll_state, *cur_element) &&

	140 cur_element !=

	141 frame_->GetDocument()->ViewportDefiningElement(nullptr))
	majidvp 2017/08/08 15:16:14 This feels odd to me. It is not clear to me why ex This feels odd to me. It is not clear to me why exactly it is needed and seems to be in conflict with the conditional statement above. e. Also, is this something that should be part of \|CanPropagate\| logic? sunyunjia 2017/08/10 16:17:27 Overscroll effect happens deep down at ApplyScroll Show quoted text On 2017/08/08 15:16:14, majidvp wrote: > This feels odd to me. It is not clear to me why exactly it is needed > and seems to be in conflict with the conditional statement above. > e. > > Also, is this something that should be part of \|CanPropagate\| logic? Overscroll effect happens deep down at ApplyScroll. If we want the overscroll information sent to browser side, we need to make sure there is something to scroll in the scroll chain so that we could walk down this path to OverscrollController. I've already moved the logic inside CanPropagate. majidvp 2017/08/15 14:30:12 We had some offline discussion about this and here Show quoted text On 2017/08/10 16:17:27, sunyunjia wrote: > On 2017/08/08 15:16:14, majidvp wrote: > > This feels odd to me. It is not clear to me why exactly it is needed > > and seems to be in conflict with the conditional statement above. > > e. > > > > Also, is this something that should be part of \|CanPropagate\| logic? > > Overscroll effect happens deep down at ApplyScroll. If we want the overscroll > information sent to browser side, we need to make sure there is something to > scroll in the scroll chain so that we could walk down this path to > OverscrollController. > > I've already moved the logic inside CanPropagate. We had some offline discussion about this and here is the summary that: - First this case should only happen when <body> is the viewport defining element. In this case body itself should actually not be scrollable! - It seems the fundamental fix here is to actually only consider scroll-boundary-behavior for element that are scrollable and can potentially be in scroll chain. But the current logic considers it for any element that is in containing block chain. So the solution we came up with was to do the following: for (element in containing block chain) { if (IsScrollable(element)) { if (CanScroll(element, scroll_state)) { scroll_chain.push(element) // add to scroll chain } if (!CanPropagate(element)) { break; // stop the chain here! } } } This should correctly filter out the viewport defining element that does not scroll.
140 break;	142 break;

141 }	143 }

142	144

143 cur_box = cur_box->ContainingBlock();	145 cur_box = cur_box->ContainingBlock();

144 }	146 }

145 }	147 }

146	148

147 bool ScrollManager::CanScroll(const ScrollState& scroll_state,	149 bool ScrollManager::CanScroll(const ScrollState& scroll_state,

148 const Element& current_element) {	150 const Element& current_element) {

149 const double delta_x = scroll_state.isBeginning() ? scroll_state.deltaXHint()	151 const double delta_x = scroll_state.isBeginning() ? scroll_state.deltaXHint()

(...skipping 577 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
727 &should_update_capture)) {	729 &should_update_capture)) {

728 if (should_update_capture)	730 if (should_update_capture)

729 scrollbar_handling_scroll_gesture_ = scrollbar;	731 scrollbar_handling_scroll_gesture_ = scrollbar;

730 return true;	732 return true;

731 }	733 }

732 }	734 }

733 return false;	735 return false;

734 }	736 }

735	737

736 } // namespace blink	738 } // namespace blink

OLD	NEW