Issue metadata
Sign in to add a comment
|
Page Info dropdowns do not stay right-aligned after permission change. |
||||||||||||||||||||||||
Issue descriptionChrome 56.0.2894.0 OSX 10.11.6 / Windows 10 What steps will reproduce the problem? (1) On Mac, enable #mac-views-webui-dialogs and #secondary-ui-md (1) Open Page Info (2) Change a permission (3) Change it again What is the expected output? The triangle of the dropdown stays right-aligned with the right bubble edge, and the dropdown text stays right-aligned with it. What do you see instead? It seems the following happens: A) If the preferred width of the permission table (a GridLayout) is smaller than the current width of the Page Info bubble (minus padding), then the GridLayout sizes to its preferred size, ignoring the FILL setting on its parent. This can be observed in first-change-windows.png B) If the permission table is already at its preferred width, then the dropdown moves to be left-aligned within its column, i.e. directly to the right of the permission label. See first-change-mac.png and second-change-windows.png In addition, this compounds with Issue 657284 (Harmony dropdown left-aligns selected value text if it is smaller than previous value)
,
Oct 19 2016
,
Oct 19 2016
There is a good chance that Issue 655880 will either fix this, or present an opportunity to fix this simultaneously.
,
Oct 20 2016
,
Nov 3 2016
Issue 655000 has been merged into this issue.
,
Nov 22 2016
,
Dec 14 2016
Issue 674248 has been merged into this issue.
,
Dec 14 2016
This is an embarrassing bug that should be fixed at least in the next milestone.
,
Dec 14 2016
,
Jan 19 2017
I looked at this a little bit today. AFAICT, the problem is in PermissionSelectorRow::ChildPreferredSizeChanged [1], wherein we size the PermissionSelectorRow to its preferred size. Changing that method to simply call Layout() (and nothing else) seems to behave as expected, at least on Linux (haven't tested on Mac). The dropdowns do not jump around, and the entire bubble resizes if necessary to accommodate a long label. The parent()->SizeToPreferredSize() call seems to have been added a long time ago in [2] but is no longer necessary, I think, because WebsiteSettingsPopupView calls SizeToContents() when a permission setting changes, and it didn't back then when that call was added. Intuitively it makes sense to me that we would want to simply do a fresh layout rather than forcing the row to its preferred size. We don't set the row to its preferred size otherwise, so I don't see why we should when a child view changes. But, I don't really know much about Views yet so maybe this makes no sense. ¯\_(ツ)_/¯ [1] https://cs.chromium.org/chromium/src/chrome/browser/ui/views/website_settings/permission_selector_row.cc?dr=C&q=permissionselectorrow::child&sq=package:chromium&l=295 [2] https://chromium.googlesource.com/chromium/src/+/445e2128b7c28657b5ef4d268300734cb72d6d8d
,
Jan 20 2017
https://codereview.chromium.org/2647633002/
,
Jan 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f24aa8ad5db849c114c91cddcc297475b8ead2c8 commit f24aa8ad5db849c114c91cddcc297475b8ead2c8 Author: estark <estark@chromium.org> Date: Fri Jan 20 08:45:41 2017 Do not size PermissionSelectorRow to preferred size PermissionSelectorRow's preferred size is smaller than the width of the Page Info Bubble, so when a permission changes, resizing to the preferred size means that the row shrinks smaller than the other rows and the dropdown jumps around. Instead, simply call Layout() to do a fresh layout of the row. The parent()->SizeToPreferredSize() call is no longer necessary because WebsiteSettingsPopupView calls SizeToContents() when a permission is changed. Note that bug 655880 may want to reintroduce the PermissionSelectorRow::ChildPreferredSizeChanged override and propagate the change to WebsiteSettingsPopupView that way instead of via OnPermissionChanged, but it's not needed now -- and I'm guessing that, for that bug, PermissionSelectorRow::ChildPreferredSizeChanged may want to call parent()->PreferredSizeChanged(), rather than calling SizeToPreferredSize() (which assumes that the parent ought to be at its preferred size). BUG= 657286 TEST=On Windows/Linux, visit https://google.com (or any other site) and open Page Info by clicking on the security indicator in the omnibox. Change a permission, change it again. Observe that the dropdown does not jump to the left of where it started. Review-Url: https://codereview.chromium.org/2647633002 Cr-Commit-Position: refs/heads/master@{#445017} [modify] https://crrev.com/f24aa8ad5db849c114c91cddcc297475b8ead2c8/chrome/browser/ui/views/website_settings/permission_selector_row.cc
,
Jan 20 2017
,
Jan 21 2017
Requesting a merge for the commit in comment 12
,
Jan 21 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/85df4fc55cf9dd7200d6ef94914dd8c553e747bc commit 85df4fc55cf9dd7200d6ef94914dd8c553e747bc Author: Emily Stark <estark@google.com> Date: Sat Jan 21 16:29:31 2017 Do not size PermissionSelectorRow to preferred size PermissionSelectorRow's preferred size is smaller than the width of the Page Info Bubble, so when a permission changes, resizing to the preferred size means that the row shrinks smaller than the other rows and the dropdown jumps around. Instead, simply call Layout() to do a fresh layout of the row. The parent()->SizeToPreferredSize() call is no longer necessary because WebsiteSettingsPopupView calls SizeToContents() when a permission is changed. Note that bug 655880 may want to reintroduce the PermissionSelectorRow::ChildPreferredSizeChanged override and propagate the change to WebsiteSettingsPopupView that way instead of via OnPermissionChanged, but it's not needed now -- and I'm guessing that, for that bug, PermissionSelectorRow::ChildPreferredSizeChanged may want to call parent()->PreferredSizeChanged(), rather than calling SizeToPreferredSize() (which assumes that the parent ought to be at its preferred size). BUG= 657286 TEST=On Windows/Linux, visit https://google.com (or any other site) and open Page Info by clicking on the security indicator in the omnibox. Change a permission, change it again. Observe that the dropdown does not jump to the left of where it started. Review-Url: https://codereview.chromium.org/2647633002 Cr-Commit-Position: refs/heads/master@{#445017} (cherry picked from commit f24aa8ad5db849c114c91cddcc297475b8ead2c8) Review-Url: https://codereview.chromium.org/2647173002 . Cr-Commit-Position: refs/branch-heads/2987@{#11} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/85df4fc55cf9dd7200d6ef94914dd8c553e747bc/chrome/browser/ui/views/website_settings/permission_selector_row.cc
,
Feb 10 2017
Issue 657284 has been merged into this issue. |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by lgar...@chromium.org
, Oct 19 2016