Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 712986 FOCUS: Broken autoscroll around focus/selecting
Starred by 7 users Project Member Reported by abom...@etouch.net, Apr 19 Back to list
Status: Fixed
Owner:
Closed: May 23
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux, Windows, Mac
Pri: 1
Type: Bug-Regression

Blocking:
issue 715889



Sign in to add a comment
Chrome 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

 
Actual1.mov
4.1 MB Download
Expected.mov
4.0 MB Download
Labels: Needs-Bisect
Status: Untriaged
Cc: hu...@opera.com
Labels: -Needs-Bisect hasbisect-per-revision Proj-MaterialDesign-WebUI
Owner: yosin@chromium.org
Status: Assigned
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.!
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.

Labels: Hotlist-MD-Settings-General
Components: -UI>Settings Blink>Editing
Labels: -Proj-MaterialDesign-WebUI -Hotlist-MD-Settings-General
Summary: Broken autoscroll when focusing hidden selection (was: Regression: Weird behaviour is seen of scrollbar on [MD-settings] page using Tab key.)
yosin@, do you know how to fix this one?
Components: -Blink>Editing Blink>Editing>Selection
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>
Blocking: 715889
Labels: -M-60
Labels: M-60
Summary: FOCUS Broken autoscroll when focusing hidden selection (was: Broken autoscroll when focusing hidden selection )
Owner: ----
Status: Available
Owner: yoichio@chromium.org
Status: Started
Summary: FOCUS: Broken autoscroll around focus/selecting (was: FOCUS Broken autoscroll when focusing hidden selection )
Cc: kavvaru@chromium.org durga.behera@chromium.org brajkumar@chromium.org a...@chromium.org
 Issue 714535  has been merged into this issue.
Cc: yosin@chromium.org
 Issue 718395  has been merged into this issue.
Status: Available
Owner: ----
Status: Started
Owner: yoichio@chromium.org
Status: WontFix
Not reproduced on Version 60.0.3100.0.
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 ?

Status: Unconfirmed
Owner: ----
Status: Available
I see, tab cycling on
 <input value="hola" style="margin-top: 150%"> 
regresses.
Owner: yosin@chromium.org
Status: Started
Looking...
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

Owner: yoichio@chromium.org
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

Project Member Comment 27 by bugdroid1@chromium.org, May 19
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

Project Member Comment 28 by bugdroid1@chromium.org, May 22 (5 days ago)
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

Comment 29 by yoichio@chromium.org, May 23 (4 days ago)
Status: Fixed
Sign in to add a comment