Request focus on mouse down events on Android [chromium/src : main]
Mark Schillaci (Gerrit)
Mark Schillaci added 1 comment![]( "Open in Gerrit")
Theresa - PTAL
Sal / Fiaz - cc for FYI
Related details
- Theresa Wellington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Theresa Wellington (Gerrit)
Theresa Wellington voted and added 1 comment![]( "Open in Gerrit")
Votes added by Theresa Wellington
| Code-Review | +1 |
1 comment
Thanks for the great description / explanation, Mark!
This seems reasonable to me, but adding @boliu who appears to have reviewed in this space before and @mustaq@ for any needed web input expertise
Related details
- Bo Liu
- Mark Schillaci
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Schillaci (Gerrit)
Mark Schillaci added 1 comment![]( "Open in Gerrit")
- Bo Liu
- Mustaq Ahmed
Bo Liu (Gerrit)
Bo Liu added 2 comments![]( "Open in Gerrit")
// touch ACTION_DOWN is filtered out in Java. We explicitly request focuswhat is this referring to? can you link to code?
// This mirrors the touch-focus logic inside OnTouchEvent().both seem kinda sketchy though.. I would expect container view to shift focus back for these cases here:
https://source.chromium.org/chromium/chromium/src/+/main:components/embedder_support/android/java/src/org/chromium/components/embedder_support/view/ContentView.java;drc=2e070bfa61f747388fddaafd8eaf9519f89e80de;l=412
why doesn't that work?
Related details
- Mark Schillaci
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Schillaci (Gerrit)
Mark Schillaci added 3 comments![]( "Open in Gerrit")
Thank you Bo for the review! Comments addressed!
// touch ACTION_DOWN is filtered out in Java. We explicitly request focuswhat is this referring to? can you link to code?
ContentView's onTouchEvent plumbs to the EventForwarder.onTouchEvent here: https://source.chromium.org/chromium/chromium/src/+/main:components/embedder_support/android/java/src/org/chromium/components/embedder_support/view/ContentView.java;drc=511e00094eaca80cb55a3be19511128e15fb2ae7;l=511
which ultimately calls through to sendNativeMouseEvent, where the ACTION_DOWN and ACTION_UP actions are consumed before going to JNI:
https://source.chromium.org/chromium/chromium/src/+/main:ui/android/java/src/org/chromium/ui/base/EventForwarder.java;drc=fcc8e2e643f4beab78d87a7a8566f0ed50332666;l=520
Added a comment to the code, thanks!
// This mirrors the touch-focus logic inside OnTouchEvent().both seem kinda sketchy though.. I would expect container view to shift focus back for these cases here:
https://source.chromium.org/chromium/chromium/src/+/main:components/embedder_support/android/java/src/org/chromium/components/embedder_support/view/ContentView.java;drc=2e070bfa61f747388fddaafd8eaf9519f89e80de;l=412why doesn't that work?
ContentView.java#onFocusChanged is never called in this case because ContentView.java#onTouchEvent doesn't ever call super.onTouchEvent. Normally that would be where a standard Android View would request focus. But we plumb our clicks/taps down to C++ to be natively handled, and it is the C++ code that actually triggers the ContentView.java#onFocusChanged call. For OnTouchEvents (taps), the Focus() call is what triggers this via JNI: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_widget_host_view_android.cc;drc=2c42d1f2cc1b1b5730aad0524d18c94f61352c55;l=1578
but without an explicit Focus() call, RWHVA::OnMouseEvent isn't ever triggering ContentView.java#onFocusChanged.
As far as sketchiness - I think this is an artifact of managing all the touch/focus in C++ so we can send it to Blink and may be something to address at a more holistic level. This CL is really just bringing symmetry to the existing implementation to effectively complete the API contract so mouse clicks act the same way as taps.
lmkwyt!
Related details
- Bo Liu
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 2 comments![]( "Open in Gerrit")
explicitly request focus. This matches the existing touch logic for
focus in OnTouchEvent (which is why taps did not have this issue).WebView is [excluded](https://crsrc.org/c/android_webview/browser/aw_field_trials.cc;drc=44669f73ac4d53bdfd4bc0be0957405c25277eef;l=235) from that touch focusing logic. Do we want to match that special case here too? If that's not the case, please add a comment in the code explaining why.
if (!HasFocus()) {May we please guard this behind a flag? We seem to have encountered regressions when adding the corresponding touch-event focusing: https://crbug.com/378779896, https://crbug.com/373672168.
Related details
- Bo Liu
- Mark Schillaci
Mark Schillaci (Gerrit)
Mark Schillaci added 3 comments![]( "Open in Gerrit")
Thank you Mustaq for the review! Comments addressed!
explicitly request focus. This matches the existing touch logic for
focus in OnTouchEvent (which is why taps did not have this issue).WebView is [excluded](https://crsrc.org/c/android_webview/browser/aw_field_trials.cc;drc=44669f73ac4d53bdfd4bc0be0957405c25277eef;l=235) from that touch focusing logic. Do we want to match that special case here too? If that's not the case, please add a comment in the code explaining why.
Oh great catch, thank you! So I did some research and I think we want to include this even on WebView. The issue with touchs/taps for WebViews AFAIU is that they would steal the focus from the native views. Even scrolling on the page would still the focus. However mouse clicks are not for scrolling and would not steal focus during a scroll, but clicks are meant to bring focus to the view. I added a comment in the code for the same.
please lmkwyt!
if (!HasFocus()) {May we please guard this behind a flag? We seem to have encountered regressions when adding the corresponding touch-event focusing: https://crbug.com/378779896, https://crbug.com/373672168.
Sure, sounds good!
I have an in-flight CL to add a flag, once landed I'll use it in this CL: crrev.com/c/7884688
Moving this CL to WIP until that time. Thanks!
Related details
- Bo Liu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu added 2 comments![]( "Open in Gerrit")
// touch ACTION_DOWN is filtered out in Java. We explicitly request focusMark Schillaciwhat is this referring to? can you link to code?
ContentView's onTouchEvent plumbs to the EventForwarder.onTouchEvent here: https://source.chromium.org/chromium/chromium/src/+/main:components/embedder_support/android/java/src/org/chromium/components/embedder_support/view/ContentView.java;drc=511e00094eaca80cb55a3be19511128e15fb2ae7;l=511
which ultimately calls through to sendNativeMouseEvent, where the ACTION_DOWN and ACTION_UP actions are consumed before going to JNI:
https://source.chromium.org/chromium/chromium/src/+/main:ui/android/java/src/org/chromium/ui/base/EventForwarder.java;drc=fcc8e2e643f4beab78d87a7a8566f0ed50332666;l=520Added a comment to the code, thanks!
well, "filtering" sounds kinda weird.. It's more like ContentView "handles" the event by returning true?
but according to the other comment, it's not returning true that's the problem, it's not calling the default onTouchEvent (ie calling super.onTouchEvent, which we obviously do not want to do) that's skipping focus request?
// This mirrors the touch-focus logic inside OnTouchEvent().Mark Schillaciboth seem kinda sketchy though.. I would expect container view to shift focus back for these cases here:
https://source.chromium.org/chromium/chromium/src/+/main:components/embedder_support/android/java/src/org/chromium/components/embedder_support/view/ContentView.java;drc=2e070bfa61f747388fddaafd8eaf9519f89e80de;l=412why doesn't that work?
ContentView.java#onFocusChanged is never called in this case because ContentView.java#onTouchEvent doesn't ever call super.onTouchEvent. Normally that would be where a standard Android View would request focus. But we plumb our clicks/taps down to C++ to be natively handled, and it is the C++ code that actually triggers the ContentView.java#onFocusChanged call. For OnTouchEvents (taps), the Focus() call is what triggers this via JNI: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_widget_host_view_android.cc;drc=2c42d1f2cc1b1b5730aad0524d18c94f61352c55;l=1578
but without an explicit Focus() call, RWHVA::OnMouseEvent isn't ever triggering ContentView.java#onFocusChanged.As far as sketchiness - I think this is an artifact of managing all the touch/focus in C++ so we can send it to Blink and may be something to address at a more holistic level. This CL is really just bringing symmetry to the existing implementation to effectively complete the API contract so mouse clicks act the same way as taps.
lmkwyt!
existing implementation is hacky, and buggy on webview..
can this be handled entirely within java side? so ContentView for chrome, and AwContents for webview
Related details
- Mark Schillaci