ListPopupWindow positioning incorrect on N+ when popup displayed above anchor view |
||||||||||||||
Issue descriptionAndroid rewrote how popup windows are positioned between N and M. On N+, when the ListPopupWindow appears above the anchor view, setting a negative vertical offset moves the popup window up rather than down. This has been broken since Android N shipped. As it's not a recent regression, fixing it is not time critical. Discussion of Android ListPopupWindow positioning here: https://bugs.chromium.org/p/chromium/issues/detail?id=705348#c12 We use ListPopupWindow in BookmarksRow.java: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkRow.java?type=cs&q=bookmark+listpopupwindow&sq=package:chromium&l=137 And DropDownPopupWindow, which is used in autofill: https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java?q=listpopupwindow&dr=C&l=17 The usage in bookmarks row is more severe since the vertical offset is larger.
,
Jun 1 2017
Rolling the app menu specific bug into this one. ListPopupWindow continues to cause issues (e.g. issue 72853 ), so bumping the priority and targeting M61 for a fix. In my prototype, using an absolutely positioned PopupWindow works quite well.
,
Jun 1 2017
Is this preventing Chrome Home from being rolled out widely? The menu is too short when you hold a phone in landscape
,
Jun 2 2017
As a P2 for Chrome Home, this will block full stable launch but will not block experimentation in non-Stable channels.
,
Jun 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ce2be9d4ba31aa44b6d18716746647537e535546 commit ce2be9d4ba31aa44b6d18716746647537e535546 Author: Theresa Wellington <twellington@google.com> Date: Thu Jun 08 15:57:02 2017 [Android] Use absolutely positioned PopupWindow for app menu Rather than using ListPopupWindow, which has positioning issues when the anchor view is at the bottom of the screen, the app menu now uses an absolutely positioned PopupWindow. BUG= 709522 ,728530 Change-Id: I86f264916fadd3a0e66d0e80802d0e951444d6ce Reviewed-on: https://chromium-review.googlesource.com/524296 Reviewed-by: Ted Choc <tedchoc@chromium.org> Commit-Queue: Theresa (OOO) <twellington@chromium.org> Cr-Commit-Position: refs/heads/master@{#477980} [add] https://crrev.com/ce2be9d4ba31aa44b6d18716746647537e535546/chrome/android/java/res/anim/menu_enter_from_bottom.xml [add] https://crrev.com/ce2be9d4ba31aa44b6d18716746647537e535546/chrome/android/java/res/anim/menu_exit_from_bottom.xml [add] https://crrev.com/ce2be9d4ba31aa44b6d18716746647537e535546/chrome/android/java/res/layout/app_menu_layout.xml [modify] https://crrev.com/ce2be9d4ba31aa44b6d18716746647537e535546/chrome/android/java/res/values-v17/styles.xml [modify] https://crrev.com/ce2be9d4ba31aa44b6d18716746647537e535546/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java [modify] https://crrev.com/ce2be9d4ba31aa44b6d18716746647537e535546/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuDragHelper.java [modify] https://crrev.com/ce2be9d4ba31aa44b6d18716746647537e535546/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java [modify] https://crrev.com/ce2be9d4ba31aa44b6d18716746647537e535546/chrome/android/javatests/src/org/chromium/chrome/browser/appmenu/AppMenuTest.java [modify] https://crrev.com/ce2be9d4ba31aa44b6d18716746647537e535546/chrome/android/javatests/src/org/chromium/chrome/browser/appmenu/ChromeHomeAppMenuTest.java
,
Jun 8 2017
The app menu has been converted, so this no longer needs to be on the Chrome Home hotlist. Bookmarks and autofill still need to be converted.
,
Oct 16 2017
If/when we create a PopupWindow wrapper class to account for ListPopupWindow's positioning issues, we should also consider handling the bug described in go/chromepostmortem479. On older versions of Android, an exception is thrown if a PopupWindow is shown while the underlying View doesn't contain any LayoutParams.
,
Dec 20 2017
+dtrainor@ - We can re-use a lot of the logic dtrainor@ added for TextBubble.java and ViewAnchoredTextBubble.java. After the holidays, let's chat about how to re-purpose the code in these two classes for non-blue-bubble applications. I have a few ideas for how we could structure this. I made a very rough prototype to test whether the existing positioning logic applied to our list popups as well: https://chromium-review.googlesource.com/c/chromium/src/+/837584
,
Jan 23 2018
Dave, what do you think of something like that attached image? Most of the code currently in TextBubble would move to AnchoredPopupWindow, and most of the code in ViewAnchoredTextBubble would move to ViewAnchordPopupWindow. ListMenuButton will have a ViewAnchoredPopupWindow, and it will pass in some parameters or call a method that lets the ViewAnchoredPopupWindow know that it should overlap the anchor. TextBubble will have an AnchoredPopupWindow + do something to tell it to center the popup horizontally with respect to the anchor. ViewAnchoredTextBubble will be a really simple class that uses extends TextBubble, but uses ViewAnchoredPopupWindow instead of the regular AnchoredPopupWindow.
,
Jan 24 2018
Attaching the PNG I forgot. New proof-of-concept prototype CL (very rough): https://chromium-review.googlesource.com/#/c/chromium/src/+/882547 I don't think ViewAnchoredTextBubble is actually needed in this model.
,
Jan 24 2018
After chatting with Dave offline, I will try to remove the ViewAnchoredPopupWindow and put its logic into something like an AnchorRectProvider that can be generically useful for things besides popup windows. I'll report back after I've had a chance to prototype and investigate.
,
Jan 25 2018
To capture some of my investigation: The bug I filed against Android hasn't been responded to b/36786138. Prior to refactoring TextBubble to re-use its positioning logic, I tried using the android:overlapAnchor property (available from API level 21) in a style set on the ListPopupWindow. This worked on an Android One running LMY47D, but it didn't work on a Nexus 5x running NMF26X or OPM1.17019.020. It also doesn't allow us to do fine-grained positioning taking into account the shadows on the popup background. Manually positioning the popup seems like the most robust option.
,
Jan 25 2018
Updated the padding to be more consistent with ToT today, which doesn't take into account the padding on the popup menu. I'm going to delete a couple of obsolete comments.
,
Jan 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/99a151abc827d91ca3dbb0addee654c0a2b985de commit 99a151abc827d91ca3dbb0addee654c0a2b985de Author: Theresa <twellington@chromium.org> Date: Fri Jan 26 18:07:47 2018 Introduce AnchoredPopupWindow and refactor TextBubble Introduce a new AnchoredPopupWindow class, containing most of the logic previously in TextBubble, and refactor TextBubble to use this new class. Also introduces a ViewRectProvider that contains most of the logic previously in ViewAnchoredTextBubble. This lays the ground work for using AnchoredPopupWindow (with extra positioning customization/logic) for ListMenuButton to work around a bug in Android ListPopupWindow positioning introduced in Android N. BUG= 709522 Change-Id: Ie6364ddcbd7b84736650ad3df8d433e1a75be6b6 Reviewed-on: https://chromium-review.googlesource.com/884328 Commit-Queue: Theresa <twellington@chromium.org> Reviewed-by: Matthew Jones <mdjones@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Cr-Commit-Position: refs/heads/master@{#532012} [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchIPH.java [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHBubbleDelegateImpl.java [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/infobar/IPHInfoBarSupport.java [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/toolbar/TabSwitcherCallout.java [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java [add] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/widget/AnchoredPopupWindow.java [add] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/widget/RectProvider.java [add] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/widget/ViewRectProvider.java [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/ChromeHomeIphBubbleController.java [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/TextBubble.java [delete] https://crrev.com/e0f2031db87e7ca20ae60f80f87593018da338de/chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/ViewAnchoredTextBubble.java [modify] https://crrev.com/99a151abc827d91ca3dbb0addee654c0a2b985de/chrome/android/java_sources.gni
,
Jan 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dd3956f40abe1fcd8234b35fe1e79b97f159bb77 commit dd3956f40abe1fcd8234b35fe1e79b97f159bb77 Author: Theresa <twellington@chromium.org> Date: Tue Jan 30 01:58:44 2018 Refactor ListMenuButton to use AnchoredPopupWindow Refactor ListMenuButton to use AnchoredPoupWindow instead of ListPopupWindow. Add extra postioning logic and customization to AnchoredPopupWindow, and add a handful of junit tests for the popup x and y calculations. BUG= 709522 Change-Id: I6862ca644e9cd6c467e126bde908f6893dfcf530 Reviewed-on: https://chromium-review.googlesource.com/887227 Commit-Queue: Theresa <twellington@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Cr-Commit-Position: refs/heads/master@{#532727} [modify] https://crrev.com/dd3956f40abe1fcd8234b35fe1e79b97f159bb77/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java [modify] https://crrev.com/dd3956f40abe1fcd8234b35fe1e79b97f159bb77/chrome/android/java/src/org/chromium/chrome/browser/widget/AnchoredPopupWindow.java [modify] https://crrev.com/dd3956f40abe1fcd8234b35fe1e79b97f159bb77/chrome/android/java/src/org/chromium/chrome/browser/widget/ListMenuButton.java [modify] https://crrev.com/dd3956f40abe1fcd8234b35fe1e79b97f159bb77/chrome/android/java/src/org/chromium/chrome/browser/widget/ViewRectProvider.java [modify] https://crrev.com/dd3956f40abe1fcd8234b35fe1e79b97f159bb77/chrome/android/java/src/org/chromium/chrome/browser/widget/textbubble/TextBubble.java [modify] https://crrev.com/dd3956f40abe1fcd8234b35fe1e79b97f159bb77/chrome/android/java_sources.gni [add] https://crrev.com/dd3956f40abe1fcd8234b35fe1e79b97f159bb77/chrome/android/junit/src/org/chromium/chrome/browser/widget/AnchoredPopupWindowTest.java
,
Jan 30 2018
Lowering priority as remaining work is just for autofill which doesn't look as bad on N+.
,
Feb 6 2018
Opened a new bug for the remaining DropdownPopupWindow work (issue 809758) |
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by twelling...@chromium.org
, Jun 1 2017