Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2211433002:
Making Android Url bar behavior to ESCAPE key same as in Desktop Chrome (Closed)
DescriptionMaking Android Url bar behavior to ESCAPE key same as in Desktop Chrome
In response to ESCAPE key desktop chrome is selecting all the contents
of Url bar. Making same behavior in Android Chrome as well.
BUG=631075
Committed: https://crrev.com/8a5629f03b20a4a94a9653b11284f29534ebf49d
Cr-Commit-Position: refs/heads/master@{#416452}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed review comments. #
Total comments: 2
Patch Set 3 : Fixed the review comments. #
Total comments: 3
Patch Set 4 : Removed redundant hideSuggestions() and hideKeyboard() calls. #
Messages
Total messages: 16 (4 generated)
ajith.v@chromium.org changed reviewers: + tedchoc@chromium.org, yfriedman@chromium.org
PTAL!
https://codereview.chromium.org/2211433002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2211433002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:339: revertChanges(); I think you only need revertChanges() and selectAll(). You already have focus here and I don't think you need setUrlBarFocus since you have direct access to selectAll https://codereview.chromium.org/2211433002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:341: mUrlBar.selectAll(); selectAll() is available as a method on LocationBarLayout, so you don't need mUrlBar here
PTAL! https://codereview.chromium.org/2211433002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2211433002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:339: revertChanges(); On 2016/08/03 23:56:41, Ted C wrote: > I think you only need revertChanges() and selectAll(). You already have focus > here and I don't think you need setUrlBarFocus since you have direct access to > selectAll Done. Thank you. Now I am clearing suggestion window as well, to match exact desktop chrome behavior. https://codereview.chromium.org/2211433002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:341: mUrlBar.selectAll(); On 2016/08/03 23:56:41, Ted C wrote: > selectAll() is available as a method on LocationBarLayout, so you don't need > mUrlBar here Done.
https://codereview.chromium.org/2211433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2211433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:340: hideSuggestions(); Hmm...I wonder if all this logic can just be put into revertChanges. I'm pretty sure when I added that, I only "really" made it work for the NTP. I think we could enhance revertChanges by wrapping the setUrl calls in mUrlBar.setIgnoreTextChangesForAutocomplete(true / false) barriers. Then call hide suggestions for both. And selectAll for the non-NTP case. I "believe" that would make revertChanges closer to desktops RevertAll (and I now realized that I'm sad that I incorrectly named the android one not to match).
On 2016/08/05 20:46:14, Ted C wrote: > https://codereview.chromium.org/2211433002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java > (right): > > https://codereview.chromium.org/2211433002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:340: > hideSuggestions(); > Hmm...I wonder if all this logic can just be put into revertChanges. I'm pretty > sure when I added that, I only "really" made it work for the NTP. > > I think we could enhance revertChanges by wrapping the setUrl calls in > mUrlBar.setIgnoreTextChangesForAutocomplete(true / false) barriers. Then call > hide suggestions for both. And selectAll for the non-NTP case. > > I "believe" that would make revertChanges closer to desktops RevertAll (and I > now realized that I'm sad that I incorrectly named the android one not to > match). @Ted - I will upload a patch for this soon. Currently I am having few priority tasks, so not able to look into this issue yet. Sorry for the delay.
PTAL! https://codereview.chromium.org/2211433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2211433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:340: hideSuggestions(); On 2016/08/05 20:46:14, Ted C wrote: > Hmm...I wonder if all this logic can just be put into revertChanges. I'm pretty > sure when I added that, I only "really" made it work for the NTP. > > I think we could enhance revertChanges by wrapping the setUrl calls in > mUrlBar.setIgnoreTextChangesForAutocomplete(true / false) barriers. Then call > hide suggestions for both. And selectAll for the non-NTP case. > > I "believe" that would make revertChanges closer to desktops RevertAll (and I > now realized that I'm sad that I incorrectly named the android one not to > match). Done. Used setUrlBarText(), which wraps setIgnoreTextChangesForAutocomplete->setUrl() call for us.
lgtm https://codereview.chromium.org/2211433002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2211433002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:913: hideSuggestions(); can hideSuggestions go outside of the if/else since it is called from both? Same for the hideKeyboard? Why are we hiding the keyboard anyway? Isn't this only triggered when you have a hardware keyboard attached? Seems like that might be unnecessary IMO.
https://codereview.chromium.org/2211433002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2211433002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:913: hideSuggestions(); On 2016/09/02 16:36:52, Ted C wrote: > can hideSuggestions go outside of the if/else since it is called from both? > Same for the hideKeyboard? > > Why are we hiding the keyboard anyway? Isn't this only triggered when you have > a hardware keyboard attached? Seems like that might be unnecessary IMO. Without hideKeyboard(), IME is not disappearing when pressing ESCAPE key from hardware keyboard.
https://codereview.chromium.org/2211433002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2211433002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:913: hideSuggestions(); On 2016/09/02 16:36:52, Ted C wrote: > can hideSuggestions go outside of the if/else since it is called from both? > Same for the hideKeyboard? > > Why are we hiding the keyboard anyway? Isn't this only triggered when you have > a hardware keyboard attached? Seems like that might be unnecessary IMO. can hideSuggestions go outside of the if/else since it is called from both? Same for the hideKeyboard? >> Done. Thanks
The CQ bit was checked by ajith.v@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2211433002/#ps60001 (title: "Removed redundant hideSuggestions() and hideKeyboard() calls.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Making Android Url bar behavior to ESCAPE key same as in Desktop Chrome In response to ESCAPE key desktop chrome is selecting all the contents of Url bar. Making same behavior in Android Chrome as well. BUG=631075 ========== to ========== Making Android Url bar behavior to ESCAPE key same as in Desktop Chrome In response to ESCAPE key desktop chrome is selecting all the contents of Url bar. Making same behavior in Android Chrome as well. BUG=631075 Committed: https://crrev.com/8a5629f03b20a4a94a9653b11284f29534ebf49d Cr-Commit-Position: refs/heads/master@{#416452} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8a5629f03b20a4a94a9653b11284f29534ebf49d Cr-Commit-Position: refs/heads/master@{#416452} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
