Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2935813002:
"Find" scrolling should be smoothly animated. (Closed)
Description"Find" scrolling should be smoothly animated.
Previously, when we "Find" (Ctr + F) something in the webpage, the page
abruptly scrolls the element into view, which makes it harder for users' eyes
to keep track. As Safari is using smooth scroll for the "Find" feature, we
implement this feature by setting the ScrollBehavior as "smooth" when calling
scrollRectoToVisible().
BUG=702966
Review-Url: https://codereview.chromium.org/2935813002
Cr-Commit-Position: refs/heads/master@{#479133}
Committed: https://chromium.googlesource.com/chromium/src/+/e790c147386fdd83a00c71b62a54908a73a27dd6
Patch Set 1 #
Total comments: 6
Patch Set 2 : Move smooth scroll logic to LayoutBox #
Total comments: 1
Patch Set 3 : Rebase and set ScrollRectToVisibleRecursive() to private. #
Messages
Total messages: 33 (23 generated)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== "Find" scrolling should be smoothly animated. BUG=702966 ========== to ========== "Find" scrolling should be smoothly animated. Previously, when we "Find" (Ctr + F) something in the webpage, the page abruptly scrolls the element into view, which makes it harder for users' eyes to keep track. As Safari is using smooth scroll for the "Find" feature, we implement this feature by calling element.ScrollIntoView() and setting the scroll behavior as "smooth". BUG=702966 ==========
The CQ bit was unchecked by sunyunjia@chromium.org
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== "Find" scrolling should be smoothly animated. Previously, when we "Find" (Ctr + F) something in the webpage, the page abruptly scrolls the element into view, which makes it harder for users' eyes to keep track. As Safari is using smooth scroll for the "Find" feature, we implement this feature by calling element.ScrollIntoView() and setting the scroll behavior as "smooth". BUG=702966 ========== to ========== "Find" scrolling should be smoothly animated. Previously, when we "Find" (Ctr + F) something in the webpage, the page abruptly scrolls the element into view, which makes it harder for users' eyes to keep track. As Safari is using smooth scroll for the "Find" feature, we implement this feature by setting the ScrollBehavior as "smooth" when calling scrollRectoToVisible(). BUG=702966 ==========
sunyunjia@chromium.org changed reviewers: + bokan@chromium.org
Hi David, This is a fix of a small bug with the help of smooth-scroll's landing. PTAL, thanks! https://codereview.chromium.org/2935813002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.cpp (left): https://codereview.chromium.org/2935813002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1621: ScrollAlignment::kAlignCenterIfNeeded, kUserScroll); Not sure why this was kUserScroll. Perhaps we should also change it to kProgrammaticScroll?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2935813002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.cpp (left): https://codereview.chromium.org/2935813002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1621: ScrollAlignment::kAlignCenterIfNeeded, kUserScroll); On 2017/06/12 21:17:57, sunyunjia wrote: > Not sure why this was kUserScroll. Perhaps we should also change it to > kProgrammaticScroll? Acknowledged. https://codereview.chromium.org/2935813002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2935813002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1618: first_node->GetDocument() This leaks too many details of how smooth scroll are implemented. Lets rearrange LayoutBox::ScrollRectToVisible a little; move the current implementation into ScrollRectToVisibleRecursive and call it from ScrollRectToVisible. LayoutBox::ScrollRectToVisible should call Abort and RunQueuedAnimations if the scroll is smooth. https://codereview.chromium.org/2935813002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1622: if (RuntimeEnabledFeatures::CSSOMSmoothScrollEnabled()) { This has no web-facing consequences so there's no need to guard it behind the flag. Lets just make this smooth in all cases.
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, thanks! https://codereview.chromium.org/2935813002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2935813002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1618: first_node->GetDocument() On 2017/06/13 01:48:08, bokan wrote: > This leaks too many details of how smooth scroll are implemented. Lets rearrange > LayoutBox::ScrollRectToVisible a little; move the current implementation into > ScrollRectToVisibleRecursive and call it from ScrollRectToVisible. > LayoutBox::ScrollRectToVisible should call Abort and RunQueuedAnimations if the > scroll is smooth. Done. https://codereview.chromium.org/2935813002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1622: if (RuntimeEnabledFeatures::CSSOMSmoothScrollEnabled()) { On 2017/06/13 01:48:08, bokan wrote: > This has no web-facing consequences so there's no need to guard it behind the > flag. Lets just make this smooth in all cases. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm % one small change https://codereview.chromium.org/2935813002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/2935813002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.h:591: void ScrollRectToVisibleRecursive(const LayoutRect&, This should be private
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sunyunjia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2935813002/#ps40001 (title: "Rebase and set ScrollRectToVisibleRecursive() to private.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497385775000230,
"parent_rev": "dd777afa856f55dc8fa734e856f508fe7724fe7b", "commit_rev":
"e790c147386fdd83a00c71b62a54908a73a27dd6"}
Message was sent while issue was closed.
Description was changed from ========== "Find" scrolling should be smoothly animated. Previously, when we "Find" (Ctr + F) something in the webpage, the page abruptly scrolls the element into view, which makes it harder for users' eyes to keep track. As Safari is using smooth scroll for the "Find" feature, we implement this feature by setting the ScrollBehavior as "smooth" when calling scrollRectoToVisible(). BUG=702966 ========== to ========== "Find" scrolling should be smoothly animated. Previously, when we "Find" (Ctr + F) something in the webpage, the page abruptly scrolls the element into view, which makes it harder for users' eyes to keep track. As Safari is using smooth scroll for the "Find" feature, we implement this feature by setting the ScrollBehavior as "smooth" when calling scrollRectoToVisible(). BUG=702966 Review-Url: https://codereview.chromium.org/2935813002 Cr-Commit-Position: refs/heads/master@{#479133} Committed: https://chromium.googlesource.com/chromium/src/+/e790c147386fdd83a00c71b62a54... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e790c147386fdd83a00c71b62a54...
Message was sent while issue was closed.
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
Message was sent while issue was closed.
Was there a UI review for this change? It feel pretty unusable on long pages as the scroll can take several seconds.
Message was sent while issue was closed.
On 2017/06/14 16:38:49, scottmg wrote: > Was there a UI review for this change? It feel pretty unusable on long pages as > the scroll can take several seconds. Additionally, I think it needs to respect the OS-level smooth scrolling behaviour. If mouse wheel and scroll bar scrolling are not smoothed, then find-in-page shouldn't be either.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2942673002/ by scottmg@chromium.org. The reason for reverting is: I believe this is a usability regression on desktop. Please reconsider, and perhaps it should be a mobile-only feature if it's an attempt to copy iOS Safari..
Message was sent while issue was closed.
On 2017/06/14 16:40:17, scottmg wrote: > On 2017/06/14 16:38:49, scottmg wrote: > > Was there a UI review for this change? It feel pretty unusable on long pages > as > > the scroll can take several seconds. We should probably scroll super fast in these long cases or just make it instant. That said, I agree we should loop in someone from UX to review - do you know who'd be relevant here? Otherwise I can try and find someone. > > Additionally, I think it needs to respect the OS-level smooth scrolling > behaviour. If mouse wheel and scroll bar scrolling are not smoothed, then > find-in-page shouldn't be either. Agreed, all smooth scrolls should respect the OS level setting (including if the page requests a smooth scroll). sunyunjia@, please file a bug for this. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
