Will update this once this is merged to help fix the last failing tests
https://chromium-review.googlesource.com/c/chromium/src/+/5664105
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
private boolean mIsOptedIntoEdgeToEdge;
Let's add a JavaDoc for this. I think it'll be true when the current webpage is opted into edge to edge or (in the future) we're showing a native page that draws edge to edge
mSystemInsets =
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
Does the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)
// It's possible for toEdge to change more than once prior to the first time
// #handleWindowInsets is called. #handleWindowInsets will call #adjustEdges using
// the current mIsActivityToEdge once insets are available.
Now that we initialize mSystemInsets in the ctor, I wonder if this comment is still relevant?
I added this in crrev.com/c/5378615 as part of an NPE fix
private boolean shouldPadAdjusters() {
boolean keyboardIsVisible = mKeyboardInsets != null && mKeyboardInsets.bottom > 0;
return mIsOptedIntoEdgeToEdge && !keyboardIsVisible;
}
I think we'll need to revisit this for the chin. For example, when the chin is scrolled off on a webpage that is not opted in we should be padding snackbars, while right now I suspect we aren't.
Maybe we could file a bug & add a TODO here?
* @return whether the UI has opted into being drawn fully edge to edge. Note that a page may
* still draw edge-to-edge without this being true if the bottom chin is enabled and has
* been fully scrolled off.
```suggestion
* @return Whether the current webpage (via opt-in) or native page is drawing edge to edge to on initial page load. Note that a page may
* still draw beneath the OS navigation bar without this being true if the bottom chin is enabled and has
* been fully scrolled off.
```
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
nit: We should read the system insets from `mInsetObserver.getLastSeenRawInsets()` instead to avoid inconsistency during start up.
mSystemInsets =
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
Does the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)
Technically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
not blocking: We are always making this call when a controller instance is created. This is currently called when native is initialized:
If we no longer have to observe the website opt-in state, I wonder if we can create the controller instance a bit earlier in the start up?
boolean isOptedIntoEdgeToEdge();
naming nit: I think it might be more clear if we baked in "Page" / "Tab" in the method name to made clear the scope of E2E.
Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mSystemInsets =
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
Wenyu FuDoes the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)
Technically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets
Since this is doing #getDecorView, won't it include the system UI even without a call to #setDecorFitsSystemWindows?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit-Queue | +1 |
Let's add a JavaDoc for this. I think it'll be true when the current webpage is opted into edge to edge or (in the future) we're showing a native page that draws edge to edge
Sure - in my mind, "opted in" included native pages that support E2E. Happy to tweak the naming if you like!
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
nit: We should read the system insets from `mInsetObserver.getLastSeenRawInsets()` instead to avoid inconsistency during start up.
Right now that method is annotated as returning a `@Nullable`, but is it actually `@Nullable`? It always seems to be initialized upon creating the `InsetObserver`
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
not blocking: We are always making this call when a controller instance is created. This is currently called when native is initialized:
If we no longer have to observe the website opt-in state, I wonder if we can create the controller instance a bit earlier in the start up?
How early would you like to set it? Originally IIUC the E2EControllerImpl was created independently of checking the website state, just that its method `#isEdgeToEdgeActive()` would be false until reaching an E2E opted-in page.
Was there a specific piece of logic that was preventing an earlier initialization?
// It's possible for toEdge to change more than once prior to the first time
// #handleWindowInsets is called. #handleWindowInsets will call #adjustEdges using
// the current mIsActivityToEdge once insets are available.
Now that we initialize mSystemInsets in the ctor, I wonder if this comment is still relevant?
I added this in crrev.com/c/5378615 as part of an NPE fix
I think this NPE shouldn't happen anymore, now that system insets are never null, so we should be good to remove this comment :)
private boolean shouldPadAdjusters() {
boolean keyboardIsVisible = mKeyboardInsets != null && mKeyboardInsets.bottom > 0;
return mIsOptedIntoEdgeToEdge && !keyboardIsVisible;
}
I think we'll need to revisit this for the chin. For example, when the chin is scrolled off on a webpage that is not opted in we should be padding snackbars, while right now I suspect we aren't.
Maybe we could file a bug & add a TODO here?
Yup, this will definitely need to be revisited. I can add a bug and TODO.
* @return whether the UI has opted into being drawn fully edge to edge. Note that a page may
* still draw edge-to-edge without this being true if the bottom chin is enabled and has
* been fully scrolled off.
```suggestion
* @return Whether the current webpage (via opt-in) or native page is drawing edge to edge to on initial page load. Note that a page may
* still draw beneath the OS navigation bar without this being true if the bottom chin is enabled and has
* been fully scrolled off.
```
Done
boolean isOptedIntoEdgeToEdge();
naming nit: I think it might be more clear if we baked in "Page" / "Tab" in the method name to made clear the scope of E2E.
Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.
Sure!
Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.
Yeah, this is exactly the subtlety I was trying to convey. Maybe `isPageOptedIntoEdgeToEdge` or `isTabContentOptedIntoEdgeToEdge`? Didn't want it to get too long...
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
This change meets the code coverage requirements.
Code-Coverage | +1 |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
lgtm % open questions & Wenyu's lgtm
mSystemInsets =
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
Wenyu FuDoes the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)
Theresa SullivanTechnically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets
Since this is doing #getDecorView, won't it include the system UI even without a call to #setDecorFitsSystemWindows?
Charles - ping on this open thread
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
Charles Hagernot blocking: We are always making this call when a controller instance is created. This is currently called when native is initialized:
If we no longer have to observe the website opt-in state, I wonder if we can create the controller instance a bit earlier in the start up?
How early would you like to set it? Originally IIUC the E2EControllerImpl was created independently of checking the website state, just that its method `#isEdgeToEdgeActive()` would be false until reaching an E2E opted-in page.
Was there a specific piece of logic that was preventing an earlier initialization?
Previously we didn't have anything that could draw edge to edge until after native initialized because of the website opt-in requirement.
Now, we indeed might want to initialize sooner. I'd be happy with a P3 TODO for revisiting as technical debt unless we know of an issue early init would resolve.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
Charles Hagernit: We should read the system insets from `mInsetObserver.getLastSeenRawInsets()` instead to avoid inconsistency during start up.
Right now that method is annotated as returning a `@Nullable`, but is it actually `@Nullable`? It always seems to be initialized upon creating the `InsetObserver`
Technically `mLastSeenRawWindowInset` can be null based on the code, but I think in practice it's almost always ready by the time InsetObserver is created. When the first inset is ready, `mLastSeenRawWindowInset` should record the raw insets.
So, there should never be a time where `mLastSeenRawWindowInset == null` while `getRootWindowInsets() != null`.
We can also use an assert not null here to be safe, too
* this being true if the bottom chin is enabled and has been fully scrolled off.
optional nit: Add code link to the BottomChinCoordinator
boolean isOptedIntoEdgeToEdge();
Charles Hagernaming nit: I think it might be more clear if we baked in "Page" / "Tab" in the method name to made clear the scope of E2E.
Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.
Sure!
Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.
Yeah, this is exactly the subtlety I was trying to convey. Maybe `isPageOptedIntoEdgeToEdge` or `isTabContentOptedIntoEdgeToEdge`? Didn't want it to get too long...
I think `isPageOptedIntoEdgeToEdge`, or `isPageOptedInToEdge` maybe good enough... not feeling very strong here, it seems we only have one caller of this method:
```
public void setNavigationBarScrimFraction(float fraction) {
if (mEdgeToEdgeControllerSupplier.get() != null
&& mEdgeToEdgeControllerSupplier.get().isOptedIntoEdgeToEdge()) {
return;
}
```
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
Charles Hagernit: We should read the system insets from `mInsetObserver.getLastSeenRawInsets()` instead to avoid inconsistency during start up.
Wenyu FuRight now that method is annotated as returning a `@Nullable`, but is it actually `@Nullable`? It always seems to be initialized upon creating the `InsetObserver`
Technically `mLastSeenRawWindowInset` can be null based on the code, but I think in practice it's almost always ready by the time InsetObserver is created. When the first inset is ready, `mLastSeenRawWindowInset` should record the raw insets.
So, there should never be a time where `mLastSeenRawWindowInset == null` while `getRootWindowInsets() != null`.
We can also use an assert not null here to be safe, too
Done
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
Charles Hagernot blocking: We are always making this call when a controller instance is created. This is currently called when native is initialized:
If we no longer have to observe the website opt-in state, I wonder if we can create the controller instance a bit earlier in the start up?
Theresa SullivanHow early would you like to set it? Originally IIUC the E2EControllerImpl was created independently of checking the website state, just that its method `#isEdgeToEdgeActive()` would be false until reaching an E2E opted-in page.
Was there a specific piece of logic that was preventing an earlier initialization?
Previously we didn't have anything that could draw edge to edge until after native initialized because of the website opt-in requirement.
Now, we indeed might want to initialize sooner. I'd be happy with a P3 TODO for revisiting as technical debt unless we know of an issue early init would resolve.
Sounds good, filed here crbug.com/350610430 - will add a TODO as well.
mSystemInsets =
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
Wenyu FuDoes the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)
Theresa SullivanTechnically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets
Theresa SullivanSince this is doing #getDecorView, won't it include the system UI even without a call to #setDecorFitsSystemWindows?
Charles - ping on this open thread
My best guess is that the system insets would be consistent before and after setting the value, though I wasn't fully certain. I need to test this, but that means fixing up some of the E2E flag values. Will report as soon as I've verified!
IIRC changing the decor fits value doesn't affect the system insets, it's more the rotations / changing between gesture nav vs 3 button nav
* this being true if the bottom chin is enabled and has been fully scrolled off.
optional nit: Add code link to the BottomChinCoordinator
Done
boolean isOptedIntoEdgeToEdge();
Charles Hagernaming nit: I think it might be more clear if we baked in "Page" / "Tab" in the method name to made clear the scope of E2E.
Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.
Wenyu FuSure!
Chrome is always drawing E2E after this controller is created, and this method is basically used for clients to adjust their UI based on the bottom insets on different pages.
Yeah, this is exactly the subtlety I was trying to convey. Maybe `isPageOptedIntoEdgeToEdge` or `isTabContentOptedIntoEdgeToEdge`? Didn't want it to get too long...
I think `isPageOptedIntoEdgeToEdge`, or `isPageOptedInToEdge` maybe good enough... not feeling very strong here, it seems we only have one caller of this method:
```
public void setNavigationBarScrimFraction(float fraction) {
if (mEdgeToEdgeControllerSupplier.get() != null
&& mEdgeToEdgeControllerSupplier.get().isOptedIntoEdgeToEdge()) {
return;
}
```
I'll likely just keep `isPageOptedIntoEdgeToEdge` for precision then...
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
mSystemInsets =
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
Wenyu FuDoes the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)
Theresa SullivanTechnically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets
Theresa SullivanSince this is doing #getDecorView, won't it include the system UI even without a call to #setDecorFitsSystemWindows?
Charles HagerCharles - ping on this open thread
My best guess is that the system insets would be consistent before and after setting the value, though I wasn't fully certain. I need to test this, but that means fixing up some of the E2E flag values. Will report as soon as I've verified!
IIRC changing the decor fits value doesn't affect the system insets, it's more the rotations / changing between gesture nav vs 3 button nav
IIRC changing the decor fits value doesn't affect the system insets,
I'd expect it to change the system insets observed by mWindowAndroid.getInsetObserver() since that's observing the insets for the root View of the Activity iirc. Before we call #setDecorFitsSystemWindows(false), the root view is set inside the status bar and nav bar so those should be 0. After we call #setDecorFitsSystemWindows(false), they'd be non-zero
That said, it might not non-0 immediately after calling #setDecorFitsSystemWindows -- wouldn't be surprised if Android needs to perform a layout pass to draw the view into the sys ui space before the window insets start reporting an updated value.
assert mInsetObserver.getLastRawWindowInsets() != null
: "The inset observer should have non-null insets by the time the"
+ " EdgeToEdgeControllerImpl is initialized.";
mSystemInsets = getSystemInsets(mInsetObserver.getLastRawWindowInsets());
Looking at the impl for InsetObserver ctor, it relies on View#getRootWindowInsets which per the JavaDocs will return null of the View isn't attached.
For use case, it's probably a fair assumption that the root view is attached by the time EdgeToEdgeControllerImpl is created. If we change init location, we may want to revisit this.
https://developer.android.com/reference/android/view/View#getRootWindowInsets()
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mSystemInsets =
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
Wenyu FuDoes the ordering of these calls matter? Or is getting the system insets using the Root View not impacted by #setDecorFitsSystemWindows? (I'm guessing it's the latter)
Theresa SullivanTechnically it doesn't matter, since when the window draws E2E, the root view will receive a different window insets
Theresa SullivanSince this is doing #getDecorView, won't it include the system UI even without a call to #setDecorFitsSystemWindows?
Charles HagerCharles - ping on this open thread
Theresa SullivanMy best guess is that the system insets would be consistent before and after setting the value, though I wasn't fully certain. I need to test this, but that means fixing up some of the E2E flag values. Will report as soon as I've verified!
IIRC changing the decor fits value doesn't affect the system insets, it's more the rotations / changing between gesture nav vs 3 button nav
IIRC changing the decor fits value doesn't affect the system insets,
I'd expect it to change the system insets observed by mWindowAndroid.getInsetObserver() since that's observing the insets for the root View of the Activity iirc. Before we call #setDecorFitsSystemWindows(false), the root view is set inside the status bar and nav bar so those should be 0. After we call #setDecorFitsSystemWindows(false), they'd be non-zero
That said, it might not non-0 immediately after calling #setDecorFitsSystemWindows -- wouldn't be surprised if Android needs to perform a layout pass to draw the view into the sys ui space before the window insets start reporting an updated value.
I believe the root view is never set inside the system bars, or at least I'm seeing the insets from before the decor fits call match those afterwards.
By default, does the `window.getDecorView().getRootView()` sit within the system bars?
https://source.chromium.org/chromium/chromium/src/+/main:ui/android/java/src/org/chromium/ui/base/WindowAndroid.java?q=%22new%20ImmutableWeakReference%3C%3E(window.getDecorView().getRootView()));%22&ss=chromium%2Fchromium%2Fsrc
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
By default, does the window.getDecorView().getRootView() sit within the system bars?
If we're always getting non-0 insets then InsetObserver wouldn't really be telling us what insets currently apply to our app views... but maybe that was intentional.
Wenyu, do you recall?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
I roughly recall that DecorView always has the window insets applied; during my debugging, by the time InsetsObserver is created, `window.getDecorView().getRootView()` is giving the DecorView itself (which make sense..., since `getRootView` "Finds the topmost view in the current view hierarchy."[1])
For this code, I think it'll be technically reads more correct to set the SystemInsets after we do the call to `setDecorFitsSystemWindows`, though in practice it probably matter less (since the window layout pass is run in the next looper task, so window insets shouldn't change right after `setDecorFitsSystemWindows`)
[1] https://developer.android.com/reference/android/view/View#getRootView()
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mSystemInsets =
getSystemInsets(
WindowInsetsCompat.toWindowInsetsCompat(
mActivity.getWindow().getDecorView().getRootWindowInsets()));
mEdgeToEdgeOSWrapper.setDecorFitsSystemWindows(mActivity.getWindow(), false);
I'm not seeing them ever change, even if I change the `decorFits` value later between true and false.
I don't mind switching the order, but I wouldn't expect that to have any effect personally...
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
This change meets the code coverage requirements.
Code-Coverage | +1 |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |