Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
312293002:
Paste popup is positioning properly during content scroll. (Closed)
|
Created:
6 years, 6 months ago by AKVT Modified:
6 years, 4 months ago Reviewers:
jdduke (slow) CC:
chromium-reviews, darin-cc_chromium.org, jam, avi.nitk, Cyan, ankit, raghu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionProperly reposition the paste popup after a resize
Normally, the paste popup is hidden when its position changes. However, there
are legitimate cases outside of scrolling where the position may change, e.g.,
due to a resize, where the popup should remain visible. Handle this case by
only hiding the popup if its position changes while scrolling.
BUG=380084
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288404
Patch Set 1 #Patch Set 2 : Rebased the patch based on new design change on text selection. #
Total comments: 2
Patch Set 3 : Taken care hiding the handles when dragging insertion handle. #Patch Set 4 : Optimized the condition checks #
Total comments: 8
Patch Set 5 : Removed extra hiding call and added repositioning code to fix the position #Patch Set 6 : Added braces as per coding standard. #
Total comments: 2
Patch Set 7 : Added unittests for the newly added event. #
Total comments: 4
Patch Set 8 : Fixed review comments and CL description issues. #Patch Set 9 : Fixed Unittest failure issues. #
Total comments: 8
Patch Set 10 : Fixed SELECTION_CLEARED unit test case failure and unit test case function semantic change issues. #Patch Set 11 : Removed reduntant condition check #
Total comments: 4
Patch Set 12 : Rebased the patch to align dependent patches. #
Total comments: 6
Patch Set 13 : Fixed review comments. #
Total comments: 6
Patch Set 14 : Fixed review comments and line breaks. #
Messages
Total messages: 59 (0 generated)
PTAL this change
+yfriedman and tedchoc for this change. I am pretty sure that exposing ContentViewCore is not ok. Yaron?
On 2014/06/05 15:57:20, aurimas wrote: > +yfriedman and tedchoc for this change. I am pretty sure that exposing > ContentViewCore is not ok. Yaron? I've never liked how HandleView is aware of the paste popup... ideally it would be known only to the InsertionHandleController, and we should definitely not be exposing ContentViewCore to the HandleView.
On 2014/06/05 15:59:32, jdduke wrote: > On 2014/06/05 15:57:20, aurimas wrote: > > +yfriedman and tedchoc for this change. I am pretty sure that exposing > > ContentViewCore is not ok. Yaron? > > I've never liked how HandleView is aware of the paste popup... ideally it would > be known only to the InsertionHandleController, and we should definitely not be > exposing ContentViewCore to the HandleView. Yep, CV should not expose CVC. The cast of getParent is already alarming enough and wouldn't work if this code is used in WebView (don't know, but we're in the content layer).
On 2014/06/05 16:55:46, Yaron wrote: > On 2014/06/05 15:59:32, jdduke wrote: > > On 2014/06/05 15:57:20, aurimas wrote: > > > +yfriedman and tedchoc for this change. I am pretty sure that exposing > > > ContentViewCore is not ok. Yaron? > > > > I've never liked how HandleView is aware of the paste popup... ideally it > would > > be known only to the InsertionHandleController, and we should definitely not > be > > exposing ContentViewCore to the HandleView. > > Yep, CV should not expose CVC. The cast of getParent is already alarming enough > and wouldn't work if this code is used in WebView (don't know, but we're in the > content layer). Basically, I wanted to know the scrolling is a touch scroll or resize based scroll. For that I wanted to access ContentViewCore. Could you please suggest any other way we can get ContentViewCore access ? Can I keep ContentViewCore reference inside HandleView ?
On 2014/06/05 18:36:12, AJITH KUMAR V wrote: > On 2014/06/05 16:55:46, Yaron wrote: > > On 2014/06/05 15:59:32, jdduke wrote: > > > On 2014/06/05 15:57:20, aurimas wrote: > > > > +yfriedman and tedchoc for this change. I am pretty sure that exposing > > > > ContentViewCore is not ok. Yaron? > > > > > > I've never liked how HandleView is aware of the paste popup... ideally it > > would > > > be known only to the InsertionHandleController, and we should definitely not > > be > > > exposing ContentViewCore to the HandleView. > > > > Yep, CV should not expose CVC. The cast of getParent is already alarming > enough > > and wouldn't work if this code is used in WebView (don't know, but we're in > the > > content layer). > > Basically, I wanted to know the scrolling is a touch scroll or resize based > scroll. For that I wanted to access ContentViewCore. Could you please suggest > any other way we can get ContentViewCore access ? > Can I keep ContentViewCore reference inside HandleView ? Unfortunately I'm not really familiar with this so don't have a good suggestion. Jared indicated that it's not appropriate though
Ajith, I think we'll be in a better position to address this after the landing of https://codereview.chromium.org/300323005/. Once that clears, we'll be nearly to a point where we no longer need to hide the selection handles when the scroll offset changes, and we can begin cleaning up some of the other tangentially related logic. Feel free to star that patch to follow related progress.
On 2014/06/06 03:53:15, jdduke wrote: > Ajith, I think we'll be in a better position to address this after the landing > of https://codereview.chromium.org/300323005/. Once that clears, we'll be > nearly to a point where we no longer need to hide the selection handles when the > scroll offset changes, and we can begin cleaning up some of the other > tangentially related logic. Feel free to star that patch to follow related > progress. Thanks jdduke for the information. I will hold this change, until https://codereview.chromium.org/300323005/ landed.
On 2014/06/06 08:55:23, AKV wrote: > On 2014/06/06 03:53:15, jdduke wrote: > > Ajith, I think we'll be in a better position to address this after the landing > > of https://codereview.chromium.org/300323005/. Once that clears, we'll be > > nearly to a point where we no longer need to hide the selection handles when > the > > scroll offset changes, and we can begin cleaning up some of the other > > tangentially related logic. Feel free to star that patch to follow related > > progress. > > Thanks jdduke for the information. I will hold this change, until > https://codereview.chromium.org/300323005/ landed. jdduke@ is this patch no longer needed?
On 2014/07/11 05:38:13, aurimas wrote: > On 2014/06/06 08:55:23, AKV wrote: > > On 2014/06/06 03:53:15, jdduke wrote: > > > Ajith, I think we'll be in a better position to address this after the > landing > > > of https://codereview.chromium.org/300323005/. Once that clears, we'll be > > > nearly to a point where we no longer need to hide the selection handles when > > the > > > scroll offset changes, and we can begin cleaning up some of the other > > > tangentially related logic. Feel free to star that patch to follow related > > > progress. > > > > Thanks jdduke for the information. I will hold this change, until > > https://codereview.chromium.org/300323005/ landed. > > jdduke@ is this patch no longer needed? We'll need something to address the issue, but it will look quite different than the current version of the patch. The blocking patch is still in review.
@jdduke PTAL. Made changes based on new text selection logic.
https://codereview.chromium.org/312293002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2154: if (getPastePopup().isShowing() && !isScrollInProgress()) { What about the case where the insertion handle is dragged? We definitely want to hide it in that case. I suppose we could have an event |INSERTION_DRAG_BEGUN| that would trigger hiding?
@jdduke PTAL
PTAL https://codereview.chromium.org/312293002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2154: if (getPastePopup().isShowing() && !isScrollInProgress()) { On 2014/07/22 17:39:33, jdduke wrote: > What about the case where the insertion handle is dragged? We definitely want to > hide it in that case. I suppose we could have an event |INSERTION_DRAG_BEGUN| > that would trigger hiding? Done.
https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2155: mWasPastePopupShowing = getPastePopup().isShowing(); Hmm, I'd like to first land a change that suppresses dragging until the touch exceeds a particular slop region. That way, we'll either get a drag or a tap, but not both. Feel free to mark this issue as blocked on crbug.com/394093. I'll be on vacation for a couple days but I can work on that when I get back.
https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2155: mWasPastePopupShowing = getPastePopup().isShowing(); On 2014/07/23 16:54:58, jdduke wrote: > Hmm, I'd like to first land a change that suppresses dragging until the touch > exceeds a particular slop region. That way, we'll either get a drag or a tap, > but not both. Feel free to mark this issue as blocked on crbug.com/394093. I'll > be on vacation for a couple days but I can work on that when I get back. Thanks for the clarification. I will hold this change until bug 394093 is addressed.
On 2014/07/23 17:19:02, AKV wrote: > https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2155: > mWasPastePopupShowing = getPastePopup().isShowing(); > On 2014/07/23 16:54:58, jdduke wrote: > > Hmm, I'd like to first land a change that suppresses dragging until the touch > > exceeds a particular slop region. That way, we'll either get a drag or a tap, > > but not both. Feel free to mark this issue as blocked on crbug.com/394093. > I'll > > be on vacation for a couple days but I can work on that when I get back. > Thanks for the clarification. I will hold this change until bug 394093 is > addressed. @jdduke as I see addressing of crbug.com/394093 will take some more time. But I have taken care the logic of appropriately handling the popup show even if INSERTION_TAPPED, and INSERTION_MOVED comes together. Once crbug.com/394093 changes landed I can optimize the flag check and refine the patch further. So can we land current changes ? Please share your thoughts on this.
https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2161: hidePastePopup(); Can we avoid hiding then immediately showing? I think you can get around this by only hiding if the condition below does not hold.
@jdduke PTAL https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2161: hidePastePopup(); On 2014/07/28 15:51:07, jdduke wrote: > Can we avoid hiding then immediately showing? I think you can get around this by > only hiding if the condition below does not hold. We have to reposition the popup even when it was showing, to do that we have to hide the popup first, otherwise it's not repositioning properly. So to simplify below code; boolean isPastePopupShowing = getPastePopup().isShowing(); if (isPastePopupShowing && !isScrollInProgress()) { hidePastePopup(); showPastePopup((int) posXDip, (int) posYDip); } else { hidePastePopup(); } I taken out hidePastePopup(); as in the current patch and captured the status of visibility before hiding. Please suggest your opinion.
https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2161: hidePastePopup(); On 2014/07/28 16:56:58, AKV wrote: > On 2014/07/28 15:51:07, jdduke wrote: > > Can we avoid hiding then immediately showing? I think you can get around this > by > > only hiding if the condition below does not hold. > We have to reposition the popup even when it was showing, to do that we have to > hide the popup first, otherwise it's not repositioning properly. So to simplify That sounds like a bug to me. What if you delete the early return in PastePopupMenu.positionAt()?
@jdduke PTAL https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2161: hidePastePopup(); On 2014/07/28 17:26:28, jdduke wrote: > On 2014/07/28 16:56:58, AKV wrote: > > On 2014/07/28 15:51:07, jdduke wrote: > > > Can we avoid hiding then immediately showing? I think you can get around > this > > by > > > only hiding if the condition below does not hold. > > We have to reposition the popup even when it was showing, to do that we have > to > > hide the popup first, otherwise it's not repositioning properly. So to > simplify > > That sounds like a bug to me. What if you delete the early return in > PastePopupMenu.positionAt()? @jdduke Thanks for pointing the issue in positionAt() There is an issue in repositioning Popup window inside positionAt() function. PopupWindow.showAtLocation() is not working correctly when Popup is already in showing state. I have fixed it using PopupWindow.update(x, y, w, h) call when Popup is already in showing state. PTAL
On 2014/07/30 12:06:56, AKV wrote: > @jdduke Thanks for pointing the issue in positionAt() > There is an issue in repositioning Popup window inside positionAt() function. > PopupWindow.showAtLocation() is not working correctly when Popup is already in > showing state. I have fixed it using PopupWindow.update(x, y, w, h) call when > Popup is already in showing state. PTAL OK this is looking good but please update https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... to properly check for the new event type (you can just put EXPECT_EQ(INSERTION_DRAG_BEGUN, GetLastEventType()); where appropriate).
I think BEGUN is probably not the best name (my bad), maybe rename to INSERTION_DRAG_STARTED. https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:276: private boolean mWasPastePopupShowing = false; Nit: booleans always default to false in Java, no need to be explicit. https://codereview.chromium.org/312293002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:284: private boolean mWasPastePopupShowing = false; Maybe rename this to |mWasPastePopupShowingOnInsertionDragStart|?
@jdduke. I have fixed review comments. Additionally, pastePopup cordinates relatively depends on the insertion handle controller cordinates. I have prevented in hiding handles, when the last event in LONG_PRESS. PTAL https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:276: private boolean mWasPastePopupShowing = false; On 2014/07/30 17:39:09, jdduke wrote: > Nit: booleans always default to false in Java, no need to be explicit. Done. https://codereview.chromium.org/312293002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:284: private boolean mWasPastePopupShowing = false; On 2014/07/30 17:39:09, jdduke wrote: > Maybe rename this to |mWasPastePopupShowingOnInsertionDragStart|? Done.
Thanks, I think we're almost there, just a couple nits. https://codereview.chromium.org/312293002/diff/120001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/312293002/diff/120001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller_unittest.cc:380: EXPECT_EQ(INSERTION_DRAG_STARTED, GetLastEventType()); Thanks for adding this. One nit, add a colon ":" after the todo, so TODO(AKV) -> TODO(AKV):, then add a period at the end of the comment. https://codereview.chromium.org/312293002/diff/120001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/312293002/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:162: // Popup is showing first time Nit: No need for this comment, I think it's pretty clear from the conditional.
Also, please update the description contents per the email I just sent you.
@jdduke PTAL https://codereview.chromium.org/312293002/diff/120001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/312293002/diff/120001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller_unittest.cc:380: EXPECT_EQ(INSERTION_DRAG_STARTED, GetLastEventType()); On 2014/08/01 15:10:51, jdduke wrote: > Thanks for adding this. One nit, add a colon ":" after the todo, so TODO(AKV) > -> TODO(AKV):, then add a period at the end of the comment. Done. https://codereview.chromium.org/312293002/diff/120001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/312293002/diff/120001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:162: // Popup is showing first time On 2014/08/01 15:10:51, jdduke wrote: > Nit: No need for this comment, I think it's pretty clear from the conditional. Done.
lgtm, thanks. I updated the description slightly (again, state *what* the patch does on the first line of the description, not *why*).
On 2014/08/01 15:31:11, jdduke wrote: > lgtm, thanks. I updated the description slightly (again, state *what* the patch > does on the first line of the description, not *why*). Thanks jdduke. I will take care issues with patch description next time. @aurimas PTAL PastePopupMenu.java changes.
On 2014/08/01 15:35:25, AKV wrote: > On 2014/08/01 15:31:11, jdduke wrote: > > lgtm, thanks. I updated the description slightly (again, state *what* the > patch > > does on the first line of the description, not *why*). > > Thanks jdduke. I will take care issues with patch description next time. > @aurimas PTAL PastePopupMenu.java changes. I'm an owner for input-related changes in all of these files so no need for aurimas@ to review (unless he wants to :)
On 2014/08/01 15:37:44, jdduke wrote: > On 2014/08/01 15:35:25, AKV wrote: > > On 2014/08/01 15:31:11, jdduke wrote: > > > lgtm, thanks. I updated the description slightly (again, state *what* the > > patch > > > does on the first line of the description, not *why*). > > > > Thanks jdduke. I will take care issues with patch description next time. > > @aurimas PTAL PastePopupMenu.java changes. > > I'm an owner for input-related changes in all of these files so no need for > aurimas@ to review (unless he wants to :) Thanks jdduke for the information. I am landing it :)
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/312293002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by jdduke@chromium.org
On 2014/08/01 23:38:55, I haz the power (commit-bot) wrote: > FYI, CQ is re-trying this CL (attempt #1). > The failing builders are: > android_dbg_triggered_tests on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) > linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_rel_swarming on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > mac_chromium_rel on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) > win_chromium_rel on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) > win_chromium_x64_rel on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) Looks like a legit unit test failure.
On 2014/08/01 23:47:35, jdduke wrote: > On 2014/08/01 23:38:55, I haz the power (commit-bot) wrote: > > FYI, CQ is re-trying this CL (attempt #1). > > The failing builders are: > > android_dbg_triggered_tests on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) > > linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > linux_chromium_rel_swarming on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > mac_chromium_rel on tryserver.chromium.mac > > > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) > > win_chromium_rel on tryserver.chromium.win > > > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) > > win_chromium_x64_rel on tryserver.chromium.win > > > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > Looks like a legit unit test failure. @jdduke - After the addition of current code some flow on unit test case has been changed. I have fixed it by modifying existing unit test cases. Thank you
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/312293002/160001
The CQ bit was unchecked by jdduke@chromium.org
https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller_unittest.cc:94: controller().HideAndDisallowShowingAutomatically(); This is not OK, you just completely changed the semantics of this call. https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller_unittest.cc:324: EXPECT_EQ(SELECTION_CLEARED, GetLastEventType()); No need to check this it's just confusing.
@juddke Pls check my inline comment https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller_unittest.cc:94: controller().HideAndDisallowShowingAutomatically(); On 2014/08/02 13:11:12, jdduke wrote: > This is not OK, you just completely changed the semantics of this call. Without this even after calling clearSelection() Event is expecting SELECTION_SHOWN instead of SELECTION_CLEARED.
On 2014/08/02 13:16:53, AKV wrote: > @juddke Pls check my inline comment > > https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... > File content/browser/renderer_host/input/touch_selection_controller_unittest.cc > (right): > > https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... > content/browser/renderer_host/input/touch_selection_controller_unittest.cc:94: > controller().HideAndDisallowShowingAutomatically(); > On 2014/08/02 13:11:12, jdduke wrote: > > This is not OK, you just completely changed the semantics of this call. > > Without this even after calling clearSelection() Event is expecting > SELECTION_SHOWN instead of SELECTION_CLEARED. I 115.280s TimeoutThread-for-3902 [3902]> pm path org.chromium.native_test I 116.298s TimeoutThread-for-3902 [3902]> pm clear org.chromium.native_test C 117.486s Main ******************************************************************************** C 117.486s Main Detailed Logs C 117.486s Main ******************************************************************************** C 117.490s Main [FAIL] TouchSelectionControllerTest.SelectionBasic: C 117.490s Main ../../content/browser/renderer_host/input/touch_selection_controller_unittest.cc:435: Failure C 117.490s Main Value of: GetLastEventType() C 117.490s Main Actual: 0 C 117.490s Main Expected: SELECTION_CLEARED C 117.490s Main Which is: 1 C 117.490s Main ../../content/browser/renderer_host/input/touch_selection_controller_unittest.cc:436: Failure C 117.490s Main Value of: GetLastEventAnchor() C 117.490s Main Actual: 5.000000,15.000000 C 117.490s Main Expected: gfx::PointF() C 117.490s Main Which is: 0.000000,0.000000 C 117.490s Main ******************************************************************************** C 117.490s Main Summary C 117.491s Main ******************************************************************************** C 117.501s Main [==========] 2562 tests ran. C 117.501s Main [ PASSED ] 2561 tests. C 117.501s Main [ FAILED ] 1 test, listed below: C 117.501s Main [ FAILED ] TouchSelectionControllerTest.SelectionBasic C 117.501s Main C 117.502s Main 1 FAILED TEST This is the failure log, if I haven't added that HideAndDisallowShowingAutomatically() call. Please share your thoughts.
not lgtm. Please wait for https://codereview.chromium.org/434583002/ to land and then rebase. https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller.cc (right): https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller.cc:89: if (last_input_event_type_ != LONG_PRESS) Did you change this? This looks wrong, and it's confusing to combine such a change with a rebase. https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller_unittest.cc:94: controller().HideAndDisallowShowingAutomatically(); On 2014/08/02 06:16:53, ajith.v wrote: > On 2014/08/02 13:11:12, jdduke wrote: > > This is not OK, you just completely changed the semantics of this call. > Without this even after calling clearSelection() Event is expecting SELECTION_SHOWN instead of SELECTION_CLEARED. Which test? If a drag is in-progress the "clearing" will not take immediate effect, this is by design.
@jdduke Thanks for the review comments. I have corrected the unit test case function semantic change issue. https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller.cc (right): https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller.cc:89: if (last_input_event_type_ != LONG_PRESS) On 2014/08/02 14:23:00, jdduke wrote: > Did you change this? This looks wrong, and it's confusing to combine such a > change with a rebase. Corrected the condition to show the insertion handle if we long press on an empty edit field, because Paste Popup position depends on insertion handle position. So resize time insertion handle position will gets used for positioning Paste Popup correctly. https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller_unittest.cc:94: controller().HideAndDisallowShowingAutomatically(); On 2014/08/02 14:23:00, jdduke wrote: > On 2014/08/02 06:16:53, ajith.v wrote: > > On 2014/08/02 13:11:12, jdduke wrote: > > > This is not OK, you just completely changed the semantics of this call. > > Without this even after calling clearSelection() Event is expecting > SELECTION_SHOWN instead of SELECTION_CLEARED. > > Which test? If a drag is in-progress the "clearing" will not take immediate > effect, this is by design. Corrected the source code to counter measure the change. https://codereview.chromium.org/312293002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller_unittest.cc:324: EXPECT_EQ(SELECTION_CLEARED, GetLastEventType()); On 2014/08/02 13:11:12, jdduke wrote: > No need to check this it's just confusing. Done.
https://codereview.chromium.org/312293002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller.cc (right): https://codereview.chromium.org/312293002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller.cc:93: ShowInsertionHandleAutomatically(); I still don't understand this logic, is this because the insertion notification is coming after the |OnSelectionEditable| call? If the user long presses, we already call | ShowInsertionHandleAutomatically();|, so it seems like adding such a call here is hiding some bugs (or creating new ones).
https://codereview.chromium.org/312293002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller.cc (right): https://codereview.chromium.org/312293002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller.cc:93: ShowInsertionHandleAutomatically(); On 2014/08/04 14:34:58, jdduke wrote: > I still don't understand this logic, is this because the insertion notification > is coming after the |OnSelectionEditable| call? If the user long presses, we > already call | ShowInsertionHandleAutomatically();|, so it seems like adding > such a call here is hiding some bugs (or creating new ones). > @jdduke Here the issue is when we long press on an empty edit field as in google.com page, the ContextMenu (Paste Popup in case it's empty field) is getting triggered from blink side with proper position, but during the positioning, page is automatically scrolled, so the position send by engine goes irrelevant. Since we get onSelectionBoundsChanged() first time without having selection or insertion, it clears the state of activate_selection_automatically_ and activate_selection_automatically_ by calling HideAndDisallowShowingAutomatically(). This prevents falling back to the required call of if (start_orientation_ == TOUCH_HANDLE_CENTER && selection_editable_) { OnInsertionChanged(); return; } Even though Selection Editable comes true in later case. So to avoid that I have added this condition. Ultimately in this scenario to position the Paste Popup correctly, we need insertion Handle to be shown, so that we can adjust the position of Popup based on insertion handle position. Please suggest your opinion, so that we can keep more optimized clean code. Note: It would be good if we can communicate over IRC, interactively, than spamming codereview emails to make clear understanding on both sides faster.
https://codereview.chromium.org/312293002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller.cc (right): https://codereview.chromium.org/312293002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller.cc:93: ShowInsertionHandleAutomatically(); On 2014/08/05 14:07:40, AKV wrote: > On 2014/08/04 14:34:58, jdduke wrote: > > I still don't understand this logic, is this because the insertion > notification > > is coming after the |OnSelectionEditable| call? If the user long presses, we > > already call | ShowInsertionHandleAutomatically();|, so it seems like adding > > such a call here is hiding some bugs (or creating new ones). > > > @jdduke > Here the issue is when we long press on an empty edit field as in http://google.com > page, the ContextMenu (Paste Popup in case it's empty field) is getting > triggered from blink side with proper position, but during the positioning, page > is automatically scrolled, so the position send by engine goes irrelevant. Since > we get onSelectionBoundsChanged() first time without having selection or > insertion, it clears the state of activate_selection_automatically_ and > activate_selection_automatically_ by calling > HideAndDisallowShowingAutomatically(). This prevents falling back to the > required call of > if (start_orientation_ == TOUCH_HANDLE_CENTER && selection_editable_) { > OnInsertionChanged(); > return; > } > Even though Selection Editable comes true in later case. So to avoid that I have > added this condition. Ultimately in this scenario to position the Paste Popup > correctly, we need insertion Handle to be shown, so that we can adjust the > position of Popup based on insertion handle position. > Please suggest your opinion, so that we can keep more optimized clean code. > Note: It would be good if we can communicate over IRC, interactively, than > spamming codereview emails to make clear understanding on both sides faster. I have a workaround here, https://codereview.chromium.org/447493002/. This change should no longer be necessary.
@jdduke I have rebased this patch based on https://codereview.chromium.org/447493002/ and verified the changes with unit test cases. PTAL https://codereview.chromium.org/312293002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/touch_selection_controller.cc (right): https://codereview.chromium.org/312293002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_selection_controller.cc:93: ShowInsertionHandleAutomatically(); On 2014/08/05 16:30:43, jdduke wrote: > On 2014/08/05 14:07:40, AKV wrote: > > On 2014/08/04 14:34:58, jdduke wrote: > > > I still don't understand this logic, is this because the insertion > > notification > > > is coming after the |OnSelectionEditable| call? If the user long presses, we > > > already call | ShowInsertionHandleAutomatically();|, so it seems like adding > > > such a call here is hiding some bugs (or creating new ones). > > > > > @jdduke > > Here the issue is when we long press on an empty edit field as in > http://google.com > > page, the ContextMenu (Paste Popup in case it's empty field) is getting > > triggered from blink side with proper position, but during the positioning, > page > > is automatically scrolled, so the position send by engine goes irrelevant. > Since > > we get onSelectionBoundsChanged() first time without having selection or > > insertion, it clears the state of activate_selection_automatically_ and > > activate_selection_automatically_ by calling > > HideAndDisallowShowingAutomatically(). This prevents falling back to the > > required call of > > if (start_orientation_ == TOUCH_HANDLE_CENTER && selection_editable_) { > > OnInsertionChanged(); > > return; > > } > > Even though Selection Editable comes true in later case. So to avoid that I > have > > added this condition. Ultimately in this scenario to position the Paste Popup > > correctly, we need insertion Handle to be shown, so that we can adjust the > > position of Popup based on insertion handle position. > > Please suggest your opinion, so that we can keep more optimized clean code. > > Note: It would be good if we can communicate over IRC, interactively, than > > spamming codereview emails to make clear understanding on both sides faster. > > > I have a workaround here, https://codereview.chromium.org/447493002/. This > change should no longer be necessary. Thanks. I have rebased the patch according to latest code and verified the changes.
https://codereview.chromium.org/312293002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2196: mWasPastePopupShowingOnInsertionDragStart = getPastePopup().isShowing(); getPastePopup() will create a new PastePopup if none exists. That's not really necessary if we just want to find out of it's showing. Please change this and the below query to: mPastePopupMenu != null && mPastePopupMenu.isShowing(); https://codereview.chromium.org/312293002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2200: case SelectionEventType.INSERTION_MOVED: if (mPastePopupMenu != null && mPastePopupMenu.isShowing() ... https://codereview.chromium.org/312293002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2210: mPastePopupMenu.hide(); Let's make this |hidePastePopup()| to match the |showPastePopup()| call.
My only concern here is if the insertion handle position is updated by, say, an animation or Javascript scrolling. Updating the PopupWindow isn't the cheapest operation, but I suppose that's a rare enough corner case that it shouldn't be our chief concern.
https://codereview.chromium.org/312293002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2196: mWasPastePopupShowingOnInsertionDragStart = getPastePopup().isShowing(); On 2014/08/07 21:18:04, jdduke wrote: > getPastePopup() will create a new PastePopup if none exists. That's not really > necessary if we just want to find out of it's showing. Please change this and > the below query to: > > mPastePopupMenu != null && mPastePopupMenu.isShowing(); Done. Thanks https://codereview.chromium.org/312293002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2200: case SelectionEventType.INSERTION_MOVED: On 2014/08/07 21:18:04, jdduke wrote: > if (mPastePopupMenu != null && mPastePopupMenu.isShowing() ... Done. https://codereview.chromium.org/312293002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2210: mPastePopupMenu.hide(); On 2014/08/07 21:18:04, jdduke wrote: > Let's make this |hidePastePopup()| to match the |showPastePopup()| call. Done.
On 2014/08/07 21:18:43, jdduke wrote: > My only concern here is if the insertion handle position is updated by, say, an > animation or Javascript scrolling. Updating the PopupWindow isn't the cheapest > operation, but I suppose that's a rare enough corner case that it shouldn't be > our chief concern. @jdduke - This issue is happening in Google page because input element is repositioned during IME is up. Since this page browsing is a common user scenario, I feel this scenario needs to be addressed as early as possible. This patch additionally covers scenario of any resize happened on ContentView (Resize of browser height on MultiWindow mode) and adjust the PastePopup according to Insertion Handle position. If you feel anything extra we need to take care please share with me.
Almost there just a couple nits. https://codereview.chromium.org/312293002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2214: mWasPastePopupShowingOnInsertionDragStart = mPastePopupMenu != null && Hmm, this is probably best formatted as mWasPastePopupShowingOnInsertionDragStart = mPastePopupMenu != null && mPastePopupMenu.isShowing(); (line break after =) https://codereview.chromium.org/312293002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2220: if (mPastePopupMenu != null && mPastePopupMenu.isShowing() && Looks like we can simplify this with: if (mPastePopupMenu == null) break; if (!isScrollInProgress() && mPastePopupMenu.isShowing()) { showPastePopup(...); } else { hidePastePopup(); } https://codereview.chromium.org/312293002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2237: mWasPastePopupShowingOnInsertionDragStart = false; No need to reset this flag, it will always be set on INSERTION_DRAG_STARTED.
@jdduke PTAL https://codereview.chromium.org/312293002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2214: mWasPastePopupShowingOnInsertionDragStart = mPastePopupMenu != null && On 2014/08/08 15:11:41, jdduke wrote: > Hmm, this is probably best formatted as > > mWasPastePopupShowingOnInsertionDragStart = > mPastePopupMenu != null && mPastePopupMenu.isShowing(); > > (line break after =) Done. Thanks https://codereview.chromium.org/312293002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2220: if (mPastePopupMenu != null && mPastePopupMenu.isShowing() && On 2014/08/08 15:11:41, jdduke wrote: > Looks like we can simplify this with: > > if (mPastePopupMenu == null) break; > if (!isScrollInProgress() && mPastePopupMenu.isShowing()) { > showPastePopup(...); > } else { > hidePastePopup(); > } > Done. https://codereview.chromium.org/312293002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2237: mWasPastePopupShowingOnInsertionDragStart = false; On 2014/08/08 15:11:41, jdduke wrote: > No need to reset this flag, it will always be set on INSERTION_DRAG_STARTED. Done. Thanks
lgtm, thanks.
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/312293002/260001
Message was sent while issue was closed.
Change committed as 288404 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
