Source/web/tests/data/advance_focus_in_form.html - Issue 939603002: Adding support for Smart GO NEXT feature in Android Chrome

Unified Diff: Source/web/tests/data/advance_focus_in_form.html

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

Patch Set: Fixed review comments in unit tests. Created 5 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

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

Download patch

Index: Source/web/tests/data/advance_focus_in_form.html

diff --git a/Source/web/tests/data/advance_focus_in_form.html b/Source/web/tests/data/advance_focus_in_form.html

new file mode 100644

index 0000000000000000000000000000000000000000..8a50ed05899143fc7c70ed85c0a70f18a8d26246

--- /dev/null

+++ b/Source/web/tests/data/advance_focus_in_form.html

@@ -0,0 +1,21 @@

+<html>

+<body>

+<form id="form1" name="form1">

+ <input type="text" name="input1" id="input1" form="form1" value="input1 from form1"/><br>

kochi 2015/07/03 04:10:35 As with <br/> -> <br>, you can close "input" with

AKV 2015/07/26 15:56:34 input1 is with "form" exists inside the form. cont

kochi 2015/07/28 08:43:02 Could you add elements which are usually focusable

AKV 2015/08/05 08:55:55 Added button element inside and outside form to co

+ <div id="contenteditable1" name="contenteditable1" contenteditable="true">contenteditable1 from form1</div><br>

+ <div id="div1" name="div1" onkeypress="alert('div1');"><br>

kochi 2015/07/03 04:10:34 What is this onkeypress handler for?

AKV 2015/07/26 15:56:34 For checking the KeyEventListener for enabling the

kochi 2015/07/28 08:43:02 I got it, thanks for the explanation. Other reade

AKV 2015/08/05 08:55:55 Done. Thanks

+ <input type="text" name="input2" id="input2" value="input2 from div1 form1"/><br>

+ </div>

+ <textarea id="textarea1" name="textarea1">textarea1 from form1</textarea><br>

+ <input type="text" name="input3" id="input3" value="input3 from form1"/><br>

kochi 2015/07/03 04:10:35 If you include <input> twice in the same form, wha

AKV 2015/07/26 15:56:34 One input with form attribute and other without fo

+</form>

+<textarea id="textarea2" name="textarea2" form="form1">textarea2 from form1, which is outside parent hierarchy</textarea><br>

+<textarea id="textarea3" name="textarea3">textarea3 from form2, which is outside parent hierarchy</textarea><br>

kochi 2015/07/03 04:10:35 How this can be from form2?

AKV 2015/07/26 15:56:34 It was a typo while rearranging elements. I correc

+<div id="contenteditable2" name="contenteditable2" contenteditable="true">contenteditable2 not from form1</div><br>

+<form id="form2" name="form2" onkeypress="alert('form2');">

+ <textarea id="textarea4" name="textarea4">textarea4 from form2</textarea><br>

+ <input type="text" name="input4" id="input4" value="input4 from form2"/><br>

+ <div id="contenteditable3" name="contenteditable3" contenteditable="true">contenteditable3 not from form2</div><br>

+</form>

+</body>

+</html>

kochi 2015/07/03 04:10:35 Other things of concerns which occurred to me: 1.

AKV 2015/07/26 15:56:34 1) As per our initial plan, we have to navigate to

kochi 2015/07/28 08:43:02 1) It would be nice to have another set of form el

AKV 2015/08/05 08:55:55 Totally agree with you. We need to cover as much c

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.

« Source/web/tests/WebViewTest.cpp ('K') | « Source/web/tests/WebViewTest.cpp ('k') | public/web/WebTextInputType.h » ('j') | public/web/WebTextInputType.h » ('J')