Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(182)

Issue 2779033004: [Android] Focus/Blur contents when window focus changes (Closed)

Created:
3 years, 8 months ago by mthiesse
Modified:
3 years, 8 months ago
CC:
agrieve+watch_chromium.org, Changwan Ryu, chromium-reviews, darin-cc_chromium.org, feature-vr-reviews_chromium.org, jam, Jinsuk Kim
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Android] Focus/Blur contents when window focus changes This ensure that when an activity loses input focus (OnWindowFocusChanged), that the web page is blurred and not considered to be focused. BUG=686232 Review-Url: https://codereview.chromium.org/2779033004 Cr-Commit-Position: refs/heads/master@{#463390} Committed: https://chromium.googlesource.com/chromium/src/+/460ffe75e23987ac6760d668963d9d2f1a83962b

Patch Set 1 #

Patch Set 2 : Add missing file #

Total comments: 5

Patch Set 3 : Address comments #

Total comments: 8

Patch Set 4 : Address comments #

Patch Set 5 : preemptive nit #

Total comments: 2

Patch Set 6 : rebase + fix nit #

Total comments: 2

Patch Set 7 : address comments #

Patch Set 8 : Fix initialization order issues. #

Patch Set 9 : Fix tests #

Patch Set 10 : Add comments #

Total comments: 2

Patch Set 11 : Fix popups #

Patch Set 12 : Fix tests #

Patch Set 13 : fix compile #

Patch Set 14 : rebase #

Patch Set 15 : Use pause/resume instead of WindowFocusChanged #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -3 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +5 lines, -3 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +49 lines, -0 lines 0 comments Download
A chrome/test/data/android/content_view_focus/content_view_blur_focus.html View 1 1 chunk +10 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +22 lines, -0 lines 0 comments Download

Messages

Total messages: 74 (36 generated)
mthiesse
PTAL
3 years, 8 months ago (2017-03-29 19:59:38 UTC) #2
mthiesse
skyostil@chromium.org: Please review changes in chrome/android/javatests.
3 years, 8 months ago (2017-03-29 20:05:46 UTC) #5
aelias_OOO_until_Jul13
https://codereview.chromium.org/2779033004/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2779033004/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode1340 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1340: if (hasFocus()) onFocusChanged(false, true /* hideKeyboardOnBlur */); This is ...
3 years, 8 months ago (2017-03-29 21:54:04 UTC) #8
Changwan Ryu
https://codereview.chromium.org/2779033004/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2779033004/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode1343 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1343: } Because ImeAdapter and SelectionPopupController already handle window and ...
3 years, 8 months ago (2017-03-30 00:05:50 UTC) #16
mthiesse
https://codereview.chromium.org/2779033004/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2779033004/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode1343 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1343: } On 2017/03/30 00:05:49, Changwan Ryu wrote: > Because ...
3 years, 8 months ago (2017-03-30 00:12:46 UTC) #17
mthiesse
ugh. getContainerView().hasWindowFocus() returns false whenever the View doesn't have view focus. It only returns true ...
3 years, 8 months ago (2017-03-30 00:23:26 UTC) #18
mthiesse
Note that I didn't implement exactly as you requested, because tests still need to override ...
3 years, 8 months ago (2017-03-30 00:49:27 UTC) #19
aelias_OOO_until_Jul13
https://codereview.chromium.org/2779033004/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2779033004/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode115 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:115: private final Map<String, Pair<Object, Class>> mJavaScriptInterfaces = new HashMap<>(); ...
3 years, 8 months ago (2017-03-30 02:06:55 UTC) #20
mthiesse
https://codereview.chromium.org/2779033004/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2779033004/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode115 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:115: private final Map<String, Pair<Object, Class>> mJavaScriptInterfaces = new HashMap<>(); ...
3 years, 8 months ago (2017-03-30 14:51:51 UTC) #21
aelias_OOO_until_Jul13
lgtm https://codereview.chromium.org/2779033004/diff/80001/chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java (right): https://codereview.chromium.org/2779033004/diff/80001/chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java#newcode40 chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java:40: private final ArrayDeque<Boolean> mFocusChanges = new ArrayDeque<>(); You ...
3 years, 8 months ago (2017-03-30 18:10:35 UTC) #22
mthiesse
https://codereview.chromium.org/2779033004/diff/80001/chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java (right): https://codereview.chromium.org/2779033004/diff/80001/chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java#newcode40 chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java:40: private final ArrayDeque<Boolean> mFocusChanges = new ArrayDeque<>(); On 2017/03/30 ...
3 years, 8 months ago (2017-03-30 18:42:31 UTC) #23
Sami
lgtm. https://codereview.chromium.org/2779033004/diff/100001/chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java (right): https://codereview.chromium.org/2779033004/diff/100001/chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java#newcode193 chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java:193: mObserver = new WebContentsObserver(getActivity().getActivityTab().getWebContents()) { nit: Looks like ...
3 years, 8 months ago (2017-04-03 10:23:45 UTC) #24
mthiesse
https://codereview.chromium.org/2779033004/diff/100001/chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java (right): https://codereview.chromium.org/2779033004/diff/100001/chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java#newcode193 chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java:193: mObserver = new WebContentsObserver(getActivity().getActivityTab().getWebContents()) { On 2017/04/03 10:23:45, Sami ...
3 years, 8 months ago (2017-04-03 15:50:03 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2779033004/120001
3 years, 8 months ago (2017-04-03 15:51:09 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/149047)
3 years, 8 months ago (2017-04-03 17:27:12 UTC) #30
mthiesse
boliu@chromium.org: Please review changes in android_webview/
3 years, 8 months ago (2017-04-03 19:52:39 UTC) #36
boliu
android_webview lgtm I assume aelias already reviewed content, so I didn't
3 years, 8 months ago (2017-04-03 19:59:46 UTC) #37
mthiesse
aelias, please re-review the changes to setContainerView. OnFocusChanged/OnWindowFocusChanged currently assume they'll only be called after ...
3 years, 8 months ago (2017-04-03 21:02:40 UTC) #38
aelias_OOO_until_Jul13
https://codereview.chromium.org/2779033004/diff/180001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2779033004/diff/180001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode560 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:560: setContainerView(viewDelegate.getContainerView(), false); Could you try just moving this to ...
3 years, 8 months ago (2017-04-03 21:57:56 UTC) #39
mthiesse
Well this is very annoying. When Android shows a popup, the CVC loses window focus, ...
3 years, 8 months ago (2017-04-04 18:42:49 UTC) #40
mthiesse
Note that I also fixed the issue of popups automatically closing after opening - by ...
3 years, 8 months ago (2017-04-05 14:11:23 UTC) #47
aelias_OOO_until_Jul13
Hmm, on further thought, not lgtm as is, sorry. I get the sense that the ...
3 years, 8 months ago (2017-04-05 19:23:42 UTC) #48
mthiesse
On 2017/04/05 19:23:42, aelias wrote: > Hmm, on further thought, not lgtm as is, sorry. ...
3 years, 8 months ago (2017-04-05 19:26:43 UTC) #49
mthiesse
On 2017/04/05 19:26:43, mthiesse wrote: > On 2017/04/05 19:23:42, aelias wrote: > > Hmm, on ...
3 years, 8 months ago (2017-04-05 19:33:17 UTC) #50
aelias_OOO_until_Jul13
Let's leave IME behavior as it unless there's a good reason to change it. I'd ...
3 years, 8 months ago (2017-04-05 20:01:47 UTC) #51
mthiesse
Alright, changed to use pause/resume. PTAL
3 years, 8 months ago (2017-04-06 01:00:36 UTC) #56
aelias_OOO_until_Jul13
I would've preferred onStart/onStop, is there a reason you used onPause/onResume instead? Those have a ...
3 years, 8 months ago (2017-04-06 01:06:07 UTC) #57
aelias_OOO_until_Jul13
jinsukkim@, could you take a look at this and see if the additional calls to ...
3 years, 8 months ago (2017-04-06 01:08:36 UTC) #58
mthiesse
On 2017/04/06 01:06:07, aelias wrote: > I would've preferred onStart/onStop, is there a reason you ...
3 years, 8 months ago (2017-04-06 01:13:21 UTC) #59
Jinsuk Kim
On 2017/04/06 01:08:36, aelias wrote: > jinsukkim@, could you take a look at this and ...
3 years, 8 months ago (2017-04-06 01:21:26 UTC) #61
aelias_OOO_until_Jul13
OK. It does seem like the best one to use in principle. Please wait for ...
3 years, 8 months ago (2017-04-06 01:22:29 UTC) #62
aelias_OOO_until_Jul13
OK, our messages raced, lgtm.
3 years, 8 months ago (2017-04-06 01:23:19 UTC) #63
mthiesse
dtrainor, please review ChromeActivity changes.
3 years, 8 months ago (2017-04-06 01:36:06 UTC) #65
David Trainor- moved to gerrit
have you checked what this does in multiwindow? is the behavior what you expect/want?
3 years, 8 months ago (2017-04-06 21:44:41 UTC) #66
mthiesse
On 2017/04/06 21:44:41, David Trainor-ping if over 24h wrote: > have you checked what this ...
3 years, 8 months ago (2017-04-06 21:53:29 UTC) #67
David Trainor- moved to gerrit
Sorry I missed your reply. lgtm!
3 years, 8 months ago (2017-04-10 18:42:38 UTC) #68
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2779033004/280001
3 years, 8 months ago (2017-04-10 18:55:14 UTC) #71
commit-bot: I haz the power
3 years, 8 months ago (2017-04-10 21:09:49 UTC) #74
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/460ffe75e23987ac6760d668963d...

Powered by Google App Engine
This is Rietveld 408576698