Source/core/page/FocusController.cpp - Issue 939603002: Adding support for Smart GO NEXT feature in Android Chrome

Unified Diff: Source/core/page/FocusController.cpp

Issue 939603002: Adding support for Smart GO NEXT feature in Android Chrome (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Fixed the review comments and reused FocusController function for Navigation. Created 5 years, 8 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:

View side-by-side diff with in-line comments

Download patch

Index: Source/core/page/FocusController.cpp

diff --git a/Source/core/page/FocusController.cpp b/Source/core/page/FocusController.cpp

index 47a87e5e5f44ade3ccdab310f586841a9309280d..b8410616dd207af3960c32415917360f007446f8 100644

--- a/Source/core/page/FocusController.cpp

+++ b/Source/core/page/FocusController.cpp

@@ -70,6 +70,7 @@ static inline bool isShadowInsertionPointFocusScopeOwner(Node& node)

class FocusNavigationScope {

STACK_ALLOCATED();

public:

+ explicit FocusNavigationScope(TreeScope*);

Node* rootNode() const;

Element* owner() const;

static FocusNavigationScope focusNavigationScopeOf(const Node&);

@@ -79,7 +80,6 @@ public:

static FocusNavigationScope ownedByIFrame(const HTMLFrameOwnerElement&);

private:

- explicit FocusNavigationScope(TreeScope*);

RawPtrWillBeMember<TreeScope> m_rootTreeScope;

};

@@ -719,6 +719,12 @@ Node* FocusController::findFocusableNode(WebFocusType type, Node& node)

return findFocusableNodeAcrossFocusScopes(type, FocusNavigationScope::focusNavigationScopeOf(node), &node);

}

+Node* FocusController::findFocusableNode(WebFocusType type, Node* stayWithin, Node& node)

kochi 2015/04/09 05:44:38 Do you want to scope the search of prev/next focus

AKV 2015/04/09 06:16:21 haveNextInput()wanted check the returned value (a

kochi 2015/04/10 02:33:50 Are you confused with "frame" and "form" or am I?

AKV 2015/04/10 06:19:12 SMART GO should go within form1. Main goal of this

SMART GO should go within form1. Main goal of this feature is to help user for fast navigation within the form, instead of scrolling manually and tap on the element below the IME to get it focused for providing input.

By considering these scenarios, you are suggesting to use existing FocusController::findFocusableNode() which added by you earlier and have a validity check in haveNextInput() using isDecendantOf() to achieve my requirement instead of adding additional method in FocusController ? I think I got confused with treescope. The reason why I added new method is to restrict the search within the parent I am passing. But since it accepts, treescope, there is not difference between new method and old one. (Old one is traversing to the parent until Document node and restricts there) Thanks for your suggestion, I will wait for your reply to continue further.

kochi 2015/04/10 06:26:20 Yes, this is what I meant in the original reply. S

On 2015/04/10 06:19:12, AKV wrote: > On 2015/04/10 02:33:50, Takayoshi Kochi wrote: > > On 2015/04/09 06:16:21, AKV wrote: > > > On 2015/04/09 05:44:38, Takayoshi Kochi wrote: > > > > Do you want to scope the search of prev/next focusable node within a > <form> > > - > > > > </form>? > > > > > > > > The TreeScope class here is probably not what you want, it is a base class > > of > > > > Document or ShadowRoot. > > > > > > > > Maybe you can achieve what you want using Node::isDecendantOf() outside > this > > > > file > > > > (WebViewImpl::haveNextInput()) to check the returned value (a node) is > > within > > > > or out of the containing form element? > > > > > > haveNextInput()wanted check the returned value (a node) is "within the > > > containing form element". > > > Initial design approved by bcwhite is to search within same form element. I > > have > > > added TODO to take care of subsequent form element and across frames if it > > > improves user experience. > > > > Are you confused with "frame" and "form" or am I? > > E.g. > > <html> > > <body> > > <form id="form1"> > > <input type="text">... > > <input type="text">... > > </form> > > <form id="form2"> > > <input type="text">... > > <input type="checkbox">... > > <input type="checkbox">... > > </form> > > ... > > </body> > > </html> > > While the user is in form1, the SMART GO feature goes next/prev only within > > form1, > > or go to form2? > > > SMART GO should go within form1. Main goal of this feature is to help user for > fast navigation within the form, instead of scrolling manually and tap on the > element below the IME to get it focused for providing input. > > > > > > The usage of FocusNavigationScope doesn't restrict the search scope within > > form1, > > but the document (<html> - </html>). > > > > The <iframe> case, e.g. > > <html> > > <body> > > <form id="form1"> > > <input type="text">... > > <input type="text">... > > <iframe> > > ...(inserted document) > > <input type="text" id="innerinput"> > > </iframe> > > </form> > > ... > > </body> > > </html> > > > > In this case, the #innerinput doesn't participate in submitting data > > for #form1. Thus it makes sense that the focus navigation search should > > be confined within the same treescope. > > By considering these scenarios, you are suggesting to use existing > FocusController::findFocusableNode() which added by you earlier and have a > validity check in haveNextInput() using isDecendantOf() to achieve my > requirement instead of adding additional method in FocusController ?

Yes, this is what I meant in the original reply. Sorry if it was unclear. I believe we are on the same page now.

Please go on!

+ ASSERT(type == WebFocusTypeForward || type == WebFocusTypeBackward);

+ return findFocusableNodeAcrossFocusScopes(type, FocusNavigationScope(&stayWithin->treeScope()), &node);

Node* FocusController::findFocusableNode(WebFocusType type, const FocusNavigationScope& scope, Node* node)

{

return type == WebFocusTypeForward ? nextFocusableNode(scope, node) : previousFocusableNode(scope, node);

« no previous file with comments | « Source/core/page/FocusController.h ('k') | Source/web/WebViewImpl.h » ('j') | no next file with comments »