FOCUS: Broken autoscroll around focus/selecting
Reported by
abom...@etouch.net,
Apr 19 2017
|
|||||||||||||||||||||||
Issue descriptionChrome Version:60.0.3074.0 (Official Build) 147bd1eedea8ce66f162faa43535d60137817d91-refs/heads/master@{#465085} OS: Windows (7,8,8.1,10),Linux (14.04 LTS),Mac OS X(10.11.6,10.12.1) Pre-condition: 1. Enable ‘Show Home button’ and set any page(chrome://settings/) as a Home. 2. Resize the browser window such that first radio button of home page is seen at the end OR 3. Sign-in the browser with validate credentials (no need to resize the browser window) What steps will reproduce the problem? 1. Launch chrome and navigate to chrome://md-settings/ 2. Press ‘Tab’ key till focus reaches to ‘Show bookmarks bar’ toggle button and then press ‘Shift+Tab’ such that focus reaches back to ‘Sign into chrome’ 3. Again press ‘Tab’ key till focus reaches second radio button of Home button, again press ‘Tab’ key once and Observe scrollbar. Actual: Page get Auto scrolled upwards after step 3. Expected: Page should not Auto scrolled upwards after step 3.(i.e. Scrollbar should be at the mid of the page and Tab focus should be seen) This is regression issue, broken in ‘M 60’ and below is Manual bisect info: Good build:59.0.3071.9 Bad build:60.0.3072.0
,
Apr 20 2017
Able to reproduce this issue on windows 7, Mac 10.11.4,Linux Ubuntu 14.04 with Chrome Canary-60.0.3074.0 Manual Bisect: -------------- Good build:59.0.3071.9-Revision-464641 Bad build:60.0.3072.0-Revision-464836 Per Revision Bisect Tool Info: ----------------------------- You are probably looking for a change made after 464697 (known good), but no later than 464698 (first known bad). CHANGELOG URL: The script might not always return single CL as suspect as some perf builds might get missing due to failure. https://chromium.googlesource.com/chromium/src/+log/188c1056bbfb9c98eca691c12f5641d4204f6066..0f65d25a4097959d977dbb1077717323438240a6 Review-Url: https://codereview.chromium.org/2616623002 Yosin@ assigning to you, as you were listed as one of the reviewers for this CL. Kindly take a look and please help us to reassign this issue to a right owner if not with respect to this change. Thanks.!
,
Apr 20 2017
I think this is the minimal test case:
<div style="background: green; padding: 20%;">
<a href="www">a focusable link</a>
<div style="height: 70%;"></div>
<div style="background: orange; padding: 20%;">
<input value="hola">
</div>
</div>
1. Use tab key to cycle through all focusable items.
2. In the >= second cycle, notice:
Actual: No scrolling (viewport shows top of page).
Expected: Scroll to <input> when it gets the focus.
,
Apr 24 2017
,
Apr 25 2017
yosin@, do you know how to fix this one?
,
Apr 25 2017
Layout Test:
<input value="hola" id="hola" style="margin-top: 150%">
<script type="text/javascript">
document.getElementById("hola").select(); // Does focus but doesn't scroll.
document.getElementById("hola").blur(); // Hides selection.
window.scrollTo(0,document.body.scrollHeight); // Scrolls to bottom.
document.getElementById("hola").focus(); // Should not trigger scroll to top.
</script>
,
Apr 27 2017
,
Apr 27 2017
,
Apr 27 2017
,
Apr 27 2017
,
Apr 28 2017
,
Apr 28 2017
,
Apr 28 2017
Issue 714535 has been merged into this issue.
,
May 8 2017
,
May 10 2017
,
May 11 2017
,
May 16 2017
,
May 16 2017
,
May 16 2017
Not reproduced on Version 60.0.3100.0.
,
May 16 2017
I see that the steps in #3 and #6 no longer reproduce the bug. But I found another way to trigger it on ToT, 60.0.3102.0. Again, cycle focus with tab key: <input value="hola" style="margin-top: 150%"> // No autoscroll. Interestingly, when the field is empty, autoscroll works: <input style="margin-top: 150%"> // Tab cycling triggers autoscroll OK. yochio@, do you see this too ?
,
May 16 2017
,
May 17 2017
I see, tab cycling on <input value="hola" style="margin-top: 150%"> regresses.
,
May 17 2017
Looking...
,
May 17 2017
The root cause is using FrameSelection::UnclippedBounds() returns empty rect in FrameSelection::RevealSelection() during TAB navigation. In #c6, when INPUT element gets focus by TAB navigation, FS::RevalSelection() is called in Document::SetFocusedElement() before calling FS::DidChangeFocus(), see stack trace below. Here is stack trace: FrameSelection::RevealSelection(const blink::ScrollAlignment & alignment, blink::RevealExtentOption reveal_extent_option) Line 1020 HTMLInputElement::UpdateFocusAppearance(blink::SelectionBehaviorOnFocus selection_behavior) Line 314 Document::SetFocusedElement(blink::Element * new_focused_element, const blink::FocusParams & params) Line 4255 FocusController::SetFocusedElement(blink::Element * element, blink::Frame * new_focused_frame, const blink::FocusParams & params) Line 1130 Element::focus(const blink::FocusParams & params) Line 2731 FocusController::AdvanceFocusInDocumentOrder(blink::LocalFrame * frame, blink::Element * start, blink::WebFocusType type, bool initial_focus, blink::InputDeviceCapabilities * source_capabilities) Line 1058 FocusController::AdvanceFocus(blink::WebFocusType type, bool initial_focus, blink::InputDeviceCapabilities * source_capabilities) Line 908 FocusController::AdvanceFocus(blink::WebFocusType type, blink::InputDeviceCapabilities * source_capabilities) Line 78 KeyboardEventManager::DefaultTabEventHandler(blink::KeyboardEvent * event) Line 416 KeyboardEventManager::DefaultKeyboardEventHandler(blink::KeyboardEvent * event, blink::Node * possible_focused_node) Line 291 EventHandler::DefaultKeyboardEventHandler(blink::KeyboardEvent * event) Line 1990 Node::DefaultEventHandler(blink::Event * event) Line 2291 HTMLElement::DefaultEventHandler(blink::Event * event) Line 1119 EventDispatcher::DispatchEventPostProcess(blink::EventDispatchHandlingState * pre_dispatch_event_handler_result) Line 288 EventDispatcher::Dispatch() Line 163 EventDispatchMediator::DispatchEvent(blink::EventDispatcher & dispatcher) Line 52 EventDispatcher::DispatchEvent(blink::Node & node, blink::EventDispatchMediator * mediator) Line 59 Node::DispatchEventInternal(blink::Event * event) Line 2162 EventTarget::DispatchEvent(blink::Event * event) Line 484 KeyboardEventManager::KeyEvent(const blink::WebKeyboardEvent & initial_key_event) Line 226 EventHandler::KeyEvent(const blink::WebKeyboardEvent & initial_key_event) Line 1985 blink_web.dll!blink::WebViewImpl::HandleKeyEvent(const blink::WebKeyboardEvent & event) Line 1165 blink_web.dll!blink::PageWidgetDelegate::HandleInputEvent(blink::PageWidgetEventHandler & handler, const blink::WebCoalescedInputEvent & coalesced_event, blink::LocalFrame * root) Line 181 blink_web.dll!blink::WebViewImpl::HandleInputEvent(const blink::WebCoalescedInputEvent & coalesced_event) Line 2249 blink_web.dll!blink::WebViewFrameWidget::HandleInputEvent(const blink::WebCoalescedInputEvent & event) Line 95
,
May 19 2017
,
May 19 2017
The root cause is we called FS::UnclippedBounds() without dirty selection flag in FS::RevealSelection() called by Document::SetFocusedElement(). Note: Document::SetFocusdElement() sets dirty selection flag at end of function instead of updating |fcosued_element_|. This is why FS::UnclipedBounds() returns empty rect. Hence, the solution is using ComputeTextRect() in FS::UnclippedBounds() instead of LayoutSelection::Bounds(). Note: We should update LayoutSelection just before paint instead of layout clean[1]. [1] http://crbug.com/724363 we should update LayoutSelection just before paint instead of LayoutClean
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d77cd716740a94334fc7c98749876f4a013b8322 commit d77cd716740a94334fc7c98749876f4a013b8322 Author: yoichio <yoichio@chromium.org> Date: Fri May 19 14:44:20 2017 Make VisibleUnits::ComputeTextRect() return unified IntRect Only Range uses Vector<IntRect> but it unifies and return a IntRect. Just unify in VisibleUnits::ComputeTextRect() directly. TEST=No change in behavior BUG= 712986 Review-Url: https://codereview.chromium.org/2891273003 Cr-Commit-Position: refs/heads/master@{#473185} [modify] https://crrev.com/d77cd716740a94334fc7c98749876f4a013b8322/third_party/WebKit/Source/core/dom/Range.cpp [modify] https://crrev.com/d77cd716740a94334fc7c98749876f4a013b8322/third_party/WebKit/Source/core/editing/VisibleUnits.cpp [modify] https://crrev.com/d77cd716740a94334fc7c98749876f4a013b8322/third_party/WebKit/Source/core/editing/VisibleUnits.h
,
May 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3638f51b033ad950d63f9186f7d0329a4eed48c9 commit 3638f51b033ad950d63f9186f7d0329a4eed48c9 Author: yoichio <yoichio@chromium.org> Date: Mon May 22 10:52:02 2017 Use current selection when tab navigation. This CL lets FrameSelection::RevealSelection() use VisibleUnits::ComputeTextRect() to compute current selection range instead of LayoutSelection::Bounds() of last selection range. VisibleUnits.cpp::ComputeTextRect(): templatize to define flat tree version. BUG= 712986 Review-Url: https://codereview.chromium.org/2894803002 Cr-Commit-Position: refs/heads/master@{#473546} [add] https://crrev.com/3638f51b033ad950d63f9186f7d0329a4eed48c9/third_party/WebKit/LayoutTests/editing/input/scroll-with-tab-to-input-regression.html [modify] https://crrev.com/3638f51b033ad950d63f9186f7d0329a4eed48c9/third_party/WebKit/Source/core/editing/FrameSelection.cpp [modify] https://crrev.com/3638f51b033ad950d63f9186f7d0329a4eed48c9/third_party/WebKit/Source/core/editing/FrameSelection.h [modify] https://crrev.com/3638f51b033ad950d63f9186f7d0329a4eed48c9/third_party/WebKit/Source/core/editing/VisibleUnits.cpp [modify] https://crrev.com/3638f51b033ad950d63f9186f7d0329a4eed48c9/third_party/WebKit/Source/core/editing/VisibleUnits.h
,
May 23 2017
,
May 30 2017
Verified the issue on windows 10, Mac 10.12.4 and Ubuntu 14.04 using chrome dev version #60.0.3112.7 as per comment #0. Observed that scrollbar stayed at the mid of the page and tab focus was seen. Hence, the fix is working as expected. Attaching screen cast for reference. Hence, adding the verified labels. Thanks...!! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by kkaluri@chromium.org
, Apr 19 2017Status: Untriaged (was: Unconfirmed)