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

Side by Side 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 unified diff | Download patch

OLD	NEW
1 /*	1 /*

2 * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.	2 * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.

3 * Copyright (C) 2008 Nuanti Ltd.	3 * Copyright (C) 2008 Nuanti Ltd.

4 *	4 *

5 * Redistribution and use in source and binary forms, with or without	5 * Redistribution and use in source and binary forms, with or without

6 * modification, are permitted provided that the following conditions	6 * modification, are permitted provided that the following conditions

7 * are met:	7 * are met:

8 * 1. Redistributions of source code must retain the above copyright	8 * 1. Redistributions of source code must retain the above copyright

9 * notice, this list of conditions and the following disclaimer.	9 * notice, this list of conditions and the following disclaimer.

10 * 2. Redistributions in binary form must reproduce the above copyright	10 * 2. Redistributions in binary form must reproduce the above copyright

(...skipping 52 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
63 using namespace HTMLNames;	63 using namespace HTMLNames;

64	64

65 static inline bool isShadowInsertionPointFocusScopeOwner(Node& node)	65 static inline bool isShadowInsertionPointFocusScopeOwner(Node& node)

66 {	66 {

67 return isActiveShadowInsertionPoint(node) && toHTMLShadowElement(node).older ShadowRoot();	67 return isActiveShadowInsertionPoint(node) && toHTMLShadowElement(node).older ShadowRoot();

68 }	68 }

69	69

70 class FocusNavigationScope {	70 class FocusNavigationScope {

71 STACK_ALLOCATED();	71 STACK_ALLOCATED();

72 public:	72 public:

	73 explicit FocusNavigationScope(TreeScope*);

73 Node* rootNode() const;	74 Node* rootNode() const;

74 Element* owner() const;	75 Element* owner() const;

75 static FocusNavigationScope focusNavigationScopeOf(const Node&);	76 static FocusNavigationScope focusNavigationScopeOf(const Node&);

76 static FocusNavigationScope ownedByNonFocusableFocusScopeOwner(Node&);	77 static FocusNavigationScope ownedByNonFocusableFocusScopeOwner(Node&);

77 static FocusNavigationScope ownedByShadowHost(const Node&);	78 static FocusNavigationScope ownedByShadowHost(const Node&);

78 static FocusNavigationScope ownedByShadowInsertionPoint(HTMLShadowElement&);	79 static FocusNavigationScope ownedByShadowInsertionPoint(HTMLShadowElement&);

79 static FocusNavigationScope ownedByIFrame(const HTMLFrameOwnerElement&);	80 static FocusNavigationScope ownedByIFrame(const HTMLFrameOwnerElement&);

80	81

81 private:	82 private:

82 explicit FocusNavigationScope(TreeScope*);

83 RawPtrWillBeMember<TreeScope> m_rootTreeScope;	83 RawPtrWillBeMember<TreeScope> m_rootTreeScope;

84 };	84 };

85	85

86 // FIXME: Some of Node* return values and Node* arguments should be Element*.	86 // FIXME: Some of Node* return values and Node* arguments should be Element*.

87	87

88 FocusNavigationScope::FocusNavigationScope(TreeScope* treeScope)	88 FocusNavigationScope::FocusNavigationScope(TreeScope* treeScope)

89 : m_rootTreeScope(treeScope)	89 : m_rootTreeScope(treeScope)

90 {	90 {

91 ASSERT(treeScope);	91 ASSERT(treeScope);

92 }	92 }

(...skipping 619 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
712 return previousNodeWithLowerTabIndex(last, startingTabIndex);	712 return previousNodeWithLowerTabIndex(last, startingTabIndex);

713 }	713 }

714	714

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

716 {	716 {

717 // FIXME: No spacial navigation code yet.	717 // FIXME: No spacial navigation code yet.

718 ASSERT(type == WebFocusTypeForward \|\| type == WebFocusTypeBackward);	718 ASSERT(type == WebFocusTypeForward \|\| type == WebFocusTypeBackward);

719 return findFocusableNodeAcrossFocusScopes(type, FocusNavigationScope::focusN avigationScopeOf(node), &node);	719 return findFocusableNodeAcrossFocusScopes(type, FocusNavigationScope::focusN avigationScopeOf(node), &node);

720 }	720 }

721	721

	722 Node* FocusController::findFocusableNode(WebFocusType type, Node* stayWithin, No de& node)
	kochi 2015/04/09 05:44:38 Do you want to scope the search of prev/next focus 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? AKV 2015/04/09 06:16:21 haveNextInput()wanted check the returned value (a Show quoted text 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. kochi 2015/04/10 02:33:50 Are you confused with "frame" and "form" or am I? Show quoted text 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? 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. AKV 2015/04/10 06:19:12 SMART GO should go within form1. Main goal of this Show quoted text 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. Show quoted text > > 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 ? 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 Show quoted text 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. Show quoted text > 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. Please go on!
	723 {

	724 ASSERT(type == WebFocusTypeForward \|\| type == WebFocusTypeBackward);

	725 return findFocusableNodeAcrossFocusScopes(type, FocusNavigationScope(&stayWi thin->treeScope()), &node);

	726 }

	727

722 Node* FocusController::findFocusableNode(WebFocusType type, const FocusNavigatio nScope& scope, Node* node)	728 Node* FocusController::findFocusableNode(WebFocusType type, const FocusNavigatio nScope& scope, Node* node)

723 {	729 {

724 return type == WebFocusTypeForward ? nextFocusableNode(scope, node) : previo usFocusableNode(scope, node);	730 return type == WebFocusTypeForward ? nextFocusableNode(scope, node) : previo usFocusableNode(scope, node);

725 }	731 }

726	732

727 static bool relinquishesEditingFocus(const Element& element)	733 static bool relinquishesEditingFocus(const Element& element)

728 {	734 {

729 ASSERT(element.hasEditableStyle());	735 ASSERT(element.hasEditableStyle());

730 return element.document().frame() && element.rootEditableElement();	736 return element.document().frame() && element.rootEditableElement();

731 }	737 }

(...skipping 279 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1011 return consumed;	1017 return consumed;

1012 }	1018 }

1013	1019

1014 DEFINE_TRACE(FocusController)	1020 DEFINE_TRACE(FocusController)

1015 {	1021 {

1016 visitor->trace(m_page);	1022 visitor->trace(m_page);

1017 visitor->trace(m_focusedFrame);	1023 visitor->trace(m_focusedFrame);

1018 }	1024 }

1019	1025

1020 } // namespace blink	1026 } // namespace blink

OLD	NEW

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