Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2939783002:
Fixing mouse focus on WebView (Closed)
DescriptionFixing mouse focus on WebView
Mouse down should request focus just like tap down and long-press. This CL
also cleans up some redundant conditionals.
BUG=724288
Review-Url: https://codereview.chromium.org/2939783002
Cr-Commit-Position: refs/heads/master@{#480085}
Committed: https://chromium.googlesource.com/chromium/src/+/b58f576ba586c18986315cb80e9b70fc8de5f71a
Patch Set 1 #
Total comments: 2
Messages
Total messages: 25 (15 generated)
The CQ bit was checked by amaralp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fixing mouse focus on WebView BUG=724288 ========== to ========== Fixing mouse focus on WebView Mouse down should request focus just like tap down and long-press. This CL also cleans up some redundant conditionals. BUG=724288 ==========
amaralp@chromium.org changed reviewers: + aelias@chromium.org
PTAL
https://codereview.chromium.org/2939783002/diff/1/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2939783002/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:554: Java_ContentViewCore_requestFocus(env, j_obj); This doesn't really have anything to do with filtering at all, it was just a convenient place to apply a side effect, so we should move it somewhere better. I suggest applying the focus change inside EventForwarder.onTouchEvent and EventForwarder.onMouseEvent instead. The container view is not available from there, so you can add it in as an additional argument to those methods similarly to what is already done for onDragEvent. (There's further cleanup possible here but it will be easier when PopupZoomer is deleted, so we can go with this for now.)
aelias@chromium.org changed reviewers: + boliu@chromium.org
+boliu for content/browser/android OWNERS https://codereview.chromium.org/2939783002/diff/1/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2939783002/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:554: Java_ContentViewCore_requestFocus(env, j_obj); On 2017/06/13 at 22:22:55, aelias wrote: > This doesn't really have anything to do with filtering at all, it was just a convenient place to apply a side effect, so we should move it somewhere better. I suggest applying the focus change inside EventForwarder.onTouchEvent and EventForwarder.onMouseEvent instead. The container view is not available from there, so you can add it in as an additional argument to those methods similarly to what is already done for onDragEvent. > > (There's further cleanup possible here but it will be easier when PopupZoomer is deleted, so we can go with this for now.) OK, Pedro explained offline this approach doesn't work with long-press, so lgtm as is.
lgtm
The CQ bit was checked by amaralp@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by amaralp@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by amaralp@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1497632855348880, "parent_rev":
"95903ec2a23312d86a8c26cdcf2721ce8b8f5d66", "commit_rev":
"2ddb641b9cf8bfef1a046240ffb9f953e3afaf51"}
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1497632855348880, "parent_rev":
"86dac68f1b3aec6d27125ce3ebbeb01aa419930d", "commit_rev":
"b58f576ba586c18986315cb80e9b70fc8de5f71a"}
Message was sent while issue was closed.
Description was changed from ========== Fixing mouse focus on WebView Mouse down should request focus just like tap down and long-press. This CL also cleans up some redundant conditionals. BUG=724288 ========== to ========== Fixing mouse focus on WebView Mouse down should request focus just like tap down and long-press. This CL also cleans up some redundant conditionals. BUG=724288 Review-Url: https://codereview.chromium.org/2939783002 Cr-Commit-Position: refs/heads/master@{#480085} Committed: https://chromium.googlesource.com/chromium/src/+/b58f576ba586c18986315cb80e9b... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/b58f576ba586c18986315cb80e9b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
