Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2839993002:
[Android] Adding Smart GO/NEXT feature in Chrome (Closed)
|
Created:
3 years, 8 months ago by AKVT Modified:
3 years, 6 months ago CC:
aelias_OOO_until_Jul13, agrieve+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, jam, kinuko+watch, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, Ted C Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Adding Smart GO/NEXT feature in Chrome
Smart Go/Next brings better user experience to the user during form submitting applications.
For navigating between form elements, user can use NEXT/PREVIOUS button from IME
without touching on individual fields. This will avoid unnecessary form submissions before
filling or visiting all fields in the form.
Additionally it will save user time and avoid redundant network requests before actually
filling/attending entire fields in the form
Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSPNg/edit?usp=sharing
BUG=410785, 648986
Review-Url: https://codereview.chromium.org/2839993002
Cr-Commit-Position: refs/heads/master@{#479126}
Committed: https://chromium.googlesource.com/chromium/src/+/8d80ad1410dcb23f55c323a97e57eed750a611dc
Patch Set 1 #
Total comments: 11
Patch Set 2 : Moved the triggering logic from WebView to WebFrame #Patch Set 3 : Fixed WebViewTest build issues after restructure. #
Total comments: 6
Patch Set 4 : Rebased the patch along with review comment fixes. #
Total comments: 32
Patch Set 5 : Moved the Focus movement logic from IMC to FocusController and addressed other review comments. #
Total comments: 4
Patch Set 6 : Brought up blink enum to Java layer instead of boolean #
Total comments: 8
Patch Set 7 : Fixed FocusController and ImeAdapter related review comments. #
Total comments: 2
Patch Set 8 : Added few test related to Smart Go/next logic #
Total comments: 5
Patch Set 9 : testAdvanceFocusNextAndPrevious() is added for test coverage. #Patch Set 10 : Reverted unnecessary touch in ImeTest. #
Total comments: 4
Patch Set 11 : Fixed ImeTest review comments. #
Total comments: 6
Patch Set 12 : Added inputType check in ImeTest #
Total comments: 30
Patch Set 13 : Fixed the ImeTest issues and added more coverage in WebViewTest #
Total comments: 9
Patch Set 14 : Fixed blink side review comments. #Patch Set 15 : Rebased the patch for running android_fyi_perf bot. #Patch Set 16 : Fixed the build break by replacing WebViewImpl object with WebViewBase #Patch Set 17 : Rebased the patch and fixed ImeTest#testShowAndHideSoftInput #Patch Set 18 : Rebased the patch for checking falky failures #Patch Set 19 : Fixed testSelectActionBarClearedOnTappingOutsideInput failure #Patch Set 20 : Rebased the patch and Fixed Webkit Layout test failure. #Patch Set 21 : Rearranged the test HTML for fixing IME falky test. #Patch Set 22 : Fixed BookmarksTest crash. #Patch Set 23 : Fixed ImeTest#testSelectActionBarClearedOnTappingOutsideInput flaky failure #
Total comments: 5
Patch Set 24 : Preserved the DCHECK and fixed browser_tests failure #
Total comments: 3
Patch Set 25 : Rebased the patch #Patch Set 26 : Rebased the patch from TOT #Messages
Total messages: 137 (38 generated)
Description was changed from ========== Adding support for Smart GO NEXT feature in Android Chrome This change takes care of providing easy navigation among text input elements inside a form. BUG=410785,648986 ========== to ========== Adding support for Smart GO NEXT feature in Android Chrome This change takes care of providing easy navigation among text input elements inside a form. BUG=410785,648986 ==========
ajith.v@samsung.com changed reviewers: + changwan@chromium.org, kochi@chromium.org
ajith.v@samsung.com changed reviewers: + tkent@chromium.org
ajith.v@samsung.com changed reviewers: + aelias@chromium.org
ajith.v@samsung.com changed reviewers: - aelias@chromium.org
ajith.v@samsung.com changed reviewers: + dcheng@chromium.org
ajith.v@samsung.com changed reviewers: + nasko@chromium.org
PTAL! In the meantime, could you help me to find the steps for running WebViewTest. As all the tests are TEST_P, I was not able to execute and correct existing tests.
On 2017/04/25 14:38:50, AKVT wrote: > PTAL! > > In the meantime, could you help me to find the steps for running WebViewTest. As > all the tests are TEST_P, I was not able to execute and correct existing tests. WebViewTest is compiled into the webkit_unit_tests target. Try that.
nasko@chromium.org changed reviewers: + ekaramad@chromium.org
Adding ekaramad@, who has done a lot of work recently on IME in the context of out-of-process iframes, so he can help ensure this CL is compatible. My main comment is that this functionality should not be on WebView, but rather be on WebFrame. https://codereview.chromium.org/2839993002/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/2839993002/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebView.h:224: virtual void advanceFocusInForm(WebFocusType focusType) {} Please move this API to WebFrame. Forms are specific to a document, therefore specific to a frame.
I am not a good reviewer for core blink changes (InputMethodController). But I left some comments for the changes to RenderFrameImpl and WebViewImpl. I agree with nasko@'s suggestion for moving the logic to WebLocalFrame instead. https://codereview.chromium.org/2839993002/diff/1/content/browser/android/ime... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2839993002/diff/1/content/browser/android/ime... content/browser/android/ime_adapter_android.cc:314: rfh->Send(new FrameMsg_AdvanceFocusInForm(rfh->GetRoutingID(), forward)); Is it important if this message arrives out of order with respect to an InputMsg_HandleInputEvent message which was sent before or after it (we send those through the call to SendKeyEvent)? I believe (sometimes) we cannot guarantee the order of arrival between some InputMsg and FrameMsg: https://cs.chromium.org/chromium/src/content/renderer/input/input_event_filte... https://codereview.chromium.org/2839993002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2839993002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2214: render_view_->webview()->advanceFocusInForm(focus_type); Following nasko@'s suggestion I wonder if moving the logic to WebLocalFrame and doing something like this might be better here: if (render_view_->webview()->FocusedFrame() != frame_) return; frame_->advanceFocusInForm(forward ? blink::WebFocusTypeForward : blink::WebFocusTypeBackward); (the first part for checking focused frame might not be necessary, but I believe it is possible to have a different frame focused on the renderer side by the time the IPC arrives). https://codereview.chromium.org/2839993002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2839993002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:2908: LocalFrame* focusedFrame = focusedLocalFrameInWidget(); I may not be understanding the patch correctly but I think this should be an early return if |focusedFrame| is not the same as the render frame which received the IPC.
On 2017/04/25 21:31:13, nasko wrote: > On 2017/04/25 14:38:50, AKVT wrote: > > PTAL! > > > > In the meantime, could you help me to find the steps for running WebViewTest. > As > > all the tests are TEST_P, I was not able to execute and correct existing > tests. > > WebViewTest is compiled into the webkit_unit_tests target. Try that. I was issuing following command # ./out/Default/bin/run_webkit_unit_tests -vvv --gtest-filter WebViewTest* Outcome of this command is @ https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/EXTRA_HDD.... I can see NativeTestApp is launching once and gets exited in the device.
PTAL! https://codereview.chromium.org/2839993002/diff/1/content/browser/android/ime... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2839993002/diff/1/content/browser/android/ime... content/browser/android/ime_adapter_android.cc:314: rfh->Send(new FrameMsg_AdvanceFocusInForm(rfh->GetRoutingID(), forward)); On 2017/04/25 22:43:08, EhsanK wrote: > Is it important if this message arrives out of order with respect to an > InputMsg_HandleInputEvent message which was sent before or after it (we send > those through the call to SendKeyEvent)? I believe (sometimes) we cannot > guarantee the order of arrival between some InputMsg and FrameMsg: > https://cs.chromium.org/chromium/src/content/renderer/input/input_event_filte... If that's a problematic situation, could you help me how we can safeguard it gracefully ? https://codereview.chromium.org/2839993002/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2839993002/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2214: render_view_->webview()->advanceFocusInForm(focus_type); On 2017/04/25 22:43:08, EhsanK wrote: > Following nasko@'s suggestion I wonder if moving the logic to WebLocalFrame and > doing something like this might be better here: > > if (render_view_->webview()->FocusedFrame() != frame_) > return; > frame_->advanceFocusInForm(forward ? blink::WebFocusTypeForward : > blink::WebFocusTypeBackward); > > (the first part for checking focused frame might not be necessary, but I believe > it is possible to have a different frame focused on the renderer side by the > time the IPC arrives). Done. https://codereview.chromium.org/2839993002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2839993002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:2908: LocalFrame* focusedFrame = focusedLocalFrameInWidget(); On 2017/04/25 22:43:08, EhsanK wrote: > I may not be understanding the patch correctly but I think this should be an > early return if |focusedFrame| is not the same as the render frame which > received the IPC. Could you help me to find an example in this file which is taking care of this scenario ? Currently, I have updated with active frame while I moved the changes in WebLocalFrameImpl.cpp https://codereview.chromium.org/2839993002/diff/1/third_party/WebKit/public/w... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/2839993002/diff/1/third_party/WebKit/public/w... third_party/WebKit/public/web/WebView.h:224: virtual void advanceFocusInForm(WebFocusType focusType) {} On 2017/04/25 21:32:45, nasko wrote: > Please move this API to WebFrame. Forms are specific to a document, therefore > specific to a frame. Done.
Thanks for addressing the issues! LGTM with suggestions. https://codereview.chromium.org/2839993002/diff/1/content/browser/android/ime... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2839993002/diff/1/content/browser/android/ime... content/browser/android/ime_adapter_android.cc:314: rfh->Send(new FrameMsg_AdvanceFocusInForm(rfh->GetRoutingID(), forward)); On 2017/04/26 10:33:04, AKVT wrote: > On 2017/04/25 22:43:08, EhsanK wrote: > > Is it important if this message arrives out of order with respect to an > > InputMsg_HandleInputEvent message which was sent before or after it (we send > > those through the call to SendKeyEvent)? I believe (sometimes) we cannot > > guarantee the order of arrival between some InputMsg and FrameMsg: > > > https://cs.chromium.org/chromium/src/content/renderer/input/input_event_filte... > > If that's a problematic situation, could you help me how we can safeguard it > gracefully ? I suggest making the IPC an InputMsg instead. I am referring to this line of code: https://cs.chromium.org/chromium/src/content/renderer/input/input_event_filte... Where it seems like InputMsg's go through a different path (most if not all messages sent by this class seem to be InputMsg). I suggest consulting someone more familiar with input about this, perhaps chongz@ (who is also familiar with InputMethodController.* fiels). https://codereview.chromium.org/2839993002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2839993002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:2908: LocalFrame* focusedFrame = focusedLocalFrameInWidget(); On 2017/04/26 10:33:04, AKVT wrote: > On 2017/04/25 22:43:08, EhsanK wrote: > > I may not be understanding the patch correctly but I think this should be an > > early return if |focusedFrame| is not the same as the render frame which > > received the IPC. > > Could you help me to find an example in this file which is taking care of this > scenario ? > > Currently, I have updated with active frame while I moved the changes in > WebLocalFrameImpl.cpp New changes in RenderFrameImpl and WebLocalFrame look good to me. Left some comments there about the focus issue. In short, I don't think it is really something to worry about. https://codereview.chromium.org/2839993002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2839993002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2213: return; My suggestion for having this early return (and the concern in WebViewImpl implementation in the older patch) was based on this scenario: 1- An advance commands is issued by ImeAndroid, 2- Something (e.g., javascript) changes focus frame/element before IPC arrives. 3- Focus might jump to next editable element in a new form. However, the chances of this scenario happening seems very small and the consequence does not seem terribly bad. Also, the same issue could happen with element focus anyway. So I guess it might not be very valuable to have this early return here. I leave that up to you or other reviewers (sorry I had strong suggestions for it earlier). https://codereview.chromium.org/2839993002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebRemoteFrameImpl.h (right): https://codereview.chromium.org/2839993002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebRemoteFrameImpl.h:157: void advanceFocusInForm(WebFocusType) override; I am not sure why we need this. From here: RenderFrameImpl can only have a WebLocalFrame. Besides, I don't think the corresponding IPC is ever sent to a RenderFrameProxy. I believe this method needs to live in WebLocalFrame only (not in WebFrame). https://codereview.chromium.org/2839993002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrame.h (right): https://codereview.chromium.org/2839993002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrame.h:429: virtual void advanceFocusInForm(WebFocusType) = 0; I suggest removing this to WebLocalFrame. I don't think it makes much sense to have the WebRemoteFrame version of the method. Besides, you only send the IPC to RenderFrameImpl so the WebFrame variant never really gets called anyway.
Did you rebase after the Blink big renaming? (which happened around early this month)
On 2017/04/27 02:03:17, kochi wrote: > Did you rebase after the Blink big renaming? > (which happened around early this month) I have started applying the changes after freezing the upstream code on 17/04/2017. I will upload a rebased patch today.
PTAL! https://codereview.chromium.org/2839993002/diff/1/content/browser/android/ime... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2839993002/diff/1/content/browser/android/ime... content/browser/android/ime_adapter_android.cc:314: rfh->Send(new FrameMsg_AdvanceFocusInForm(rfh->GetRoutingID(), forward)); On 2017/04/26 21:31:59, EhsanK wrote: > On 2017/04/26 10:33:04, AKVT wrote: > > On 2017/04/25 22:43:08, EhsanK wrote: > > > Is it important if this message arrives out of order with respect to an > > > InputMsg_HandleInputEvent message which was sent before or after it (we send > > > those through the call to SendKeyEvent)? I believe (sometimes) we cannot > > > guarantee the order of arrival between some InputMsg and FrameMsg: > > > > > > https://cs.chromium.org/chromium/src/content/renderer/input/input_event_filte... > > > > If that's a problematic situation, could you help me how we can safeguard it > > gracefully ? > > I suggest making the IPC an InputMsg instead. I am referring to this line of > code: > https://cs.chromium.org/chromium/src/content/renderer/input/input_event_filte... > > Where it seems like InputMsg's go through a different path (most if not all > messages sent by this class seem to be InputMsg). > > I suggest consulting someone more familiar with input about this, perhaps > chongz@ (who is also familiar with InputMethodController.* fiels). Thank you for your suggestion. I will wait for respective file owner comments to get more clarity before I make a change corresponds to this. https://codereview.chromium.org/2839993002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2839993002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2213: return; On 2017/04/26 21:31:59, EhsanK wrote: > My suggestion for having this early return (and the concern in WebViewImpl > implementation in the older patch) was based on this scenario: > 1- An advance commands is issued by ImeAndroid, > 2- Something (e.g., javascript) changes focus frame/element before IPC arrives. > 3- Focus might jump to next editable element in a new form. > > However, the chances of this scenario happening seems very small and the > consequence does not seem terribly bad. Also, the same issue could happen with > element focus anyway. So I guess it might not be very valuable to have this > early return here. I leave that up to you or other reviewers (sorry I had strong > suggestions for it earlier). Thanks for your suggestion. https://codereview.chromium.org/2839993002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebRemoteFrameImpl.h (right): https://codereview.chromium.org/2839993002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebRemoteFrameImpl.h:157: void advanceFocusInForm(WebFocusType) override; On 2017/04/26 21:31:59, EhsanK wrote: > I am not sure why we need this. From here: > RenderFrameImpl can only have a WebLocalFrame. Besides, I don't think the > corresponding IPC is ever sent to a RenderFrameProxy. > > I believe this method needs to live in WebLocalFrame only (not in WebFrame). Done. https://codereview.chromium.org/2839993002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrame.h (right): https://codereview.chromium.org/2839993002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrame.h:429: virtual void advanceFocusInForm(WebFocusType) = 0; On 2017/04/26 21:31:59, EhsanK wrote: > I suggest removing this to WebLocalFrame. I don't think it makes much sense to > have the WebRemoteFrame version of the method. Besides, you only send the IPC to > RenderFrameImpl so the WebFrame variant never really gets called anyway. Done.
Also, please address kochi's comment about naming in Blink. Blink mostly follows Chrome's naming conventions now so variables are named_like_this instead of namedLikeThis. https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2575: if (!GetFrame()->GetDocument()) This check is unnecessary and should be a DCHECK if anything.
Please add some tests to ImeTest.java . https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:625: restartInput(); I don't think you need to restart input here at all. Focus change in FocusController should finish composing text on the previous input and cause restart input after next input is focused. If you saw evidence that it did not work, let me know. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:646: public void advanceFocusInForm(boolean forward) { public -> private https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java (right): https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:125: boolean enterKeyAction = false; 1. You need to put this in front of switch statement above, because inputMode should be able to override implicit ime actions. 2. Instead of keeping local variables, how about the following structure? if () { // Comment ... } else if () { // Comment ... } else if () { // Comment ... } Or better yet, you could refactor this into a small function such as: int getImeActions(int inputType, int inputMode, boolean listeningToKeyboardEvents) { // Input mode overrides implicit IME actions. switch(inputMode) { return ...; } if () { return ...; } if () { return ...; } if () { return ...; } return EditorInfo.IME_ACTION_NONE; } https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:126: // For multiline text or any text input with key event listeners, Enter key is needed. nit: ENTER (all cap) or enter (all small) https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:128: == EditorInfo.TYPE_TEXT_FLAG_MULTI_LINE) instead of == you can use > 0 or != 0 https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:130: // TODO(ajith.v) Find an option to remove Enter Key from IME (assume by default No need to have a TODO on our end. Most likely IME's decision and/or ecosystem issue. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:132: // TODO(ajith.v) For text area with implicit form submission how do we handle, By implicit form submission, do you mean JavaScript key handling? If so, IME_ACTION_NONE should be enough, and you can instead leave a comment such as: For textarea that sends you to another webpage on enter key press using JavaScript, we will only show ENTER. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:146: // issues in IME This is quite unfortunate. Could you file/refer to a crbug or be specific about on which IMEs you see this? https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:1162: bool InputMethodController::IsListeningToKeyboardEvents( I'm not sure if this makes much sense... There are tons of different ways to listen to text changes, too. Also, the webpage may not be particularly listening to these events to submit a form or send you somewhere. I think you should remove this logic, and just show 'enter', and expect webpage to use input mode if they want to show 'GO'.
changwan@chromium.org changed reviewers: + yosin@chromium.org
also adding yosin@ for InputMethodController. yosin@ and kochi@, does it make sense for IMC to have the logic or should it be moved to FocusController?
Could you provide design doc? I'm not sure what "Smart Go NEXT feature." Thanks in advance. https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:1176: Element* InputMethodController::NextFocusableElementInForm( This function should be in FocusController instead of InputeMethodController(IMC). IMC should be passive about focus.
On 2017/04/28 01:07:28, yosin_OOO_til_May_7 wrote: > Could you provide design doc? I'm not sure what "Smart Go NEXT feature." > Thanks in advance. yosin@, have you checked crbug 410785?
On 2017/04/28 at 01:14:33, kochi wrote: > On 2017/04/28 01:07:28, yosin_OOO_til_May_7 wrote: > > Could you provide design doc? I'm not sure what "Smart Go NEXT feature." > > Thanks in advance. > > yosin@, have you checked crbug 410785? Thanks! I found https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP...
https://codereview.chromium.org/2839993002/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2839993002/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:2244: void RenderFrameImpl::OnAdvanceFocusInForm(bool forward) { Please avoid to use |bool| parameter[1]. [1] http://dev.chromium.org/blink/coding-style [names-enum-to-bool] https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:1162: bool InputMethodController::IsListeningToKeyboardEvents( On 2017/04/27 at 16:34:54, Changwan Ryu wrote: > I'm not sure if this makes much sense... There are tons of different ways to listen to text changes, too. Also, the webpage may not be particularly listening to these events to submit a form or send you somewhere. > > I think you should remove this logic, and just show 'enter', and expect webpage to use input mode if they want to show 'GO'. Agree with changewan@. This is too specific and coverage narrow. We may want to show "Go" for simple submit button, <input type=submit>, button w/ "click"(include tap) event handler. https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:1176: Element* InputMethodController::NextFocusableElementInForm( On 2017/04/28 at 01:07:28, yosin_OOO_til_May_7 wrote: > This function should be in FocusController instead of InputeMethodController(IMC). > IMC should be passive about focus. I think this function should be implemented on top of TAB move function to respect "tabindex" attribute, which TAB move function handled.
PTAL! I am in process of writing few tests in ImeTest.java and will upload in next patchset. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:625: restartInput(); On 2017/04/27 16:34:53, Changwan Ryu wrote: > I don't think you need to restart input here at all. Focus change in > FocusController should finish composing text on the previous input and cause > restart input after next input is focused. If you saw evidence that it did not > work, let me know. Done. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:646: public void advanceFocusInForm(boolean forward) { On 2017/04/27 16:34:53, Changwan Ryu wrote: > public -> private Done. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java (right): https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:125: boolean enterKeyAction = false; On 2017/04/27 16:34:53, Changwan Ryu wrote: > > 1. You need to put this in front of switch statement above, because inputMode > should be able to override implicit ime actions. > > 2. Instead of keeping local variables, how about the following structure? > > if () { > // Comment > ... > } else if () { > // Comment > ... > } else if () { > // Comment > ... > } > > Or better yet, you could refactor this into a small function such as: > > int getImeActions(int inputType, int inputMode, boolean > listeningToKeyboardEvents) { > // Input mode overrides implicit IME actions. > switch(inputMode) { > return ...; > } > > if () { > return ...; > } > if () { > return ...; > } > if () { > return ...; > } > return EditorInfo.IME_ACTION_NONE; > } You mean we should overwrite inputMode imeAction ? I also felt same, because fir WebTextInputMode.TEL, WebTextInputMode.URL etc. it's by default keeping ACTION_NEXT. But ideally not needed based on near by element presence as per this logic. WDYT ? I thought to have your comment before modifying existing switch (inputMode) {} code. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:126: // For multiline text or any text input with key event listeners, Enter key is needed. On 2017/04/27 16:34:54, Changwan Ryu wrote: > nit: ENTER (all cap) or enter (all small) Done. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:128: == EditorInfo.TYPE_TEXT_FLAG_MULTI_LINE) On 2017/04/27 16:34:54, Changwan Ryu wrote: > instead of == you can use > 0 or != 0 Done. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:130: // TODO(ajith.v) Find an option to remove Enter Key from IME (assume by default On 2017/04/27 16:34:54, Changwan Ryu wrote: > No need to have a TODO on our end. Most likely IME's decision and/or ecosystem > issue. Done. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:132: // TODO(ajith.v) For text area with implicit form submission how do we handle, On 2017/04/27 16:34:54, Changwan Ryu wrote: > By implicit form submission, do you mean JavaScript key handling? If so, > IME_ACTION_NONE should be enough, and you can instead leave a comment such as: > > For textarea that sends you to another webpage on enter key press using > JavaScript, we will only show ENTER. Yes implicit submission I meant with that only. On ENTER key press, it will submit the form instead of adding a new line (Very rare content though). But if we have both the keys (ENTER and GO), then we can achieve both. (New line on ENTER key and form submission on GO). WDYT ? https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:146: // issues in IME On 2017/04/27 16:34:54, Changwan Ryu wrote: > This is quite unfortunate. Could you file/refer to a crbug or be specific about > on which IMEs you see this? Currently I am using Google Pixel device default Google IME for my testing. In fact this problem I have noticed long back on Samsung device IME as well. AFAIU we may have to file this bug to Google IME team. https://codereview.chromium.org/2839993002/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2839993002/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:2244: void RenderFrameImpl::OnAdvanceFocusInForm(bool forward) { On 2017/04/28 03:32:10, yosin_OOO_til_May_7 wrote: > Please avoid to use |bool| parameter[1]. > > [1] http://dev.chromium.org/blink/coding-style [names-enum-to-bool] Without bool, I have to expose kWebFocusTypeForward/Backward until Java layer to identify the direction using auto generated enums. Is that fine ? If yes, I will upload in next patch set. Otherwise please share your suggestion. https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:1162: bool InputMethodController::IsListeningToKeyboardEvents( On 2017/04/27 16:34:54, Changwan Ryu wrote: > I'm not sure if this makes much sense... There are tons of different ways to > listen to text changes, too. Also, the webpage may not be particularly listening > to these events to submit a form or send you somewhere. > > I think you should remove this logic, and just show 'enter', and expect webpage > to use input mode if they want to show 'GO'. Yes I am convinced here, as it has a very narrow coverage. As I pointed in ImeUtils.java, I have still doubt on inputMode as it's still going as trivial assignment, without having a thought of nearby (previous or next) elements. https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:1176: Element* InputMethodController::NextFocusableElementInForm( On 2017/04/28 01:07:28, yosin_OOO_til_May_7 wrote: > This function should be in FocusController instead of > InputeMethodController(IMC). > IMC should be passive about focus. Done. https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:1176: Element* InputMethodController::NextFocusableElementInForm( On 2017/04/28 01:07:28, yosin_OOO_til_May_7 wrote: > This function should be in FocusController instead of > InputeMethodController(IMC). > IMC should be passive about focus. Done. https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2839993002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2575: if (!GetFrame()->GetDocument()) On 2017/04/27 15:33:02, dcheng (OOO through May 2) wrote: > This check is unnecessary and should be a DCHECK if anything. Done.
The C++ parts of content/ look mostly good. One recommendation on the IPC and I'll be happy to stamp the CL once the reviewers of Java parts are happy. https://codereview.chromium.org/2839993002/diff/80001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/2839993002/diff/80001/content/common/frame_me... content/common/frame_messages.h:967: // Tells the renderer to advance the focus to next input node in the form by nit: s/renderer/RenderFrame/ https://codereview.chromium.org/2839993002/diff/80001/content/common/frame_me... content/common/frame_messages.h:970: IPC_MESSAGE_ROUTED1(FrameMsg_AdvanceFocusInForm, bool /* forward */) Instead of passing a bool as parameter, why not pass the enum value directly? It will be more clear at the callsite too.
PTAL! https://codereview.chromium.org/2839993002/diff/80001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/2839993002/diff/80001/content/common/frame_me... content/common/frame_messages.h:967: // Tells the renderer to advance the focus to next input node in the form by On 2017/05/03 23:53:25, nasko wrote: > nit: s/renderer/RenderFrame/ Done. https://codereview.chromium.org/2839993002/diff/80001/content/common/frame_me... content/common/frame_messages.h:970: IPC_MESSAGE_ROUTED1(FrameMsg_AdvanceFocusInForm, bool /* forward */) On 2017/05/03 23:53:25, nasko wrote: > Instead of passing a bool as parameter, why not pass the enum value directly? It > will be more clear at the callsite too. Done.
Thanks! content/ (minus java parts) LGTM.
https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java (right): https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:125: boolean enterKeyAction = false; On 2017/05/03 14:35:12, AKVT wrote: > On 2017/04/27 16:34:53, Changwan Ryu wrote: > > > > 1. You need to put this in front of switch statement above, because inputMode > > should be able to override implicit ime actions. > > > > 2. Instead of keeping local variables, how about the following structure? > > > > if () { > > // Comment > > ... > > } else if () { > > // Comment > > ... > > } else if () { > > // Comment > > ... > > } > > > > Or better yet, you could refactor this into a small function such as: > > > > int getImeActions(int inputType, int inputMode, boolean > > listeningToKeyboardEvents) { > > // Input mode overrides implicit IME actions. > > switch(inputMode) { > > return ...; > > } > > > > if () { > > return ...; > > } > > if () { > > return ...; > > } > > if () { > > return ...; > > } > > return EditorInfo.IME_ACTION_NONE; > > } > > You mean we should overwrite inputMode imeAction ? I also felt same, because fir > WebTextInputMode.TEL, WebTextInputMode.URL etc. it's by default keeping > ACTION_NEXT. But ideally not needed based on near by element presence as per > this logic. WDYT ? > I thought to have your comment before modifying existing switch (inputMode) {} > code. My bad, I got confused about input mode. Input mode action should also be fully replaced by the new ones, instead of OR-ing new values. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:132: // TODO(ajith.v) For text area with implicit form submission how do we handle, On 2017/05/03 14:35:12, AKVT wrote: > On 2017/04/27 16:34:54, Changwan Ryu wrote: > > By implicit form submission, do you mean JavaScript key handling? If so, > > IME_ACTION_NONE should be enough, and you can instead leave a comment such as: > > > > For textarea that sends you to another webpage on enter key press using > > JavaScript, we will only show ENTER. > > Yes implicit submission I meant with that only. On ENTER key press, it will > submit the form instead of adding a new line (Very rare content though). But if > we have both the keys (ENTER and GO), then we can achieve both. (New line on > ENTER key and form submission on GO). WDYT ? multiline --> always ENTER single line --> always GO/NEXT/PREV what's wrong with this? https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:146: // issues in IME On 2017/05/03 14:35:12, AKVT wrote: > On 2017/04/27 16:34:54, Changwan Ryu wrote: > > This is quite unfortunate. Could you file/refer to a crbug or be specific > about > > on which IMEs you see this? > > Currently I am using Google Pixel device default Google IME for my testing. > > In fact this problem I have noticed long back on Samsung device IME as well. > > AFAIU we may have to file this bug to Google IME team. Could you leave those keyboards (not the device) as examples in the comment? https://codereview.chromium.org/2839993002/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java (right): https://codereview.chromium.org/2839993002/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:135: // IME This comment is not needed at all. https://codereview.chromium.org/2839993002/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:140: // If ENTER/NEXT key is already applied, then don't apply PREVIOUS as it can result No need to mention ENTER case here. Did you mean GO? BTW, GO case handling is missing.
core/editing/InputMethodController.cpp lgtm
Reviewed FocusController.*. Could you also add a link to your design doc in the CL description? https://codereview.chromium.org/2839993002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2839993002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:1099: // Skip disabled or readonly editable elements. This comment does not make much sense, as the condition calls "IsDisabledOrReadOnly()". https://codereview.chromium.org/2839993002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:1100: if (form_element->IsDisabledOrReadOnly()) Merge conditions on L1097 and L1100 as if ((form_element->formOwner() != form_owner) || (form_element->IsDisabledOrReadOnly())) continue;
On 2017/04/25 21:31:13, nasko wrote: > On 2017/04/25 14:38:50, AKVT wrote: > > PTAL! > > > > In the meantime, could you help me to find the steps for running WebViewTest. > As > > all the tests are TEST_P, I was not able to execute and correct existing > tests. > > WebViewTest is compiled into the webkit_unit_tests target. Try that. I replied to another private mail, if you select TEST_P() tests with --gtest_filter option, you need a prefix (in this source file, given in INSTANTIATE_TEST_CASE_P(), as "All") so you need to give --gtest_filter="All/WebViewTest.*" (not --gtest_filter="WebViewTest.*").
On 2017/05/09 02:51:36, kochi wrote: > On 2017/04/25 21:31:13, nasko wrote: > > On 2017/04/25 14:38:50, AKVT wrote: > > > PTAL! > > > > > > In the meantime, could you help me to find the steps for running > WebViewTest. > > As > > > all the tests are TEST_P, I was not able to execute and correct existing > > tests. > > > > WebViewTest is compiled into the webkit_unit_tests target. Try that. > > I replied to another private mail, if you select TEST_P() tests with > --gtest_filter option, you need a prefix (in this source file, given in > INSTANTIATE_TEST_CASE_P(), as "All") so you need to give > --gtest_filter="All/WebViewTest.*" (not --gtest_filter="WebViewTest.*"). Thanks Kochi. It worked well and I could fix 2 failures in WebViewTest.
Description was changed from ========== Adding support for Smart GO NEXT feature in Android Chrome This change takes care of providing easy navigation among text input elements inside a form. BUG=410785,648986 ========== to ========== Adding support for Smart GO NEXT feature in Android Chrome This change takes care of providing easy navigation among text input elements inside a form. Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p BUG=410785,648986 ==========
PTAL! @changwan - May plan for testing in ImeTest is as below; 1) Modify the focusedNodeChanged to supply Focused node id as well as a String param. 2) On every Previous/Next, Compare the focused node id with expected id. 3) On every Previous/Next, Compare the imeOptions with expected imeOptions. If you have any suggestion, please let me know. https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java (right): https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:125: boolean enterKeyAction = false; On 2017/05/04 20:38:15, Changwan Ryu wrote: > On 2017/05/03 14:35:12, AKVT wrote: > > On 2017/04/27 16:34:53, Changwan Ryu wrote: > > > > > > 1. You need to put this in front of switch statement above, because > inputMode > > > should be able to override implicit ime actions. > > > > > > 2. Instead of keeping local variables, how about the following structure? > > > > > > if () { > > > // Comment > > > ... > > > } else if () { > > > // Comment > > > ... > > > } else if () { > > > // Comment > > > ... > > > } > > > > > > Or better yet, you could refactor this into a small function such as: > > > > > > int getImeActions(int inputType, int inputMode, boolean > > > listeningToKeyboardEvents) { > > > // Input mode overrides implicit IME actions. > > > switch(inputMode) { > > > return ...; > > > } > > > > > > if () { > > > return ...; > > > } > > > if () { > > > return ...; > > > } > > > if () { > > > return ...; > > > } > > > return EditorInfo.IME_ACTION_NONE; > > > } > > > > You mean we should overwrite inputMode imeAction ? I also felt same, because > fir > > WebTextInputMode.TEL, WebTextInputMode.URL etc. it's by default keeping > > ACTION_NEXT. But ideally not needed based on near by element presence as per > > this logic. WDYT ? > > I thought to have your comment before modifying existing switch (inputMode) {} > > code. > > My bad, I got confused about input mode. Input mode action should also be fully > replaced by the new ones, instead of OR-ing new values. Thanks. I corrected in PS7 https://codereview.chromium.org/2839993002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:132: // TODO(ajith.v) For text area with implicit form submission how do we handle, On 2017/05/04 20:38:15, Changwan Ryu wrote: > On 2017/05/03 14:35:12, AKVT wrote: > > On 2017/04/27 16:34:54, Changwan Ryu wrote: > > > By implicit form submission, do you mean JavaScript key handling? If so, > > > IME_ACTION_NONE should be enough, and you can instead leave a comment such > as: > > > > > > For textarea that sends you to another webpage on enter key press using > > > JavaScript, we will only show ENTER. > > > > Yes implicit submission I meant with that only. On ENTER key press, it will > > submit the form instead of adding a new line (Very rare content though). But > if > > we have both the keys (ENTER and GO), then we can achieve both. (New line on > > ENTER key and form submission on GO). WDYT ? > > multiline --> always ENTER > single line --> always GO/NEXT/PREV > > what's wrong with this? Yes currently I have done like this. Also NEXT/GO/PREV I am setting individually because multiple ORing has issues in display and is executing only one action at a time, because it's sharing with one key space. https://codereview.chromium.org/2839993002/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java (right): https://codereview.chromium.org/2839993002/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:135: // IME On 2017/05/04 20:38:15, Changwan Ryu wrote: > This comment is not needed at all. Done. https://codereview.chromium.org/2839993002/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:140: // If ENTER/NEXT key is already applied, then don't apply PREVIOUS as it can result On 2017/05/04 20:38:15, Changwan Ryu wrote: > No need to mention ENTER case here. Did you mean GO? BTW, GO case handling is > missing. Yes, I missed GO in subsequent review cycle. Also PREVIOUS and GO I am not able to apply as it's always taking PREVIOUS as default action. So last element inside form is difficult to use as submitting option. Same case with NEXT and PREVIOUS together, it takes PREVIOUS as primary action, so ending up switching between only 2 elements inside form. Others are unreachable without manual touch. I have tested using Samsung Keyboard 1.5.02 on Galaxy S7 Edge SM-935FD device with Android 7.0 G935FXXU1DPLT binary. https://codereview.chromium.org/2839993002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2839993002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:1099: // Skip disabled or readonly editable elements. On 2017/05/09 02:47:43, kochi wrote: > This comment does not make much sense, as the condition calls > "IsDisabledOrReadOnly()". Done. https://codereview.chromium.org/2839993002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:1100: if (form_element->IsDisabledOrReadOnly()) On 2017/05/09 02:47:43, kochi wrote: > Merge conditions on L1097 and L1100 as > if ((form_element->formOwner() != form_owner) || > (form_element->IsDisabledOrReadOnly())) > continue; Done.
Regarding testing approach, Please do NOT try to inject fake focus IDs. You can focus on the first element (as in setUp), record the current imeOptions on every reset (TestInputMethodManager's restartInput / showSoftInput) and advance in both directions until you reach multiline textarea. Then you can verify the previously recorded imeOptions. https://codereview.chromium.org/2839993002/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java (right): https://codereview.chromium.org/2839993002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:49: boolean enterKeyAction = false; remove?
On 2017/05/09 14:43:58, Changwan Ryu wrote: > Regarding testing approach, > > Please do NOT try to inject fake focus IDs. > > You can focus on the first element (as in setUp), > record the current imeOptions on every reset (TestInputMethodManager's > restartInput / showSoftInput) I thought of caching the imeOptions in ImeUtils.computeEditorInfo() and query in tests. Also focus node id I will be sending "id" attribute of node. If "id" attribute is not there I will be sending native node pointer itself as id to Java layer. > > and advance in both directions until you reach multiline textarea. > Then you can verify the previously recorded imeOptions. I am trying to use same html files, which I used in WebViewTest and mimic same test logic in Java. WDYT ? > > https://codereview.chromium.org/2839993002/diff/120001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java > (right): > > https://codereview.chromium.org/2839993002/diff/120001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:49: > boolean enterKeyAction = false; > remove?
On 2017/05/09 14:52:49, AKVT wrote: > On 2017/05/09 14:43:58, Changwan Ryu wrote: > > Regarding testing approach, > > > > Please do NOT try to inject fake focus IDs. > > > > You can focus on the first element (as in setUp), > > record the current imeOptions on every reset (TestInputMethodManager's > > restartInput / showSoftInput) > > > I thought of caching the imeOptions in ImeUtils.computeEditorInfo() and query in > tests. I don't think we want to add such logic to the production code. Please check references to onCreateInputConnection in TestInputMethodManagerWrapper.java. > > Also focus node id I will be sending "id" attribute of node. If "id" attribute > is not there I will be sending native node pointer itself as id to Java layer. I don't think you need to check IDs at all, as long as you can change the attributes slightly differently across different inputs (e.g. input types) if you intend to verify the advancing of inputs actually occurred or not. But if you really want to check IDs, you'll need to execute JavaScript to query the current ID, which is quite expensive. > > > > > > and advance in both directions until you reach multiline textarea. > > Then you can verify the previously recorded imeOptions. > > > I am trying to use same html files, which I used in WebViewTest and mimic same > test logic in Java. WDYT ? I hope you could use the same HTML file in ImeTest.java because that way you can avoid additional setup, but it's up to you. Please make sure to include at least one contenteditable. > > > > > > https://codereview.chromium.org/2839993002/diff/120001/content/public/android... > > File > > > content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java > > (right): > > > > > https://codereview.chromium.org/2839993002/diff/120001/content/public/android... > > > content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:49: > > boolean enterKeyAction = false; > > remove?
BTW, could you describe your CL in more details in the commit subject / description? Smart GO NEXT is somewhat abstract, it would help to describe how you actually distinguish those action types, along with potential caveat of the approach, if any. Also, the design doc link is incorrect.
Description was changed from ========== Adding support for Smart GO NEXT feature in Android Chrome This change takes care of providing easy navigation among text input elements inside a form. Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p BUG=410785,648986 ========== to ========== Adding support for Smart GO NEXT feature in Android Chrome This change takes care of providing easy navigation among text input elements inside a form. Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ==========
Description was changed from ========== Adding support for Smart GO NEXT feature in Android Chrome This change takes care of providing easy navigation among text input elements inside a form. Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ========== to ========== Adding support for Smart GO NEXT feature in Android Chrome Smart Go/Next brings better user experience to the user during form submit applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ==========
PTAL! @changwan - Regarding test failure, could you give some initial inputs ? https://codereview.chromium.org/2839993002/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java (right): https://codereview.chromium.org/2839993002/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:49: boolean enterKeyAction = false; On 2017/05/09 14:43:57, Changwan Ryu wrote: > remove? Done. https://codereview.chromium.org/2839993002/diff/140001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:428: Assert.assertEquals(DEFAULT_IME_OPTIONS | EditorInfo.IME_ACTION_NEXT, I am seeing a failure in this line. As per the logic it supposed to be DEFAULT_IME_OPTIONS | EditorInfo.IME_ACTION_NEXT And visually also it's correctly appearing on IME But test is failing as it's coming with DEFAULT_IME_OPTIONS | EditorInfo.IME_ACTION_NONE.
Description was changed from ========== Adding support for Smart GO NEXT feature in Android Chrome Smart Go/Next brings better user experience to the user during form submit applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ========== to ========== Adding support for Smart GO NEXT feature in Android Chrome Smart Go/Next brings better user experience to the user during form submit applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ==========
Description was changed from ========== Adding support for Smart GO NEXT feature in Android Chrome Smart Go/Next brings better user experience to the user during form submit applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ========== to ========== Adding support for Smart GO NEXT feature in Android Chrome Smart Go/Next brings better user experience to the user during form submit applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ==========
https://codereview.chromium.org/2839993002/diff/140001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:43: EditorInfo.IME_FLAG_NO_FULLSCREEN | EditorInfo.IME_FLAG_NO_EXTRACT_UI; Instead of make another dependency on the production logic, how about adding IME_ACTION_MASKS = EditorInfo.IME_ACTION_NEXT | EditorInfo.IME_ACTION_PREVIOUS | EditorInfo.IME_ACTION_GO and using & operator to check the final value against expected value? https://codereview.chromium.org/2839993002/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:428: Assert.assertEquals(DEFAULT_IME_OPTIONS | EditorInfo.IME_ACTION_NEXT, On 2017/05/09 17:22:18, AKVT wrote: > I am seeing a failure in this line. As per the logic it supposed to be > DEFAULT_IME_OPTIONS | EditorInfo.IME_ACTION_NEXT > > And visually also it's correctly appearing on IME > > But test is failing as it's coming with DEFAULT_IME_OPTIONS | > EditorInfo.IME_ACTION_NONE. My initial guess is that assertWaitForKeyboardStatus(true) isn't actually waiting for anything since keyboard is already showing from setUp(). It takes a round trip to the renderer process for the click event to restart / show input. You'd consider calling mRule.focusElement("input_text") which should wait for states to be correctly updated.
PTAL! https://codereview.chromium.org/2839993002/diff/140001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:43: EditorInfo.IME_FLAG_NO_FULLSCREEN | EditorInfo.IME_FLAG_NO_EXTRACT_UI; On 2017/05/09 18:12:16, Changwan Ryu wrote: > Instead of make another dependency on the production logic, how about adding > IME_ACTION_MASKS = EditorInfo.IME_ACTION_NEXT | EditorInfo.IME_ACTION_PREVIOUS | > EditorInfo.IME_ACTION_GO > > and using & operator to check the final value against expected value? Done. That seems to be more easier. https://codereview.chromium.org/2839993002/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:428: Assert.assertEquals(DEFAULT_IME_OPTIONS | EditorInfo.IME_ACTION_NEXT, On 2017/05/09 18:12:16, Changwan Ryu wrote: > On 2017/05/09 17:22:18, AKVT wrote: > > I am seeing a failure in this line. As per the logic it supposed to be > > DEFAULT_IME_OPTIONS | EditorInfo.IME_ACTION_NEXT > > > > And visually also it's correctly appearing on IME > > > > But test is failing as it's coming with DEFAULT_IME_OPTIONS | > > EditorInfo.IME_ACTION_NONE. > > My initial guess is that assertWaitForKeyboardStatus(true) isn't actually > waiting for anything since keyboard is already showing from setUp(). > It takes a round trip to the renderer process for the click event to restart / > show input. > > You'd consider calling mRule.focusElement("input_text") which should wait for > states to be correctly updated. Thanks. I corrected it using mRule.focusElement();
https://codereview.chromium.org/2839993002/diff/180001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:423: // Form1 forward and backward focus Do we really need to test both forms? If not, could you remove form1 part of the test? https://codereview.chromium.org/2839993002/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:426: (mRule.getInputMethodManagerWrapper().getImeOptions() & EditorInfo.IME_ACTION_NEXT) Could you add a new function private int getImeActions() { return mRule.getInputMethodManagerWrapper().getImeOptions() & EditorInfo.IME_MASK_ACTION; } And change this line as Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, getImeActions()); here and in other places.
FocusController.* lgtm
PTAL! https://codereview.chromium.org/2839993002/diff/180001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:423: // Form1 forward and backward focus On 2017/05/10 16:20:58, Changwan Ryu wrote: > Do we really need to test both forms? If not, could you remove form1 part of the > test? Ok. Kind of redundant. Thanks https://codereview.chromium.org/2839993002/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:426: (mRule.getInputMethodManagerWrapper().getImeOptions() & EditorInfo.IME_ACTION_NEXT) On 2017/05/10 16:20:59, Changwan Ryu wrote: > Could you add a new function > > private int getImeActions() { > return mRule.getInputMethodManagerWrapper().getImeOptions() & > EditorInfo.IME_MASK_ACTION; > } > > And change this line as > Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, getImeActions()); > > here and in other places. Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, getImeActions()); is not working correctly. Instead I had used Assert.assertTrue((EditorInfo.IME_ACTION_NEXT & getImeActions()) != 0);
https://codereview.chromium.org/2839993002/diff/200001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/200001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:437: Assert.assertTrue((EditorInfo.IME_ACTION_NEXT & getImeActions()) != 0); This is somewhat concerning. What values do you get from getImeActions()? https://codereview.chromium.org/2839993002/diff/200001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:443: // NEXT. So, the last element is content editable, right? Could you verify it by checking inputType against EditorInfo.TYPE_TEXT_FLAG_MULTI_LINE and add some comment as such? I just want to make sure 1) that you cover content editable case. 2) that this really is the last element.
I doubt we need to get the data from computeEditorInfo() and make available for querying. Though I am not sure, that's the best approach. https://codereview.chromium.org/2839993002/diff/200001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/200001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:437: Assert.assertTrue((EditorInfo.IME_ACTION_NEXT & getImeActions()) != 0); On 2017/05/11 09:10:52, Changwan Ryu wrote: > This is somewhat concerning. What values do you get from getImeActions()? I am getting IME_ACTION_NONE java.lang.AssertionError: expected:<5> but was:<1> https://codereview.chromium.org/2839993002/diff/200001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:443: // NEXT. On 2017/05/11 09:10:52, Changwan Ryu wrote: > So, the last element is content editable, right? > Could you verify it by checking inputType against > EditorInfo.TYPE_TEXT_FLAG_MULTI_LINE and add some comment as such? I just want > to make sure 1) that you cover content editable case. 2) that this really is the > last element. Even my check to Assert.assertEquals(EditorInfo.TYPE_TEXT_FLAG_MULTI_LINE | InputType.TYPE_TEXT_FLAG_CAP_SENTENCES | EditorInfo.TYPE_CLASS_TEXT | EditorInfo.TYPE_TEXT_VARIATION_WEB_EDIT_TEXT, getInputType()); is failing java.lang.AssertionError: expected:<147617> but was:<164001>
PTAL! Added inputType check in tests.
https://codereview.chromium.org/2839993002/diff/200001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/200001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:437: Assert.assertTrue((EditorInfo.IME_ACTION_NEXT & getImeActions()) != 0); On 2017/05/11 09:48:49, AKVT wrote: > On 2017/05/11 09:10:52, Changwan Ryu wrote: > > This is somewhat concerning. What values do you get from getImeActions()? > > I am getting IME_ACTION_NONE > > java.lang.AssertionError: expected:<5> but was:<1> IME_ACTION_NEXT = 5, IME_ACTION_NONE = 1 You shouldn't mask the final value here. Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeActions()); here and in other places.
@changwan - Please address my query and continue review on PS12 https://codereview.chromium.org/2839993002/diff/200001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/200001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:437: Assert.assertTrue((EditorInfo.IME_ACTION_NEXT & getImeActions()) != 0); On 2017/05/11 12:04:37, Changwan Ryu wrote: > On 2017/05/11 09:48:49, AKVT wrote: > > On 2017/05/11 09:10:52, Changwan Ryu wrote: > > > This is somewhat concerning. What values do you get from getImeActions()? > > > > I am getting IME_ACTION_NONE > > > > java.lang.AssertionError: expected:<5> but was:<1> > > IME_ACTION_NEXT = 5, IME_ACTION_NONE = 1 > > You shouldn't mask the final value here. > > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeActions()); > > here and in other places. But, I was expecting IME_ACTION_NEXT, because it's a <input id="input_number1" type="number" /> field. What do you suggest ?
On 2017/05/11 12:16:44, AKVT wrote: > @changwan - Please address my query and continue review on PS12 > > https://codereview.chromium.org/2839993002/diff/200001/content/public/android... > File > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java > (right): > > https://codereview.chromium.org/2839993002/diff/200001/content/public/android... > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:437: > Assert.assertTrue((EditorInfo.IME_ACTION_NEXT & getImeActions()) != 0); > On 2017/05/11 12:04:37, Changwan Ryu wrote: > > On 2017/05/11 09:48:49, AKVT wrote: > > > On 2017/05/11 09:10:52, Changwan Ryu wrote: > > > > This is somewhat concerning. What values do you get from getImeActions()? > > > > > > I am getting IME_ACTION_NONE > > > > > > java.lang.AssertionError: expected:<5> but was:<1> > > > > IME_ACTION_NEXT = 5, IME_ACTION_NONE = 1 > > > > You shouldn't mask the final value here. > > > > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeActions()); > > > > here and in other places. > > But, I was expecting IME_ACTION_NEXT, because it's a <input id="input_number1" > type="number" /> field. What do you suggest ? Hmm.. Could you add a similar test to WebViewTest.cpp and check whether the problem is in the blink logic or in the java layer?
On 2017/05/11 15:32:21, Changwan Ryu wrote: > On 2017/05/11 12:16:44, AKVT wrote: > > @changwan - Please address my query and continue review on PS12 > > > > > https://codereview.chromium.org/2839993002/diff/200001/content/public/android... > > File > > > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java > > (right): > > > > > https://codereview.chromium.org/2839993002/diff/200001/content/public/android... > > > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:437: > > Assert.assertTrue((EditorInfo.IME_ACTION_NEXT & getImeActions()) != 0); > > On 2017/05/11 12:04:37, Changwan Ryu wrote: > > > On 2017/05/11 09:48:49, AKVT wrote: > > > > On 2017/05/11 09:10:52, Changwan Ryu wrote: > > > > > This is somewhat concerning. What values do you get from > getImeActions()? > > > > > > > > I am getting IME_ACTION_NONE > > > > > > > > java.lang.AssertionError: expected:<5> but was:<1> > > > > > > IME_ACTION_NEXT = 5, IME_ACTION_NONE = 1 > > > > > > You shouldn't mask the final value here. > > > > > > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeActions()); > > > > > > here and in other places. > > > > But, I was expecting IME_ACTION_NEXT, because it's a <input id="input_number1" > > type="number" /> field. What do you suggest ? > > Hmm.. Could you add a similar test to WebViewTest.cpp and check whether the > problem is in the blink logic or in the java layer? Currently added WebViewTest is passing correctly. Also the values coming blink wrt to WebTextInputFlags is correctly verified in WebViewTest. I feel Java side code is not hitting correctly while running instrumentation test.
On 2017/05/11 15:49:41, AKVT wrote: > On 2017/05/11 15:32:21, Changwan Ryu wrote: > > On 2017/05/11 12:16:44, AKVT wrote: > > > @changwan - Please address my query and continue review on PS12 > > > > > > > > > https://codereview.chromium.org/2839993002/diff/200001/content/public/android... > > > File > > > > > > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2839993002/diff/200001/content/public/android... > > > > > > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:437: > > > Assert.assertTrue((EditorInfo.IME_ACTION_NEXT & getImeActions()) != 0); > > > On 2017/05/11 12:04:37, Changwan Ryu wrote: > > > > On 2017/05/11 09:48:49, AKVT wrote: > > > > > On 2017/05/11 09:10:52, Changwan Ryu wrote: > > > > > > This is somewhat concerning. What values do you get from > > getImeActions()? > > > > > > > > > > I am getting IME_ACTION_NONE > > > > > > > > > > java.lang.AssertionError: expected:<5> but was:<1> > > > > > > > > IME_ACTION_NEXT = 5, IME_ACTION_NONE = 1 > > > > > > > > You shouldn't mask the final value here. > > > > > > > > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeActions()); > > > > > > > > here and in other places. > > > > > > But, I was expecting IME_ACTION_NEXT, because it's a <input > id="input_number1" > > > type="number" /> field. What do you suggest ? > > > > Hmm.. Could you add a similar test to WebViewTest.cpp and check whether the > > problem is in the blink logic or in the java layer? > > Currently added WebViewTest is passing correctly. Also the values coming blink > wrt to WebTextInputFlags is correctly verified in WebViewTest. I feel Java side > code is not hitting correctly while running instrumentation test. Ok, I think you should either record ime actions and verify them later (like I originally suggested), or call resetUpdateSelection() / waitAndVerifyUpdateSelection() in each step.
On 2017/05/11 15:55:26, Changwan Ryu wrote: > On 2017/05/11 15:49:41, AKVT wrote: > > On 2017/05/11 15:32:21, Changwan Ryu wrote: > > > On 2017/05/11 12:16:44, AKVT wrote: > > > > @changwan - Please address my query and continue review on PS12 > > > > > > > > > > > > > > https://codereview.chromium.org/2839993002/diff/200001/content/public/android... > > > > File > > > > > > > > > > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2839993002/diff/200001/content/public/android... > > > > > > > > > > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:437: > > > > Assert.assertTrue((EditorInfo.IME_ACTION_NEXT & getImeActions()) != 0); > > > > On 2017/05/11 12:04:37, Changwan Ryu wrote: > > > > > On 2017/05/11 09:48:49, AKVT wrote: > > > > > > On 2017/05/11 09:10:52, Changwan Ryu wrote: > > > > > > > This is somewhat concerning. What values do you get from > > > getImeActions()? > > > > > > > > > > > > I am getting IME_ACTION_NONE > > > > > > > > > > > > java.lang.AssertionError: expected:<5> but was:<1> > > > > > > > > > > IME_ACTION_NEXT = 5, IME_ACTION_NONE = 1 > > > > > > > > > > You shouldn't mask the final value here. > > > > > > > > > > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeActions()); > > > > > > > > > > here and in other places. > > > > > > > > But, I was expecting IME_ACTION_NEXT, because it's a <input > > id="input_number1" > > > > type="number" /> field. What do you suggest ? > > > > > > Hmm.. Could you add a similar test to WebViewTest.cpp and check whether the > > > problem is in the blink logic or in the java layer? > > > > Currently added WebViewTest is passing correctly. Also the values coming blink > > wrt to WebTextInputFlags is correctly verified in WebViewTest. I feel Java > side > > code is not hitting correctly while running instrumentation test. > > Ok, I think you should either record ime actions and verify them later (like I > originally suggested), > or call resetUpdateSelection() / waitAndVerifyUpdateSelection() in each step. Ok, I think you should either record ime actions and verify them later (like I > originally suggested) -> Could you brief with a pseudo code for this approach ? Actually I didn't get your idea clearly.
To answer the previous question, advancing the focus takes a round trip to the Blink in the renderer, so you can't get an immediate result. I've detailed out how you should change test structure. Also, I digged a bit further to prevent potential issues. https://codereview.chromium.org/2839993002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java (right): https://codereview.chromium.org/2839993002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:65: imeAction = EditorInfo.IME_ACTION_SEARCH; I didn't notice this earlier, but you're overriding this case. Could you rewrite it as int imeAction = EditorInfo.IME_ACTION_NONE; if (inputMode == WebTextInputMode.DEFAULT && inputType == TextInputType.SEARCH) { imeAction = EditorInfo.IME_ACTION_SEARCH; } else if (...) // the rest of the logic goes here. } outAttrs.imeOptions |= imeAction; https://codereview.chromium.org/2839993002/diff/220001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeActivityTestRule.java (right): https://codereview.chromium.org/2839993002/diff/220001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeActivityTestRule.java:275: void resetUpdateSelectionList() { Previously we only clear selection list in setUp() and other places, and you'll encounter test failure because mEditorInfoList won't be cleared correctly. Please remove this function and change all references to resetAllStates(). https://codereview.chromium.org/2839993002/diff/220001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/220001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:419: private int getImeActions() { private static int getImeAction(EditorInfo editorInfo) { return editorInfo.imeOptions & EditorInfo.IME_MASK_ACTION; } https://codereview.chromium.org/2839993002/diff/220001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:423: private int getInputType() { remove https://codereview.chromium.org/2839993002/diff/220001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:430: public void testAdvanceFocusNextAndPrevious() throws Exception { I suggest the following structure: public void testAdvanceFocusNextAndPrevious() throws Exception { mRule.focusElement("textarea"); // Forward direction focus. Excessive focus advance should be ignored. for (int i = 0; i < 10; ++i) { mRule.performEditorAction(EditorInfo.IME_ACTION_NEXT); } mRule.waitForKeyboardStates(7, 0, 7, new Integer[] {TextInputType.TEXT_AREA, TextInputType.TEXT_AREA, TextInputType.NUMBER, TextInputType.NUMBER, TextInputType.CONTENT_EDITABLE, TextInputType.SEARCH, TextInputType.TEXT}); ArrayList<EditorInfo> editorInfoList = mRule.getInputMethodManagerWrapper().getEditorInfoList(); Assert.assertEquals(7, editorInfoList.size()); // textarea. Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeAction(editorInfoList.get(0))); // textarea2. Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeAction(editorInfoList.get(1))); // input_number1. Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, getImeAction(editorInfoList.get(2))); // input_number2. Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, getImeAction(editorInfoList.get(3))); // content_editable1. Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeAction(editorInfoList.get(4))); // search1. Assert.assertEquals(EditorInfo.IME_ACTION_SEARCH, getImeAction(editorInfoList.get(5))); // input_text1. Assert.assertEquals(EditorInfo.IME_ACTION_GO, getImeAction(editorInfoList.get(6))); mRule.resetAllStates(); // Backward direction focus. Excessive focus advance should be ignored. for (int i = 0; i < 10; ++i) { // Backward direction focus. mRule.performEditorAction(EditorInfo.IME_ACTION_PREVIOUS); } mRule.waitForKeyboardStates(6, 0, 6, new Integer[] {TextInputType.SEARCH, TextInputType.CONTENT_EDITABLE, TextInputType.NUMBER, TextInputType.NUMBER, TextInputType.TEXT_AREA, TextInputType.TEXT_AREA}); editorInfoList = mRule.getInputMethodManagerWrapper().getEditorInfoList(); Assert.assertEquals(6, editorInfoList.size()); // search1. Assert.assertEquals(EditorInfo.IME_ACTION_SEARCH, getImeAction(editorInfoList.get(0))); // content_editable1. Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeAction(editorInfoList.get(1))); // input_number2. Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, getImeAction(editorInfoList.get(2))); // input_number1. Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, getImeAction(editorInfoList.get(3))); // textarea2. Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeAction(editorInfoList.get(4))); // textarea. Assert.assertEquals(EditorInfo.IME_ACTION_NONE, getImeAction(editorInfoList.get(5))); } https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java (right): https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:42: private int mImeOptions; private final ArrayList<EditorInfo> mEditorInfoList = new ArrayList<>(); https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:43: private int mInputType; not needed at all. please remove all references. https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:58: mImeOptions = editorInfo.imeOptions; mEditorInfoList.add(editorInfo); https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:70: mImeOptions = editorInfo.imeOptions; mEditorInfoList.add(editorInfo); https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:146: mUpdateSelectionList.clear(); Please add mEditorInfoList.clear(); https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:177: public int getImeOptions() { public ArrayList<EditorInfo> getEditorInfoList() { return mEditorInfoList; } https://codereview.chromium.org/2839993002/diff/220001/content/test/data/andr... File content/test/data/android/input/input_forms.html (right): https://codereview.chromium.org/2839993002/diff/220001/content/test/data/andr... content/test/data/android/input/input_forms.html:9: <div id="contenteditable1" contenteditable>contenteditable1 from form1</div> remove https://codereview.chromium.org/2839993002/diff/220001/content/test/data/andr... content/test/data/android/input/input_forms.html:12: <form id="form2"> remove id https://codereview.chromium.org/2839993002/diff/220001/content/test/data/andr... content/test/data/android/input/input_forms.html:25: <div id="contenteditable2" contenteditable>contenteditable2 from form2</div> <div id="contenteditable1" contenteditable>contenteditable1</div> <input id="search1" type="search" size="10" /> <input id="input_text1" type="text" size="10" /> https://codereview.chromium.org/2839993002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2839993002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebViewTest.cpp:1629: MoveFocusToNextFocusableElementInFormWithKeyEventListenersAndNonEditableElements) { This test is not strong enough. You need to actually call FocusController::NextFocusableElementInForm() and check the values against previous / next element's ID. For example, even if contenteditable1 were not detected as next focusable element, the test would still pass because of the elements that are located further away. Also, as I suggested earlier, please try to add the failing cases from ImeTest to WebViewTest (type=number).
PTAL! https://codereview.chromium.org/2839993002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java (right): https://codereview.chromium.org/2839993002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java:65: imeAction = EditorInfo.IME_ACTION_SEARCH; On 2017/05/11 22:57:12, Changwan Ryu wrote: > I didn't notice this earlier, but you're overriding this case. > Could you rewrite it as > > int imeAction = EditorInfo.IME_ACTION_NONE; > if (inputMode == WebTextInputMode.DEFAULT && inputType == TextInputType.SEARCH) > { > imeAction = EditorInfo.IME_ACTION_SEARCH; > } else if (...) > // the rest of the logic goes here. > } > > outAttrs.imeOptions |= imeAction; Done. https://codereview.chromium.org/2839993002/diff/220001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeActivityTestRule.java (right): https://codereview.chromium.org/2839993002/diff/220001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeActivityTestRule.java:275: void resetUpdateSelectionList() { On 2017/05/11 22:57:12, Changwan Ryu wrote: > Previously we only clear selection list in setUp() and other places, and you'll > encounter test failure because mEditorInfoList won't be cleared correctly. > > Please remove this function and change all references to resetAllStates(). Done. https://codereview.chromium.org/2839993002/diff/220001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/2839993002/diff/220001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:419: private int getImeActions() { On 2017/05/11 22:57:13, Changwan Ryu wrote: > private static int getImeAction(EditorInfo editorInfo) { > return editorInfo.imeOptions & EditorInfo.IME_MASK_ACTION; > } Done. https://codereview.chromium.org/2839993002/diff/220001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:423: private int getInputType() { On 2017/05/11 22:57:13, Changwan Ryu wrote: > remove Done. https://codereview.chromium.org/2839993002/diff/220001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:430: public void testAdvanceFocusNextAndPrevious() throws Exception { On 2017/05/11 22:57:13, Changwan Ryu wrote: > I suggest the following structure: > > public void testAdvanceFocusNextAndPrevious() throws Exception { > mRule.focusElement("textarea"); > // Forward direction focus. Excessive focus advance should be ignored. > for (int i = 0; i < 10; ++i) { > mRule.performEditorAction(EditorInfo.IME_ACTION_NEXT); > } > > mRule.waitForKeyboardStates(7, 0, 7, new Integer[] > {TextInputType.TEXT_AREA, > TextInputType.TEXT_AREA, TextInputType.NUMBER, > TextInputType.NUMBER, > TextInputType.CONTENT_EDITABLE, TextInputType.SEARCH, > TextInputType.TEXT}); > ArrayList<EditorInfo> editorInfoList = > mRule.getInputMethodManagerWrapper().getEditorInfoList(); > Assert.assertEquals(7, editorInfoList.size()); > // textarea. > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, > getImeAction(editorInfoList.get(0))); > // textarea2. > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, > getImeAction(editorInfoList.get(1))); > // input_number1. > Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, > getImeAction(editorInfoList.get(2))); > // input_number2. > Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, > getImeAction(editorInfoList.get(3))); > // content_editable1. > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, > getImeAction(editorInfoList.get(4))); > // search1. > Assert.assertEquals(EditorInfo.IME_ACTION_SEARCH, > getImeAction(editorInfoList.get(5))); > // input_text1. > Assert.assertEquals(EditorInfo.IME_ACTION_GO, > getImeAction(editorInfoList.get(6))); > > mRule.resetAllStates(); > > // Backward direction focus. Excessive focus advance should be ignored. > for (int i = 0; i < 10; ++i) { > // Backward direction focus. > mRule.performEditorAction(EditorInfo.IME_ACTION_PREVIOUS); > } > mRule.waitForKeyboardStates(6, 0, 6, new Integer[] > {TextInputType.SEARCH, > TextInputType.CONTENT_EDITABLE, TextInputType.NUMBER, > TextInputType.NUMBER, > TextInputType.TEXT_AREA, TextInputType.TEXT_AREA}); > editorInfoList = > mRule.getInputMethodManagerWrapper().getEditorInfoList(); > Assert.assertEquals(6, editorInfoList.size()); > // search1. > Assert.assertEquals(EditorInfo.IME_ACTION_SEARCH, > getImeAction(editorInfoList.get(0))); > // content_editable1. > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, > getImeAction(editorInfoList.get(1))); > // input_number2. > Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, > getImeAction(editorInfoList.get(2))); > // input_number1. > Assert.assertEquals(EditorInfo.IME_ACTION_NEXT, > getImeAction(editorInfoList.get(3))); > // textarea2. > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, > getImeAction(editorInfoList.get(4))); > // textarea. > Assert.assertEquals(EditorInfo.IME_ACTION_NONE, > getImeAction(editorInfoList.get(5))); > } Done. Thanks for the detailed code. https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java (right): https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:42: private int mImeOptions; On 2017/05/11 22:57:13, Changwan Ryu wrote: > private final ArrayList<EditorInfo> mEditorInfoList = new ArrayList<>(); Done. https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:43: private int mInputType; On 2017/05/11 22:57:13, Changwan Ryu wrote: > not needed at all. please remove all references. Done. https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:58: mImeOptions = editorInfo.imeOptions; On 2017/05/11 22:57:13, Changwan Ryu wrote: > mEditorInfoList.add(editorInfo); Done. https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:70: mImeOptions = editorInfo.imeOptions; On 2017/05/11 22:57:13, Changwan Ryu wrote: > mEditorInfoList.add(editorInfo); Done. https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:146: mUpdateSelectionList.clear(); On 2017/05/11 22:57:13, Changwan Ryu wrote: > Please add > mEditorInfoList.clear(); Done. https://codereview.chromium.org/2839993002/diff/220001/content/public/test/an... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java:177: public int getImeOptions() { On 2017/05/11 22:57:13, Changwan Ryu wrote: > public ArrayList<EditorInfo> getEditorInfoList() { > return mEditorInfoList; > } Done. https://codereview.chromium.org/2839993002/diff/220001/content/test/data/andr... File content/test/data/android/input/input_forms.html (right): https://codereview.chromium.org/2839993002/diff/220001/content/test/data/andr... content/test/data/android/input/input_forms.html:9: <div id="contenteditable1" contenteditable>contenteditable1 from form1</div> On 2017/05/11 22:57:13, Changwan Ryu wrote: > remove Done. https://codereview.chromium.org/2839993002/diff/220001/content/test/data/andr... content/test/data/android/input/input_forms.html:12: <form id="form2"> On 2017/05/11 22:57:13, Changwan Ryu wrote: > remove id Done. https://codereview.chromium.org/2839993002/diff/220001/content/test/data/andr... content/test/data/android/input/input_forms.html:25: <div id="contenteditable2" contenteditable>contenteditable2 from form2</div> On 2017/05/11 22:57:13, Changwan Ryu wrote: > <div id="contenteditable1" contenteditable>contenteditable1</div> > <input id="search1" type="search" size="10" /> > <input id="input_text1" type="text" size="10" /> Done. https://codereview.chromium.org/2839993002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2839993002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebViewTest.cpp:1629: MoveFocusToNextFocusableElementInFormWithKeyEventListenersAndNonEditableElements) { On 2017/05/11 22:57:13, Changwan Ryu wrote: > This test is not strong enough. > You need to actually call FocusController::NextFocusableElementInForm() and > check the values against previous / next element's ID. > > For example, even if contenteditable1 were not detected as next focusable > element, the test would still pass because of the elements that are located > further away. > > Also, as I suggested earlier, > please try to add the failing cases from ImeTest to WebViewTest (type=number). Done. https://codereview.chromium.org/2839993002/diff/240001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeActivityTestRule.java (right): https://codereview.chromium.org/2839993002/diff/240001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeActivityTestRule.java:573: return mFactory.initializeAndGet(view, imeAdapter, inputType, inputFlags, inputMode, I noticed a param miss match here and corrected.
lgtm https://codereview.chromium.org/2839993002/diff/240001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeActivityTestRule.java (right): https://codereview.chromium.org/2839993002/diff/240001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeActivityTestRule.java:573: return mFactory.initializeAndGet(view, imeAdapter, inputType, inputFlags, inputMode, On 2017/05/12 13:26:25, AKVT wrote: > I noticed a param miss match here and corrected. Good catch! Thanks for fixing this.
@nasko and @dcheng - PTAL content/common/frame_messages.h changes as it needs SECURITY_OWNERS stamping. @tkent - PTAL third_party/WebKit/* changes.
I think nasko already approved, but ipc LGTM as well. https://codereview.chromium.org/2839993002/diff/240001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeActivityTestRule.java (right): https://codereview.chromium.org/2839993002/diff/240001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/input/ImeActivityTestRule.java:573: return mFactory.initializeAndGet(view, imeAdapter, inputType, inputFlags, inputMode, On 2017/05/12 22:22:02, Changwan Ryu wrote: > On 2017/05/12 13:26:25, AKVT wrote: > > I noticed a param miss match here and corrected. > > Good catch! Thanks for fixing this. FWIW, might be good to see if we can make this more typesafe in the future, seems like a pretty easy mistake to make.
The previous CL was reverted due to a power regression [1]. Does this CL address the power issue? [1] https://bugs.chromium.org/p/chromium/issues/detail?id=410785#c36 https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:1102: // TODO(ajith.v) Extend it for Select element, Radio button and Check Select -> select Radio -> radio Check boxes -> check box (or "select elements, radio buttons, and check boxes") https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:547: // Shift + TAB. (Will be extended to other form Controls like Select element Controls -> controls Select -> select https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:548: // , Checkbox, Radio etc.) Move "," to the end of the previous line. Checkbox -> checkbox Radio -> radio
PTAL! https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:1102: // TODO(ajith.v) Extend it for Select element, Radio button and Check On 2017/05/14 23:24:58, tkent wrote: > Select -> select > Radio -> radio > Check boxes -> check box (or "select elements, radio buttons, and check boxes") Done. https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:547: // Shift + TAB. (Will be extended to other form Controls like Select element On 2017/05/14 23:24:59, tkent wrote: > Controls -> controls > Select -> select Done. https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:548: // , Checkbox, Radio etc.) On 2017/05/14 23:24:59, tkent wrote: > Move "," to the end of the previous line. > > Checkbox -> checkbox > Radio -> radio Done.
On 2017/05/14 23:24:59, tkent wrote: > The previous CL was reverted due to a power regression [1]. Does this CL > address the power issue? > I am trying to execute power related test locally in Google Pixel. I will update you after execution. If any other easy way, we can verify power issues, (may be using bots or some other manner), please let me know. > > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=410785#c36 > > https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/page/FocusController.cpp (right): > > https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/page/FocusController.cpp:1102: // TODO(ajith.v) > Extend it for Select element, Radio button and Check > Select -> select > Radio -> radio > Check boxes -> check box (or "select elements, radio buttons, and check boxes") > > https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/pub... > File third_party/WebKit/public/web/WebLocalFrame.h (right): > > https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/pub... > third_party/WebKit/public/web/WebLocalFrame.h:547: // Shift + TAB. (Will be > extended to other form Controls like Select element > Controls -> controls > Select -> select > > https://codereview.chromium.org/2839993002/diff/240001/third_party/WebKit/pub... > third_party/WebKit/public/web/WebLocalFrame.h:548: // , Checkbox, Radio etc.) > Move "," to the end of the previous line. > > Checkbox -> checkbox > Radio -> radio
@tkent & @kochi - I initiated try jobs for measuring power.android_acceptance in android port. Please help me to interpret if any issues observed after execution.
Description was changed from ========== Adding support for Smart GO NEXT feature in Android Chrome Smart Go/Next brings better user experience to the user during form submit applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ========== to ========== [Android] Adding Smart GO/NEXT feature in Chrome Smart Go/Next brings better user experience to the user during form submitting applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. This will avoid unnecessary form submissions before filling or visiting all fields in the form. Additionally it will save user time and avoid redundant network requests before actually filling/attending entire fields in the form Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ==========
Description was changed from ========== [Android] Adding Smart GO/NEXT feature in Chrome Smart Go/Next brings better user experience to the user during form submitting applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. This will avoid unnecessary form submissions before filling or visiting all fields in the form. Additionally it will save user time and avoid redundant network requests before actually filling/attending entire fields in the form Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ========== to ========== [Android] Adding Smart GO/NEXT feature in Chrome Smart Go/Next brings better user experience to the user during form submitting applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. This will avoid unnecessary form submissions before filling or visiting all fields in the form. Additionally it will save user time and avoid redundant network requests before actually filling/attending entire fields in the form Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ==========
Description was changed from ========== [Android] Adding Smart GO/NEXT feature in Chrome Smart Go/Next brings better user experience to the user during form submitting applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. This will avoid unnecessary form submissions before filling or visiting all fields in the form. Additionally it will save user time and avoid redundant network requests before actually filling/attending entire fields in the form Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ========== to ========== [Android] Adding Smart GO/NEXT feature in Chrome Smart Go/Next brings better user experience to the user during form submitting applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. This will avoid unnecessary form submissions before filling or visiting all fields in the form. Additionally it will save user time and avoid redundant network requests before actually filling/attending entire fields in the form Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ==========
Description was changed from ========== [Android] Adding Smart GO/NEXT feature in Chrome Smart Go/Next brings better user experience to the user during form submitting applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. This will avoid unnecessary form submissions before filling or visiting all fields in the form. Additionally it will save user time and avoid redundant network requests before actually filling/attending entire fields in the form Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ========== to ========== [Android] Adding Smart GO/NEXT feature in Chrome Smart Go/Next brings better user experience to the user during form submitting applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. This will avoid unnecessary form submissions before filling or visiting all fields in the form. Additionally it will save user time and avoid redundant network requests before actually filling/attending entire fields in the form Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ==========
On 2017/05/15 13:06:14, AKVT wrote: > @tkent & @kochi - I initiated try jobs for measuring power.android_acceptance in > android port. Please help me to interpret if any issues observed after > execution. Just talked to kochi@. AKVT, could you run a telemetry test? We're not sure whether green results from bisect bots necessarily mean it will pass power regression tests or not when you land the change. The instruction has changed recently, but it should work similarly: https://sites.google.com/a/chromium.org/dev/developers/telemetry/run_locally The result should look like this: http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-07... I'd recommend the following tests: page_recycler.typical_25 power.top25 The logic applies to all the platforms, so I think running generic tests should still work. If I understand correctly, this may affect the power consumption only when input is activated. I am not sure if the above tests can cover activated input case correctly or not, but you can run more tests if you feel that it can cover more of those cases.
On 2017/05/16 04:56:06, Changwan Ryu wrote: > On 2017/05/15 13:06:14, AKVT wrote: > > @tkent & @kochi - I initiated try jobs for measuring power.android_acceptance > in > > android port. Please help me to interpret if any issues observed after > > execution. > > Just talked to kochi@. AKVT, could you run a telemetry test? We're not sure > whether green results from bisect bots necessarily mean > it will pass power regression tests or not when you land the change. > > The instruction has changed recently, but it should work similarly: > https://sites.google.com/a/chromium.org/dev/developers/telemetry/run_locally > > The result should look like this: > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-07... > > I'd recommend the following tests: > > page_recycler.typical_25 > power.top25 > > The logic applies to all the platforms, so I think running generic tests should > still work. > > If I understand correctly, this may affect the power consumption only when input > is activated. > I am not sure if the above tests can cover activated input case correctly or > not, but you can run more tests if you feel that it can cover more of those > cases. @changwan - I initiated power.top_25, page_cycler_v2.typical_25, power.typical_10_mobile, power.trivial_pages tests in trybot now.
@changwan, @kochi and @tkent - Suggested test run is completed by bot now. Most of them are Green. Could you comment your opinion based on results ?
On 2017/05/17 06:48:03, AKVT wrote: > @changwan, @kochi and @tkent - Suggested test run is completed by bot now. Most > of them are Green. > Could you comment your opinion based on results ? could you point us to the telemetry result? what exact command did you run? I couldn't find any test result from your buildbot runs.
On 2017/05/17 14:29:50, Changwan Ryu wrote: > On 2017/05/17 06:48:03, AKVT wrote: > > @changwan, @kochi and @tkent - Suggested test run is completed by bot now. > Most > > of them are Green. > > Could you comment your opinion based on results ? > > could you point us to the telemetry result? what exact command did you run? I > couldn't find any test result from your buildbot runs. I have run following commands; tools/perf/run_benchmark trybot all power.top_25 tools/perf/run_benchmark trybot all page_cycler_v2.typical_25 tools/perf/run_benchmark trybot all power.typical_10_mobile tools/perf/run_benchmark trybot all power.trivial_pages Prior to this before your suggestion of power.top_25, I have ran following command as well; tools/perf/run_benchmark trybot all-android power.android_acceptance
On 2017/05/17 14:41:04, AKVT wrote: > On 2017/05/17 14:29:50, Changwan Ryu wrote: > > On 2017/05/17 06:48:03, AKVT wrote: > > > @changwan, @kochi and @tkent - Suggested test run is completed by bot now. > > Most > > > of them are Green. > > > Could you comment your opinion based on results ? > > > > could you point us to the telemetry result? what exact command did you run? I > > couldn't find any test result from your buildbot runs. > > I have run following commands; > > tools/perf/run_benchmark trybot all power.top_25 > tools/perf/run_benchmark trybot all page_cycler_v2.typical_25 > tools/perf/run_benchmark trybot all power.typical_10_mobile > tools/perf/run_benchmark trybot all power.trivial_pages > > Prior to this before your suggestion of power.top_25, I have ran following > command as well; > > tools/perf/run_benchmark trybot all-android power.android_acceptance I couldn't find results from the telemetry trybot runs. Also, I tried running it separately, but to no avail: https://codereview.chromium.org/2891763002/ Overall, I could not find a good way to test power regression. tkent@, would it be ok to let this land and have it reverted in case of an issue?
> tkent@, would it be ok to let this land and have it reverted in case of an issue? ok, let's try. lgtm
The CQ bit was checked by ajith.v@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from ekaramad@chromium.org, nasko@chromium.org, yosin@chromium.org, kochi@chromium.org, changwan@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2839993002/#ps280001 (title: "Rebased the patch for running android_fyi_perf bot.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/05/19 04:28:04, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Build break was due to upstream change of WebViewImpl -> WebViewBase. Same has been incorporated in WebViewTest.cpp
The CQ bit was checked by ajith.v@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from ekaramad@chromium.org, tkent@chromium.org, changwan@chromium.org, dcheng@chromium.org, kochi@chromium.org, nasko@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2839993002/#ps300001 (title: "Fixed the build break by replacing WebViewImpl object with WebViewBase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by aelias@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
@changwan - After rebase I saw testShowAndHideSoftInput is failing, so I preserved resetUpdateSelectionList() method. PTAL!
On 2017/05/30 14:54:46, AKVT wrote: > @changwan - After rebase I saw testShowAndHideSoftInput is failing, so I > preserved resetUpdateSelectionList() method. > > PTAL! Could you try again? The failure is probably orthogonal to your change, and got fixed at https://chromium-review.googlesource.com/517986.
On 2017/05/30 20:04:08, Changwan Ryu wrote: > On 2017/05/30 14:54:46, AKVT wrote: > > @changwan - After rebase I saw testShowAndHideSoftInput is failing, so I > > preserved resetUpdateSelectionList() method. > > > > PTAL! > > Could you try again? The failure is probably orthogonal to your change, and got > fixed at https://chromium-review.googlesource.com/517986. @changwan I just took TOT and applied this patch and still I found problem in testShowAndHideSoftInput due to my change (replacement of resetUpdateSelectionList with resetAllStates). So I untouched that part now. I am initiating the previously failed trybot for verification once again.
Unfortunately, none of the failures are not able to reproduce locally. ImeTest, AutoFillPopupTest etc. all are passing on Google Pixel with N OS and O OS. Layout test is not able to execute in Android device due to time out. I am not sure layout test is not possible to run on Android device now, and can only run on Linux build. BTW, I am setting linux build for verification of failed tests.
@kochi & @changwan - PTAL. I have removed a DCHECK from Element.cpp for solving launch time crash of Bookmarks test and it's cause of most of the failures in the build bots.
@changwan - probably you are better at looking at this recent changes. could you take a look?
PTAL - PS23 I have fixed the flaky failures in ImeTest.
I have hard time diff'ing patch 22 and patch 23 for the change you made. Could you explain how you fixed flakiness of ImeTest? https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2897: !GetDocument().NeedsLayoutTreeUpdateForNode(*this)); yosin@ / kochi@ may have more knowledge, but I don't think you should remove this DCHECK. Could you first pinpoint which one is broken? If it's GetDocument().NeedsLayoutTreeUpdateForNode() that is returning false, then probably you need to call GetDocument().UpdateStyleAndLayoutTree() somewhere.
https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:1072: WebFocusType focus_type) { could you try adding element->GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets(); at the beginning of this function?
On 2017/06/09 15:40:04, Changwan Ryu wrote: > I have hard time diff'ing patch 22 and patch 23 for the change you made. Could > you explain how you fixed flakiness of ImeTest? > > https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Element.cpp (left): > > https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Element.cpp:2897: > !GetDocument().NeedsLayoutTreeUpdateForNode(*this)); > yosin@ / kochi@ may have more knowledge, but I don't think you should remove > this DCHECK. Could you first pinpoint which one is broken? > If it's GetDocument().NeedsLayoutTreeUpdateForNode() that is returning false, > then probably you need to call GetDocument().UpdateStyleAndLayoutTree() > somewhere. Since i have added additional html elements, in that test html, Domutils is not working for focusing input_radio because it is going behind the text selection popup. So i had moved it at the end for easy access using Domutils
On 2017/06/09 15:40:04, Changwan Ryu wrote: > I have hard time diff'ing patch 22 and patch 23 for the change you made. Could > you explain how you fixed flakiness of ImeTest? > > https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Element.cpp (left): > > https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Element.cpp:2897: > !GetDocument().NeedsLayoutTreeUpdateForNode(*this)); > yosin@ / kochi@ may have more knowledge, but I don't think you should remove > this DCHECK. Could you first pinpoint which one is broken? > If it's GetDocument().NeedsLayoutTreeUpdateForNode() that is returning false, > then probably you need to call GetDocument().UpdateStyleAndLayoutTree() > somewhere. Since i have added additional html elements, in that test html, Domutils is not working for focusing input_radio because it is going behind the text selection popup. So i had moved it at the end for easy access using Domutils
On 2017/06/09 15:40:04, Changwan Ryu wrote: > I have hard time diff'ing patch 22 and patch 23 for the change you made. Could > you explain how you fixed flakiness of ImeTest? > > https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Element.cpp (left): > > https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Element.cpp:2897: > !GetDocument().NeedsLayoutTreeUpdateForNode(*this)); > yosin@ / kochi@ may have more knowledge, but I don't think you should remove > this DCHECK. Could you first pinpoint which one is broken? > If it's GetDocument().NeedsLayoutTreeUpdateForNode() that is returning false, > then probably you need to call GetDocument().UpdateStyleAndLayoutTree() > somewhere. Since i have added additional html elements, in that test html, Domutils is not working for focusing input_radio because it is going behind the text selection popup. So i had moved it at the end for easy access using Domutils
On 2017/06/09 15:40:04, Changwan Ryu wrote: > I have hard time diff'ing patch 22 and patch 23 for the change you made. Could > you explain how you fixed flakiness of ImeTest? > > https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Element.cpp (left): > > https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Element.cpp:2897: > !GetDocument().NeedsLayoutTreeUpdateForNode(*this)); > yosin@ / kochi@ may have more knowledge, but I don't think you should remove > this DCHECK. Could you first pinpoint which one is broken? > If it's GetDocument().NeedsLayoutTreeUpdateForNode() that is returning false, > then probably you need to call GetDocument().UpdateStyleAndLayoutTree() > somewhere. Since i have added additional html elements, in that test html, Domutils is not working for focusing input_radio because it is going behind the text selection popup. So i had moved it at the end for easy access using Domutils
On 2017/06/09 15:40:04, Changwan Ryu wrote: > I have hard time diff'ing patch 22 and patch 23 for the change you made. Could > you explain how you fixed flakiness of ImeTest? > > https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Element.cpp (left): > > https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Element.cpp:2897: > !GetDocument().NeedsLayoutTreeUpdateForNode(*this)); > yosin@ / kochi@ may have more knowledge, but I don't think you should remove > this DCHECK. Could you first pinpoint which one is broken? > If it's GetDocument().NeedsLayoutTreeUpdateForNode() that is returning false, > then probably you need to call GetDocument().UpdateStyleAndLayoutTree() > somewhere. Since i have added additional html elements, in that test html, Domutils is not working for focusing input_radio because it is going behind the text selection popup. So i had moved it at the end for easy access using Domutils
@changwan - i will try to call UpdateStyleAndLayoutIgnorePendingStylesheets() and share result in next patch. Thank you for your help https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:1072: WebFocusType focus_type) { On 2017/06/09 19:34:30, Changwan Ryu wrote: > could you try adding > element->GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets(); > at the beginning of this function? I had tried to call NeedsLayoutTreeUpdateForNode earlier. But that didn't help to pass BookmarkTest*
PTAL! https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (left): https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:2897: !GetDocument().NeedsLayoutTreeUpdateForNode(*this)); On 2017/06/09 15:40:04, Changwan Ryu wrote: > yosin@ / kochi@ may have more knowledge, but I don't think you should remove > this DCHECK. Could you first pinpoint which one is broken? > If it's GetDocument().NeedsLayoutTreeUpdateForNode() that is returning false, > then probably you need to call GetDocument().UpdateStyleAndLayoutTree() > somewhere. Done. https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2839993002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/FocusController.cpp:1072: WebFocusType focus_type) { On 2017/06/09 19:34:30, Changwan Ryu wrote: > could you try adding > element->GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets(); > at the beginning of this function? Done. Thank you.
https://codereview.chromium.org/2839993002/diff/460001/content/test/data/andr... File content/test/data/android/input/input_forms.html (right): https://codereview.chromium.org/2839993002/diff/460001/content/test/data/andr... content/test/data/android/input/input_forms.html:25: <input id="input_radio" type="radio" style="width:50px;height:50px" /><br/> could you explain why and how this fixes the flakiness that you mentioned in patch 23?
@changwan - PTAL https://codereview.chromium.org/2839993002/diff/460001/content/test/data/andr... File content/test/data/android/input/input_forms.html (right): https://codereview.chromium.org/2839993002/diff/460001/content/test/data/andr... content/test/data/android/input/input_forms.html:25: <input id="input_radio" type="radio" style="width:50px;height:50px" /><br/> On 2017/06/12 20:47:24, Changwan Ryu wrote: > could you explain why and how this fixes the flakiness that you mentioned in > patch 23? testSelectActionBarClearedOnTappingOutsideInput() second part is clicking on the input_radio while text selection and selection popup is active on input_text element. The Popup is very close to input_radio element, so while processing DomUtils.clickNode(), it's not able to focus exactly that element. Also in that case, click is happening to some input element in the bottom of the page and IME is not dismissing, so test goes flaky, and I observed this visual difference on Google Pixel device, where test is not passing in first attempt, but after maximum attempts, it's passing. Then I had a thought to keep at somewhere down, where no way popup can interfere with element, and the issue got fixed. But I am little clueless, why this issue only happened after I added 4 elements in this test page. I am doubting it's an issue with DomUtils in clickNode() operation in finding proper element.
lgtm https://codereview.chromium.org/2839993002/diff/460001/content/test/data/andr... File content/test/data/android/input/input_forms.html (right): https://codereview.chromium.org/2839993002/diff/460001/content/test/data/andr... content/test/data/android/input/input_forms.html:25: <input id="input_radio" type="radio" style="width:50px;height:50px" /><br/> On 2017/06/13 06:41:49, AKVT wrote: > On 2017/06/12 20:47:24, Changwan Ryu wrote: > > could you explain why and how this fixes the flakiness that you mentioned in > > patch 23? > > testSelectActionBarClearedOnTappingOutsideInput() second part is clicking on the > input_radio while text selection and selection popup is active on input_text > element. The Popup is very close to input_radio element, so while processing > DomUtils.clickNode(), it's not able to focus exactly that element. Also in that > case, click is happening to some input element in the bottom of the page and IME > is not dismissing, so test goes flaky, and I observed this visual difference on > Google Pixel device, where test is not passing in first attempt, but after > maximum attempts, it's passing. Then I had a thought to keep at somewhere down, > where no way popup can interfere with element, and the issue got fixed. > But I am little clueless, why this issue only happened after I added 4 elements > in this test page. I am doubting it's an issue with DomUtils in clickNode() > operation in finding proper element. I see. In case things don't work out, you can also try reducing the number of rows of the textarea and or adding another radio button at the top.
The CQ bit was checked by ajith.v@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from ekaramad@chromium.org, tkent@chromium.org, dcheng@chromium.org, kochi@chromium.org, nasko@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2839993002/#ps460001 (title: "Preserved the DCHECK and fixed browser_tests failure")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/06/13 15:43:27, Changwan Ryu wrote: > lgtm > > https://codereview.chromium.org/2839993002/diff/460001/content/test/data/andr... > File content/test/data/android/input/input_forms.html (right): > > https://codereview.chromium.org/2839993002/diff/460001/content/test/data/andr... > content/test/data/android/input/input_forms.html:25: <input id="input_radio" > type="radio" style="width:50px;height:50px" /><br/> > On 2017/06/13 06:41:49, AKVT wrote: > > On 2017/06/12 20:47:24, Changwan Ryu wrote: > > > could you explain why and how this fixes the flakiness that you mentioned in > > > patch 23? > > > > testSelectActionBarClearedOnTappingOutsideInput() second part is clicking on > the > > input_radio while text selection and selection popup is active on input_text > > element. The Popup is very close to input_radio element, so while processing > > DomUtils.clickNode(), it's not able to focus exactly that element. Also in > that > > case, click is happening to some input element in the bottom of the page and > IME > > is not dismissing, so test goes flaky, and I observed this visual difference > on > > Google Pixel device, where test is not passing in first attempt, but after > > maximum attempts, it's passing. Then I had a thought to keep at somewhere > down, > > where no way popup can interfere with element, and the issue got fixed. > > But I am little clueless, why this issue only happened after I added 4 > elements > > in this test page. I am doubting it's an issue with DomUtils in clickNode() > > operation in finding proper element. > > I see. In case things don't work out, you can also try reducing the number of > rows of the textarea and or adding another radio button at the top. Thank you changwan, I will try above tips for fixing similar issues in the future.
The CQ bit was checked by ajith.v@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from ekaramad@chromium.org, tkent@chromium.org, changwan@chromium.org, dcheng@chromium.org, kochi@chromium.org, nasko@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2839993002/#ps480001 (title: "Rebased the patch")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ajith.v@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from ekaramad@chromium.org, tkent@chromium.org, changwan@chromium.org, dcheng@chromium.org, kochi@chromium.org, nasko@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2839993002/#ps500001 (title: "Rebased the patch from TOT")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1497382504908830,
"parent_rev": "294e7bd43741b116595998bbd43ad99ebe89e80d", "commit_rev":
"8d80ad1410dcb23f55c323a97e57eed750a611dc"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Adding Smart GO/NEXT feature in Chrome Smart Go/Next brings better user experience to the user during form submitting applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. This will avoid unnecessary form submissions before filling or visiting all fields in the form. Additionally it will save user time and avoid redundant network requests before actually filling/attending entire fields in the form Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 ========== to ========== [Android] Adding Smart GO/NEXT feature in Chrome Smart Go/Next brings better user experience to the user during form submitting applications. For navigating between form elements, user can use NEXT/PREVIOUS button from IME without touching on individual fields. This will avoid unnecessary form submissions before filling or visiting all fields in the form. Additionally it will save user time and avoid redundant network requests before actually filling/attending entire fields in the form Design Document: https://docs.google.com/document/d/1h0diigZ8LUi7A3UKJ_zwNUbvNQoe-Nwr55_p6ivSP... BUG=410785,648986 Review-Url: https://codereview.chromium.org/2839993002 Cr-Commit-Position: refs/heads/master@{#479126} Committed: https://chromium.googlesource.com/chromium/src/+/8d80ad1410dcb23f55c323a97e57... ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001) as https://chromium.googlesource.com/chromium/src/+/8d80ad1410dcb23f55c323a97e57...
Message was sent while issue was closed.
A revert of this CL (patchset #26 id:500001) has been created in https://codereview.chromium.org/2938123002/ by kochi@chromium.org. The reason for reverting is: This caused several clusterfuzz bugs (733197, 733218, 733282) - let me revert for investigation on those.. |
