Spatnav should consider another candidate if found container cannot consume input |
|||||||||
Issue description
UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.91 Safari/537.36
Steps to reproduce the problem:
1. Launch browser with --enable-spatial-navigation
2. Load attached html files
3. Focus to top element (has text 'c')
4. Hit down arrow key
What is the expected behavior?
Focus should be moved to left-down element (has text 'b')
What went wrong?
Focus not moved.
Because current implementation is incorrect.
bool FocusController::AdvanceFocusDirectionallyInContainer() {
...
if (!AdvanceFocusDirectionallyInContainer(
ToLocalFrame(frame_element->ContentFrame())->GetDocument(),
rect,
type)) {
return AdvanceFocusDirectionallyInContainer(
container,
NodeRectInAbsoluteCoordinates(focus_candidate.visible_node, true),
type);
}
}
As you can see above code, if recursively finding phase is fail, spatnav try to find another candidate from rect of current 'candidate'.
It is definitely incorrect, we should find another candidate from rect of current 'focused element'.
Did this work before? No
Chrome version: 61.0.3163.91 Channel: canary
OS Version:
Flash Version:
,
Nov 15 2017
,
Nov 16 2017
Able to reproduce this issue on Mac 10.12.6, Win-10 and Ubuntu 14.04 using chrome reported version #61.0.3163.91 and latest canary #64.0.3269.3. This is a non-regression issue as it is observed from M50 old builds. Hence, marking it as untriaged to get more inputs from dev team. Thanks...!!
,
Nov 16 2017
junho0924.seo@, sounds like you're working on this bug? If so you should be assigned as owner but your user name doesn't show up. Have you followed the chromium contributor process: https://www.chromium.org/developers/contributing-code
,
Nov 17 2017
bokan@, yes I am. As far as I know, a person who has committer status can be assigned as owner filed. But I'm not committer :) Also, previously spatial navigation issues have Blink>Focus component. So I think this issue also moved to Blink>Focus. Please refer to https://bugs.chromium.org/p/chromium/issues/detail?id=763206
,
Nov 17 2017
Ah, ok, thanks for explaining. Is there a committer you're working with we can assign the bug to?
,
Nov 20 2017
,
Nov 20 2017
,
Nov 20 2017
For now, I get 'edit-bug-access' bit, so that can change owner field(also assign myself) and add a label.
,
Dec 14 2017
,
Jan 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/604e3794f34769458dd7bc0791f583d061dd9cd3 commit 604e3794f34769458dd7bc0791f583d061dd9cd3 Author: JunHo Seo <junho0924.seo@lge.com> Date: Fri Jan 19 05:27:35 2018 Snav: Remove recursion and ignore non-navigable subtrees This CL contains a major refactoring. 1. Ignore non-navigable iframes. Previously, spatnav only dug into the first iframe it found. Now, in case that is an iframe without focusables, we ignore it and go look for the next candidate. Note that additional time and space can be increased in the worst case (the next closest element is always an iframe that cannot consume input). Example: <a href='#'>focused element</a> <iframe></iframe> .. many iframes <iframe></iframe> <a href='#'>target element</a> 2. Remove recursion. Now we use a HeapVector and regard it as stack for managing search order instead the call stack. Also we cleaned up some dead code related with is_offscreen_after_ scrolling condition statements. The condition never hit because is_offscreen_after_scrolling==false is one of condition for being candidate. 3. Don't revisit subtrees we've already searched. When the search scope is expanded from "current container" C to C's parent container, AdvanceFocusDirectionallyInContainer() searched C's tree again. Now we skip already searched subtrees. Bug: 770147, 784320 Change-Id: Idcce4e9125ec05d35481ecdc17b1336a85bc968a Reviewed-on: https://chromium-review.googlesource.com/781326 Commit-Queue: JunHo Seo <junho0924.seo@lge.com> Reviewed-by: Stefan Zager <szager@chromium.org> Reviewed-by: Hugo Holgersson <hugoh@vewd.com> Cr-Commit-Position: refs/heads/master@{#530429} [add] https://crrev.com/604e3794f34769458dd7bc0791f583d061dd9cd3/third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-backtrack-out-of-non-navigable-iframe-expected.txt [add] https://crrev.com/604e3794f34769458dd7bc0791f583d061dd9cd3/third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-backtrack-out-of-non-navigable-iframe.html [modify] https://crrev.com/604e3794f34769458dd7bc0791f583d061dd9cd3/third_party/WebKit/Source/core/page/FocusController.cpp [modify] https://crrev.com/604e3794f34769458dd7bc0791f583d061dd9cd3/third_party/WebKit/Source/core/page/FocusController.h
,
Jan 19 2018
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by junho092...@lge.com
, Nov 13 2017