Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
236733006:
Allow the user to tap the selected editable again after rotation to trigger another zoom/center anim (Closed)
|
Created:
6 years, 8 months ago by samsung13630592 Modified:
6 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, kbalazs, Inactive, AviD, AKVT Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionallow the user to tap the selected editable again after rotation to trigger
another zoom/center animation.
On device rotation, the value of has_scrolled_focused_editable_node_into_rect_ was not getting reset on orientation change.
Added code to handle this.
BUG=363059
Patch Set 1 #
Total comments: 4
Patch Set 2 : Made changes according to review comments #
Total comments: 1
Patch Set 3 : PTAL modified according to review comments #Messages
Total messages: 16 (0 generated)
PTAL
https://codereview.chromium.org/236733006/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/236733006/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1632: scrollFocusedEditableNodeIntoView(); But what if the user has explicitly scrolled away from the focused editable? We have some viewport anchoring logic in blink (see https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), but it looks like the orientation resize is different from the keyboard resize (it's a shame those can't be synchronized, I'll file a bug...). We should really only send the focus request if the editable is still visible, or perhaps don't send it if the user has interacted with the page via touch (similar to cancelRequestToScrollFocusedEditableNodeIntoView())?
If my understanding is not right, Please correct me with the expected behavior. https://codereview.chromium.org/236733006/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/236733006/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1632: scrollFocusedEditableNodeIntoView(); Currently when user inputs some text in edit box and Keypad is still in composing mode then on rotation edit box comes into view even if user has explicitly scrolled the node out of view. Assuming that this scenario is correct, focussed editable node should be scrolled into view irrespective of the below scenario: 1. Keypad is in composing mode or not, and 2. Text is present in editable node or not. When Keypad is up it is assumed that user wants to input some text into focused editbox. If user doesnot want Keypad to show he/she touches anywhere else on screen which will trigger Keypad hide.
aelias@: It seems wrong to bring the focused editable into view upon rotation if the user has explicitly moved it out of view, but I don't really have a strong opinion here, wdyt? Ideally we could do this all atomically in WebViewImpl as part of the anchoring code, but from crbug.com/363182 we see that the orientation resize does not include the OSK visibility. https://codereview.chromium.org/236733006/diff/1/content/renderer/render_view... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/236733006/diff/1/content/renderer/render_view... content/renderer/render_view_impl.cc:3353: is_orientation_changed_ = true; Why not just set |has_scrolled_focused_editable_node_into_rect_| as false here?
I agree with Jared that it would be better to fix the existing anchoring logic than make this change. This change makes the rotation behavior even more complex and slow than it was before, and we should be aiming to simplify it instead.
https://codereview.chromium.org/236733006/diff/1/content/renderer/render_view... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/236733006/diff/1/content/renderer/render_view... content/renderer/render_view_impl.cc:3353: is_orientation_changed_ = true; On 2014/04/15 14:36:00, jdduke wrote: > Why not just set |has_scrolled_focused_editable_node_into_rect_| as false here? Until we have a solution to the multiple resize problem (crbug.com/363182), let's go ahead and just set |has_scrolled_focused_editable_node_into_rect_ = false| here. Then at least, if the user taps the editable again, it will scroll and zoom the editable into view properly. The rest of the changes won't be necessary.
PTAL Made changes according to review comments
https://codereview.chromium.org/236733006/diff/10001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/236733006/diff/10001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1629: if (mSelectionEditable) { I don't think this will actually fix anything. As noted previously, we're getting multiple resize events after the display is rotated while the OSK is visible. Because of this, only the first call to | scrollFocusedEditableNodeIntoView();| after the resize will have any effect, and that will occur *before* the OSK is actually factored into the size.
Are you sure you wanted me as a reviewer? Perhaps you meant jeremya@ ?
On 2014/04/22 08:10:50, jeremy wrote: > Are you sure you wanted me as a reviewer? Perhaps you meant jeremya@ ? I found 'per-file *.sb=jeremy@chromium.org' inside owners file in content/renderer folder. So I wanted you as a reviewer.
PTAL
lgtm but you'll need an owner.
Would it be possible to have an automated test for this ?
On 2014/04/29 23:40:46, lgombos wrote: > Would it be possible to have an automated test for this ? Actually, the CL description is no longer accurate. All this patch does now is allow the user to tap the selected editable again after rotation to trigger another zoom/center animation. Please update the description accordingly.
Message was sent while issue was closed.
Closed the issue because the account used to upload the patch was not correct. Raised a new review request for the same changes at https://codereview.chromium.org/268753003/. PTAL new patch set. |
