|
|
Created:
5 years, 2 months ago by ymalik Modified:
4 years, 9 months ago CC:
AKV, Changwan Ryu, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, jdduke (slow), nasko+codewatch_chromium.org, nona+watch_chromium.org, Rick Byers, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionResize only the virtual viewport when the OSK triggers a resize.
Initial CL in unifying the keyboard behavior between ChromeOS and Android.
If the enable-osk-overscroll flag is set, this change will keep the Blink
viewport size stable and set the visible_viewport_size to the smaller value.
This CL adds a new View (InsetObserverView) to the View hierarchy that will
store the value of insets (OSK, status bar). When there is a resize due to
OSK show, we keep the view bounds the same and change the visible viewport
size.
Design doc: http://go/osk-unification
BUG=404315
Committed: https://crrev.com/3edbd8d05df275432f79649ac740070d329b26ea
Cr-Commit-Position: refs/heads/master@{#381590}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Worked on review comments. #
Total comments: 2
Patch Set 3 : rebase master Inset handling logic #
Total comments: 30
Patch Set 4 : address review comments #
Total comments: 2
Patch Set 5 : moved insets to InsetConsumerView #Patch Set 6 : added test #Patch Set 7 : fix test #Patch Set 8 : fix select bar bug #
Total comments: 3
Patch Set 9 : add inset consumer view one off the root view #
Total comments: 28
Patch Set 10 : Added InsetConsumerView as root view's first child + other review comments #Patch Set 11 : #
Total comments: 10
Patch Set 12 : addressed more review comments #Patch Set 13 : s/InsetConsumerView/InsetObserverView #Messages
Total messages: 72 (14 generated)
ymalik@chromium.org changed reviewers: + bokan@chromium.org, jdduke@chromium.org
Hey guys, can you please take a look and let me know what you think of the approach here. Note that I tested this manually, but still need to write tests and ensure that having the flag on doesn't break anything. Just need an agreement on the approach here.
https://codereview.chromium.org/1386403003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1386403003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:621: return gfx::Rect(content_view_core_->GetViewSizeWithoutOSK()); @jdduke GetViewBounds has a bunch of callers. Having this flag on will always return the size of the layout viewport. This seems reasonable to me, but are there any tests around this for android that I can run locally? Also, are there any android try bots that I can run locally to ensure nothing breaks?
Some less consequential comments below. Overall, the approach looks fine to me but Jared has more insight in this area than I do. Perhaps we should instead have OSKHeight and OSKShrinksBlinkSize variables to mirror the TopControls case? (OSKShrinksBlinkSize would then always be false if your flag is on) https://codereview.chromium.org/1386403003/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1386403003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5572: + Enable OSK overscroll support. With this flag on, the resize events by the OSK will only resize the visible viewport. I would remove "the resize events by" and s/visible/visual. "With this flag on, the OSK will only resize the visual viewport." sounds more succinct and technically correct. https://codereview.chromium.org/1386403003/diff/1/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1386403003/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:716: if (j_obj.is_null()) I think you should skip the bits below if this is null, yes? https://codereview.chromium.org/1386403003/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:722: gfx::Size size_dip = gfx::ScaleToCeiledSize(size_pix, 1.0f / dpi_scale()); Why not just make ContentViewCore.getViewportHeightWithoutOSK return DIPs? https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:505: private int mViewportHeightWithoutOSKPix; This is misleading since it implies we're excluding the OSK rect from the screen rect. I think this should be ViewportHeightIncludingOSK or ViewportHeightWithOSKHidden or something like that. https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1628: public void onSizeChanged(int wPix, int hPix, int owPix, int ohPix) { It seems we might now always get an onSizeChanged: crbug.com/294908 Jared might know if this is ok or what we can do about it. https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1631: boolean displayRectChanged = false, triggeredByOSKShow = false; Nit: Variable declarations should get their own lines. https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1644: // in which case the mViewportHeightWithoutOSK is hPix. Comments should describe the "why?" not the "how". Your last sentence "in which case the mViewportHeightWithoutOSK is hPix" adds nothing to understanding. Why is it hPix? A: "there's no OSK to include in the height" https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1668: cancelRequestToScrollFocusedEditableNodeIntoView(); Is this accidentally duplicated? It makes the above 'if' nonsensical.
https://codereview.chromium.org/1386403003/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1386403003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5572: + Enable OSK overscroll support. With this flag on, the resize events by the OSK will only resize the visible viewport. On 2015/10/08 22:38:21, bokan wrote: > I would remove "the resize events by" and s/visible/visual. "With this flag on, > the OSK will only resize the visual viewport." sounds more succinct and > technically correct. Done. https://codereview.chromium.org/1386403003/diff/1/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1386403003/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:716: if (j_obj.is_null()) On 2015/10/08 22:38:21, bokan wrote: > I think you should skip the bits below if this is null, yes? Absolutely. https://codereview.chromium.org/1386403003/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:722: gfx::Size size_dip = gfx::ScaleToCeiledSize(size_pix, 1.0f / dpi_scale()); On 2015/10/08 22:38:21, bokan wrote: > Why not just make ContentViewCore.getViewportHeightWithoutOSK return DIPs? It seems to be the case that ContentViewCore.getViewportHeight returns the size in pix. Its because some callers for that need the pix value. I tried to be consistent, but don't really have a strong preference. https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:505: private int mViewportHeightWithoutOSKPix; On 2015/10/08 22:38:21, bokan wrote: > This is misleading since it implies we're excluding the OSK rect from the screen > rect. I think this should be ViewportHeightIncludingOSK or > ViewportHeightWithOSKHidden or something like that. Done. https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1631: boolean displayRectChanged = false, triggeredByOSKShow = false; On 2015/10/08 22:38:21, bokan wrote: > Nit: Variable declarations should get their own lines. Done. https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1644: // in which case the mViewportHeightWithoutOSK is hPix. On 2015/10/08 22:38:21, bokan wrote: > Comments should describe the "why?" not the "how". Your last sentence "in which > case the mViewportHeightWithoutOSK is hPix" adds nothing to understanding. Why > is it hPix? A: "there's no OSK to include in the height" Thanks. Done. https://codereview.chromium.org/1386403003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1668: cancelRequestToScrollFocusedEditableNodeIntoView(); On 2015/10/08 22:38:21, bokan wrote: > Is this accidentally duplicated? It makes the above 'if' nonsensical. Most definitely an accident.
On 2015/10/08 22:38:21, bokan wrote: > Some less consequential comments below. Overall, the approach looks fine to me > but Jared has more insight in this area than I do. Perhaps we should instead > have OSKHeight and OSKShrinksBlinkSize variables to mirror the TopControls case? > (OSKShrinksBlinkSize would then always be false if your flag is on) I think that's another way to go about the solution. But, OSKShrinksBlinkSize sounds like we're changing the height of the layout viewport, when what we actually want is to resize the virtual viewport. I believe in the case of TopControls, we wan't to actually resize the LayoutViewport? So the semantics there are slightly different. I think this implementation is clear in communicating that "having this flag on will resize the virtual viewport when the OSK comes up"
On 2015/10/09 01:18:17, ymalik1 wrote: > On 2015/10/08 22:38:21, bokan wrote: > > Some less consequential comments below. Overall, the approach looks fine to me > > but Jared has more insight in this area than I do. Perhaps we should instead > > have OSKHeight and OSKShrinksBlinkSize variables to mirror the TopControls > case? > > (OSKShrinksBlinkSize would then always be false if your flag is on) > > I think that's another way to go about the solution. But, OSKShrinksBlinkSize > sounds > like we're changing the height of the layout viewport, That's exactly what I'm proposing. Turning on your flag would make ShrinksBlinkSize = false. > > I think this implementation is clear in communicating that "having this flag on > will > resize the virtual viewport when the OSK comes up" Sure, if Jared's fine with this approach I'm ok with it. We'll probably want to have a larger rethink of how this should work in a more general case of popup bubbles and the like but I don't think we should block on that.
On 2015/10/09 13:16:01, bokan wrote: > On 2015/10/09 01:18:17, ymalik1 wrote: > > On 2015/10/08 22:38:21, bokan wrote: > > > Some less consequential comments below. Overall, the approach looks fine to > me > > > but Jared has more insight in this area than I do. Perhaps we should instead > > > have OSKHeight and OSKShrinksBlinkSize variables to mirror the TopControls > > case? > > > (OSKShrinksBlinkSize would then always be false if your flag is on) > > > > I think that's another way to go about the solution. But, OSKShrinksBlinkSize > > sounds > > like we're changing the height of the layout viewport, > > That's exactly what I'm proposing. Turning on your flag would make > ShrinksBlinkSize = false. > > > > > I think this implementation is clear in communicating that "having this flag > on > > will > > resize the virtual viewport when the OSK comes up" > > Sure, if Jared's fine with this approach I'm ok with it. We'll probably want to > have a larger rethink of how this should work in a more general case of popup > bubbles and the like but I don't think we should block on that. Agreed that we will want to have a more general approach to this. This is just there to kind of gauge the things it will break.
On 2015/10/09 13:16:01, bokan wrote: > On 2015/10/09 01:18:17, ymalik1 wrote: > > On 2015/10/08 22:38:21, bokan wrote: > > > Some less consequential comments below. Overall, the approach looks fine to > me > > > but Jared has more insight in this area than I do. Perhaps we should instead > > > have OSKHeight and OSKShrinksBlinkSize variables to mirror the TopControls > > case? > > > (OSKShrinksBlinkSize would then always be false if your flag is on) > > > > I think that's another way to go about the solution. But, OSKShrinksBlinkSize > > sounds > > like we're changing the height of the layout viewport, > > That's exactly what I'm proposing. Turning on your flag would make > ShrinksBlinkSize = false. > > > > > I think this implementation is clear in communicating that "having this flag > on > > will > > resize the virtual viewport when the OSK comes up" > > Sure, if Jared's fine with this approach I'm ok with it. We'll probably want to > have a larger rethink of how this should work in a more general case of popup > bubbles and the like but I don't think we should block on that. Agreed that we will want to have a more general approach to this. This is just there to kind of gauge the things it will break.
jdduke@chromium.org changed reviewers: + aelias@chromium.org
Hard to comment further without seeing this wired up. +aelias,changwan who have been working more in this area recently. https://codereview.chromium.org/1386403003/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1386403003/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1386: switches::kEnableOSKOverscroll, This flag is unneeded by the renderer process, right? https://codereview.chromium.org/1386403003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1386403003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1648: mViewportHeightWithOSKHiddenPix = mViewportHeightPix; Like I said, the current heuristic is less than perfect for guessing that this is an OSK show. At some point we might need to tighten the logic for cases like dual window (http://www.androidcentral.com/using-dual-window-lg-g4) where the window is resized is a persistent way vertically, e.g., make sure the height *decreases* for the OSK showing case.
Let's not introduce a flag for this. Please write an end-to-end change you believe should work for all cases and let's just land that on trunk. If there's a bug with it, we can revert it. Flag-based development is painful and there's much less reason to do things that way now that Blink is merged. Please make sure to test with fullscreen API, as that's a weird corner case for viewport size. Try making videos fullscreen on http://m.youtube.com/, going back, then make sure the Youtube search box (behind the magnifying glass icon) brings up OSK properly.
On 2015/10/09 21:56:30, aelias wrote: > Let's not introduce a flag for this. Please write an end-to-end change you > believe should work for all cases and let's just land that on trunk. If there's > a bug with it, we can revert it. Flag-based development is painful and there's > much less reason to do things that way now that Blink is merged. I'm not sure we can just go straight to landing it. We don't have a plausible story for sites that want to keep some bar above the keyboard (G+ does this). I think device-fixed is our solution to that but I'd like us to experiment with this without blocking on that. The other option is to enable this for desktop sites only, where there isn't any expectation of an OSK. Thinking about it now, that's probably the better way forward (I expect most of the benefits here to be for legacy/desktop sites anyway).
ymalik@chromium.org changed reviewers: + rbyers@chromium.org
On 2015/10/09 21:56:30, aelias wrote: > Let's not introduce a flag for this. Please write an end-to-end change you > believe should work for all cases and let's just land that on trunk. If there's > a bug with it, we can revert it. Flag-based development is painful and there's > much less reason to do things that way now that Blink is merged. > > Please make sure to test with fullscreen API, as that's a weird corner case for > viewport size. Try making videos fullscreen on http://m.youtube.com/, going > back, then make sure the Youtube search box (behind the magnifying glass icon) > brings up OSK properly. The reason that this change is behind a flag is that we wanted an easy way to gauge the number of things that break if the OSK is made relative to the visual viewport. One case is that of "position:fixed", where the fixed element would appear behind the OSK (shipping this change is blocked on issue 542770). But I do agree that flag-based development is painful and should be avoided. Perhaps we should shelve this change until the blocking features are in? WDYT? +rybers for his opinion.
On 2015/10/13 17:54:09, bokan wrote: > On 2015/10/09 21:56:30, aelias wrote: > > Let's not introduce a flag for this. Please write an end-to-end change you > > believe should work for all cases and let's just land that on trunk. If > there's > > a bug with it, we can revert it. Flag-based development is painful and > there's > > much less reason to do things that way now that Blink is merged. > > I'm not sure we can just go straight to landing it. We don't have a plausible > story for sites that want to keep some bar above the keyboard (G+ does this). I > think device-fixed is our solution to that but I'd like us to experiment with > this without blocking on that. The other option is to enable this for desktop > sites only, where there isn't any expectation of an OSK. Thinking about it now, > that's probably the better way forward (I expect most of the benefits here to be > for legacy/desktop sites anyway). Oops just saw that you already replied to this. I do think that enabling it for legacy/desktop sites is a good option instead of blocking on device-fixed.
On 2015/10/13 at 17:54:09, bokan wrote: > I'm not sure we can just go straight to landing it. We don't have a plausible story for sites that want to keep some bar above the keyboard (G+ does this). I think device-fixed is our solution to that but I'd like us to experiment with this without blocking on that. The other option is to enable this for desktop sites only, where there isn't any expectation of an OSK. Thinking about it now, that's probably the better way forward (I expect most of the benefits here to be for legacy/desktop sites anyway). I just tried Google+ on Safari on my iPod and it has that problem already. So I don't agree that's a blocker. Google mobile web teams primarily target iOS Safari anyway, and I predict they wouldn't even bother to add position: device-fixed lines of CSS just for Chrome. > The reason that this change is behind a flag is that we wanted an easy way to gauge the number of things that break if the OSK is made relative to the visual viewport. A better way to gauge the number of things that break is to ship the change to beta channel.
On 2015/10/13 18:35:16, aelias wrote: > On 2015/10/13 at 17:54:09, bokan wrote: > > I'm not sure we can just go straight to landing it. We don't have a plausible > story for sites that want to keep some bar above the keyboard (G+ does this). I > think device-fixed is our solution to that but I'd like us to experiment with > this without blocking on that. The other option is to enable this for desktop > sites only, where there isn't any expectation of an OSK. Thinking about it now, > that's probably the better way forward (I expect most of the benefits here to be > for legacy/desktop sites anyway). > > I just tried Google+ on Safari on my iPod and it has that problem already. So I > don't agree that's a blocker. Google mobile web teams primarily target iOS > Safari anyway, and I predict they wouldn't even bother to add position: > device-fixed lines of CSS just for Chrome. I don't agree. I've spent a lot of time talking with the Tau / mobile G+ teams and they aim to build the best possible experience on Chrome, and know they have to live with some sub-optimal behavior on Safari (where they have less ability to get browser behavior fixed). I could start a discussion with them around this, but I'm fairly sure they'll see this as a regression and would instead want us moving the web in a direction that would let them provide this user experience on all browsers. Let's focus on answering this question first - should we ship this change without also shipping position: device-fixed. > > The reason that this change is behind a flag is that we wanted an easy way to > gauge the number of things that break if the OSK is made relative to the visual > viewport. > > A better way to gauge the number of things that break is to ship the change to > beta channel. New/changed web platform features are often launched first behind a RuntimeEnabledFeature so we can get some feedback from web developers using --enable-experimental-web-platform-features. But I agree adding a whole new chrome flag for this seems like a lot of extra pain - perhaps we just want a status=experimental RuntimeEnabledFeature instead? In particular, if we end up agreeing that we really can't ship this change without also shipping device-fixed, then perhaps this should all be covered by a single RuntimeEnabledFeature?
On 2015/10/15 at 00:05:53, rbyers wrote: > New/changed web platform features are often launched first behind a RuntimeEnabledFeature so we can get some feedback from web developers using --enable-experimental-web-platform-features. But I agree adding a whole new chrome flag for this seems like a lot of extra pain - perhaps we just want a status=experimental RuntimeEnabledFeature instead? > > In particular, if we end up agreeing that we really can't ship this change without also shipping device-fixed, then perhaps this should all be covered by a single RuntimeEnabledFeature? That sounds OK, but I've never seen a RuntimeEnabledFeature affect browser-side code. If it's easy/clean to do then great, but if it's a huge mess then I guess the flag is fine. Looking again at what code it affects, the forking is not so great that it's really worth pushing back on any further, you can go ahead if you feel the flag will be useful for your deployment plan. Beyond that, I'm starting to have a bad feeling about the heuristic way this determines the keyboard height. By design, the Android system hides from apps the keyboard size, so this approach is overpromising to the web platform knowledge that we don't really have.. Tying a web-facing behavior to a brittle guess seems like a problem, particularly with multi-window (which may already be broken on the LG and Samsung forks that ship it today, and may also be if/when Android officially adds such a feature). Could you buy one of the multi-window supporting devices from each vendor and see how they interact with this? There are two types of OSKs, "overlay-style" -- as on iOS, ChromeOS and Windows (I think?) -- and "resize-style" -- as on Android (and maybe nothing else?) -- and maybe it actually makes sense to only tie the visual viewport size to the overlay-style ones. If we really are determined to resolve this divergence, maybe the right place to start with a feature request to Android team to have an option to create the OSK as an overlay.
On 2015/10/15 01:16:41, aelias wrote: > Beyond that, I'm starting to have a bad feeling about the heuristic way this > determines the keyboard height. By design, the Android system hides from apps > the keyboard size, so this approach is overpromising to the web platform > knowledge that we don't really have.. Tying a web-facing behavior to a brittle > guess seems like a problem, particularly with multi-window (which may already be > broken on the LG and Samsung forks that ship it today, and may also be if/when > Android officially adds such a feature). Could you buy one of the multi-window > supporting devices from each vendor and see how they interact with this? Right, that's the concern I shared in reply #11. As for which devices have multi-window, I know the LG G4 supports it.
On 2015/10/15 01:16:41, aelias wrote: > On 2015/10/15 at 00:05:53, rbyers wrote: > > New/changed web platform features are often launched first behind a > RuntimeEnabledFeature so we can get some feedback from web developers using > --enable-experimental-web-platform-features. But I agree adding a whole new > chrome flag for this seems like a lot of extra pain - perhaps we just want a > status=experimental RuntimeEnabledFeature instead? > > > > In particular, if we end up agreeing that we really can't ship this change > without also shipping device-fixed, then perhaps this should all be covered by a > single RuntimeEnabledFeature? > > That sounds OK, but I've never seen a RuntimeEnabledFeature affect browser-side > code. If it's easy/clean to do then great, but if it's a huge mess then I guess > the flag is fine. Looking again at what code it affects, the forking is not so > great that it's really worth pushing back on any further, you can go ahead if > you feel the flag will be useful for your deployment plan. > > > Beyond that, I'm starting to have a bad feeling about the heuristic way this > determines the keyboard height. By design, the Android system hides from apps > the keyboard size, so this approach is overpromising to the web platform > knowledge that we don't really have.. Tying a web-facing behavior to a brittle > guess seems like a problem, particularly with multi-window (which may already be > broken on the LG and Samsung forks that ship it today, and may also be if/when > Android officially adds such a feature). Could you buy one of the multi-window > supporting devices from each vendor and see how they interact with this? > > There are two types of OSKs, "overlay-style" -- as on iOS, ChromeOS and Windows > (I think?) -- and "resize-style" -- as on Android (and maybe nothing else?) -- > and maybe it actually makes sense to only tie the visual viewport size to the > overlay-style ones. If we really are determined to resolve this divergence, > maybe the right place to start with a feature request to Android team to have an > option to create the OSK as an overlay. Interesting. I agree we don't want to rely on brittle heuristics here and over-promise something we can't really deliver reliably. Doesn't Android really support both of these modes with android:windowSoftInputMode adjustResize and adjustPan (http://developer.android.com/guide/topics/manifest/activity-element.html)? To me we really just want 'adjustPan' behavior except that we control our own panning. How would the feature we'd ask the Android team for differ from 'adjustPan'?
On 2015/10/15 at 01:57:29, rbyers wrote: > Interesting. I agree we don't want to rely on brittle heuristics here and over-promise something we can't really deliver reliably. > > Doesn't Android really support both of these modes with android:windowSoftInputMode adjustResize and adjustPan > (http://developer.android.com/guide/topics/manifest/activity-element.html)? To me we really just want 'adjustPan' behavior except that we control our own panning. How would the feature we'd ask the Android team for differ from 'adjustPan'? Ah, right. I think tedchoc@ mentioned to me in the past that this mode existed. But it would only be usable if we could query the size somehow. Without a new API, it wouldn't be usable at all, unless we did an even more questionable hack like creating an invisible ScrollView and checking by how many pixels it got panned.
On 2015/10/15 02:58:12, aelias wrote: > On 2015/10/15 at 01:57:29, rbyers wrote: > > Interesting. I agree we don't want to rely on brittle heuristics here and > over-promise something we can't really deliver reliably. > > > > Doesn't Android really support both of these modes with > android:windowSoftInputMode adjustResize and adjustPan > > (http://developer.android.com/guide/topics/manifest/activity-element.html) > To me we really just want 'adjustPan' behavior except that we control our own > panning. How would the feature we'd ask the Android team for differ from > 'adjustPan'? > > Ah, right. I think tedchoc@ mentioned to me in the past that this mode existed. > But it would only be usable if we could query the size somehow. Without a new > API, it wouldn't be usable at all, unless we did an even more questionable hack > like creating an invisible ScrollView and checking by how many pixels it got > panned. Ugh, right. Do you think it's worth doing some digging to try to confirm that there's really no good way for us to hook into this auto-panning process? I guess we can think of Chrome as really needing both modes simultaneously - the layout viewport getting overlaid while the visual viewport is resized. I like the idea of working with the Android team on a feature request here, but hopefully we can do both - have a heuristic that's good enough in practice with a plan to eliminate the need for that heuristic in a future version of Android.
Sure, feel free to dig in, read the Android source code etc to learn about if there's some trick to get the information more reliably. As you say, it can be more reasonable to ship a somehow brittle hack on older versions only, in that we only need to prove that it works on "all existing versions" rather than "anything possible moving forward".
The adjustPan mode is problematic because it only guarantees that it'll scroll the cursor into view. The user isn't able to scroll the view around. They'd have to improve the usability in adjustPan mode. We can use adjustResize if they'd just give us a little more information. The crux here is that we need to know whether a resize comes from the OSK or something else since we want to do different things. A new window being added on the screen should cause Blink to relayout into that new size. The OSK coming up should not. FWIW, I've started prodding the Anrdoid team to expose some more keyboard information in b/25992772. This has been dismissed in the past (it seems to be a common request on StackOverflow and the web) but I think with multiwindow on the horizon we may be able to get something usable. I suspect the Samsung split screen experience is niche and probably already flaky in all sorts of other cases so I wouldn't block on getting that to work. Perhaps we could just special case those devices/modes? I think that if we at least have a plan from the Android team on how to make this more reliable for Android multi-window then we could move forward with the heuristic as it is now.
On 2015/12/04 21:23:09, bokan wrote: > The adjustPan mode is problematic because it only guarantees that it'll scroll > the cursor into view. The user isn't able to scroll the view around. They'd have > to improve the usability in adjustPan mode. > > We can use adjustResize if they'd just give us a little more information. The > crux here is that we need to know whether a resize comes from the OSK or > something else since we want to do different things. A new window being added on > the screen should cause Blink to relayout into that new size. The OSK coming up > should not. > > FWIW, I've started prodding the Anrdoid team to expose some more keyboard > information in b/25992772. This has been dismissed in the past (it seems to be a > common request on StackOverflow and the web) but I think with multiwindow on the > horizon we may be able to get something usable. > > I suspect the Samsung split screen experience is niche and probably already > flaky in all sorts of other cases so I wouldn't block on getting that to work. > Perhaps we could just special case those devices/modes? I think that if we at > least have a plan from the Android team on how to make this more reliable for > Android multi-window then we could move forward with the heuristic as it is now. Just wanted to bring back this discussion To summarize, the following concerns were raised >> Let's not introduce a flag for this. Please write an end-to-end change you >> believe should work for all cases and let's just land that on trunk. We concluded that landing it without an alternative (position:device-fixed) will cause sites like G+ to break, that care a lot about user experience. So we can launch it behind a flag for now (RuntimeEnabledFeature not supported in the browser) > I'm starting to have a bad feeling about the heuristic way this > determines the keyboard height. By design, the Android system hides from apps > the keyboard size, so this approach is overpromising to the web platform > knowledge that we don't really have. > If we really are determined to resolve this divergence, > maybe the right place to start with a feature request to Android team to have an > option to create the OSK as an overlay. We concluded that it's a good idea to work with the Android team on a feature request to determine this information more reliably for future versions of Android, but the current heuristic may suffice for all existing versions. We may not need this, see below. > We can use adjustResize if they'd just give us a little more information. The > crux here is that we need to know whether a resize comes from the OSK or > something else since we want to do different things I agree that crux here is that we need to know whether a resize comes from the OSK or something else. @aelias Digging into the code further, I see that we have a callback that gets called everytime the OSK is shown (see getNewShowKeyboardReceiver). Here we set mFocusPreOSKViewportRect, which we use in onSizeChanged. Can we not just use this instead of the “did width change” heuristic?
On 2016/01/07 at 17:39:59, ymalik wrote: > I agree that crux here is that we need to know whether a resize comes from the OSK or > something else. > > @aelias Digging into the code further, I see that we have a callback that gets called > everytime the OSK is shown (see getNewShowKeyboardReceiver). Here we set > mFocusPreOSKViewportRect, which we use in onSizeChanged. Can we not just use this instead > of the “did width change” heuristic? You can give it a try, I didn't remember that one. It doesn't seem like a perfect solution because it doesn't give you the size as part of the callback. If I'm reading it correctly, you need to wait for an onResize event to receive it, which might not ever arrive (see http://crbug.com/294908) and when it does, in theory might be caused a non-keyboard-related source of resize that's racing with the keyboard change. But, maybe combined with other tricks this information might be good enough to make something shippable.
On 2016/01/07 22:27:31, aelias wrote: > On 2016/01/07 at 17:39:59, ymalik wrote: > > I agree that crux here is that we need to know whether a resize comes from the > OSK or > > something else. > > > > @aelias Digging into the code further, I see that we have a callback that gets > called > > everytime the OSK is shown (see getNewShowKeyboardReceiver). Here we set > > mFocusPreOSKViewportRect, which we use in onSizeChanged. Can we not just use > this instead > > of the “did width change” heuristic? > > You can give it a try, I didn't remember that one. It doesn't seem like a > perfect solution because it doesn't give you the size as part of the callback. > If I'm reading it correctly, you need to wait for an onResize event to receive > it, which might not ever arrive (see http://crbug.com/294908) and when it does, > in theory might be caused a non-keyboard-related source of resize that's racing > with the keyboard change. But, maybe combined with other tricks this > information might be good enough to make something shippable. @aelias PTAL :) I changed the code to use View.onApplyWindowInsets and View.fitsSystemWindows instead. I have a summary of this change in this doc: https://docs.google.com/a/google.com/document/d/1KsqTnpsoiIngQV6bpH2zbvB2h9TH... Please approve of the high-level change so I can test further and add tests :)
https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/main.xml:1: <?xml version="1.0" encoding="utf-8"?> FYI, The differ doesn't see this, but all I am doing here is wrapping everything in <merge> with <org.chromium.chrome.browser.InsetConsumerView>
Overall approach looks reasonable to me, thanks. https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:353: while (parent.getParent() != null && parent.getParent() instanceof View) { This can be simplified to: for (View parent = mCompositorViewHolder; parent instanceof View; parent = parent.getParent()) { parent.setFitsSystemWindows(false); } https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:361: mInsetConsumerView.setFitsSystemWindows(true); The Android docs on this method says "Note that if you are providing your own implementation of fitSystemWindows(Rect), then there is no need to set this flag to true -- your implementation will be overriding the default implementation that checks this flag." https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:58: @Deprecated It's not conventional to mark something like this deprecated, that's for API surfaces, whereas this is an implementation detail. Other code that makes use of deprecated APIs does @SuppressWarnings("deprecation") https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:67: cvc.setWindowInsets(new Rect(insets)); I don't see any reason to use 'new' here instead of just passing the original object. https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:73: @SuppressLint("NewApi") I think the correct annotation is @TargetApi(Build.VERSION_CODES.LOLLIPOP) https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:79: cvc.setWindowInsets(new WindowInsets(insets)); Likewise, I don't see any reason to use 'new' here instead of just passing the original object. https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:474: private Rect mWindowInsets; Please add code that will guarantee this is populated correctly on a newly created ContentViewCore. https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:650: mWindowInsets = new Rect(insets.getSystemWindowInsetLeft(), Could you create this once at construction time and use Rect.set() instead? (Constructing objects is something we avoid in Java when feasible due to GC pauses.) https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1059: return mViewportHeightPix + getWindowInsets().bottom; Your design doc says this includes the status bar, should there be code correcting for that?
On furthet thought, I suggest storing the rect state in the InsetConsumerView and making the ContentViewCore getter call up to it. That will simplify the plumbing a bit and in particular solve the CVC initialization issue.
https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:361: mInsetConsumerView.setFitsSystemWindows(true); On 2016/01/28 05:04:55, aelias wrote: > The Android docs on this method says "Note that if you are providing your own > implementation of fitSystemWindows(Rect), then there is no need to set this flag > to true -- your implementation will be overriding the default implementation > that checks this flag." I think you need to set it since the override of fitSystemWindows calls the super method. https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1059: return mViewportHeightPix + getWindowInsets().bottom; On 2016/01/28 05:04:55, aelias wrote: > Your design doc says this includes the status bar, should there be code > correcting for that? The status bar would presumably be in getWindowInsets().top. But the problem stands for the "button bar" (Action Bar?), that should be included in bottom here. Is there a way to differentiate between the action bar and the OSK?
ajith.v@chromium.org changed reviewers: + ajith.v@chromium.org
Please check in line comments. https://codereview.chromium.org/1386403003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1386403003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:634: gfx::Size RenderWidgetHostViewAndroid::GetVisibleViewportSize() const { Is this method getting used ? https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:647: public void setWindowInsets(WindowInsets insets) { private https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:648: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) return; if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP || insets == null) return;
On 2016/01/28 05:16:45, aelias wrote: > On furthet thought, I suggest storing the rect state in the InsetConsumerView > and making the ContentViewCore getter call up to it. That will simplify the > plumbing a bit and in particular solve the CVC initialization issue. Oops, I just saw this msg after I addressed your review feedback. Can you see if this is better? If not I can do what you suggested. BTW, What exactly is the initialization issue?
@aelias, I addressed your review feedback. My apologies I saw your suggestion regarding moving the insets into InsetViewConsumer after I made these changes. I still need to do some more manual testing and ensure that we actually only resize the visual viewport (my tests so far have been just visual, still have to analyze some traces). Also, do you know how I can go about writing tests for a feature like this on Android? We probably want to test the inset logic in some way and whether or not the visual viewport is the one that resizes. Are there any sample tests to look at? https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:353: while (parent.getParent() != null && parent.getParent() instanceof View) { On 2016/01/28 05:04:55, aelias wrote: > This can be simplified to: > > for (View parent = mCompositorViewHolder; parent instanceof View; parent = > parent.getParent()) { > parent.setFitsSystemWindows(false); > } We don't check here if the actual parent of the view is an instance of View. Instead, we check if the View is an instance of View which is always true. https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:58: @Deprecated On 2016/01/28 05:04:55, aelias wrote: > It's not conventional to mark something like this deprecated, that's for API > surfaces, whereas this is an implementation detail. > > Other code that makes use of deprecated APIs does > @SuppressWarnings("deprecation") Makes sense. Thanks! https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:67: cvc.setWindowInsets(new Rect(insets)); On 2016/01/28 05:04:55, aelias wrote: > I don't see any reason to use 'new' here instead of just passing the original > object. See comment below. https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:73: @SuppressLint("NewApi") On 2016/01/28 05:04:55, aelias wrote: > I think the correct annotation is @TargetApi(Build.VERSION_CODES.LOLLIPOP) Done. Thanks! https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:79: cvc.setWindowInsets(new WindowInsets(insets)); On 2016/01/28 05:04:55, aelias wrote: > Likewise, I don't see any reason to use 'new' here instead of just passing the > original object. You're right, there is no need for new in this case because we were just setting the rect in CVC. The reason I had 'new' here is because the actual insets eventually become 0 when they're fully consumed. In this particular case, we are only setting the insets in cvc so we can read it later and not to actually apply any inset setting logic. Thus, 'new' ensures that the function you're calling doesn't actually mess with the inset setting logic in an instance where it modifies the actual inset object. In any case, I changed the function in CVC to take the four insets as params which makes this a little clearer. https://codereview.chromium.org/1386403003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1386403003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:634: gfx::Size RenderWidgetHostViewAndroid::GetVisibleViewportSize() const { On 2016/02/10 11:35:28, AKV wrote: > Is this method getting used ? Yes. This method is called from a bunch of places. For example https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:474: private Rect mWindowInsets; On 2016/01/28 05:04:55, aelias wrote: > Please add code that will guarantee this is populated correctly on a newly > created ContentViewCore. Done. https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:647: public void setWindowInsets(WindowInsets insets) { On 2016/02/10 11:35:28, AKV wrote: > private The setWindowInsets function is used by InsetConsumerView and can not be private. https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:648: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) return; On 2016/02/10 11:35:28, AKV wrote: > if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP || insets == null) > return; Removed this function. https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:650: mWindowInsets = new Rect(insets.getSystemWindowInsetLeft(), On 2016/01/28 05:04:55, aelias wrote: > Could you create this once at construction time and use Rect.set() instead? > (Constructing objects is something we avoid in Java when feasible due to GC > pauses.) I have removed this function and changed it to take the L T R B params. See comment in InsetViewConsumer. https://codereview.chromium.org/1386403003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1059: return mViewportHeightPix + getWindowInsets().bottom; On 2016/01/28 15:53:42, bokan wrote: > On 2016/01/28 05:04:55, aelias wrote: > > Your design doc says this includes the status bar, should there be code > > correcting for that? > > The status bar would presumably be in getWindowInsets().top. But the problem > stands for the "button bar" (Action Bar?), that should be included in bottom > here. Is there a way to differentiate between the action bar and the OSK? That's right. The status bar is in getWindowInsets().bottom. The navigation bar is actually not a part of WindowInsets.bottom from where we consume it. The navigation bar will only be included in the insets if an application sets to draw behind the nav buttons (using SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN, etc). https://developer.android.com/reference/android/view/View.html#SYSTEM_UI_FLAG... Clank does set this flag when in fullscreen mode, but in that case, we actually do want to include the navigation bar in the insetso this works well.
Testing-wise, you might look at AutofillPopupWithKeyboardTest.java which involves waiting for the on-screen-keyboard to appear. Also, you can verify the viewport is sized as expected from the JS point of view by checking properties using executeJavaScriptAndWaitForResult(). You might also try populating your HTML with a bottom-fixed-position element and try to click it using something like DOMUtils.clickNode(), and check scroll positions are as expected, for instance. https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:353: while (parent.getParent() != null && parent.getParent() instanceof View) { On 2016/02/11 at 01:06:22, ymalik1 wrote: > On 2016/01/28 05:04:55, aelias wrote: > > This can be simplified to: > > > > for (View parent = mCompositorViewHolder; parent instanceof View; parent = > > parent.getParent()) { > > parent.setFitsSystemWindows(false); > > } > > We don't check here if the actual parent of the view is an instance of View. Instead, we check if the View is an instance of View which is always true. OK, I stand corrected. Anyway, at least please delete "parent.getParent() != null" check because that's already considered by instanceof. https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:361: mInsetConsumerView.setFitsSystemWindows(true); On 2016/01/28 at 15:53:41, bokan wrote: > On 2016/01/28 05:04:55, aelias wrote: > > The Android docs on this method says "Note that if you are providing your own > > implementation of fitSystemWindows(Rect), then there is no need to set this flag > > to true -- your implementation will be overriding the default implementation > > that checks this flag." > > I think you need to set it since the override of fitSystemWindows calls the super method. How about simply not calling super.fitSystemWindows in our override then, like the Android docs suggest? https://codereview.chromium.org/1386403003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1386403003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:467: private Rect mWindowInsets; As I mentioned in #31, I'd like this to be stored in InsetConsumerView instead.
@aelias, PTAL :) I moved the insets to InsetConsumerView and added tests. https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:353: while (parent.getParent() != null && parent.getParent() instanceof View) { On 2016/02/24 02:43:58, aelias wrote: > On 2016/02/11 at 01:06:22, ymalik1 wrote: > > On 2016/01/28 05:04:55, aelias wrote: > > > This can be simplified to: > > > > > > for (View parent = mCompositorViewHolder; parent instanceof View; parent = > > > parent.getParent()) { > > > parent.setFitsSystemWindows(false); > > > } > > > > We don't check here if the actual parent of the view is an instance of View. > Instead, we check if the View is an instance of View which is always true. > > OK, I stand corrected. Anyway, at least please delete "parent.getParent() != > null" check because that's already considered by instanceof. Done. https://codereview.chromium.org/1386403003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:361: mInsetConsumerView.setFitsSystemWindows(true); On 2016/02/24 02:43:58, aelias wrote: > On 2016/01/28 at 15:53:41, bokan wrote: > > On 2016/01/28 05:04:55, aelias wrote: > > > The Android docs on this method says "Note that if you are providing your > own > > > implementation of fitSystemWindows(Rect), then there is no need to set this > flag > > > to true -- your implementation will be overriding the default implementation > > > that checks this flag." > > > > I think you need to set it since the override of fitSystemWindows calls the > super method. > > How about simply not calling super.fitSystemWindows in our override then, like > the Android docs suggest? super.fitSystemWindows is what actually applies the insets. The override in InsetConsumerView just stores it, so we need to call it. https://codereview.chromium.org/1386403003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1386403003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:467: private Rect mWindowInsets; On 2016/02/24 02:43:59, aelias wrote: > As I mentioned in #31, I'd like this to be stored in InsetConsumerView instead. Done.
aelias@chromium.org changed reviewers: + tedchoc@chromium.org - ajith.v@chromium.org, jdduke@chromium.org
lgtm, thanks. Adding tedchoc@ for OWNERS. (As some context, after some research ymalik@ found a way to obtain the current keyboard size that's much more reliable than previous tricks, so this patch exposes the keyboard-free size separately from the keyboard-ful size to the renderer. The immediate benefit is a smarter treatment of fixed-position elements, we eventually want to expose it separately to JS as well.)
Description was changed from ========== Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. BUG=404315 ========== to ========== Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. Design doc: http://go/osk-unification BUG=404315 ==========
Could you expand on the changes made by this CL in the description. The design doc is great but go/ links and the doc itself are non-public. More generally the description should have a summary of the actual changes for easy scanning (e.g. git log).
On 2016/03/04 07:31:42, aelias wrote: > lgtm, thanks. Adding tedchoc@ for OWNERS. > > (As some context, after some research ymalik@ found a way to obtain the current > keyboard size that's much more reliable than previous tricks, so this patch > exposes the keyboard-free size separately from the keyboard-ful size to the > renderer. The immediate benefit is a smarter treatment of fixed-position > elements, we eventually want to expose it separately to JS as well.) I found a bug in this implementation when the copy/paste menu comes in from the top. I am currently working on fixing that, tedchoc@ please hold off on review until then :)
Description was changed from ========== Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. Design doc: http://go/osk-unification BUG=404315 ========== to ========== Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. Design doc: http://go/osk-unification BUG=404315 ==========
rbyers@chromium.org changed reviewers: - rbyers@chromium.org
On 2016/03/04 07:31:42, aelias wrote: > lgtm, thanks. Adding tedchoc@ for OWNERS. > > (As some context, after some research ymalik@ found a way to obtain the current > keyboard size that's much more reliable than previous tricks, so this patch > exposes the keyboard-free size separately from the keyboard-ful size to the > renderer. The immediate benefit is a smarter treatment of fixed-position > elements, we eventually want to expose it separately to JS as well.) @aelias, PTAL :) So the problem was with where I was inserting the InsetConsumerView in the view hierarchy. I assumed that having it right before the views in main.xml would suffice, but this failed when the select text menu showed up from the top. This is because the select text menu shows up as an action bar (which is higher in the view hierarchy than InsetViewConsumer, and it expects the top insets to be consumed at this point). As for the fix, instead of wrapping main.xml views in InsetConsumerView, we iterate up the view hierarchy to find the eldest parent that consumes the insets, and add InsetConsumerView right after. Let me know if this is still lgtm.
https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:378: addAfter.setFitsSystemWindows(false); Hmm, this algorithm is more complicated than I'd like -- I'll accept it if it's the only method but I'd like to make sure it's the simplest. Since you're disabling the inset-consuming behavior in this existing View anyway, wouldn't it work to simply put your InsetConsumerView above it in the hierarchy and not bother calling setFitsSystemWindows(false); on any of the children? If so, you could just place InsetConsumerView something like one off the root View, instead of being so dynamic?
https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:378: addAfter.setFitsSystemWindows(false); On 2016/03/09 01:34:43, aelias wrote: > Hmm, this algorithm is more complicated than I'd like -- I'll accept it if it's > the only method but I'd like to make sure it's the simplest. Since you're > disabling the inset-consuming behavior in this existing View anyway, wouldn't it > work to simply put your InsetConsumerView above it in the hierarchy and not > bother calling setFitsSystemWindows(false); on any of the children? If so, you > could just place InsetConsumerView something like one off the root View, instead > of being so dynamic? Yes with the current view hierarchy it will work if you put the InsetConsumerView one off the root view. I am just not sure how that plays with the compat views on older versions of Android (I tested Kitkat and above), because that fails if the root view is the one that actually consumes the insets. I am not sure if we gain too much simplicity there because we would still need to transfer the children view hierarchy. Doing this dynamically makes us more compatible with the platform changes, WDYT?
https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:378: addAfter.setFitsSystemWindows(false); I don't think the worry the root View might capture insets in the future is especially plausible. I'm more worried about the possibility that whatever place you happen to inject yourself in the hierarchy will have expectations about the child substructure and crash when it doesn't match. So the priority should be to make sure the injection point is at a safe place no library has expectations about. Even if you're worried about the root, there's no need to select the location dynamically. You can just always go one off the root and call root.setFitsSystemWindows(false), which is just the same as your dynamic algorithm would do anyway.
On 2016/03/09 22:18:59, aelias wrote: > https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > (right): > > https://codereview.chromium.org/1386403003/diff/140001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:378: > addAfter.setFitsSystemWindows(false); > I don't think the worry the root View might capture insets in the future is > especially plausible. I'm more worried about the possibility that whatever > place you happen to inject yourself in the hierarchy will have expectations > about the child substructure and crash when it doesn't match. So the priority > should be to make sure the injection point is at a safe place no library has > expectations about. > > Even if you're worried about the root, there's no need to select the location > dynamically. You can just always go one off the root and call > root.setFitsSystemWindows(false), which is just the same as your dynamic > algorithm would do anyway. @aelias Done! PTAL :)
lgtm, over to tedchoc@ for OWNERS.
On 2016/03/10 02:08:17, aelias wrote: > lgtm, over to tedchoc@ for OWNERS. tedchoc@ Currently, hierarchy viewer crashes with InsetConsumerView on Kitkat and below because WindowInsets is not defined. Presumably I can subclass InsetConsumerView to not have the onApplyWindowInsets override for Kitkat and below. Is there a recommended way of fixing this?
This will be really useful. I'm hopeful we can use this logic (or something very similar) to remove the terrible hacks in CompositorView#onMeasure. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/inset_consumer_view.xml (right): https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/inset_consumer_view.xml:6: <org.chromium.chrome.browser.InsetConsumerView This file isn't used. Delete https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:361: // determine the portion of the screen obscured by non-content I don't really like mucking with the view hierarchy if we can avoid it. I played around with this a bit and you can just do: mInsetConsumerView = new InsetConsumerView(this); rootView.addView(mInsetConsumerView, 0); It needs to be at position 0 because of how insets are consumed. The first child of a view that states it has consumed the insets wins. By adding ourselves first, we can see them but signal we don't use them. This does have some implications, but I will mention those in the InsetConsumerView class. I think this is much nicer though. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:363: ViewGroup rootView = (ViewGroup) getWindow().getDecorView().getRootView(); this is also needed above in setRootView, so let's pull it out as a shared var. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:366: rootView.setFitsSystemWindows(false); this didn't seem necessary in my prototyping https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:370: insetConsumerView.setFitsSystemWindows(true); move this to the constructor https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java (right): https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:20: public class InsetConsumerView extends FrameLayout { if we go sibling child route, can this just be a View instead of a FrameLayout? https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:22: private Rect mWindowInsets; make this final https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:31: mWindowInsets = new Rect(); also if we go with the sibling child route, add setVisibility(INVISIBLE) https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:89: return super.fitSystemWindows(insets); if you return false here, then adding it at position 0 works as described in the previous comments. False means we did not consume the inset https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:94: public WindowInsets onApplyWindowInsets(WindowInsets insets) { As for fixing this, you want to do something like is done in ContentView.java You'll need to make mWindowInsets protected though ----------------- public static InsetConsumerView create(Context context) { if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) { return new InsetConsumerView(context); } else { return new InsetConsumerViewApi21(context); } } @TargetApi(Build.VERSION_CODES.LOLLIPOP) private static class InsetConsumerViewApi21 extends InsetConsumerView { InsetConsumerViewApi21(Context context) { super(context); } @Override public WindowInsets onApplyWindowInsets(WindowInsets insets) { mWindowInsets.set( insets.getSystemWindowInsetLeft(), insets.getSystemWindowInsetTop(), insets.getSystemWindowInsetRight(), insets.getSystemWindowInsetBottom()); return super.onApplyWindowInsets(insets); } } https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:434: public int getSystemWindowInsetLeft() { instead of having 4 methods, I wonder if we should add an interface like SystemWindowInsetProvider that the InsetConsumerView would implement. Then there would only be one method in ContentViewClient (getSystemWindowInsetProvider). Thoughts? https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:436: if (activity != null && activity.getInsetConsumerView() != null) { As far as I can tell from your patch, the inset consumer can't be null as long as the activity isn't so that seems to be unnecessary https://codereview.chromium.org/1386403003/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java (right): https://codereview.chromium.org/1386403003/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java:93: Assert.assertFalse("Failed to retrieve bounds for element: fn", you should be able to just call assertFalse here w/o the leading Assert since you're in a test class Also, I would probably do: MoreAsserts.assertNotEquals(jsonText().trim().toLowerCase(), "null"); https://codereview.chromium.org/1386403003/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java:97: assert false : "Unexpected Exception"; Use the fail(...) method https://codereview.chromium.org/1386403003/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java:102: private boolean almostEqual(int a, int b) { i like this name...that is all
@tedchoc PTAL :) I will rename InsetConsumerView to InsetObserverView now that it doesn't actually consume the insets. Kept it for now to make reviewing easier. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/inset_consumer_view.xml (right): https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/inset_consumer_view.xml:6: <org.chromium.chrome.browser.InsetConsumerView On 2016/03/12 00:01:43, Ted C wrote: > This file isn't used. Delete Done. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:361: // determine the portion of the screen obscured by non-content On 2016/03/12 00:01:43, Ted C wrote: > I don't really like mucking with the view hierarchy if we can avoid it. I > played around with this a bit and you can just do: > > mInsetConsumerView = new InsetConsumerView(this); > rootView.addView(mInsetConsumerView, 0); > > It needs to be at position 0 because of how insets are consumed. The first > child of a view that states it has consumed the insets wins. By adding > ourselves first, we can see them but signal we don't use them. > > This does have some implications, but I will mention those in the > InsetConsumerView class. I think this is much nicer though. Done. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:363: ViewGroup rootView = (ViewGroup) getWindow().getDecorView().getRootView(); On 2016/03/12 00:01:43, Ted C wrote: > this is also needed above in setRootView, so let's pull it out as a shared var. Done. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:366: rootView.setFitsSystemWindows(false); On 2016/03/12 00:01:43, Ted C wrote: > this didn't seem necessary in my prototyping I had this in case the compat lib was ever changed and the root view did actually consume the insets. I can get rid of this if this is not a legitimate concern. WDYT? https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:370: insetConsumerView.setFitsSystemWindows(true); On 2016/03/12 00:01:44, Ted C wrote: > move this to the constructor I don't think we need this with the sibling approach (since we're not actually consuming the insets) https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java (right): https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:20: public class InsetConsumerView extends FrameLayout { On 2016/03/12 00:01:44, Ted C wrote: > if we go sibling child route, > > can this just be a View instead of a FrameLayout? Yes. Done. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:22: private Rect mWindowInsets; On 2016/03/12 00:01:44, Ted C wrote: > make this final Done. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:31: mWindowInsets = new Rect(); On 2016/03/12 00:01:44, Ted C wrote: > also if we go with the sibling child route, > > add > setVisibility(INVISIBLE) Done. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:89: return super.fitSystemWindows(insets); On 2016/03/12 00:01:44, Ted C wrote: > if you return false here, then adding it at position 0 works as described in the > previous comments. > > False means we did not consume the inset Done. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:94: public WindowInsets onApplyWindowInsets(WindowInsets insets) { On 2016/03/12 00:01:44, Ted C wrote: > As for fixing this, you want to do something like is done in ContentView.java > > You'll need to make mWindowInsets protected though > > ----------------- > > public static InsetConsumerView create(Context context) { > if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) { > return new InsetConsumerView(context); > } else { > return new InsetConsumerViewApi21(context); > } > } > > @TargetApi(Build.VERSION_CODES.LOLLIPOP) > private static class InsetConsumerViewApi21 extends InsetConsumerView { > InsetConsumerViewApi21(Context context) { > super(context); > } > > @Override > public WindowInsets onApplyWindowInsets(WindowInsets insets) { > mWindowInsets.set( > insets.getSystemWindowInsetLeft(), > insets.getSystemWindowInsetTop(), > insets.getSystemWindowInsetRight(), > insets.getSystemWindowInsetBottom()); > return super.onApplyWindowInsets(insets); > } > } Done. Thanks for the snippet :)! https://codereview.chromium.org/1386403003/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java (right): https://codereview.chromium.org/1386403003/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java:93: Assert.assertFalse("Failed to retrieve bounds for element: fn", On 2016/03/12 00:01:44, Ted C wrote: > you should be able to just call assertFalse here w/o the leading Assert since > you're in a test class > > Also, I would probably do: > > MoreAsserts.assertNotEquals(jsonText().trim().toLowerCase(), "null"); Done. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java:97: assert false : "Unexpected Exception"; On 2016/03/12 00:01:44, Ted C wrote: > Use the fail(...) method Done. https://codereview.chromium.org/1386403003/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/OSKOverscrollTest.java:102: private boolean almostEqual(int a, int b) { On 2016/03/12 00:01:44, Ted C wrote: > i like this name...that is all Acknowledged :)
It would be good to test this in multi-window if you have access to the devices. It has raised a non-trivial number of issues in the past, so it would be good to sanity check it. Otherwise, mainly nits. https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:362: rootView.setFitsSystemWindows(false); It would be nice to guard these lines with ChromeSwitches.ENABLE_OSK_OVERSCROLL if it actually is only needed by them. But that would require you hooking into initializeCompositor() instead of here (that is the first call after native has been initialized). Although you "could" get into a situation where the keyboard is already showing, so you'd need to request requestFitSystemWindows(). More tried to isolate this if we want to roll it out as a unit. Rolling this out now will help us track down issues, so it might be worth it. Also, you should try to get a multi-window supporting samsung phone or try out with the new android n multi-window to make sure this logic is sane there. https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java (right): https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:15: * The purpose of this view is to store the insets (OSK resizes, status bar) for line length for comments in java is 100 (applies elsewhere) https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:16: * later use before they are consumed by the the children views in the view comment needs to be updated https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:27: * access the current theme, resources, etc. align with "The Context..." above. https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:49: * @return The left system window inset. for each of these, you can drop the @return. For simple one liners, we are fine with just the top sentence.
@tedchoc, PTAL :) - Addressed review comments. - Tested on Android M Multiwindow and Samsung Note 5 - Still need to rename InsetConsumerView Thanks! https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:362: rootView.setFitsSystemWindows(false); On 2016/03/14 22:53:06, Ted C wrote: > It would be nice to guard these lines with ChromeSwitches.ENABLE_OSK_OVERSCROLL > if it actually is only needed by them. > > But that would require you hooking into initializeCompositor() instead of here > (that is the first call after native has been initialized). > > Although you "could" get into a situation where the keyboard is already showing, > so you'd need to request requestFitSystemWindows(). > > More tried to isolate this if we want to roll it out as a unit. Rolling this > out now will help us track down issues, so it might be worth it. I like the idea of rolling the inset logic out now so we can track down issues. Is it okay if I leave it here for now? > Also, you should try to get a multi-window supporting samsung phone or try out > with the new android n multi-window to make sure this logic is sane there. I tried the on Android M with multiwindow (enabled through developer settings) and on Samsung Note 5. On Android M with multiwindow, I found that the insets were not reported at all (portrait and landscape mode; window stuck to top and bottom). It was as if the keyboard just overlayed on top of the window. So this change doesn't really affect multiwindow on Android M. On Samsung Note 5, the behavior was interesting. The value of the insets reported are truly the amount of screen obscured by the insets. Consider the following example to better explain the Samsung case: Chrome is stuck to the top, Height of the screen is X pixels, Chrome occupies y pixels before OSK, App 2 occupies z pixels, where y + z = X and y > z Height of the keyboard is w pixels, where w > z, What chrome would see is w - z pixels of bottom insets. (Note that I didn't actually measure this, but based on the numbers, this is what it seemed like.) Similarly, when I put chrome on the bottom and the OSK showed up, it pushed the window above the keyboard and reported the portion of the screen obscured by the keyboard. I think the way Samsung does it makes sense to me and we should do it the same way. In any case, I would say that this change should't really break things. WDYT? https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java (right): https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:15: * The purpose of this view is to store the insets (OSK resizes, status bar) for On 2016/03/14 22:53:07, Ted C wrote: > line length for comments in java is 100 (applies elsewhere) Done. Update other comments. https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:16: * later use before they are consumed by the the children views in the view On 2016/03/14 22:53:07, Ted C wrote: > comment needs to be updated Done. https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:27: * access the current theme, resources, etc. On 2016/03/14 22:53:07, Ted C wrote: > align with "The Context..." above. Done. https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:49: * @return The left system window inset. On 2016/03/14 22:53:07, Ted C wrote: > for each of these, you can drop the @return. For simple one liners, we are fine > with just the top sentence. Done.
lgtm w/ the proposed rename On 2016/03/15 16:23:26, ymalik1 wrote: > @tedchoc, PTAL :) > - Addressed review comments. > - Tested on Android M Multiwindow and Samsung Note 5 > - Still need to rename InsetConsumerView > > Thanks! > > https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > (right): > > https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:362: > rootView.setFitsSystemWindows(false); > On 2016/03/14 22:53:06, Ted C wrote: > > It would be nice to guard these lines with > ChromeSwitches.ENABLE_OSK_OVERSCROLL > > if it actually is only needed by them. > > > > But that would require you hooking into initializeCompositor() instead of here > > (that is the first call after native has been initialized). > > > > Although you "could" get into a situation where the keyboard is already > showing, > > so you'd need to request requestFitSystemWindows(). > > > > More tried to isolate this if we want to roll it out as a unit. Rolling this > > out now will help us track down issues, so it might be worth it. > > I like the idea of rolling the inset logic out now so we can track down issues. > Is it okay if I leave it here for now? Yeah. Something we'll need to keep an eye out for if we start seeing any screen layout issues. > > > Also, you should try to get a multi-window supporting samsung phone or try out > > with the new android n multi-window to make sure this logic is sane there. > > I tried the on Android M with multiwindow (enabled through developer settings) > and on Samsung Note 5. > > On Android M with multiwindow, I found that the insets were not reported at all > (portrait and landscape mode; window stuck to top and bottom). It was as if the > keyboard just overlayed on top of the window. So this change doesn't really > affect multiwindow on Android M. M didn't really have a fully functional multiwindow, so I wouldn't take that behavior for how it will work in N. Won't block on this, but N is definitely something we should test before turning this on more broadly. > > On Samsung Note 5, the behavior was interesting. The value of the insets > reported are truly the amount of screen obscured by the insets. > Consider the following example to better explain the Samsung case: > Chrome is stuck to the top, > Height of the screen is X pixels, > Chrome occupies y pixels before OSK, > App 2 occupies z pixels, where y + z = X and y > z > Height of the keyboard is w pixels, where w > z, > What chrome would see is w - z pixels of bottom insets. (Note that I didn't > actually measure this, but based on the numbers, this is what it seemed like.) > > Similarly, when I put chrome on the bottom and the OSK showed up, it pushed the > window above the keyboard and reported the portion of the screen obscured by the > keyboard. > > I think the way Samsung does it makes sense to me and we should do it the same > way. In any case, I would say that this change should't really break things. > WDYT? sgtm > > https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java > (right): > > https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:15: * > The purpose of this view is to store the insets (OSK resizes, status bar) for > On 2016/03/14 22:53:07, Ted C wrote: > > line length for comments in java is 100 (applies elsewhere) > > Done. Update other comments. > > https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:16: * > later use before they are consumed by the the children views in the view > On 2016/03/14 22:53:07, Ted C wrote: > > comment needs to be updated > > Done. > > https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:27: * > access the current theme, resources, etc. > On 2016/03/14 22:53:07, Ted C wrote: > > align with "The Context..." above. > > Done. > > https://codereview.chromium.org/1386403003/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/InsetConsumerView.java:49: * > @return The left system window inset. > On 2016/03/14 22:53:07, Ted C wrote: > > for each of these, you can drop the @return. For simple one liners, we are > fine > > with just the top sentence. > > Done.
Description was changed from ========== Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. Design doc: http://go/osk-unification BUG=404315 ========== to ========== Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. This CL adds a new View (InsetObserverView) to the View hierarchy that will store the value of insets (OSK, status bar). When there is a resize due to OSK show, we keep the view bounds the same and change the visible viewport size. Design doc: http://go/osk-unification BUG=404315 ==========
On 2016/03/07 20:20:06, bokan wrote: > Could you expand on the changes made by this CL in the description. The design > doc is great but go/ links and the doc itself are non-public. More generally the > description should have a summary of the actual changes for easy scanning (e.g. > git log). Done.
ymalik@chromium.org changed reviewers: + avi@chromium.org, isherman@chromium.org
+isherman@ for histograms.xml +avi for content/public/common/content_switches
+isherman@ for histograms.xml +avi for content/public/common/content_switches
On 2016/03/16 20:49:56, ymalik1 wrote: > +isherman@ for histograms.xml > +avi for content/public/common/content_switches LGTM; we are eventually going to remove that switch?
On 2016/03/16 21:01:41, Avi wrote: > On 2016/03/16 20:49:56, ymalik1 wrote: > > +isherman@ for histograms.xml > > +avi for content/public/common/content_switches > > LGTM; we are eventually going to remove that switch? Yes, that's right. When we are ready to enable it by default (blocking on some other feature).
histograms.xml lgtm
The CQ bit was checked by ymalik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/1386403003/#ps240001 (title: "s/InsetConsumerView/InsetObserverView")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1386403003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1386403003/240001
Message was sent while issue was closed.
Description was changed from ========== Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. This CL adds a new View (InsetObserverView) to the View hierarchy that will store the value of insets (OSK, status bar). When there is a resize due to OSK show, we keep the view bounds the same and change the visible viewport size. Design doc: http://go/osk-unification BUG=404315 ========== to ========== Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. This CL adds a new View (InsetObserverView) to the View hierarchy that will store the value of insets (OSK, status bar). When there is a resize due to OSK show, we keep the view bounds the same and change the visible viewport size. Design doc: http://go/osk-unification BUG=404315 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. This CL adds a new View (InsetObserverView) to the View hierarchy that will store the value of insets (OSK, status bar). When there is a resize due to OSK show, we keep the view bounds the same and change the visible viewport size. Design doc: http://go/osk-unification BUG=404315 ========== to ========== Resize only the virtual viewport when the OSK triggers a resize. Initial CL in unifying the keyboard behavior between ChromeOS and Android. If the enable-osk-overscroll flag is set, this change will keep the Blink viewport size stable and set the visible_viewport_size to the smaller value. This CL adds a new View (InsetObserverView) to the View hierarchy that will store the value of insets (OSK, status bar). When there is a resize due to OSK show, we keep the view bounds the same and change the visible viewport size. Design doc: http://go/osk-unification BUG=404315 Committed: https://crrev.com/3edbd8d05df275432f79649ac740070d329b26ea Cr-Commit-Position: refs/heads/master@{#381590} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/3edbd8d05df275432f79649ac740070d329b26ea Cr-Commit-Position: refs/heads/master@{#381590} |