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

Side by Side 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, 5 months ago

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

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <html>

	2 <body>

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

	4 <input type="text" name="input1" id="input1" form="form1" value="input1 from f orm1"/><br>
	kochi 2015/07/03 04:10:35 As with <br/> -> <br>, you can close "input" with 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. AKV 2015/07/26 15:56:34 input1 is with "form" exists inside the form. cont Show quoted text 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 ? kochi 2015/07/28 08:43:02 Could you add elements which are usually focusable Show quoted text 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>)? AKV 2015/08/05 08:55:55 Added button element inside and outside form to co Show quoted text 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.
	5 <div id="contenteditable1" name="contenteditable1" contenteditable="true">cont enteditable1 from form1</div><br>

	6 <div id="div1" name="div1" onkeypress="alert('div1');"><br>
	kochi 2015/07/03 04:10:34 What is this onkeypress handler for? What is this onkeypress handler for? AKV 2015/07/26 15:56:34 For checking the KeyEventListener for enabling the Show quoted text 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. kochi 2015/07/28 08:43:02 I got it, thanks for the explanation. Other reade Show quoted text 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=...>) AKV 2015/08/05 08:55:55 Done. Thanks Show quoted text 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
	7 <input type="text" name="input2" id="input2" value="input2 from div1 form1"/ ><br>

	8 </div>

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

	10 <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 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? AKV 2015/07/26 15:56:34 One input with form attribute and other without fo Show quoted text 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.
	11 </form>

	12 <textarea id="textarea2" name="textarea2" form="form1">textarea2 from form1, whi ch is outside parent hierarchy</textarea><br>

	13 <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? How this can be from form2? AKV 2015/07/26 15:56:34 It was a typo while rearranging elements. I correc Show quoted text 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
	14 <div id="contenteditable2" name="contenteditable2" contenteditable="true">conten teditable2 not from form1</div><br>

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

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

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

	18 <div id="contenteditable3" name="contenteditable3" contenteditable="true">cont enteditable3 not from form2</div><br>

	19 </form>

	20 </body>

	21 </html>
	kochi 2015/07/03 04:10:35 Other things of concerns which occurred to me: 1. 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? AKV 2015/07/26 15:56:34 1) As per our initial plan, we have to navigate to Show quoted text 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. kochi 2015/07/28 08:43:02 1) It would be nice to have another set of form el Show quoted text 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. AKV 2015/08/05 08:55:55 Totally agree with you. We need to cover as much c Show quoted text 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.
OLD	NEW

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