Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
499393002:
Set the Insertion handle correctly while showing the paste popup menu (Closed)
|
Created:
6 years, 4 months ago by raghu Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, penghuang+watch_chromium.org, James Su, yukishiino+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSet the Insertion handle correctly while showing the paste popup menu
The selection handle was wrongly set to left handle instead of center.
Corrected it. Also added test case to verify that the paste popup is
shown when long pressed on empty input with some text in clipboard
BUG=407185
Committed: https://crrev.com/c80b563a0074eac1491ccea5c7c65acef2795d33
Cr-Commit-Position: refs/heads/master@{#292304}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added wait for the long press event to respond in the unit test #
Total comments: 4
Patch Set 3 : Added the correct wait condition as per review #
Total comments: 4
Patch Set 4 : Removed unwanted line breaks #
Total comments: 3
Patch Set 5 : Removed few move unwanted line breaks #
Messages
Total messages: 18 (0 generated)
Hi Jdduke, This issue was happening because we are setting the type of the handle correctly while showing the paste popup menu. I have also added a test case to ensure that the paste popup is shown when required. PTAL
Good catch, thanks. https://codereview.chromium.org/499393002/diff/1/content/public/android/javat... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/1/content/public/android/javat... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:564: PastePopupMenu pastePopup = mContentViewCore.getPastePopupForTest(); Don't you need to wait here? I'm not sure that DOMUtils.longPressNode will wait properly for the event response.
PTAL https://codereview.chromium.org/499393002/diff/1/content/public/android/javat... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/1/content/public/android/javat... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:564: PastePopupMenu pastePopup = mContentViewCore.getPastePopupForTest(); On 2014/08/25 17:40:45, jdduke wrote: > Don't you need to wait here? I'm not sure that DOMUtils.longPressNode will wait > properly for the event response. Done.
https://codereview.chromium.org/499393002/diff/20001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:561: assertWaitForKeyboardStatus(false); I don't think this is the right wait condition. The keyboard should be hidden by the |cut| operation, right? So how does this guarantee we'll have gotten the long press response?
https://codereview.chromium.org/499393002/diff/20001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:561: assertWaitForKeyboardStatus(false); On 2014/08/26 15:06:43, jdduke wrote: > I don't think this is the right wait condition. The keyboard should be hidden by > the |cut| operation, right? So how does this guarantee we'll have gotten the > long press response? Cut doesnt hide the keyboard. Also in this test since we are long pressing on an empty input, keyboard status doesnt change at all. Please suggest me here.
https://codereview.chromium.org/499393002/diff/20001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:561: assertWaitForKeyboardStatus(false); On 2014/08/26 15:45:56, rvg wrote: > Cut doesnt hide the keyboard. Also in this test since we are long pressing on an > empty input, keyboard status doesnt change at all. > Please suggest me here. If the keyboard status doesn't change, why are you using it as a wait condition? Rather than waiting on an external condition, why not simply wait until pastePopup.isShowing() is true: final PastePopupMenu pastePopup = mContentViewCore.getPastePopupForTest(); assertTrue(CriteriaHelper.pollForCriteria(new Criteria() { @Override public boolean isSatisfied() { return pastePopup.isShowing(); } }));
PTAL https://codereview.chromium.org/499393002/diff/20001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/20001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:561: assertWaitForKeyboardStatus(false); On 2014/08/26 16:01:22, jdduke wrote: > On 2014/08/26 15:45:56, rvg wrote: > > Cut doesnt hide the keyboard. Also in this test since we are long pressing on > an > > empty input, keyboard status doesnt change at all. > > Please suggest me here. > > If the keyboard status doesn't change, why are you using it as a wait condition? > Rather than waiting on an external condition, why not simply wait until > pastePopup.isShowing() is true: > > final PastePopupMenu pastePopup = mContentViewCore.getPastePopupForTest(); > assertTrue(CriteriaHelper.pollForCriteria(new Criteria() { > @Override > public boolean isSatisfied() { > return pastePopup.isShowing(); > } > })); Done.
https://codereview.chromium.org/499393002/diff/40001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:558: mConnection.mImeUpdateQueue, 1, "hello", 5, 5, -1, -1); This line break isn't necessary, neither are the line breaks below for waitAndVerifyEditableCallback (Java width is 100 characters). Please follow the existing style in these tests. https://codereview.chromium.org/499393002/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:570: final PastePopupMenu pastePopup = Line break is not necessary.
https://codereview.chromium.org/499393002/diff/40001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:558: mConnection.mImeUpdateQueue, 1, "hello", 5, 5, -1, -1); On 2014/08/27 14:57:13, jdduke wrote: > This line break isn't necessary, neither are the line breaks below for > waitAndVerifyEditableCallback (Java width is 100 characters). Please follow the > existing style in these tests. Done. https://codereview.chromium.org/499393002/diff/40001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:570: final PastePopupMenu pastePopup = On 2014/08/27 14:57:13, jdduke wrote: > Line break is not necessary. Done.
lgtm with nit. https://codereview.chromium.org/499393002/diff/60001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/60001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:566: Nit: Adding line breaks here doesn't really help, I would put the longPressNode, pastePopup assignment and asserTrue lines all next to each other.
r.ghatage@samsung.com changed reviewers: + aelias@chromium.org
Adding aelias@ PTAL https://codereview.chromium.org/499393002/diff/60001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/60001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:566: On 2014/08/27 17:05:31, jdduke wrote: > Nit: Adding line breaks here doesn't really help, I would put the longPressNode, > pastePopup assignment and asserTrue lines all next to each other. will do the change
lgtm
https://codereview.chromium.org/499393002/diff/60001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/499393002/diff/60001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:566: On 2014/08/27 18:45:11, rvg wrote: > On 2014/08/27 17:05:31, jdduke wrote: > > Nit: Adding line breaks here doesn't really help, I would put the > longPressNode, > > pastePopup assignment and asserTrue lines all next to each other. > > will do the change Done.
The CQ bit was checked by r.ghatage@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/r.ghatage@samsung.com/499393002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 1550d64d324999b4a29ff6d30b664ec33ce18cab
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c80b563a0074eac1491ccea5c7c65acef2795d33 Cr-Commit-Position: refs/heads/master@{#292304} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
