Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2064163003:
Press back when floating popup is active backgrounds chrome (Closed)
DescriptionPress back when floating popup is active backgrounds chrome
Added logic to allow the WebActionMode to know if it is currently visible on the screen.
Changed back press code to check if there is a visible menu and if so clearing it instead of backgrounding chrome.
BUG=620001
Committed: https://crrev.com/5e4f3880b0bdbceaed247827240701fe5fde1551
Cr-Commit-Position: refs/heads/master@{#399955}
Patch Set 1 #
Total comments: 3
Patch Set 2 : removed geometry check #Patch Set 3 : Added back RecordUserAction #
Total comments: 3
Messages
Total messages: 25 (10 generated)
Description was changed from ========== Press back when floating popup is active backgrounds chrome Added logic to allow the WebActionMode to know if it is currently visible on the screen. Changed back press code to check if there is a visible menu and if so clearing it instead of backgrounding chrome. BUG=620001 ========== to ========== Press back when floating popup is active backgrounds chrome Added logic to allow the WebActionMode to know if it is currently visible on the screen. Changed back press code to check if there is a visible menu and if so clearing it instead of backgrounding chrome. BUG=620001 ==========
amaralp@chromium.org changed reviewers: + aelias@chromium.org
On 2016/06/14 18:27:38, amaralp wrote: > mailto:amaralp@chromium.org changed reviewers: > + mailto:aelias@chromium.org PTAL
https://codereview.chromium.org/2064163003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (left): https://codereview.chromium.org/2064163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1435: RecordUserAction.record("SystemBack"); It looks like you deleted this stat recording line by accident, please bring it back. https://codereview.chromium.org/2064163003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/WebActionMode.java (right): https://codereview.chromium.org/2064163003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:148: mView.getDrawingRect(rect); Considering geometry intersection seems overcomplicated and it doesn't match how Android TextViews behave. I tried selecting text in a gmail compose box, scrolling it offscreen, then pressing back, and what happens is that the selection is cleared instead of going back. So we should do the same thing.
On 2016/06/14 21:16:01, aelias wrote: > https://codereview.chromium.org/2064163003/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > (left): > > https://codereview.chromium.org/2064163003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1435: > RecordUserAction.record("SystemBack"); > It looks like you deleted this stat recording line by accident, please bring it > back. > > https://codereview.chromium.org/2064163003/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/WebActionMode.java > (right): > > https://codereview.chromium.org/2064163003/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/WebActionMode.java:148: > mView.getDrawingRect(rect); > Considering geometry intersection seems overcomplicated and it doesn't match how > Android TextViews behave. I tried selecting text in a gmail compose box, > scrolling it offscreen, then pressing back, and what happens is that the > selection is cleared instead of going back. So we should do the same thing. I removed the geometry check. Selection is cleared regardless of visibility.
https://codereview.chromium.org/2064163003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (left): https://codereview.chromium.org/2064163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1435: RecordUserAction.record("SystemBack"); On 2016/06/14 at 21:16:01, aelias wrote: > It looks like you deleted this stat recording line by accident, please bring it back. Could you address this comment? Because of the number of early-returns now, I think the best way to handle this is to move this line to the top of this method to run unconditionally. Then, delete the same line from ChromeTabbedActivity.handleBackPressed().
On 2016/06/14 22:31:28, aelias wrote: > https://codereview.chromium.org/2064163003/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > (left): > > https://codereview.chromium.org/2064163003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1435: > RecordUserAction.record("SystemBack"); > On 2016/06/14 at 21:16:01, aelias wrote: > > It looks like you deleted this stat recording line by accident, please bring > it back. > > Could you address this comment? Because of the number of early-returns now, I > think the best way to handle this is to move this line to the top of this method > to run unconditionally. Then, delete the same line from > ChromeTabbedActivity.handleBackPressed(). Sorry about that. I missed that comment. It's back now.
aelias@chromium.org changed reviewers: + dfalcantara@chromium.org
lgtm, adding dfalcantara@ for OWNERS.
https://codereview.chromium.org/2064163003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2064163003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1442: if (mContextualSearchManager != null && mContextualSearchManager.onBackPressed()) return; Why'd you pull this logic out? Before, mContextualSearchManager was only checked if mCompositorViewHolder wasn't null.
https://codereview.chromium.org/2064163003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2064163003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1442: if (mContextualSearchManager != null && mContextualSearchManager.onBackPressed()) return; On 2016/06/14 23:30:10, dfalcantara wrote: > Why'd you pull this logic out? Before, mContextualSearchManager was only > checked if mCompositorViewHolder wasn't null. I thought that the check wouldn't be necessary and it was cleaner this way. I can put it back though.
lgtm https://chromiumcodereview.appspot.com/2064163003/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://chromiumcodereview.appspot.com/2064163003/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1442: if (mContextualSearchManager != null && mContextualSearchManager.onBackPressed()) return; On 2016/06/14 23:50:12, amaralp wrote: > On 2016/06/14 23:30:10, dfalcantara wrote: > > Why'd you pull this logic out? Before, mContextualSearchManager was only > > checked if mCompositorViewHolder wasn't null. > > I thought that the check wouldn't be necessary and it was cleaner this way. I > can put it back though. Hokay. So after lots and lots of archaeology, we found this from 2014: https://chrome-internal-review.googlesource.com/#/c/176856/2/java/apps/chrome... Matt and I believe that the CompositorViewHolder check was originally necessary because Contextual Search used to be its own layout, which may have consumed the event. That's changed considerably since then and probably isn't needed anymore.
The CQ bit was checked by amaralp@chromium.org
The CQ bit was unchecked by amaralp@chromium.org
The CQ bit was checked by amaralp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2064163003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by amaralp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2064163003/40001
Message was sent while issue was closed.
Description was changed from ========== Press back when floating popup is active backgrounds chrome Added logic to allow the WebActionMode to know if it is currently visible on the screen. Changed back press code to check if there is a visible menu and if so clearing it instead of backgrounding chrome. BUG=620001 ========== to ========== Press back when floating popup is active backgrounds chrome Added logic to allow the WebActionMode to know if it is currently visible on the screen. Changed back press code to check if there is a visible menu and if so clearing it instead of backgrounding chrome. BUG=620001 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Press back when floating popup is active backgrounds chrome Added logic to allow the WebActionMode to know if it is currently visible on the screen. Changed back press code to check if there is a visible menu and if so clearing it instead of backgrounding chrome. BUG=620001 ========== to ========== Press back when floating popup is active backgrounds chrome Added logic to allow the WebActionMode to know if it is currently visible on the screen. Changed back press code to check if there is a visible menu and if so clearing it instead of backgrounding chrome. BUG=620001 Committed: https://crrev.com/5e4f3880b0bdbceaed247827240701fe5fde1551 Cr-Commit-Position: refs/heads/master@{#399955} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5e4f3880b0bdbceaed247827240701fe5fde1551 Cr-Commit-Position: refs/heads/master@{#399955} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
