|
|
DescriptionAdding support for Smart GO NEXT feature in Android Chrome
This change takes care of providing easy navigation among text input
elements inside a form.
Corresponding changes to control the PREVIOUS, NEXT and GO button is done
at https://codereview.chromium.org/1080693002/
BUG=410785
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200398
Patch Set 1 #
Total comments: 20
Patch Set 2 : Fixed the review comments and reused FocusController function for Navigation. #
Total comments: 5
Patch Set 3 : Fixed the treescope problem for the current form. #
Total comments: 2
Patch Set 4 : Breaking the traversal when reaches outside current form scope. #
Total comments: 22
Patch Set 5 : Fixed the review comments by taking into consideration of input text elements outside of form, but … #Patch Set 6 : Fixed the naming issue in Keyboard function name. #
Total comments: 2
Patch Set 7 : Extended the navigation support on ContentEditable element inside form. #
Total comments: 18
Patch Set 8 : Corrected the eventlistener identification till topmost element. #
Total comments: 4
Patch Set 9 : Refined the Key Event listener finding method. #Patch Set 10 : Added test coverage for Smart GO/NEXT. #
Total comments: 10
Patch Set 11 : Fixed review comments in unit tests. #
Total comments: 32
Patch Set 12 : #Patch Set 13 : Fixed the test failure after the refined tests. #
Total comments: 6
Patch Set 14 : Added tabindex and disabled element coverage in unit tests. #Patch Set 15 : Rebased the patch to solve build breaks in Trybot. #Patch Set 16 : Fixed unit test failures related tabindex. #
Total comments: 6
Patch Set 17 : Splitted the tests for easy readability. #Patch Set 18 : Fixed build breaks in disabled element form. #
Total comments: 24
Patch Set 19 : Made all test case as deterministic with EXPECT_EQ #
Total comments: 12
Patch Set 20 : Removed duplicate code in tests using arrays. #Patch Set 21 : Fixed Windows platform build failure. #
Total comments: 4
Patch Set 22 : Changed while loop to for loop. #
Total comments: 1
Patch Set 23 : Fixed unit test breaks due to one element skip. #Messages
Total messages: 129 (31 generated)
ajith.v@chromium.org changed reviewers: + bcwhite@chromium.org
@bcwhite PTAL blink side changes.
ajith.v@chromium.org changed reviewers: + aelias@chromium.org, darin@chromium.org
@bcwhite, aelias and darin PTAL blink side changes.
Flags look fine. I'll leave the Blink analysis to the pros. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2454: HTMLFormControlElement* formControlElement = toHTMLFormControlElement(element); 80 character line limits https://codereview.chromium.org/939603002/diff/1/public/web/WebView.h File public/web/WebView.h (right): https://codereview.chromium.org/939603002/diff/1/public/web/WebView.h#newcode189 public/web/WebView.h:189: virtual void advanceFocusToNextInputField(bool direction) { } "direction" doesn't clearly define which matches "true" and which matches "false". Perhaps "forward" or "backward"?
aelias@chromium.org changed reviewers: + yosin@chromium.org - darin@chromium.org
I'm not an editing expert, adding yosin@ for review.
Please consider to use FocusControl to detect/move input focus. I think users want to stop at checkbox or radio button in addition to text edit controls. It seems FocusControl doesn't expose predicates to detect next/previous focus stop, but you can expose them if you needed. BTW, in Blink, there are very few chances to use |reinterpret_cast<T*>|, most of them are decoding memory block. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2454: HTMLFormControlElement* formControlElement = toHTMLFormControlElement(element); On 2015/02/24 17:06:23, bcwhite wrote: > 80 character line limits Blink coding style rule allows lines to over 80 characters: http://dev.chromium.org/blink/coding-style https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2455: Node* parentFormNode = reinterpret_cast<Node*>(formControlElement->formOwner()); We don't need to use |reinterpret_cast<Node*>|. |formOwner()| returns |HTMLFormElement|. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2457: return 0; nit: better to use |nullptr|. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2458: Node* nextNode = static_cast<Node*>(element)->nextSibling(); nit: We don't need to cast to |Node| for |element|, which typed |Element|. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2461: while ((nextNode = NodeRenderingTraversal::next(nextNode, parentFormNode))) { I think we should use tab order rather than rendering order. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2470: return 0; nit: better to use |nullptr|. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2476: return 0; nit: better to use |nullptr|. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2479: Node* parentFormNode = reinterpret_cast<Node*>(formControlElement->formOwner()); We don't need to use |reinterpret_cast<Node*>|. |formOwner()| returns |HTMLFormElement|. https://codereview.chromium.org/939603002/diff/1/public/web/WebView.h File public/web/WebView.h (right): https://codereview.chromium.org/939603002/diff/1/public/web/WebView.h#newcode189 public/web/WebView.h:189: virtual void advanceFocusToNextInputField(bool direction) { } Please considering use |WebFocusType| enum for representing direction related to focus.
yosin@chromium.org changed reviewers: + kochi@chromium.org
+kochi@, he is focus management expert.
I'm not sure about the chromium side change, but do Androids need 'hasNext(Previous)InputField' flag? (I assume yes, for indicating the existence on navigation button UI etc.) I'm also not sure per Yosi's advice, Android users want to toggle checkbox etc. via on-screen keyboard. You may want to look into WebKit/Source/core/page/FocusController.{h,cpp}. There's a logic to follow tabindex attribute and probably it's better idea to use the same order for Smart GO NEXT.
On 2015/02/25 03:33:27, Takayoshi Kochi wrote: > I'm not sure about the chromium side change, but do > Androids need 'hasNext(Previous)InputField' flag? > (I assume yes, for indicating the existence on navigation > button UI etc.) > > I'm also not sure per Yosi's advice, Android users want to > toggle checkbox etc. via on-screen keyboard. I supposed to reveal checkbox/radio button. I saw sometimes software keyboard hide checkbox/radio button below text field. How about sending TAB key code (U+0009) and Shift+TAB key code and handle them in |InputMethodController| like BACKSAPCE? Is it bad idea to use desktop/hardware keyboard analogy?
On 2015/02/25 03:33:27, Takayoshi Kochi wrote: > I'm not sure about the chromium side change, but do > Androids need 'hasNext(Previous)InputField' flag? > (I assume yes, for indicating the existence on navigation > button UI etc.) > > I'm also not sure per Yosi's advice, Android users want to > toggle checkbox etc. via on-screen keyboard. > > You may want to look into WebKit/Source/core/page/FocusController.{h,cpp}. > > There's a logic to follow tabindex attribute and probably > it's better idea to use the same order for Smart GO NEXT. Thanks Takayoshi Kochi. previousFocusableNode() ,nextFocusableNode() from FocusController will help me to achieve this.
On 2015/02/25 04:26:43, Yosi_UTC9 wrote: > On 2015/02/25 03:33:27, Takayoshi Kochi wrote: > > I'm not sure about the chromium side change, but do > > Androids need 'hasNext(Previous)InputField' flag? > > (I assume yes, for indicating the existence on navigation > > button UI etc.) > > > > I'm also not sure per Yosi's advice, Android users want to > > toggle checkbox etc. via on-screen keyboard. > I supposed to reveal checkbox/radio button. I saw sometimes > software keyboard hide checkbox/radio button below text field. > > How about sending TAB key code (U+0009) and Shift+TAB key code > and handle them in |InputMethodController| like BACKSAPCE? > > Is it bad idea to use desktop/hardware keyboard analogy? @Yosi - Could you pls elaborate bit more on this ? Using IMC is better than explicitly calculating the next/previous elements and bringing them using on screen scrollIntoViewIfNeeded() ?
On 2015/02/25 11:34:10, AKV wrote: > On 2015/02/25 04:26:43, Yosi_UTC9 wrote: > > On 2015/02/25 03:33:27, Takayoshi Kochi wrote: > > > I'm not sure about the chromium side change, but do > > > Androids need 'hasNext(Previous)InputField' flag? > > > (I assume yes, for indicating the existence on navigation > > > button UI etc.) > > > > > > I'm also not sure per Yosi's advice, Android users want to > > > toggle checkbox etc. via on-screen keyboard. > > I supposed to reveal checkbox/radio button. I saw sometimes > > software keyboard hide checkbox/radio button below text field. > > > > How about sending TAB key code (U+0009) and Shift+TAB key code > > and handle them in |InputMethodController| like BACKSAPCE? > > > > Is it bad idea to use desktop/hardware keyboard analogy? > > @Yosi - Could you pls elaborate bit more on this ? Using IMC is better than > explicitly calculating the next/previous elements and bringing them using on > screen scrollIntoViewIfNeeded() ? Probably what yosi meant was that not introduce a new interface in WebView but sending TAB or SHIFT+TAB keyevent from chromium side to Blink so that Blink can interpret them as focus navigation. I don't think it works when you are on <textarea>, so I doubt the feasibility.
On 2015/02/26 04:28:17, Takayoshi Kochi wrote: > On 2015/02/25 11:34:10, AKV wrote: > > On 2015/02/25 04:26:43, Yosi_UTC9 wrote: > > > On 2015/02/25 03:33:27, Takayoshi Kochi wrote: > > > > I'm not sure about the chromium side change, but do > > > > Androids need 'hasNext(Previous)InputField' flag? > > > > (I assume yes, for indicating the existence on navigation > > > > button UI etc.) > > > > > > > > I'm also not sure per Yosi's advice, Android users want to > > > > toggle checkbox etc. via on-screen keyboard. > > > I supposed to reveal checkbox/radio button. I saw sometimes > > > software keyboard hide checkbox/radio button below text field. > > > > > > How about sending TAB key code (U+0009) and Shift+TAB key code > > > and handle them in |InputMethodController| like BACKSAPCE? > > > > > > Is it bad idea to use desktop/hardware keyboard analogy? > > > > @Yosi - Could you pls elaborate bit more on this ? Using IMC is better than > > explicitly calculating the next/previous elements and bringing them using on > > screen scrollIntoViewIfNeeded() ? > > Probably what yosi meant was that not introduce a new interface in WebView but > sending TAB or SHIFT+TAB keyevent from chromium side to Blink so that > Blink can interpret them as focus navigation. > > I don't think it works when you are on <textarea>, so I doubt the feasibility. Thanks kochi@ for followup. Good point. As of |EventHandler::defaultTabEventHandler()| WebFocusType focusType = event->shiftKey() ? WebFocusTypeBackward : WebFocusTypeForward; // Tabs can be used in design mode editing. if (m_frame->document()->inDesignMode()) return; if (page->focusController().advanceFocus(focusType)) event->setDefaultHandled(); It seems new API can be implemented to call FocusControll::advanceFocus().
On 2015/02/26 05:03:21, Yosi_UTC9 wrote: > On 2015/02/26 04:28:17, Takayoshi Kochi wrote: > > On 2015/02/25 11:34:10, AKV wrote: > > > On 2015/02/25 04:26:43, Yosi_UTC9 wrote: > > > > On 2015/02/25 03:33:27, Takayoshi Kochi wrote: > > > > > I'm not sure about the chromium side change, but do > > > > > Androids need 'hasNext(Previous)InputField' flag? > > > > > (I assume yes, for indicating the existence on navigation > > > > > button UI etc.) > > > > > > > > > > I'm also not sure per Yosi's advice, Android users want to > > > > > toggle checkbox etc. via on-screen keyboard. > > > > I supposed to reveal checkbox/radio button. I saw sometimes > > > > software keyboard hide checkbox/radio button below text field. > > > > > > > > How about sending TAB key code (U+0009) and Shift+TAB key code > > > > and handle them in |InputMethodController| like BACKSAPCE? > > > > > > > > Is it bad idea to use desktop/hardware keyboard analogy? > > > > > > @Yosi - Could you pls elaborate bit more on this ? Using IMC is better than > > > explicitly calculating the next/previous elements and bringing them using on > > > screen scrollIntoViewIfNeeded() ? > > > > Probably what yosi meant was that not introduce a new interface in WebView but > > sending TAB or SHIFT+TAB keyevent from chromium side to Blink so that > > Blink can interpret them as focus navigation. > > > > I don't think it works when you are on <textarea>, so I doubt the feasibility. > > Thanks kochi@ for followup. Good point. > > As of |EventHandler::defaultTabEventHandler()| > > WebFocusType focusType = event->shiftKey() ? WebFocusTypeBackward : > WebFocusTypeForward; > > // Tabs can be used in design mode editing. > if (m_frame->document()->inDesignMode()) > return; > > if (page->focusController().advanceFocus(focusType)) > event->setDefaultHandled(); > > > It seems new API can be implemented to call FocusControll::advanceFocus(). Yeah, the closest public API for this purpose would be FocusController::advanceFocus(), (nextFocusableNode/previousFocusableNode are file-local static functions) but the API is intended to handle TAB/SHIFT+TAB on the browser, so it can move focus out of web contents (e.g. to URL bar) when all the focusable nodes are exhausted. To fit the purpose for this smart go feature, you need to split the functionality into some pieces (it shouldn't be so difficult).
On 2015/02/26 07:09:32, Takayoshi Kochi wrote: > On 2015/02/26 05:03:21, Yosi_UTC9 wrote: > > On 2015/02/26 04:28:17, Takayoshi Kochi wrote: > > > On 2015/02/25 11:34:10, AKV wrote: > > > > On 2015/02/25 04:26:43, Yosi_UTC9 wrote: > > > > > On 2015/02/25 03:33:27, Takayoshi Kochi wrote: > > > > > > I'm not sure about the chromium side change, but do > > > > > > Androids need 'hasNext(Previous)InputField' flag? > > > > > > (I assume yes, for indicating the existence on navigation > > > > > > button UI etc.) > > > > > > > > > > > > I'm also not sure per Yosi's advice, Android users want to > > > > > > toggle checkbox etc. via on-screen keyboard. > > > > > I supposed to reveal checkbox/radio button. I saw sometimes > > > > > software keyboard hide checkbox/radio button below text field. > > > > > > > > > > How about sending TAB key code (U+0009) and Shift+TAB key code > > > > > and handle them in |InputMethodController| like BACKSAPCE? > > > > > > > > > > Is it bad idea to use desktop/hardware keyboard analogy? > > > > > > > > @Yosi - Could you pls elaborate bit more on this ? Using IMC is better > than > > > > explicitly calculating the next/previous elements and bringing them using > on > > > > screen scrollIntoViewIfNeeded() ? > > > > > > Probably what yosi meant was that not introduce a new interface in WebView > but > > > sending TAB or SHIFT+TAB keyevent from chromium side to Blink so that > > > Blink can interpret them as focus navigation. > > > > > > I don't think it works when you are on <textarea>, so I doubt the > feasibility. > > > > Thanks kochi@ for followup. Good point. > > > > As of |EventHandler::defaultTabEventHandler()| > > > > WebFocusType focusType = event->shiftKey() ? WebFocusTypeBackward : > > WebFocusTypeForward; > > > > // Tabs can be used in design mode editing. > > if (m_frame->document()->inDesignMode()) > > return; > > > > if (page->focusController().advanceFocus(focusType)) > > event->setDefaultHandled(); > > > > > > It seems new API can be implemented to call FocusControll::advanceFocus(). > > Yeah, the closest public API for this purpose would be > FocusController::advanceFocus(), > (nextFocusableNode/previousFocusableNode are file-local static functions) > but the API is intended to handle TAB/SHIFT+TAB on the browser, so it can move > focus > out of web contents (e.g. to URL bar) when all the focusable nodes are > exhausted. > > To fit the purpose for this smart go feature, you need to split the > functionality > into some pieces (it shouldn't be so difficult). Just FYI: As a part of crbug/380445, I made findFocusableNode() in class FocusController public, and it should be usable for this smart go purpose. Any progress on this?
On 2015/04/06 05:01:33, Takayoshi Kochi wrote: > On 2015/02/26 07:09:32, Takayoshi Kochi wrote: > > On 2015/02/26 05:03:21, Yosi_UTC9 wrote: > > > On 2015/02/26 04:28:17, Takayoshi Kochi wrote: > > > > On 2015/02/25 11:34:10, AKV wrote: > > > > > On 2015/02/25 04:26:43, Yosi_UTC9 wrote: > > > > > > On 2015/02/25 03:33:27, Takayoshi Kochi wrote: > > > > > > > I'm not sure about the chromium side change, but do > > > > > > > Androids need 'hasNext(Previous)InputField' flag? > > > > > > > (I assume yes, for indicating the existence on navigation > > > > > > > button UI etc.) > > > > > > > > > > > > > > I'm also not sure per Yosi's advice, Android users want to > > > > > > > toggle checkbox etc. via on-screen keyboard. > > > > > > I supposed to reveal checkbox/radio button. I saw sometimes > > > > > > software keyboard hide checkbox/radio button below text field. > > > > > > > > > > > > How about sending TAB key code (U+0009) and Shift+TAB key code > > > > > > and handle them in |InputMethodController| like BACKSAPCE? > > > > > > > > > > > > Is it bad idea to use desktop/hardware keyboard analogy? > > > > > > > > > > @Yosi - Could you pls elaborate bit more on this ? Using IMC is better > > than > > > > > explicitly calculating the next/previous elements and bringing them > using > > on > > > > > screen scrollIntoViewIfNeeded() ? > > > > > > > > Probably what yosi meant was that not introduce a new interface in WebView > > but > > > > sending TAB or SHIFT+TAB keyevent from chromium side to Blink so that > > > > Blink can interpret them as focus navigation. > > > > > > > > I don't think it works when you are on <textarea>, so I doubt the > > feasibility. > > > > > > Thanks kochi@ for followup. Good point. > > > > > > As of |EventHandler::defaultTabEventHandler()| > > > > > > WebFocusType focusType = event->shiftKey() ? WebFocusTypeBackward : > > > WebFocusTypeForward; > > > > > > // Tabs can be used in design mode editing. > > > if (m_frame->document()->inDesignMode()) > > > return; > > > > > > if (page->focusController().advanceFocus(focusType)) > > > event->setDefaultHandled(); > > > > > > > > > It seems new API can be implemented to call FocusControll::advanceFocus(). > > > > Yeah, the closest public API for this purpose would be > > FocusController::advanceFocus(), > > (nextFocusableNode/previousFocusableNode are file-local static functions) > > but the API is intended to handle TAB/SHIFT+TAB on the browser, so it can move > > focus > > out of web contents (e.g. to URL bar) when all the focusable nodes are > > exhausted. > > > > To fit the purpose for this smart go feature, you need to split the > > functionality > > into some pieces (it shouldn't be so difficult). > > Just FYI: > As a part of crbug/380445, I made findFocusableNode() in class FocusController > public, > and it should be usable for this smart go purpose. > > Any progress on this? @Kochi Thanks for the information. I am facing few issues in Chromium side patch. Once it addressed I will rebase this patch to accommodate new changes. Sorry for the late reply.
@Kochi and Yosi PTAL https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2454: HTMLFormControlElement* formControlElement = toHTMLFormControlElement(element); On 2015/02/25 01:25:36, Yosi_UTC9 wrote: > On 2015/02/24 17:06:23, bcwhite wrote: > > 80 character line limits > > Blink coding style rule allows lines to over 80 characters: > http://dev.chromium.org/blink/coding-style Done. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2457: return 0; On 2015/02/25 01:25:36, Yosi_UTC9 wrote: > nit: better to use |nullptr|. Done. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2461: while ((nextNode = NodeRenderingTraversal::next(nextNode, parentFormNode))) { On 2015/02/25 01:25:36, Yosi_UTC9 wrote: > I think we should use tab order rather than rendering order. Done. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2470: return 0; On 2015/02/25 01:25:36, Yosi_UTC9 wrote: > nit: better to use |nullptr|. Done. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2476: return 0; On 2015/02/25 01:25:36, Yosi_UTC9 wrote: > nit: better to use |nullptr|. Done. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2476: return 0; On 2015/02/25 01:25:36, Yosi_UTC9 wrote: > nit: better to use |nullptr|. Done. https://codereview.chromium.org/939603002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:2479: Node* parentFormNode = reinterpret_cast<Node*>(formControlElement->formOwner()); On 2015/02/25 01:25:36, Yosi_UTC9 wrote: > We don't need to use |reinterpret_cast<Node*>|. |formOwner()| returns > |HTMLFormElement|. I wanted to use Node pointer to pass to FocusController, so I converted to Node from Element. https://codereview.chromium.org/939603002/diff/1/public/web/WebView.h File public/web/WebView.h (right): https://codereview.chromium.org/939603002/diff/1/public/web/WebView.h#newcode189 public/web/WebView.h:189: virtual void advanceFocusToNextInputField(bool direction) { } On 2015/02/24 17:06:23, bcwhite wrote: > "direction" doesn't clearly define which matches "true" and which matches > "false". Perhaps "forward" or "backward"? Done. Thanks https://codereview.chromium.org/939603002/diff/1/public/web/WebView.h#newcode189 public/web/WebView.h:189: virtual void advanceFocusToNextInputField(bool direction) { } On 2015/02/25 01:25:36, Yosi_UTC9 wrote: > Please considering use |WebFocusType| enum for representing direction related to > focus. Done. Thanks
https://codereview.chromium.org/939603002/diff/20001/Source/core/page/FocusCo... File Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/939603002/diff/20001/Source/core/page/FocusCo... Source/core/page/FocusController.cpp:722: Node* FocusController::findFocusableNode(WebFocusType type, Node* stayWithin, Node& node) 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?
@Kochi - Thanks for the review. Please see my inline comment. https://codereview.chromium.org/939603002/diff/20001/Source/core/page/FocusCo... File Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/939603002/diff/20001/Source/core/page/FocusCo... Source/core/page/FocusController.cpp:722: Node* FocusController::findFocusableNode(WebFocusType type, Node* stayWithin, Node& node) 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.
https://codereview.chromium.org/939603002/diff/20001/Source/core/page/FocusCo... File Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/939603002/diff/20001/Source/core/page/FocusCo... Source/core/page/FocusController.cpp:722: Node* FocusController::findFocusableNode(WebFocusType type, Node* stayWithin, Node& node) 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.
https://codereview.chromium.org/939603002/diff/20001/Source/core/page/FocusCo... File Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/939603002/diff/20001/Source/core/page/FocusCo... Source/core/page/FocusController.cpp:722: Node* FocusController::findFocusableNode(WebFocusType type, Node* stayWithin, Node& node) 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 ? 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.
https://codereview.chromium.org/939603002/diff/20001/Source/core/page/FocusCo... File Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/939603002/diff/20001/Source/core/page/FocusCo... Source/core/page/FocusController.cpp:722: Node* FocusController::findFocusableNode(WebFocusType type, Node* stayWithin, Node& node) 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. > 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!
ajith.v@samsung.com changed reviewers: + ajith.v@samsung.com
@Kochi PTAL
ajith.v@chromium.org changed reviewers: - ajith.v@samsung.com
Sorry for the delay for this review. https://codereview.chromium.org/939603002/diff/40001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/40001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2551: // TODO(AKV) Do we need to quit as and when we found a node which is not child of this form. (Need to discuss with Kochi) Yeah, I think once you go out of the current form, you should break here. So I'd write the code as while (...) { if (!nextNode->isDescendantOf(parentFormNode)) break; // or return nullptr; ? LayoutObject* layout = nextNode->layoutObject(); ... }
@Kochi - Thanks for the review. I did the changes. PTAL https://codereview.chromium.org/939603002/diff/40001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/40001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2551: // TODO(AKV) Do we need to quit as and when we found a node which is not child of this form. (Need to discuss with Kochi) On 2015/04/14 05:08:30, Takayoshi Kochi wrote: > Yeah, I think once you go out of the current form, you should break here. > So I'd write the code as > > while (...) { > if (!nextNode->isDescendantOf(parentFormNode)) > break; // or return nullptr; ? > LayoutObject* layout = nextNode->layoutObject(); > ... > } Thanks. I did same :)
Thanks, lgtm Does any test from chromium side (browser_tests?) cover this code path?
On 2015/04/14 06:06:47, Takayoshi Kochi wrote: > Thanks, lgtm > > Does any test from chromium side (browser_tests?) cover > this code path? Once https://codereview.chromium.org/1080693002/ is approved by bcwhite, I will write Unit test probably on ImeTest.java or related files to cover this code path. Thank you :)
On 2015/04/14 06:08:37, AKV wrote: > On 2015/04/14 06:06:47, Takayoshi Kochi wrote: > > Thanks, lgtm > > > > Does any test from chromium side (browser_tests?) cover > > this code path? > > Once https://codereview.chromium.org/1080693002/ is approved by bcwhite, I will > write Unit test probably on ImeTest.java or related files to cover this code > path. Thank you :) Sounds okay. It's better to have the code and its corresponding tests at once, but WebViewImpl is an interface to chromium so it sometimes is hard to do it simultaneously. Could you add the reference to the chromium side of the corresponding change (and vice versa, link to this change in the chromium side change) in the CL description? It makes easier to follow the linkage of two changes than finding the changes in the issue tracker and correlate them manually.
On 2015/04/14 06:14:23, Takayoshi Kochi wrote: > On 2015/04/14 06:08:37, AKV wrote: > > On 2015/04/14 06:06:47, Takayoshi Kochi wrote: > > > Thanks, lgtm > > > > > > Does any test from chromium side (browser_tests?) cover > > > this code path? > > > > Once https://codereview.chromium.org/1080693002/ is approved by bcwhite, I > will > > write Unit test probably on ImeTest.java or related files to cover this code > > path. Thank you :) > > Sounds okay. It's better to have the code and its corresponding tests at once, > but WebViewImpl is an interface to chromium so it sometimes is hard to do it > simultaneously. > > Could you add the reference to the chromium side of the corresponding change > (and vice versa, link to this change in the chromium side change) in the > CL description? It makes easier to follow the linkage of two changes > than finding the changes in the issue tracker and correlate them manually. Thanks for your suggestions. I updated both CLs with dependent CL URLs for easy tracking.
The CQ bit was checked by ajith.v@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/939603002/60001
You still need l-g-t-m from someone in Source/web/OWNERS.
On 2015/04/14 06:32:16, Takayoshi Kochi wrote: > You still need l-g-t-m from someone in Source/web/OWNERS. Sorry. I didn't noticed it. I will uncheck the bit and request for review and l-g-t-m. Thanks for notifying me :)
The CQ bit was unchecked by ajith.v@chromium.org
ajith.v@chromium.org changed reviewers: + adamk@chromium.org, darin@chromium.org, rbyers@chromium.org - bcwhite@chromium.org, yosin@chromium.org
@Rick Byers/Darin/Aelias/Adamk PTAL this change.
rbyers@chromium.org changed reviewers: + tkent@chromium.org - adamk@chromium.org, aelias@chromium.org, darin@chromium.org, rbyers@chromium.org
On 2015/04/14 06:59:23, AKV wrote: > @Rick Byers/Darin/Aelias/Adamk PTAL this change. Asking multiple people to provide OWNERS review will often result in none of them doing it (each expecting someone else to do it). In this case I think tkent@ is a good OWNER (forms related) for both public/web and Source/web
@tkent PTAL this change.
ajith.v@chromium.org changed reviewers: + jochen@chromium.org
@jochen PTAL this change.
tkent@chromium.org changed reviewers: - jochen@chromium.org
I'm an appropriate reviewer. Remove jochen@.
https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2535: Element* WebViewImpl::haveNextInput(Element* element, WebFocusType focusType) The function name is not reasonable. - The name looks like it returns a bool, but it returns an element. The name should be |nextInput|. - The function returns not only <input>, but also <textarea> and contenteditable elements. The name should be |nextEditableElement|. - A TODO comment says this will be expanded to other form controls. The name should be |nextFocusableElement| or something. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2537: if (!element->isFormControlElement()) Do you want to reject a contenteditable element though this function can return a contenteditable element? https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2541: Node* parentFormNode = reinterpret_cast<Node*>(formControlElement->formOwner()); |parentFormNode| is not a good name because it's not a parent, and it's an element. |formOwner| is a good candidate. You don't need to use reinterpret_cast<>. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2547: if (!nextNode->isDescendantOf(parentFormNode)) This doesn't work for the following case. <form id=f1> <input > <!-- This is the current focused element --> </form> <input form=f1> We should check HTMLFormControlElement::formOwner() equivalence. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2552: if (nextNode->isElementNode()) No need to check isElementNode. Only elements are focusable. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2559: bool WebViewImpl::wantEnterEvents(Element* element) The argument should be |const Element&|. The function name and the implementation doesn't match. The name should be |isListeningToKeyboardEvents| if we follow the implementation. What's the intention of this function? https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2564: return (element->hasEventListeners(EventTypeNames::keydown) || element->hasEventListeners(EventTypeNames::keypress) || element->hasEventListeners(EventTypeNames::keystatuseschange) || element->hasEventListeners(EventTypeNames::keyup)); Why do you check keystatuschange event, which is for encrypted media? https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2578: nextElement->dispatchSimulatedClick(0, SendMouseUpDownEvents); 0 -> nullptr. Why do you need to dispatch a click event? https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2579: nextElement->focus(false, focusType); The click event handler can delete |nextElement|. We need to make |nextElement| |RefPtrWIllBeRawPtr<Element>|.
https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2547: if (!nextNode->isDescendantOf(parentFormNode)) On 2015/04/14 23:27:19, tkent wrote: > This doesn't work for the following case. > > <form id=f1> > <input > <!-- This is the current focused element --> > </form> > <input form=f1> > > We should check HTMLFormControlElement::formOwner() equivalence. Hmm, I was not aware of this way of declaring form element outside <form></form>. It means that until the focus controller reaches browser chrome (out of content), we have to search for a focusable element associated with the current form. IIUC this is not a performance-critical path, so it would not matter much for iterating nodes exhaustively. It might be worth exploring the solution to iterate through HTMLFormElement::associatedElements().
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
We should not land this until the upstream change has been tested and approved.
On 2015/04/15 18:34:46, jdduke wrote: > We should not land this until the upstream change has been tested and approved. Sorry I mean the downstream Chromium change.
On 2015/04/15 18:35:12, jdduke wrote: > On 2015/04/15 18:34:46, jdduke wrote: > > We should not land this until the upstream change has been tested and > approved. > > Sorry I mean the downstream Chromium change. @jdduke - Once I fix the review comments on blink side and chromium side, I will share a detailed document about the required changes needs to be done on Keyboard to bcwhite. Based on that probably, we can decide when this patch can land on upstream branch. Thanks for your review.
On 2015/04/15 18:45:34, AKV wrote: > On 2015/04/15 18:35:12, jdduke wrote: > > On 2015/04/15 18:34:46, jdduke wrote: > > > We should not land this until the upstream change has been tested and > > approved. > > > > Sorry I mean the downstream Chromium change. > > @jdduke - Once I fix the review comments on blink side and chromium side, I will > share a detailed document about the required changes needs to be done on > Keyboard to bcwhite. Based on that probably, we can decide when this patch can > land on upstream branch. Thanks for your review. Sounds good, and I didn't mean to come across as overly critical. This will be a really nice addition after a bit of polishing, so thanks for sticking with it!
@tkent & kochi PTAL. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2535: Element* WebViewImpl::haveNextInput(Element* element, WebFocusType focusType) On 2015/04/14 23:27:19, tkent wrote: > The function name is not reasonable. > - The name looks like it returns a bool, but it returns an element. The name > should be |nextInput|. > - The function returns not only <input>, but also <textarea> and contenteditable > elements. The name should be |nextEditableElement|. > - A TODO comment says this will be expanded to other form controls. The name > should be |nextFocusableElement| or something. Done. nextFocusableElement is ideal. Thanks :) https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2537: if (!element->isFormControlElement()) On 2015/04/14 23:27:19, tkent wrote: > Do you want to reject a contenteditable element though this function can return > a contenteditable element? I care about elements which are inside the form. If the ContentEditable is child of a form, then also isFormControlElement() function returns false ? Could you please clarify on this ? https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2541: Node* parentFormNode = reinterpret_cast<Node*>(formControlElement->formOwner()); On 2015/04/14 23:27:19, tkent wrote: > |parentFormNode| is not a good name because it's not a parent, and it's an > element. |formOwner| is a good candidate. > You don't need to use reinterpret_cast<>. Done. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2547: if (!nextNode->isDescendantOf(parentFormNode)) On 2015/04/14 23:27:19, tkent wrote: > This doesn't work for the following case. > > <form id=f1> > <input > <!-- This is the current focused element --> > </form> > <input form=f1> > > We should check HTMLFormControlElement::formOwner() equivalence. Thanks. I have taken care this case by checking formOwner. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2547: if (!nextNode->isDescendantOf(parentFormNode)) On 2015/04/15 01:09:18, Takayoshi Kochi wrote: > On 2015/04/14 23:27:19, tkent wrote: > > This doesn't work for the following case. > > > > <form id=f1> > > <input > <!-- This is the current focused element --> > > </form> > > <input form=f1> > > > > We should check HTMLFormControlElement::formOwner() equivalence. > > Hmm, I was not aware of this way of declaring form element outside > <form></form>. > It means that until the focus controller reaches browser chrome (out of > content), > we have to search for a focusable element associated with the current form. > > IIUC this is not a performance-critical path, so it would not matter much for > iterating nodes exhaustively. It might be worth exploring the solution to > iterate through HTMLFormElement::associatedElements(). Thanks. I have taken care this case by checking formOwner. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2552: if (nextNode->isElementNode()) On 2015/04/14 23:27:19, tkent wrote: > No need to check isElementNode. Only elements are focusable. Done. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2559: bool WebViewImpl::wantEnterEvents(Element* element) On 2015/04/14 23:27:19, tkent wrote: > The argument should be |const Element&|. > > The function name and the implementation doesn't match. The name should be > |isListeningToKeyboardEvents| if we follow the implementation. What's the > intention of this function? I wanted to check whether the element has any listener for "Enter Key". If no enter key listener, we don't want to show Enter key from IME side. I will check specific case of Enter Key instead of whole key events. Do you have any suggestion to check Enter key listener is present or not ? Thanks. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2564: return (element->hasEventListeners(EventTypeNames::keydown) || element->hasEventListeners(EventTypeNames::keypress) || element->hasEventListeners(EventTypeNames::keystatuseschange) || element->hasEventListeners(EventTypeNames::keyup)); On 2015/04/14 23:27:19, tkent wrote: > Why do you check keystatuschange event, which is for encrypted media? > Right. this is not required. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2578: nextElement->dispatchSimulatedClick(0, SendMouseUpDownEvents); On 2015/04/14 23:27:19, tkent wrote: > 0 -> nullptr. > > Why do you need to dispatch a click event? Not required. Thanks. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2579: nextElement->focus(false, focusType); On 2015/04/14 23:27:19, tkent wrote: > The click event handler can delete |nextElement|. We need to make |nextElement| > |RefPtrWIllBeRawPtr<Element>|. Thanks. I corrected it.
@tkent & kochi PTAL Patch Set 6!
lgtm for the focus control part. Defer anything else to tkent.
https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2537: if (!element->isFormControlElement()) On 2015/04/16 19:05:19, AKV wrote: > On 2015/04/14 23:27:19, tkent wrote: > > Do you want to reject a contenteditable element though this function can > return > > a contenteditable element? > > I care about elements which are inside the form. If the ContentEditable is child > of a form, then also isFormControlElement() function returns false ? Could you > please clarify on this ? IsFormControlElement() return false for contenteditable elements. They are not form controls. https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:2559: bool WebViewImpl::wantEnterEvents(Element* element) On 2015/04/16 19:05:19, AKV wrote: > On 2015/04/14 23:27:19, tkent wrote: > > The argument should be |const Element&|. > > > > The function name and the implementation doesn't match. The name should be > > |isListeningToKeyboardEvents| if we follow the implementation. What's the > > intention of this function? > > I wanted to check whether the element has any listener for "Enter Key". If no > enter key listener, we don't want to show Enter key from IME side. I will check > specific case of Enter Key instead of whole key events. Do you have any > suggestion to check Enter key listener is present or not ? Thanks. > No way to check only Enter key. BTW, If we don't show the enter key in IME, it's impossible for users to submit a form without a submit button like: <form action="... "> <input name=foo> </form> https://codereview.chromium.org/939603002/diff/100001/public/web/WebView.h File public/web/WebView.h (right): https://codereview.chromium.org/939603002/diff/100001/public/web/WebView.h#ne... public/web/WebView.h:190: virtual void advanceFocusToNextInputField(WebFocusType focusType) { } The name should be advanceFocusToNextFormControl
@tkent PTAL https://codereview.chromium.org/939603002/diff/100001/public/web/WebView.h File public/web/WebView.h (right): https://codereview.chromium.org/939603002/diff/100001/public/web/WebView.h#ne... public/web/WebView.h:190: virtual void advanceFocusToNextInputField(WebFocusType focusType) { } On 2015/04/17 04:12:35, tkent wrote: > The name should be advanceFocusToNextFormControl Done.
On 2015/04/17 04:12:35, tkent wrote: > https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... > Source/web/WebViewImpl.cpp:2537: if (!element->isFormControlElement()) > On 2015/04/16 19:05:19, AKV wrote: > > On 2015/04/14 23:27:19, tkent wrote: > > > Do you want to reject a contenteditable element though this function can > > return > > > a contenteditable element? > > > > I care about elements which are inside the form. If the ContentEditable is > child > > of a form, then also isFormControlElement() function returns false ? Could you > > please clarify on this ? > > IsFormControlElement() return false for contenteditable elements. They are not > form controls. Thanks for the information. I added additional logic to cover it. > > https://codereview.chromium.org/939603002/diff/60001/Source/web/WebViewImpl.c... > Source/web/WebViewImpl.cpp:2559: bool WebViewImpl::wantEnterEvents(Element* > element) > On 2015/04/16 19:05:19, AKV wrote: > > On 2015/04/14 23:27:19, tkent wrote: > > > The argument should be |const Element&|. > > > > > > The function name and the implementation doesn't match. The name should be > > > |isListeningToKeyboardEvents| if we follow the implementation. What's the > > > intention of this function? > > > > I wanted to check whether the element has any listener for "Enter Key". If no > > enter key listener, we don't want to show Enter key from IME side. I will > check > > specific case of Enter Key instead of whole key events. Do you have any > > suggestion to check Enter key listener is present or not ? Thanks. > > > > No way to check only Enter key. > > BTW, If we don't show the enter key in IME, it's impossible for users to submit > a form without a submit button like: If NEXT is there, it's taking priority on top ENTER/GO. This is due to Keyboard don't have enough space. By default enter key will be activated. If we no enter key event listener, then we keep GO which will send ENTER key event to engine. If we have additional space given for GO/ENTER and PREV and NEXT, then this kind of problem won't be visible. I will discuss with Keyboard team through bcwhite to modify keyboard based on our needs. > > <form action="... "> > <input name=foo> > </form> > > https://codereview.chromium.org/939603002/diff/100001/public/web/WebView.h > File public/web/WebView.h (right): > > https://codereview.chromium.org/939603002/diff/100001/public/web/WebView.h#ne... > public/web/WebView.h:190: virtual void advanceFocusToNextInputField(WebFocusType > focusType) { } > The name should be advanceFocusToNextFormControl
This change needs tests. > > BTW, If we don't show the enter key in IME, it's impossible for users to submit > > a form without a submit button like: > If NEXT is there, it's taking priority on top ENTER/GO. This is due to Keyboard > don't have enough space. > By default enter key will be activated. If we no enter key event listener, then > we keep GO which will send ENTER key event > to engine. If we have additional space given for GO/ENTER and PREV and NEXT, > then this kind of problem won't be visible. > I will discuss with Keyboard team through bcwhite to modify keyboard based on > our needs. > > <form action="... "> > > <input name=foo> > > </form> Hmm, my example was not good. Revised example: <form action="..."> <input type=text name=name> <input type=date name=bod> </form> Suppose that input[name=name] is focused. It has NEXT. However there is no ways to submit the form other than pressing ENTER on the input[name=name]. https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2543: Element* parent = element; formOwner = Traversal<HTMLFormElement>::firstAncestor(*element); https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2547: formOwner = reinterpret_cast<HTMLFormElement*>(toHTMLElement(parent)); Do not use reinterpret_cast<>. https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2551: HTMLFormControlElement* formControlElement = toHTMLFormControlElement(element); formOwner = toHTMLFormControlElement(element)->formOwner(); https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2560: if (nextNode->isContentEditable() && nextNode->isDescendantOf(reinterpret_cast<Node*>(formOwner))) Do not use reinterpret_cast<>. https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2569: // TODO(AKV) Extend it for Select element, Radio button and Check boxes Please use the username part of your @chromium.org address in TODO(). https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2578: if (!element.isFormControlElement()) This returns a wrong result for contenteditable elements with keyboard event listeners. https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2581: return (element.hasEventListeners(EventTypeNames::keydown) || element.hasEventListeners(EventTypeNames::keypress) || element.hasEventListeners(EventTypeNames::keyup)); I found this didn't work for the following case. <body onkeypress="handleKeys(event)"> <form action=...> <input name=query> </form> </body> https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2584: void WebViewImpl::advanceFocusToNextFormControl(WebFocusType focusType) You clarified you'd like to handle contenteditable elements. So, this should be advanceForusToNextFocusableElement(). It's consistent with nextFocusableElement(). However, now I feel these names are too general. I think advanceFocusInForm and nextFocusableElementInForm are better. https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2590: Element* nextElement = nextFocusableElement(element, focusType); You can make |nextElement| RefPtrWillBeRawPtr<Element>, and remove |nextFocusElement|.
On 2015/04/20 01:40:21, tkent wrote: > This change needs tests. >>>>>>>> > Tests at Java layer is sufficient or do I need to add tests in blink also ? I thought of writing tests in Java layer earlier. If it needs to be done on blink side, also I will do it. <<<<<<<<< > > > BTW, If we don't show the enter key in IME, it's impossible for users to > submit > > > a form without a submit button like: > > > If NEXT is there, it's taking priority on top ENTER/GO. This is due to > Keyboard > > don't have enough space. > > By default enter key will be activated. If we no enter key event listener, > then > > we keep GO which will send ENTER key event > > to engine. If we have additional space given for GO/ENTER and PREV and NEXT, > > then this kind of problem won't be visible. > > I will discuss with Keyboard team through bcwhite to modify keyboard based on > > our needs. > > > > <form action="... "> > > > <input name=foo> > > > </form> > > Hmm, my example was not good. > Revised example: > > <form action="..."> > <input type=text name=name> > <input type=date name=bod> > </form> > > Suppose that input[name=name] is focused. It has NEXT. However there is no > ways to submit the form other than pressing ENTER on the input[name=name]. >>>>>>>> Ok Thanks for pointing it. If this is the case at max we have to show all 3 buttons at same time on IME. ENTER/GO, PREV, NEXT. This also I will include in the report to Keyboard team. <<<<<<<<< > > https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:2543: Element* parent = element; > formOwner = Traversal<HTMLFormElement>::firstAncestor(*element); > > https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:2547: formOwner = > reinterpret_cast<HTMLFormElement*>(toHTMLElement(parent)); > Do not use reinterpret_cast<>. > > https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:2551: HTMLFormControlElement* formControlElement = > toHTMLFormControlElement(element); > formOwner = toHTMLFormControlElement(element)->formOwner(); > > https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:2560: if (nextNode->isContentEditable() && > nextNode->isDescendantOf(reinterpret_cast<Node*>(formOwner))) > Do not use reinterpret_cast<>. > > https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:2569: // TODO(AKV) Extend it for Select element, > Radio button and Check boxes > Please use the username part of your @chromium.org address in TODO(). > > https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:2578: if (!element.isFormControlElement()) > This returns a wrong result for contenteditable elements with keyboard event > listeners. > > https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:2581: return > (element.hasEventListeners(EventTypeNames::keydown) || > element.hasEventListeners(EventTypeNames::keypress) || > element.hasEventListeners(EventTypeNames::keyup)); > I found this didn't work for the following case. > > <body onkeypress="handleKeys(event)"> > <form action=...> > <input name=query> > </form> > </body> > > https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:2584: void > WebViewImpl::advanceFocusToNextFormControl(WebFocusType focusType) > You clarified you'd like to handle contenteditable elements. So, this should be > advanceForusToNextFocusableElement(). It's consistent with > nextFocusableElement(). > > However, now I feel these names are too general. I think advanceFocusInForm and > nextFocusableElementInForm are better. > > https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:2590: Element* nextElement = > nextFocusableElement(element, focusType); > You can make |nextElement| RefPtrWillBeRawPtr<Element>, and remove > |nextFocusElement|.
@tkent PTAL https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2543: Element* parent = element; On 2015/04/20 01:40:20, tkent wrote: > formOwner = Traversal<HTMLFormElement>::firstAncestor(*element); Done. Thanks., this is very simple :) https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2547: formOwner = reinterpret_cast<HTMLFormElement*>(toHTMLElement(parent)); On 2015/04/20 01:40:20, tkent wrote: > Do not use reinterpret_cast<>. Done. https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2551: HTMLFormControlElement* formControlElement = toHTMLFormControlElement(element); On 2015/04/20 01:40:21, tkent wrote: > formOwner = toHTMLFormControlElement(element)->formOwner(); Done. https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2560: if (nextNode->isContentEditable() && nextNode->isDescendantOf(reinterpret_cast<Node*>(formOwner))) On 2015/04/20 01:40:21, tkent wrote: > Do not use reinterpret_cast<>. Done. https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2569: // TODO(AKV) Extend it for Select element, Radio button and Check boxes On 2015/04/20 01:40:21, tkent wrote: > Please use the username part of your @chromium.org address in TODO(). Done. https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2578: if (!element.isFormControlElement()) On 2015/04/20 01:40:21, tkent wrote: > This returns a wrong result for contenteditable elements with keyboard event > listeners. > Thanks. I corrected :) https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2581: return (element.hasEventListeners(EventTypeNames::keydown) || element.hasEventListeners(EventTypeNames::keypress) || element.hasEventListeners(EventTypeNames::keyup)); On 2015/04/20 01:40:20, tkent wrote: > I found this didn't work for the following case. > > <body onkeypress="handleKeys(event)"> > <form action=...> > <input name=query> > </form> > </body> Thanks. I just corrected using document listener and parent listeners. Please let me know can we achieve this in a better way than this. https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2584: void WebViewImpl::advanceFocusToNextFormControl(WebFocusType focusType) On 2015/04/20 01:40:21, tkent wrote: > You clarified you'd like to handle contenteditable elements. So, this should be > advanceForusToNextFocusableElement(). It's consistent with > nextFocusableElement(). > > However, now I feel these names are too general. I think advanceFocusInForm and > nextFocusableElementInForm are better. > Done. Thanks https://codereview.chromium.org/939603002/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2590: Element* nextElement = nextFocusableElement(element, focusType); On 2015/04/20 01:40:21, tkent wrote: > You can make |nextElement| RefPtrWillBeRawPtr<Element>, and remove > |nextFocusElement|. Done.
> > This change needs tests. > > >>>>>>>> > > Tests at Java layer is sufficient or do I need to add tests in blink also ? I > thought of writing tests in Java layer earlier. If it needs to be done on blink > side, also I will do it. Please add tests in both of Blink and Java layer. Blink try bots and commit-queue can detect errors If we have tests in Blink. Otherwise, Blink developer would break this behavior frequently.
On 2015/04/21 07:33:50, tkent wrote: > > > This change needs tests. > > > > >>>>>>>> > > > Tests at Java layer is sufficient or do I need to add tests in blink also ? > I > > thought of writing tests in Java layer earlier. If it needs to be done on > blink > > side, also I will do it. > > Please add tests in both of Blink and Java layer. Blink try bots and > commit-queue can detect errors If we have tests in Blink. Otherwise, Blink > developer would break this behavior frequently. @tkent - Thanks for your reviews. Do I need to modify anything extra in the current logic in terms of optimization ? Or shall I proceed with the tests by assuming this is the final code ? I will wait for your reply to proceed.
https://codereview.chromium.org/939603002/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2589: hasKeyEventListener = formOwner->hasEventListeners(EventTypeNames::keydown) || formOwner->hasEventListeners(EventTypeNames::keypress) || formOwner->hasEventListeners(EventTypeNames::keyup); You don't need to check event listeners on the form owner. Non-ancestor form owner doesn't receive key events on a form control. The following parent lookup is enough. https://codereview.chromium.org/939603002/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2594: while ((parent = parent->parentElement())) { Node* node = element; do { if (node->hasEventListener(.... return true; } while ((node = node->parentNode())); // Not parentElement(). return false; is enough. You don't need to check Document and |element| independently.
On 2015/04/21 07:38:44, AKV wrote: > @tkent - Thanks for your reviews. Do I need to modify anything extra in the > current logic in terms of optimization ? Or shall I proceed with the tests by > assuming this is the final code ? I will wait for your reply to proceed. The code logic is still incomplete. If we always show ENTER key, isListeningToKeyboardEvents() is unnecessary, right? Otherwise, Blink needs to provide information that the focused element supports implicit form submission by ENTER key.
https://codereview.chromium.org/939603002/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2589: hasKeyEventListener = formOwner->hasEventListeners(EventTypeNames::keydown) || formOwner->hasEventListeners(EventTypeNames::keypress) || formOwner->hasEventListeners(EventTypeNames::keyup); On 2015/04/21 07:41:27, tkent wrote: > You don't need to check event listeners on the form owner. Non-ancestor form > owner doesn't receive key events on a form control. The following parent lookup > is enough. Thanks. My idea was to return early, rather than traversing in parent chain. But as you said, Non-ancestor form owner doesn't receive key events on form control may cause logic break if I keep this. So I am removing it. https://codereview.chromium.org/939603002/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2594: while ((parent = parent->parentElement())) { On 2015/04/21 07:41:27, tkent wrote: > Node* node = element; > do { > if (node->hasEventListener(.... > return true; > } while ((node = node->parentNode())); // Not parentElement(). > return false; > > is enough. You don't need to check Document and |element| independently. > For returning early purpose, I added Element and Document check early on top. If you recommend only this loop is sufficient, then I will make changes according to that.
On 2015/04/21 07:45:49, tkent wrote: > On 2015/04/21 07:38:44, AKV wrote: > > @tkent - Thanks for your reviews. Do I need to modify anything extra in the > > current logic in terms of optimization ? Or shall I proceed with the tests by > > assuming this is the final code ? I will wait for your reply to proceed. > > The code logic is still incomplete. > If we always show ENTER key, isListeningToKeyboardEvents() is unnecessary, > right? > Otherwise, Blink needs to provide information that the focused element supports > implicit form submission by ENTER key. I think you are right. I will have a thought on this and get back with a suitable logic to handle this. Thanks.
On 2015/04/21 07:45:49, tkent wrote: > On 2015/04/21 07:38:44, AKV wrote: > > @tkent - Thanks for your reviews. Do I need to modify anything extra in the > > current logic in terms of optimization ? Or shall I proceed with the tests by > > assuming this is the final code ? I will wait for your reply to proceed. > > The code logic is still incomplete. > If we always show ENTER key, isListeningToKeyboardEvents() is unnecessary, > right? > Otherwise, Blink needs to provide information that the focused element supports > implicit form submission by ENTER key. I addressed it by keeping GO key when there is no explicit enter key event listener for the node. If key listener is there, then I won't remove the enter key from keyboard, which can be used for form submissions.
@tkent PTAL
Please add tests.
On 2015/04/29 23:38:12, Slow until end of May wrote: > Please add tests. @tkent - I will upload the blink side tests soon.
@tkent & kochi PTAL
Welcome back. First round of comments. https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3061: Element* elementList[20] = { nullptr }; You shouldn't use an array in a loose manner like this, even in a unit test. This doesn't have to be an array at all in the following tests (e.g. no loops through the elements), no element 11-19 are used which is bad, although the numbering makes some sense when you focus backwards (like lines 3095-). Probably it's better delete this line, and declare each element like Element* el0 = document->getElementById("input1"); ... Element* el1 = document->getElementById("contenteditable1") ... Or maybe, you can rename the variable identically with the element id Element* input1 = document->getElementById("input1"); ... Element* contenteditable1 = document->getElementById("contenteditable1") ... to make it easier to match between C++ test and the HTML. https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3062: elementList[0] = document->getElementById("input1"); It would be more readable if you split lines 3062-3093 for each element. e.g. <newline> elementList[0] = ... .. .. <newline> elementList[1] = ... https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/data/a... File Source/web/tests/data/advance-focus-in-form.html (right): https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/data/a... Source/web/tests/data/advance-focus-in-form.html:1: <html> Usually file names in this directory uses "advance_focus_in_form.html" style. Could you rename this file? https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/data/a... Source/web/tests/data/advance-focus-in-form.html:4: <input type="text" name="input1" id="input1" form="form1" value="input1 from form1"/><br/> This should be more readable if you indent these elements. https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/data/a... Source/web/tests/data/advance-focus-in-form.html:4: <input type="text" name="input1" id="input1" form="form1" value="input1 from form1"/><br/> You can write <br> instead of <br/>.
@tkent Thanks for your review comments. PTAL new patch. https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3061: Element* elementList[20] = { nullptr }; On 2015/07/02 11:04:36, Takayoshi Kochi wrote: > You shouldn't use an array in a loose manner like this, even in a unit test. > > This doesn't have to be an array at all in the following tests > (e.g. no loops through the elements), no element 11-19 are used which is bad, > although the numbering makes some sense when you focus backwards > (like lines 3095-). > > Probably it's better delete this line, and declare each element like > > Element* el0 = document->getElementById("input1"); > ... > Element* el1 = document->getElementById("contenteditable1") > ... > > Or maybe, you can rename the variable identically with the element id > > Element* input1 = document->getElementById("input1"); > ... > Element* contenteditable1 = document->getElementById("contenteditable1") > ... > > to make it easier to match between C++ test and the HTML. Done. Thanks https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3062: elementList[0] = document->getElementById("input1"); On 2015/07/02 11:04:36, Takayoshi Kochi wrote: > It would be more readable if you split lines 3062-3093 for each element. > > e.g. > <newline> > elementList[0] = ... > .. > .. > <newline> > elementList[1] = ... Done. https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/data/a... File Source/web/tests/data/advance-focus-in-form.html (right): https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/data/a... Source/web/tests/data/advance-focus-in-form.html:1: <html> On 2015/07/02 11:04:36, Takayoshi Kochi wrote: > Usually file names in this directory uses > "advance_focus_in_form.html" > style. > > Could you rename this file? Done. https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/data/a... Source/web/tests/data/advance-focus-in-form.html:4: <input type="text" name="input1" id="input1" form="form1" value="input1 from form1"/><br/> On 2015/07/02 11:04:36, Takayoshi Kochi wrote: > You can write <br> instead of <br/>. Done. https://codereview.chromium.org/939603002/diff/180001/Source/web/tests/data/a... Source/web/tests/data/advance-focus-in-form.html:4: <input type="text" name="input1" id="input1" form="form1" value="input1 from form1"/><br/> On 2015/07/02 11:04:36, Takayoshi Kochi wrote: > This should be more readable if you indent these elements. Done.
Here are second round of comments. https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:125: #include "public/platform/WebFocusType.h" It's strange you already used "WebFocusType" in WebViewImpl.h declaration but include WebFocusType.h here. WebFocusType.h is already included in public/web/WebView.h, which WebViewImpl.h includes. You can remove this line. https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2537: Node* nextNode = element; Now FocusController::findFocusableElement() returns Element*, so you can use Element* instead of Node* here. And change the variable name accordingly. https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2540: return toElement(nextNode); If the pointer type is changed to Element*, you don't need toElement() here. https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2541: if (!toElement(nextNode)->isFormControlElement()) Ditto. https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2549: return toElement(nextNode); Ditto. https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3064: document->setFocusedElement(input1); You can also do this by "input1->focus();". Element::focus() does more than Document::setFocusedElement() and is almost equivalent to JavaScript input1.focus(). This means you exercise more code than calling setFocusedElement() directly, and the coverage is more close to what happens in real use scenario. https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3124: webViewImpl->advanceFocusInForm(WebFocusTypeForward); Could you add a comment here why you expect the focus didn't move? https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... File Source/web/tests/data/advance_focus_in_form.html (right): https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:4: <input type="text" name="input1" id="input1" form="form1" value="input1 from form1"/><br> As with <br/> -> <br>, you can close "input" with ">". Please remove attributes which are unnecessary to the test. (same for other elements in this file.) Even "name=" is unnecessary, as this is not for real form submission. If you think "form=..." is necessary, your test case should be explicitly written to cover all the cases that "form=" exists, not exists, "form=" exists and inside the form, "form=" exists but outside the form, "form=" exists but its form name is invalid, etc. etc. https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:6: <div id="div1" name="div1" onkeypress="alert('div1');"><br> What is this onkeypress handler for? https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:10: <input type="text" name="input3" id="input3" value="input3 from form1"/><br> If you include <input> twice in the same form, what is the reason? If you want various focusable elements, you may include other focusable element, or do you want to have "4" focusable elements? https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:13: <textarea id="textarea3" name="textarea3">textarea3 from form2, which is outside parent hierarchy</textarea><br> How this can be from form2? https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:21: </html> Other things of concerns which occurred to me: 1. Is the focusing order of elements with tabindex correct? Especially, does the code correctly skip tabindex="-1" is specified? 2. Do you want to test any disabled form controls? https://codereview.chromium.org/939603002/diff/200001/public/web/WebTextInput... File public/web/WebTextInputType.h (right): https://codereview.chromium.org/939603002/diff/200001/public/web/WebTextInput... public/web/WebTextInputType.h:91: WebTextInputFlagHavePreviousFocusableElement = 1 << 12, nit: to match the original code's style, you can omit the "," at the end of line (optional).
ajith.v@chromium.org changed reviewers: - jdduke@chromium.org
@kochi & tkent PTAL. Please address my queries as well. Thank you. https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:125: #include "public/platform/WebFocusType.h" On 2015/07/03 04:10:34, Takayoshi Kochi wrote: > It's strange you already used "WebFocusType" in WebViewImpl.h declaration > but include WebFocusType.h here. > > WebFocusType.h is already included in public/web/WebView.h, which WebViewImpl.h > includes. > > You can remove this line. Done. https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2537: Node* nextNode = element; On 2015/07/03 04:10:34, Takayoshi Kochi wrote: > Now FocusController::findFocusableElement() returns Element*, so you can > use Element* instead of Node* here. And change the variable name accordingly. Done. https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2540: return toElement(nextNode); On 2015/07/03 04:10:34, Takayoshi Kochi wrote: > If the pointer type is changed to Element*, you don't need toElement() here. Done. https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2541: if (!toElement(nextNode)->isFormControlElement()) On 2015/07/03 04:10:34, Takayoshi Kochi wrote: > Ditto. Done. https://codereview.chromium.org/939603002/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2549: return toElement(nextNode); On 2015/07/03 04:10:34, Takayoshi Kochi wrote: > Ditto. Done. https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3064: document->setFocusedElement(input1); On 2015/07/03 04:10:34, Takayoshi Kochi wrote: > You can also do this by "input1->focus();". > > Element::focus() does more than Document::setFocusedElement() and is > almost equivalent to JavaScript input1.focus(). > > This means you exercise more code than calling setFocusedElement() > directly, and the coverage is more close to what happens in real use scenario. Thanks Done. https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3124: webViewImpl->advanceFocusInForm(WebFocusTypeForward); On 2015/07/03 04:10:34, Takayoshi Kochi wrote: > Could you add a comment here why you expect the focus didn't move? Done. https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... File Source/web/tests/data/advance_focus_in_form.html (right): https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:4: <input type="text" name="input1" id="input1" form="form1" value="input1 from form1"/><br> On 2015/07/03 04:10:35, Takayoshi Kochi wrote: > As with <br/> -> <br>, you can close "input" with ">". > > Please remove attributes which are unnecessary to the test. > (same for other elements in this file.) > > Even "name=" is unnecessary, as this is not for real form submission. > > If you think "form=..." is necessary, your test case should be explicitly > written to cover all the cases that "form=" exists, not exists, "form=" > exists and inside the form, "form=" exists but outside the form, > "form=" exists but its form name is invalid, etc. etc. > input1 is with "form" exists inside the form. contenteditable1 is with "form" not exists inside the form. textarea2 is with "form" exists outside the form. textarea3 is with form not exists outside the form. textarea4 is with invalid "form" attribute. Do we need to add additional constraints like inside a form, element has a "form" attribute which is outside of the form ? https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:6: <div id="div1" name="div1" onkeypress="alert('div1');"><br> On 2015/07/03 04:10:34, Takayoshi Kochi wrote: > What is this onkeypress handler for? For checking the KeyEventListener for enabling the enter key. if Key event listener is not there, we won't show enter key in Keyboard. Go key will be used for implicit form submission. https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:10: <input type="text" name="input3" id="input3" value="input3 from form1"/><br> On 2015/07/03 04:10:35, Takayoshi Kochi wrote: > If you include <input> twice in the same form, what is the reason? > If you want various focusable elements, you may include other focusable > element, or do you want to have "4" focusable elements? One input with form attribute and other without form attribute. Ok. I will add other non-input elements too in the test html. https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:13: <textarea id="textarea3" name="textarea3">textarea3 from form2, which is outside parent hierarchy</textarea><br> On 2015/07/03 04:10:35, Takayoshi Kochi wrote: > How this can be from form2? It was a typo while rearranging elements. I corrected it. Thanks https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:21: </html> On 2015/07/03 04:10:35, Takayoshi Kochi wrote: > Other things of concerns which occurred to me: > > 1. Is the focusing order of elements with tabindex correct? > Especially, does the code correctly skip tabindex="-1" is specified? > 2. Do you want to test any disabled form controls? 1) As per our initial plan, we have to navigate to focus based on visual order of elements. So I was following Document order. Later on, as per your suggestion, we started using findFocusableElement(). I think this side is still open to discussion. According to me tab index flow is fine to follow same as TAB/Shift + TAB flow. Additionally do you wanted to add Tab index also in test case ? 2) I am not sure about disabled form control will get focus on tab key. So I ignored it. As per the requirement disabled element no need to process. If you recommend to add test cases to cover it, I will do that. https://codereview.chromium.org/939603002/diff/200001/public/web/WebTextInput... File public/web/WebTextInputType.h (right): https://codereview.chromium.org/939603002/diff/200001/public/web/WebTextInput... public/web/WebTextInputType.h:91: WebTextInputFlagHavePreviousFocusableElement = 1 << 12, On 2015/07/03 04:10:35, Takayoshi Kochi wrote: > nit: to match the original code's style, you can omit the > "," at the end of line (optional). Done.
https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... File Source/web/tests/data/advance_focus_in_form.html (right): https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:4: <input type="text" name="input1" id="input1" form="form1" value="input1 from form1"/><br> On 2015/07/26 15:56:34, AKV wrote: > On 2015/07/03 04:10:35, Takayoshi Kochi wrote: > > As with <br/> -> <br>, you can close "input" with ">". > > > > Please remove attributes which are unnecessary to the test. > > (same for other elements in this file.) > > > > Even "name=" is unnecessary, as this is not for real form submission. > > > > If you think "form=..." is necessary, your test case should be explicitly > > written to cover all the cases that "form=" exists, not exists, "form=" > > exists and inside the form, "form=" exists but outside the form, > > "form=" exists but its form name is invalid, etc. etc. > > > > input1 is with "form" exists inside the form. > contenteditable1 is with "form" not exists inside the form. > textarea2 is with "form" exists outside the form. > textarea3 is with form not exists outside the form. > textarea4 is with invalid "form" attribute. > > Do we need to add additional constraints like inside a form, element has a > "form" attribute which is outside of the form ? Could you add elements which are usually focusable (via TAB key navigation) but ignored for prev/next feature (e.g. <button>)? https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:6: <div id="div1" name="div1" onkeypress="alert('div1');"><br> On 2015/07/26 15:56:34, AKV wrote: > On 2015/07/03 04:10:34, Takayoshi Kochi wrote: > > What is this onkeypress handler for? > > For checking the KeyEventListener for enabling the enter key. if Key event > listener is not there, we won't show enter key in Keyboard. Go key will be used > for implicit form submission. I got it, thanks for the explanation. Other readers of this file may be confused as I was. How about adding comment line <!-- ... --> above or comment inline in alert('...') to note "if key event is handled, software keyboard should show enter key instead of Go key"? (same goes for line 15, <form onkeypress=...>) https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:21: </html> On 2015/07/26 15:56:34, AKV wrote: > On 2015/07/03 04:10:35, Takayoshi Kochi wrote: > > Other things of concerns which occurred to me: > > > > 1. Is the focusing order of elements with tabindex correct? > > Especially, does the code correctly skip tabindex="-1" is specified? > > 2. Do you want to test any disabled form controls? > > 1) As per our initial plan, we have to navigate to focus based on visual order > of elements. So I was following Document order. Later on, as per your > suggestion, we started using findFocusableElement(). I think this side is still > open to discussion. According to me tab index flow is fine to follow same as > TAB/Shift + TAB flow. Additionally do you wanted to add Tab index also in test > case ? > 2) I am not sure about disabled form control will get focus on tab key. So I > ignored it. As per the requirement disabled element no need to process. If you > recommend to add test cases to cover it, I will do that. 1) It would be nice to have another set of form elements, with tabindex (e.g. <input tabindex=2><input tabindex=0><input tabindex=1> and check if next/prev is working as expected. Even though the "visual order" plan is still open to discussion, already written code expects prev/next button will follow the TAB navigation order, it makes sense to have a test to check the written code works as expected if not as comprehensive as TAB navigation tests. You made some modification to the order (e.g. form association check, key event handler check), some change outside this CL may break what this CL assumes. Also, I think having "tabindex=-1" element in each form to see if that element is properly skipped in prev/next button is also useful. 2) Yes, please add a test case. Disabled form control should be skipped via TAB navigation, so does prev/next button. TAB navigation should be tested somewhere else, but prev/next button should be tested here. https://codereview.chromium.org/939603002/diff/240001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/240001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3103: // No Next/Previous element to this element because it's not part of any form. Hence focus won't change wrt NEXT/PREVIOS. nit: s/PREVIOS/PREVIOUS/ https://codereview.chromium.org/939603002/diff/240001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3114: // No Next/Previous element to this element because it's not part of any form. Hence focus won't change wrt NEXT/PREVIOS. nit: s/PREVIOS/PREVIOUS/ https://codereview.chromium.org/939603002/diff/240001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3126: // No Next/Previous element to this element because it's not part of any form. Hence focus won't change wrt NEXT/PREVIOS. nit: s/PREVIOS/PREVIOUS/
Patchset #14 (id:260001) has been deleted
@kochi and tkent PTAL! https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... File Source/web/tests/data/advance_focus_in_form.html (right): https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:4: <input type="text" name="input1" id="input1" form="form1" value="input1 from form1"/><br> On 2015/07/28 08:43:02, Takayoshi Kochi wrote: > On 2015/07/26 15:56:34, AKV wrote: > > On 2015/07/03 04:10:35, Takayoshi Kochi wrote: > > > As with <br/> -> <br>, you can close "input" with ">". > > > > > > Please remove attributes which are unnecessary to the test. > > > (same for other elements in this file.) > > > > > > Even "name=" is unnecessary, as this is not for real form submission. > > > > > > If you think "form=..." is necessary, your test case should be explicitly > > > written to cover all the cases that "form=" exists, not exists, "form=" > > > exists and inside the form, "form=" exists but outside the form, > > > "form=" exists but its form name is invalid, etc. etc. > > > > > > > input1 is with "form" exists inside the form. > > contenteditable1 is with "form" not exists inside the form. > > textarea2 is with "form" exists outside the form. > > textarea3 is with form not exists outside the form. > > textarea4 is with invalid "form" attribute. > > > > Do we need to add additional constraints like inside a form, element has a > > "form" attribute which is outside of the form ? > > Could you add elements which are usually focusable (via TAB key navigation) > but ignored for prev/next feature (e.g. <button>)? > Added button element inside and outside form to cover non-editable focused element case coverage in case of NEXT/PREVIOUS action. https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:6: <div id="div1" name="div1" onkeypress="alert('div1');"><br> On 2015/07/28 08:43:02, Takayoshi Kochi wrote: > On 2015/07/26 15:56:34, AKV wrote: > > On 2015/07/03 04:10:34, Takayoshi Kochi wrote: > > > What is this onkeypress handler for? > > > > For checking the KeyEventListener for enabling the enter key. if Key event > > listener is not there, we won't show enter key in Keyboard. Go key will be > used > > for implicit form submission. > > I got it, thanks for the explanation. > > Other readers of this file may be confused as I was. > > How about adding comment line <!-- ... --> above or > comment inline in alert('...') to note > "if key event is handled, software keyboard should show enter key instead of Go > key"? > (same goes for line 15, <form onkeypress=...>) Done. Thanks https://codereview.chromium.org/939603002/diff/200001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:21: </html> On 2015/07/28 08:43:02, Takayoshi Kochi wrote: > On 2015/07/26 15:56:34, AKV wrote: > > On 2015/07/03 04:10:35, Takayoshi Kochi wrote: > > > Other things of concerns which occurred to me: > > > > > > 1. Is the focusing order of elements with tabindex correct? > > > Especially, does the code correctly skip tabindex="-1" is specified? > > > 2. Do you want to test any disabled form controls? > > > > 1) As per our initial plan, we have to navigate to focus based on visual order > > of elements. So I was following Document order. Later on, as per your > > suggestion, we started using findFocusableElement(). I think this side is > still > > open to discussion. According to me tab index flow is fine to follow same as > > TAB/Shift + TAB flow. Additionally do you wanted to add Tab index also in test > > case ? > > 2) I am not sure about disabled form control will get focus on tab key. So I > > ignored it. As per the requirement disabled element no need to process. If you > > recommend to add test cases to cover it, I will do that. > > 1) It would be nice to have another set of form elements, > with tabindex (e.g. <input tabindex=2><input tabindex=0><input tabindex=1> > and check if next/prev is working as expected. > Even though the "visual order" plan is still open to discussion, > already written code expects prev/next button will follow the TAB navigation > order, > it makes sense to have a test to check the written code works as expected > if not as comprehensive as TAB navigation tests. > You made some modification to the order (e.g. form association check, > key event handler check), some change outside this CL may break > what this CL assumes. > > Also, I think having "tabindex=-1" element in each form > to see if that element is properly skipped in prev/next > button is also useful. > > 2) Yes, please add a test case. > > Disabled form control should be skipped via TAB navigation, so does prev/next > button. TAB navigation should be > tested somewhere else, but prev/next button > should be tested here. Totally agree with you. We need to cover as much coverage without assuming other parts. Added test cases with valid and invalid tab index, ie. -1 case. Added disabled and readonly element coverage. Thank you. https://codereview.chromium.org/939603002/diff/240001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/240001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3103: // No Next/Previous element to this element because it's not part of any form. Hence focus won't change wrt NEXT/PREVIOS. On 2015/07/28 08:43:02, Takayoshi Kochi wrote: > nit: s/PREVIOS/PREVIOUS/ Done. https://codereview.chromium.org/939603002/diff/240001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3114: // No Next/Previous element to this element because it's not part of any form. Hence focus won't change wrt NEXT/PREVIOS. On 2015/07/28 08:43:02, Takayoshi Kochi wrote: > nit: s/PREVIOS/PREVIOUS/ Done. https://codereview.chromium.org/939603002/diff/240001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3126: // No Next/Previous element to this element because it's not part of any form. Hence focus won't change wrt NEXT/PREVIOS. On 2015/07/28 08:43:03, Takayoshi Kochi wrote: > nit: s/PREVIOS/PREVIOUS/ Done.
https://codereview.chromium.org/939603002/diff/320001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/320001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3033: { This test has become very long... Could you consider splitting this into several pieces? As there are 4 forms in the HTML file, for example, you can split into 4 tests for each form. If you think having form1 and form2 in one HTML makes sense, you can keep them in one HTML, of course. Preferably, each test has its theme for what is being tested within it, and ideally each test is orthogonal. I understand that these tests are for combinations of several factors (editable, non-editable, associated with form, has key event handlers, ...) and they can't be so clearly separated, but as you already split the form into 4 forms, I think you already got the idea. https://codereview.chromium.org/939603002/diff/320001/Source/web/tests/data/a... File Source/web/tests/data/advance_focus_in_form.html (right): https://codereview.chromium.org/939603002/diff/320001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:8: <!-- If any of this form element can handle key event, software keyboard should show ENTER key instead of GO key, for textarea element it's exceptional, as it requires ENTER key by default for enabling multiline inputs. (This will be determined from Chromium layer)" --> I'd prefer entering break the line at some point (say, 90 column) even though Blink project does not force the 80 or 100 columns. The comment line is much longer than 100 chars so it is okay to have line breaks. https://codereview.chromium.org/939603002/diff/320001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:23: <!-- If any of this form element can handle key event, software keyboard should show ENTER key instead of GO key, for textarea element it's exceptional, as it requires ENTER key by default for enabling multiline inputs. (This will be determined from Chromium layer)" --> Ditto - it may look smarter if these comments are merged into one place as the contents are identical.
@kochi PTAL https://codereview.chromium.org/939603002/diff/320001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/320001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3033: { On 2015/08/05 10:15:13, Takayoshi Kochi wrote: > This test has become very long... > Could you consider splitting this into several pieces? > > As there are 4 forms in the HTML file, for example, you can split into 4 tests > for each form. > If you think having form1 and form2 in one HTML makes sense, you can keep them > in one > HTML, of course. > > Preferably, each test has its theme for what is being tested within it, > and ideally each test is orthogonal. I understand that these tests are for > combinations of several factors (editable, non-editable, associated with form, > has key event handlers, ...) and they can't be so clearly separated, but as you > already split the form into 4 forms, I think you already got the idea. Done. https://codereview.chromium.org/939603002/diff/320001/Source/web/tests/data/a... File Source/web/tests/data/advance_focus_in_form.html (right): https://codereview.chromium.org/939603002/diff/320001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:8: <!-- If any of this form element can handle key event, software keyboard should show ENTER key instead of GO key, for textarea element it's exceptional, as it requires ENTER key by default for enabling multiline inputs. (This will be determined from Chromium layer)" --> On 2015/08/05 10:15:13, Takayoshi Kochi wrote: > I'd prefer entering break the line at some point (say, 90 column) > even though Blink project does not force the 80 or 100 columns. > The comment line is much longer than 100 chars so it is okay to > have line breaks. Done. https://codereview.chromium.org/939603002/diff/320001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form.html:23: <!-- If any of this form element can handle key event, software keyboard should show ENTER key instead of GO key, for textarea element it's exceptional, as it requires ENTER key by default for enabling multiline inputs. (This will be determined from Chromium layer)" --> On 2015/08/05 10:15:13, Takayoshi Kochi wrote: > Ditto - it may look smarter if these comments are merged into one place as the > contents are identical. Done. Moved to top of the first form. Thanks
I appreciate the quick turnaround. We are almost there. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3106: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); Could you specify the exact expected result of |textInputInfo.flags| as the first parameter to change this from EXPECT_NE() to EXPECT_EQ()? I think this test is deterministically return the same result here. I'd prefer stricter test for expected results. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3116: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); Ditto. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3128: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); Ditto. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3140: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); Ditto. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3152: EXPECT_NE(WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); Ditto. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3164: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); Ditto. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3222: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); Ditto. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... File Source/web/tests/data/advance_focus_in_form_with_disabled_and_readonly_elements.html (right): https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form_with_disabled_and_readonly_elements.html:3: <h3>form4 starts here</h3><br> nit: as the big HTML file is split and this file only contains one form, you don't need this marker. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form_with_disabled_and_readonly_elements.html:4: <form id="form4"> nit: as this file contains only one form, any id is unique for this form - renumbering these ids is not essential, but it makes these tests look better. In the future, someone may wonder why the number starts from 0 or 1, and he/she has to research the review history of this file to learn why the number starts from 4. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form_with_disabled_and_readonly_elements.html:11: <h3>form4 ends here</h3><br> nit: ditto as line 3. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... File Source/web/tests/data/advance_focus_in_form_with_tabindex_elements.html (right): https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form_with_tabindex_elements.html:3: <h3>form3 starts here</h3><br> nit: As the big HTML file are split, this marker is not necessary. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form_with_tabindex_elements.html:12: <h3>form3 ends here</h3><br> nit: ditto.
@kochi PTAL https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3106: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > Could you specify the exact expected result of |textInputInfo.flags| as the > first > parameter to change this from EXPECT_NE() to EXPECT_EQ()? > I think this test is deterministically return the same result here. > > I'd prefer stricter test for expected results. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3116: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); On 2015/08/06 02:20:59, Takayoshi Kochi wrote: > Ditto. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3128: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > Ditto. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3140: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > Ditto. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3152: EXPECT_NE(WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > Ditto. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3164: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > Ditto. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3222: EXPECT_NE(defaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement, textInputInfo.flags); On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > Ditto. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... File Source/web/tests/data/advance_focus_in_form_with_disabled_and_readonly_elements.html (right): https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form_with_disabled_and_readonly_elements.html:3: <h3>form4 starts here</h3><br> On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > nit: as the big HTML file is split and this file only contains one form, you > don't need this marker. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form_with_disabled_and_readonly_elements.html:4: <form id="form4"> On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > nit: as this file contains only one form, any id is unique for this form - > renumbering these ids is not essential, but it makes these tests look better. > In the future, someone may wonder why the number starts from 0 or 1, and he/she > has to research the review history of this file to learn why the number starts > from 4. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form_with_disabled_and_readonly_elements.html:11: <h3>form4 ends here</h3><br> On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > nit: ditto as line 3. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... File Source/web/tests/data/advance_focus_in_form_with_tabindex_elements.html (right): https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form_with_tabindex_elements.html:3: <h3>form3 starts here</h3><br> On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > nit: As the big HTML file are split, this marker is not necessary. Done. https://codereview.chromium.org/939603002/diff/360001/Source/web/tests/data/a... Source/web/tests/data/advance_focus_in_form_with_tabindex_elements.html:12: <h3>form3 ends here</h3><br> On 2015/08/06 02:21:00, Takayoshi Kochi wrote: > nit: ditto. Done.
lgtm Please also get this reviewed by tkent (OWNERS).
On 2015/08/06 10:10:55, Takayoshi Kochi wrote: > lgtm > > Please also get this reviewed by tkent (OWNERS). @kochi Thanks for your quick and constructive reviews so far. @tkent PTAL this patch.
https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3036: WebView* webViewImpl = m_webViewHelper.initializeAndLoad(m_baseURL + testFile, true, 0); The last argument should be |nullptr|, or you can omit it. https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3052: Element* contenteditable1 = document->getElementById("contenteditable1"); Repeating similar code is not fun. How about making an array of struct with element ID and expected textInputInfo.flags, and looping over the array? Something like struct FocusedElement { const char* elementID; int textInputFlags; } focusedElements[] = { {"contenteditable1", WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement}, {"input2", efaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | WebTextInputFlagHavePreviousFocusableElement | WebTextInputFlagListeningToKeyboardEvents},}, .... We may omit button1/anchor1 checks because EXPECT_EQ(..., document->focusedElement()) checks follow them. https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3102: // Setting a non editable element as focus in form1, and ensuring editable navigation is fine in forward and backward. nit: I recommend to wrap code comments in 80 columns though it's not a style rule. https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3178: WebView* webViewImpl = m_webViewHelper.initializeAndLoad(m_baseURL + testFile, true, 0); 0 should be nullptr or omitted. https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3237: WebView* webViewImpl = m_webViewHelper.initializeAndLoad(m_baseURL + testFile, true, 0); 0 should be nullptr or omitted.
@tkent - Could you pls address my queries ? https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3052: Element* contenteditable1 = document->getElementById("contenteditable1"); On 2015/08/07 01:40:58, tkent wrote: > Repeating similar code is not fun. How about making an array of struct with > element ID and expected textInputInfo.flags, and looping over the array? > > Something like > > struct FocusedElement { > const char* elementID; > int textInputFlags; > } focusedElements[] = { > {"contenteditable1", WebTextInputFlagHaveNextFocusableElement | > WebTextInputFlagHavePreviousFocusableElement}, > {"input2", efaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement | > WebTextInputFlagHavePreviousFocusableElement | > WebTextInputFlagListeningToKeyboardEvents},}, > .... > > We may omit button1/anchor1 checks because EXPECT_EQ(..., > document->focusedElement()) checks follow them. @tkent - Could you provide little more details about this approach. With arrays also, I have to query the DOM element, every time to check for focused element. Also the next element is not actually same as the element that's present in DOM, due to non-editable element or non-form element. So with this array approach, how it's improving the readability or reducing code ? Could you pls expand your snipper bit more, so that I can take reference from it to proceed further.
https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3052: Element* contenteditable1 = document->getElementById("contenteditable1"); On 2015/08/07 06:56:53, AKV wrote: > On 2015/08/07 01:40:58, tkent wrote: > > Repeating similar code is not fun. How about making an array of struct with > > element ID and expected textInputInfo.flags, and looping over the array? > > > > Something like > > > > struct FocusedElement { > > const char* elementID; > > int textInputFlags; > > } focusedElements[] = { > > {"contenteditable1", WebTextInputFlagHaveNextFocusableElement | > > WebTextInputFlagHavePreviousFocusableElement}, > > {"input2", efaultTextInputFlags | WebTextInputFlagHaveNextFocusableElement > | > > WebTextInputFlagHavePreviousFocusableElement | > > WebTextInputFlagListeningToKeyboardEvents},}, > > .... > > > > We may omit button1/anchor1 checks because EXPECT_EQ(..., > > document->focusedElement()) checks follow them. > > @tkent - Could you provide little more details about this approach. > With arrays also, I have to query the DOM element, every time to check for > focused element. Also the next element is not actually same as the element > that's present in DOM, due to non-editable element or non-form element. So with > this array approach, how it's improving the readability or reducing code ? > Could you pls expand your snipper bit more, so that I can take reference from it > to proceed further. Something like: for (int i = 0; i < WTF_ARRAY_LENGTH(focusedElement); ++i) { Element* element = document->getElementById(focusedElement[i].elementId); EXPECT_EQ(element, document->focusedElement()); textInputInfo = webViewImpl->textInputInfo(); EXPECTEQ(focusedELement[i].textInputFlags, textInputInfo.flags); webViewImpl->advanceFocusInForm(WebFocusTypeForward); }
@tkent PTAL https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3036: WebView* webViewImpl = m_webViewHelper.initializeAndLoad(m_baseURL + testFile, true, 0); On 2015/08/07 01:40:58, tkent wrote: > The last argument should be |nullptr|, or you can omit it. Done. https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3052: Element* contenteditable1 = document->getElementById("contenteditable1"); On 2015/08/07 07:05:48, tkent wrote: > On 2015/08/07 06:56:53, AKV wrote: > > On 2015/08/07 01:40:58, tkent wrote: > > > Repeating similar code is not fun. How about making an array of struct with > > > element ID and expected textInputInfo.flags, and looping over the array? > > > > > > Something like > > > > > > struct FocusedElement { > > > const char* elementID; > > > int textInputFlags; > > > } focusedElements[] = { > > > {"contenteditable1", WebTextInputFlagHaveNextFocusableElement | > > > WebTextInputFlagHavePreviousFocusableElement}, > > > {"input2", efaultTextInputFlags | > WebTextInputFlagHaveNextFocusableElement > > | > > > WebTextInputFlagHavePreviousFocusableElement | > > > WebTextInputFlagListeningToKeyboardEvents},}, > > > .... > > > > > > We may omit button1/anchor1 checks because EXPECT_EQ(..., > > > document->focusedElement()) checks follow them. > > > > @tkent - Could you provide little more details about this approach. > > With arrays also, I have to query the DOM element, every time to check for > > focused element. Also the next element is not actually same as the element > > that's present in DOM, due to non-editable element or non-form element. So > with > > this array approach, how it's improving the readability or reducing code ? > > Could you pls expand your snipper bit more, so that I can take reference from > it > > to proceed further. > > Something like: > > for (int i = 0; i < WTF_ARRAY_LENGTH(focusedElement); ++i) { > Element* element = document->getElementById(focusedElement[i].elementId); > EXPECT_EQ(element, document->focusedElement()); > textInputInfo = webViewImpl->textInputInfo(); > EXPECTEQ(focusedELement[i].textInputFlags, textInputInfo.flags); > webViewImpl->advanceFocusInForm(WebFocusTypeForward); > } @tkent - Thanks for the quick response. I have taken care it maximum possible level for all trivial cases. https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3102: // Setting a non editable element as focus in form1, and ensuring editable navigation is fine in forward and backward. On 2015/08/07 01:40:58, tkent wrote: > nit: I recommend to wrap code comments in 80 columns though it's not a style > rule. > Done. https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3178: WebView* webViewImpl = m_webViewHelper.initializeAndLoad(m_baseURL + testFile, true, 0); On 2015/08/07 01:40:58, tkent wrote: > 0 should be nullptr or omitted. Done. https://codereview.chromium.org/939603002/diff/380001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:3237: WebView* webViewImpl = m_webViewHelper.initializeAndLoad(m_baseURL + testFile, true, 0); On 2015/08/07 01:40:58, tkent wrote: > 0 should be nullptr or omitted. Done.
The CQ bit was checked by tkent@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org Link to the patchset: https://codereview.chromium.org/939603002/#ps400001 (title: "Removed duplicate code in tests using arrays.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/939603002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/939603002/400001
The CQ bit was unchecked by commit-bot@chromium.org
The author ajith.v@chromium.org has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by ajith.v@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/939603002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/939603002/400001
The CQ bit was unchecked by commit-bot@chromium.org
The author ajith.v@chromium.org has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
On 2015/08/10 07:50:04, commit-bot: I haz the power wrote: > The author mailto:ajith.v@chromium.org has not signed Google Contributor License > Agreement. Please visit https://cla.developers.google.com to sign and manage > CLA. @tkent & kochi - I have started the CLA for ajith.v@chromium.org id. Once it's approved I will try to land it.
l.gombos@samsung.com changed reviewers: + l.gombos@samsung.com
> I have started the CLA for ajith.v@chromium.org id. > Once it's approved I will try to land it. I think a better way to resolve this is to continue to contribute under your @samsung.com account (which is on the CLA) even though you have a @chromium.org account as well. This is also hinted by Thiago Farina - https://groups.google.com/a/chromium.org/d/msg/chromium-dev/HK0-a7v5_P8/GWMbQ...
The CQ bit was checked by ajith.v@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/939603002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/939603002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was checked by ajith.v@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, kochi@chromium.org Link to the patchset: https://codereview.chromium.org/939603002/#ps420001 (title: "Fixed Windows platform build failure.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/939603002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/939603002/420001
https://codereview.chromium.org/939603002/diff/420001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/420001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2554: return nullptr; How about using for()? for (Element* nextElement = element; nextElement; nextElement = page()->focusController().findFocusableElement(focusType, *nextElement)) ? https://codereview.chromium.org/939603002/diff/420001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2579: do { Ditto. for (Node* node = element; node; node = node->parentNode()) ?
The CQ bit was unchecked by ajith.v@chromium.org
@kochi - PTAL Thanks. https://codereview.chromium.org/939603002/diff/420001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/420001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2554: return nullptr; On 2015/08/12 10:47:33, Takayoshi Kochi wrote: > How about using for()? > > for (Element* nextElement = element; > nextElement; > nextElement = page()->focusController().findFocusableElement(focusType, > *nextElement)) > > ? Done. https://codereview.chromium.org/939603002/diff/420001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2579: do { On 2015/08/12 10:47:33, Takayoshi Kochi wrote: > Ditto. > > for (Node* node = element; node; node = node->parentNode()) > > ? Done.
lgtm
The CQ bit was checked by ajith.v@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org Link to the patchset: https://codereview.chromium.org/939603002/#ps440001 (title: "Changed while loop to for loop.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/939603002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/939603002/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/65919)
https://codereview.chromium.org/939603002/diff/440001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/939603002/diff/440001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2550: for (Element* nextElement = element; nextElement; nextElement = page()->focusController().findFocusableElement(focusType, *nextElement)) { Oops - my previous comment was wrong. For the first iteration in this code, nextElement is element, but it should be page()->focusController().findFocusableElement(focusType, *element). In that case, the previous code look fine, though I slightly prefer "break" than "return nullptr" on line 2554. Sorry for the mess.
The CQ bit was checked by ajith.v@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, kochi@chromium.org Link to the patchset: https://codereview.chromium.org/939603002/#ps460001 (title: "Fixed unit test breaks due to one element skip.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/939603002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/939603002/460001
Message was sent while issue was closed.
Committed patchset #23 (id:460001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200398
Message was sent while issue was closed.
On 2015/08/12 15:40:27, commit-bot: I haz the power wrote: > Committed patchset #23 (id:460001) as > https://src.chromium.org/viewvc/blink?view=rev&revision=200398 @tkent & kochi - Thank you very much for your extended and constructive reviews for landing this feature in Android.
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:460001) has been created in https://codereview.chromium.org/1303123002/ by tkent@chromium.org. The reason for reverting is: Regression in power.android_acceptance. crbug.com/520952 . |