[Hub Bottombar] Fix the bottom incorrect position issue [chromium/src : main]
Jingping Sun (Gerrit)
Jingping Sun added 1 comment![]( "Open in Gerrit")
Hi, Chromium committers. Hope you are doing well!
I'm trying to fix the issue of incorrect hub bottom bar position after this CL's hub layout change.
7056503: [Pinned Tabs] fix blinking effect when search bar is hidden | https://chromium-review.googlesource.com/c/chromium/src/+/7056503
This CL adds a LinearLayout as the container of the hub pane host layout and the bottom bar which is added manually. It can make sure the bottom bar is at the bottom of hub_pane_host_layout and not affect the previous pin tab layout issue.
Please help review, thanks!
Related details
- Calder Kitagawa
- Hitarth Kothari
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 1 comment![]( "Open in Gerrit")
@hitarth...@google.com or @madhav...@google.com can one of you download this patch and verify it doesn't cause any issues with pinned tabs?
Related details
- Hitarth Kothari
- Jingping Sun
- Madhav Pruthi
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jingping Sun (Gerrit)
Jingping Sun added 1 comment![]( "Open in Gerrit")
@hitarth...@google.com or @madhav...@google.com can one of you download this patch and verify it doesn't cause any issues with pinned tabs?
I have tested the pinned tabs locally. It works fine. Please help verify it again. Thanks!
Related details
- Calder Kitagawa
- Hitarth Kothari
- Madhav Pruthi
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madhav Pruthi (Gerrit)
Madhav Pruthi added 1 comment![]( "Open in Gerrit")
Jingping Sun@hitarth...@google.com or @madhav...@google.com can one of you download this patch and verify it doesn't cause any issues with pinned tabs?
I have tested the pinned tabs locally. It works fine. Please help verify it again. Thanks!
I tested locally too and pinned tabs look fine to me. Thanks.
Related details
- Calder Kitagawa
- Hitarth Kothari
- Jingping Sun
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Calder Kitagawa voted and added 1 comment![]( "Open in Gerrit")
Votes added by Calder Kitagawa
| Code-Review | +1 |
1 comment
Related details
- Hitarth Kothari
- Jingping Sun
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 1 comment![]( "Open in Gerrit")
<LinearLayout
android:id="@+id/hub_main_container"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical">@gurm...@google.com We should probably have someone with an XR device test #android-pinned-tabs and check it looks good or update it. We don't have an XR device to test with.
Related details
- Gurmeet Kalra
- Hitarth Kothari
- Jingping Sun
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jingping Sun (Gerrit)
Jingping Sun added 3 comments![]( "Open in Gerrit")
Jingping Sun@hitarth...@google.com or @madhav...@google.com can one of you download this patch and verify it doesn't cause any issues with pinned tabs?
Madhav PruthiI have tested the pinned tabs locally. It works fine. Please help verify it again. Thanks!
I tested locally too and pinned tabs look fine to me. Thanks.
Thanks for the test!
LGTM
Thanks for the review! I uploaded a text fix. Please help review again. Thank you very much!
<LinearLayout
android:id="@+id/hub_main_container"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical">@gurm...@google.com We should probably have someone with an XR device test #android-pinned-tabs and check it looks good or update it. We don't have an XR device to test with.
Yes. Should we align the XR layout with the normal layout? The previous pinned tabs fix only changed the normal layout.
Related details
- Calder Kitagawa
- Gurmeet Kalra
- Hitarth Kothari
- Madhav Pruthi
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 1 comment![]( "Open in Gerrit")
<LinearLayout
android:id="@+id/hub_main_container"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical">Jingping Sun@gurm...@google.com We should probably have someone with an XR device test #android-pinned-tabs and check it looks good or update it. We don't have an XR device to test with.
Yes. Should we align the XR layout with the normal layout? The previous pinned tabs fix only changed the normal layout.
- Gurmeet Kalra
- Hitarth Kothari
- Jingping Sun
- Madhav Pruthi
- Sky Malice
Calder Kitagawa (Gerrit)
Calder Kitagawa added 1 comment![]( "Open in Gerrit")
<Space
android:layout_height="@dimen/hub_xr_vertical_gap_between_toolbar_and_host_pane"
android:layout_width="0dp" />This gap might not be feasible to maintain anymore with how pinned tabs in implemented as a carousel at the top of the hub...
Calder Kitagawa (Gerrit)
Calder Kitagawa added 1 comment![]( "Open in Gerrit")
<LinearLayout
android:id="@+id/hub_main_container"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical">Jingping Sun@gurm...@google.com We should probably have someone with an XR device test #android-pinned-tabs and check it looks good or update it. We don't have an XR device to test with.
Calder KitagawaYes. Should we align the XR layout with the normal layout? The previous pinned tabs fix only changed the normal layout.
Yes I think the two need to be aligned.
Don't worry about fixing this Jingping since your change is independent of XR.
Calder Kitagawa (Gerrit)
Calder Kitagawa voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Gurmeet Kalra
- Hitarth Kothari
- Jingping Sun
- Madhav Pruthi
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
Jingping Sun (Gerrit)
Jingping Sun added 2 comments![]( "Open in Gerrit")
Jingping Sun@hitarth...@google.com or @madhav...@google.com can one of you download this patch and verify it doesn't cause any issues with pinned tabs?
Madhav PruthiI have tested the pinned tabs locally. It works fine. Please help verify it again. Thanks!
Jingping SunI tested locally too and pinned tabs look fine to me. Thanks.
Thanks for the test!
Done
<LinearLayout
android:id="@+id/hub_main_container"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical">Jingping Sun@gurm...@google.com We should probably have someone with an XR device test #android-pinned-tabs and check it looks good or update it. We don't have an XR device to test with.
Calder KitagawaYes. Should we align the XR layout with the normal layout? The previous pinned tabs fix only changed the normal layout.
Calder KitagawaYes I think the two need to be aligned.
Don't worry about fixing this Jingping since your change is independent of XR.
Thank you very much Calder! I think I can create a new CL to align the XR layout and Gurmeet can help test it.
Related details
- Calder Kitagawa
- Gurmeet Kalra
- Hitarth Kothari
- Madhav Pruthi
- Sky Malice
Calder Kitagawa (Gerrit)
Calder Kitagawa added 1 comment![]( "Open in Gerrit")
<LinearLayout
android:id="@+id/hub_main_container"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical">Jingping Sun@gurm...@google.com We should probably have someone with an XR device test #android-pinned-tabs and check it looks good or update it. We don't have an XR device to test with.
Calder KitagawaYes. Should we align the XR layout with the normal layout? The previous pinned tabs fix only changed the normal layout.
Calder KitagawaYes I think the two need to be aligned.
Jingping SunDon't worry about fixing this Jingping since your change is independent of XR.
Thank you very much Calder! I think I can create a new CL to align the XR layout and Gurmeet can help test it.
- Gurmeet Kalra
- Hitarth Kothari
- Jingping Sun
- Madhav Pruthi
- Sky Malice
Sky Malice (Gerrit)
Sky Malice voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Gurmeet Kalra
- Hitarth Kothari
- Jingping Sun
- Madhav Pruthi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jingping Sun (Gerrit)
Jingping Sun added 2 comments![]( "Open in Gerrit")
<LinearLayout
android:id="@+id/hub_main_container"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical">Jingping Sun@gurm...@google.com We should probably have someone with an XR device test #android-pinned-tabs and check it looks good or update it. We don't have an XR device to test with.
Calder KitagawaYes. Should we align the XR layout with the normal layout? The previous pinned tabs fix only changed the normal layout.
Calder KitagawaYes I think the two need to be aligned.
Jingping SunDon't worry about fixing this Jingping since your change is independent of XR.
Calder KitagawaThank you very much Calder! I think I can create a new CL to align the XR layout and Gurmeet can help test it.
That would be awesome thanks!
Will change it in another CL.
<Space
android:layout_height="@dimen/hub_xr_vertical_gap_between_toolbar_and_host_pane"
android:layout_width="0dp" />This gap might not be feasible to maintain anymore with how pinned tabs in implemented as a carousel at the top of the hub...
I think we can let @gurm...@google.com to test it in XR device.
Related details
- Gurmeet Kalra
- Hitarth Kothari
- Madhav Pruthi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jingping Sun (Gerrit)
Jingping Sun voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Hub Bottombar] Fix the bottom incorrect position issue
Recently, the hub layout's hub_main_container was changed from
LinearLayout to FrameLayout. It caused the bottom bar view can't be
added to the hub layout's bottom. This CL added a new
hub_pane_host_container as the container of hub pane host view and the
bottom bar view which is added manually.
- M chrome/android/features/tab_ui/javatests/src/org/chromium/chrome/browser/tasks/tab_management/TabGridDialogTest.java
- M chrome/browser/hub/internal/android/java/src/org/chromium/chrome/browser/hub/HubCoordinator.java
- M chrome/browser/hub/internal/android/res/layout/hub_layout.xml
- M chrome/browser/hub/internal/android/res/layout/hub_xr_layout.xml
Code-Review: +1 by Calder Kitagawa, +1 by Sky Malice
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |