Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(94)

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.

Description

Properly 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -11 lines) Patch
M content/browser/renderer_host/input/selection_event_type_list.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/input/touch_selection_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -1 line 0 comments Download
M content/browser/renderer_host/input/touch_selection_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 5 chunks +6 lines, -5 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +15 lines, -4 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -1 line 0 comments Download

Messages

Total messages: 59 (0 generated)
AKVT
PTAL this change
6 years, 6 months ago (2014-06-05 15:25:53 UTC) #1
aurimas (slooooooooow)
+yfriedman and tedchoc for this change. I am pretty sure that exposing ContentViewCore is not ...
6 years, 6 months ago (2014-06-05 15:57:20 UTC) #2
jdduke (slow)
On 2014/06/05 15:57:20, aurimas wrote: > +yfriedman and tedchoc for this change. I am pretty ...
6 years, 6 months ago (2014-06-05 15:59:32 UTC) #3
Yaron
On 2014/06/05 15:59:32, jdduke wrote: > On 2014/06/05 15:57:20, aurimas wrote: > > +yfriedman and ...
6 years, 6 months ago (2014-06-05 16:55:46 UTC) #4
AKVT
On 2014/06/05 16:55:46, Yaron wrote: > On 2014/06/05 15:59:32, jdduke wrote: > > On 2014/06/05 ...
6 years, 6 months ago (2014-06-05 18:36:12 UTC) #5
Yaron
On 2014/06/05 18:36:12, AJITH KUMAR V wrote: > On 2014/06/05 16:55:46, Yaron wrote: > > ...
6 years, 6 months ago (2014-06-06 01:40:34 UTC) #6
jdduke (slow)
Ajith, I think we'll be in a better position to address this after the landing ...
6 years, 6 months ago (2014-06-06 03:53:15 UTC) #7
AKVT
On 2014/06/06 03:53:15, jdduke wrote: > Ajith, I think we'll be in a better position ...
6 years, 6 months ago (2014-06-06 08:55:23 UTC) #8
aurimas (slooooooooow)
On 2014/06/06 08:55:23, AKV wrote: > On 2014/06/06 03:53:15, jdduke wrote: > > Ajith, I ...
6 years, 5 months ago (2014-07-11 05:38:13 UTC) #9
jdduke (slow)
On 2014/07/11 05:38:13, aurimas wrote: > On 2014/06/06 08:55:23, AKV wrote: > > On 2014/06/06 ...
6 years, 5 months ago (2014-07-11 14:51:34 UTC) #10
AKVT
@jdduke PTAL. Made changes based on new text selection logic.
6 years, 5 months ago (2014-07-22 16:36:35 UTC) #11
jdduke (slow)
https://codereview.chromium.org/312293002/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2154 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2154: if (getPastePopup().isShowing() && !isScrollInProgress()) { What about the case ...
6 years, 5 months ago (2014-07-22 17:39:34 UTC) #12
AKVT
@jdduke PTAL
6 years, 5 months ago (2014-07-23 14:33:58 UTC) #13
AKVT
PTAL https://codereview.chromium.org/312293002/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2154 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2154: if (getPastePopup().isShowing() && !isScrollInProgress()) { On 2014/07/22 17:39:33, ...
6 years, 5 months ago (2014-07-23 16:27:47 UTC) #14
jdduke (slow)
https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2155 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2155: mWasPastePopupShowing = getPastePopup().isShowing(); Hmm, I'd like to first land ...
6 years, 5 months ago (2014-07-23 16:54:58 UTC) #15
AKVT
https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2155 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2155: mWasPastePopupShowing = getPastePopup().isShowing(); On 2014/07/23 16:54:58, jdduke wrote: > ...
6 years, 5 months ago (2014-07-23 17:19:02 UTC) #16
AKVT
On 2014/07/23 17:19:02, AKV wrote: > https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > ...
6 years, 4 months ago (2014-07-28 14:20:32 UTC) #17
jdduke (slow)
https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2161 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2161: hidePastePopup(); Can we avoid hiding then immediately showing? I ...
6 years, 4 months ago (2014-07-28 15:51:07 UTC) #18
AKVT
@jdduke PTAL https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2161 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2161: hidePastePopup(); On 2014/07/28 15:51:07, jdduke wrote: > ...
6 years, 4 months ago (2014-07-28 16:56:58 UTC) #19
jdduke (slow)
https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2161 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 ...
6 years, 4 months ago (2014-07-28 17:26:28 UTC) #20
AKVT
@jdduke PTAL https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2161 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2161: hidePastePopup(); On 2014/07/28 17:26:28, jdduke wrote: > ...
6 years, 4 months ago (2014-07-30 12:06:56 UTC) #21
jdduke (slow)
On 2014/07/30 12:06:56, AKV wrote: > @jdduke Thanks for pointing the issue in positionAt() > ...
6 years, 4 months ago (2014-07-30 17:36:36 UTC) #22
jdduke (slow)
I think BEGUN is probably not the best name (my bad), maybe rename to INSERTION_DRAG_STARTED. ...
6 years, 4 months ago (2014-07-30 17:39:09 UTC) #23
AKVT
@jdduke. I have fixed review comments. Additionally, pastePopup cordinates relatively depends on the insertion handle ...
6 years, 4 months ago (2014-08-01 14:42:08 UTC) #24
jdduke (slow)
Thanks, I think we're almost there, just a couple nits. https://codereview.chromium.org/312293002/diff/120001/content/browser/renderer_host/input/touch_selection_controller_unittest.cc File content/browser/renderer_host/input/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/312293002/diff/120001/content/browser/renderer_host/input/touch_selection_controller_unittest.cc#newcode380 ...
6 years, 4 months ago (2014-08-01 15:10:51 UTC) #25
jdduke (slow)
Also, please update the description contents per the email I just sent you.
6 years, 4 months ago (2014-08-01 15:11:31 UTC) #26
AKVT
@jdduke PTAL https://codereview.chromium.org/312293002/diff/120001/content/browser/renderer_host/input/touch_selection_controller_unittest.cc File content/browser/renderer_host/input/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/312293002/diff/120001/content/browser/renderer_host/input/touch_selection_controller_unittest.cc#newcode380 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: ...
6 years, 4 months ago (2014-08-01 15:23:25 UTC) #27
jdduke (slow)
lgtm, thanks. I updated the description slightly (again, state *what* the patch does on the ...
6 years, 4 months ago (2014-08-01 15:31:11 UTC) #28
AKVT
On 2014/08/01 15:31:11, jdduke wrote: > lgtm, thanks. I updated the description slightly (again, state ...
6 years, 4 months ago (2014-08-01 15:35:25 UTC) #29
jdduke (slow)
On 2014/08/01 15:35:25, AKV wrote: > On 2014/08/01 15:31:11, jdduke wrote: > > lgtm, thanks. ...
6 years, 4 months ago (2014-08-01 15:37:44 UTC) #30
AKVT
On 2014/08/01 15:37:44, jdduke wrote: > On 2014/08/01 15:35:25, AKV wrote: > > On 2014/08/01 ...
6 years, 4 months ago (2014-08-01 15:40:29 UTC) #31
AKVT
The CQ bit was checked by ajith.v@samsung.com
6 years, 4 months ago (2014-08-01 15:40:42 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/312293002/140001
6 years, 4 months ago (2014-08-01 15:42:08 UTC) #33
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux ...
6 years, 4 months ago (2014-08-01 23:38:55 UTC) #34
jdduke (slow)
The CQ bit was unchecked by jdduke@chromium.org
6 years, 4 months ago (2014-08-01 23:47:08 UTC) #35
jdduke (slow)
On 2014/08/01 23:38:55, I haz the power (commit-bot) wrote: > FYI, CQ is re-trying this ...
6 years, 4 months ago (2014-08-01 23:47:35 UTC) #36
AKVT
On 2014/08/01 23:47:35, jdduke wrote: > On 2014/08/01 23:38:55, I haz the power (commit-bot) wrote: ...
6 years, 4 months ago (2014-08-02 11:46:24 UTC) #37
AKVT
The CQ bit was checked by ajith.v@samsung.com
6 years, 4 months ago (2014-08-02 11:46:38 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/312293002/160001
6 years, 4 months ago (2014-08-02 11:47:42 UTC) #39
jdduke (slow)
The CQ bit was unchecked by jdduke@chromium.org
6 years, 4 months ago (2014-08-02 13:06:35 UTC) #40
jdduke (slow)
https://codereview.chromium.org/312293002/diff/160001/content/browser/renderer_host/input/touch_selection_controller_unittest.cc File content/browser/renderer_host/input/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/312293002/diff/160001/content/browser/renderer_host/input/touch_selection_controller_unittest.cc#newcode94 content/browser/renderer_host/input/touch_selection_controller_unittest.cc:94: controller().HideAndDisallowShowingAutomatically(); This is not OK, you just completely changed ...
6 years, 4 months ago (2014-08-02 13:11:12 UTC) #41
AKVT
@juddke Pls check my inline comment https://codereview.chromium.org/312293002/diff/160001/content/browser/renderer_host/input/touch_selection_controller_unittest.cc File content/browser/renderer_host/input/touch_selection_controller_unittest.cc (right): https://codereview.chromium.org/312293002/diff/160001/content/browser/renderer_host/input/touch_selection_controller_unittest.cc#newcode94 content/browser/renderer_host/input/touch_selection_controller_unittest.cc:94: controller().HideAndDisallowShowingAutomatically(); On 2014/08/02 ...
6 years, 4 months ago (2014-08-02 13:16:53 UTC) #42
AKVT
On 2014/08/02 13:16:53, AKV wrote: > @juddke Pls check my inline comment > > https://codereview.chromium.org/312293002/diff/160001/content/browser/renderer_host/input/touch_selection_controller_unittest.cc ...
6 years, 4 months ago (2014-08-02 13:31:20 UTC) #43
jdduke (slow)
not lgtm. Please wait for https://codereview.chromium.org/434583002/ to land and then rebase. https://codereview.chromium.org/312293002/diff/160001/content/browser/renderer_host/input/touch_selection_controller.cc File content/browser/renderer_host/input/touch_selection_controller.cc (right): ...
6 years, 4 months ago (2014-08-02 14:23:01 UTC) #44
AKVT
@jdduke Thanks for the review comments. I have corrected the unit test case function semantic ...
6 years, 4 months ago (2014-08-04 13:45:33 UTC) #45
jdduke (slow)
https://codereview.chromium.org/312293002/diff/200001/content/browser/renderer_host/input/touch_selection_controller.cc File content/browser/renderer_host/input/touch_selection_controller.cc (right): https://codereview.chromium.org/312293002/diff/200001/content/browser/renderer_host/input/touch_selection_controller.cc#newcode93 content/browser/renderer_host/input/touch_selection_controller.cc:93: ShowInsertionHandleAutomatically(); I still don't understand this logic, is this ...
6 years, 4 months ago (2014-08-04 14:34:59 UTC) #46
AKVT
https://codereview.chromium.org/312293002/diff/200001/content/browser/renderer_host/input/touch_selection_controller.cc File content/browser/renderer_host/input/touch_selection_controller.cc (right): https://codereview.chromium.org/312293002/diff/200001/content/browser/renderer_host/input/touch_selection_controller.cc#newcode93 content/browser/renderer_host/input/touch_selection_controller.cc:93: ShowInsertionHandleAutomatically(); On 2014/08/04 14:34:58, jdduke wrote: > I still ...
6 years, 4 months ago (2014-08-05 14:07:40 UTC) #47
jdduke (slow)
https://codereview.chromium.org/312293002/diff/200001/content/browser/renderer_host/input/touch_selection_controller.cc File content/browser/renderer_host/input/touch_selection_controller.cc (right): https://codereview.chromium.org/312293002/diff/200001/content/browser/renderer_host/input/touch_selection_controller.cc#newcode93 content/browser/renderer_host/input/touch_selection_controller.cc:93: ShowInsertionHandleAutomatically(); On 2014/08/05 14:07:40, AKV wrote: > On 2014/08/04 ...
6 years, 4 months ago (2014-08-05 16:30:43 UTC) #48
AKVT
@jdduke I have rebased this patch based on https://codereview.chromium.org/447493002/ and verified the changes with unit ...
6 years, 4 months ago (2014-08-07 12:01:22 UTC) #49
jdduke (slow)
https://codereview.chromium.org/312293002/diff/220001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/220001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2196 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2196: mWasPastePopupShowingOnInsertionDragStart = getPastePopup().isShowing(); getPastePopup() will create a new PastePopup ...
6 years, 4 months ago (2014-08-07 21:18:04 UTC) #50
jdduke (slow)
My only concern here is if the insertion handle position is updated by, say, an ...
6 years, 4 months ago (2014-08-07 21:18:43 UTC) #51
AKVT
https://codereview.chromium.org/312293002/diff/220001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/220001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2196 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2196: mWasPastePopupShowingOnInsertionDragStart = getPastePopup().isShowing(); On 2014/08/07 21:18:04, jdduke wrote: > ...
6 years, 4 months ago (2014-08-08 12:54:44 UTC) #52
AKVT
On 2014/08/07 21:18:43, jdduke wrote: > My only concern here is if the insertion handle ...
6 years, 4 months ago (2014-08-08 13:26:08 UTC) #53
jdduke (slow)
Almost there just a couple nits. https://codereview.chromium.org/312293002/diff/240001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/240001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2214 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2214: mWasPastePopupShowingOnInsertionDragStart = mPastePopupMenu ...
6 years, 4 months ago (2014-08-08 15:11:41 UTC) #54
AKVT
@jdduke PTAL https://codereview.chromium.org/312293002/diff/240001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/312293002/diff/240001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2214 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2214: mWasPastePopupShowingOnInsertionDragStart = mPastePopupMenu != null && On ...
6 years, 4 months ago (2014-08-08 15:25:43 UTC) #55
jdduke (slow)
lgtm, thanks.
6 years, 4 months ago (2014-08-08 15:41:24 UTC) #56
AKVT
The CQ bit was checked by ajith.v@samsung.com
6 years, 4 months ago (2014-08-08 16:00:32 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/312293002/260001
6 years, 4 months ago (2014-08-08 16:01:56 UTC) #58
commit-bot: I haz the power
6 years, 4 months ago (2014-08-08 18:55:31 UTC) #59
Message was sent while issue was closed.
Change committed as 288404

Powered by Google App Engine
This is Rietveld 408576698