New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 657286 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression
Team-Security-UX

Blocked on:
issue 655880



Sign in to add a comment

Page Info dropdowns do not stay right-aligned after permission change.

Project Member Reported by lgar...@chromium.org, Oct 19 2016

Issue description

Chrome 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)
 
Labels: -maci Proj-MacViews OS-Chrome OS-Linux OS-Mac OS-Windows
first-change-windows.png
136 KB View Download
second-change-windows.png
121 KB View Download
first-change-mac.png
172 KB View Download
Blockedon: 655880
There is a good chance that  Issue 655880  will either fix this, or present an opportunity to fix this simultaneously.
Components: -UI>Browser>Omnibox>PageInfo UI>Browser>Bubbles>PageInfo
Cc: nyerramilli@chromium.org ranjitkan@chromium.org lgar...@chromium.org tkonch...@chromium.org
 Issue 655000  has been merged into this issue.
Cc: msrchandra@chromium.org
 Issue 667255  has been merged into this issue.
 Issue 674248  has been merged into this issue.

Comment 8 by shrike@chromium.org, Dec 14 2016

Cc: shrike@chromium.org emilyschechter@chromium.org
Labels: -Pri-3 M-57 Pri-1
Owner: lgar...@chromium.org
Status: Assigned (was: Available)
This is an embarrassing bug that should be fixed at least in the next milestone.

Comment 9 by shrike@chromium.org, Dec 14 2016

Labels: -Type-Bug Type-Bug-Regression
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
Owner: est...@chromium.org
Status: Started (was: Assigned)
https://codereview.chromium.org/2647633002/
Project Member

Comment 12 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Labels: Merge-Request-57
Requesting a merge for the commit in comment 12
Project Member

Comment 15 by sheriffbot@chromium.org, Jan 21 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
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
Project Member

Comment 16 by bugdroid1@chromium.org, Jan 21 2017

Labels: -merge-approved-57 merge-merged-2987
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

 Issue 657284  has been merged into this issue.

Sign in to add a comment