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

Issue 794480 link

Starred by 2 users

Issue metadata

Status: Duplicate
Merged: issue 757673
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Should checkbox to label distance still be 16pt?

Reported by rp...@etouch.net, Dec 13 2017

Issue description

Version: 65.0.3293.0 5d03c84689520e76673121751641d53314db60a7-refs/heads/master@{#523641}
OS: Windows (7,8,8.1,10),Linux (14.04 LTS),Mac OS X(10.12.6,10.13.1)
Pre condition : Enable Material Design in the rest of the browser's native UI flag from chrome://flags
URL : https://chrome.google.com/webstore/detail/session-buddy/edacconmaakjimmfgnblocblbcdcpbko?utm_source=chrome-ntp-icon

What steps will reproduce the problem?
1. Launch chrome, navigate to above url and add extension.
2. Now remove added extension by right clicking on extension icon and observe Report Abuse check box in remove extension overlay

Actual: Unwanted extra space is seen between check box and Report Abuse text label
Expected: Unwanted extra space should not be seen between check box and Report Abuse text label

This is regression issue, broken in ‘M 62’ and below is the bisect info :
Good build: 62.0.3166.0  (Revision: 489162).
Bad build: 62.0.3167.0 (Revision: 489499).

You are probably looking for a change made after 489335 (known good), but no later than 489336 (first known bad).

CHANGELOG URL:

The script might not always return single CL as suspect as some perf builds might get missing due to failure.

https://chromium.googlesource.com/chromium/src/+log/a417678f1f2f7f45c2077eb417baaa8fc2c90821..cfb8556546fdcaa1419452903e8c061454d4af78

From the CL above, assigning the issue to the concern owner 

@ellyjones- Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Suspect : https://chromium.googlesource.com/chromium/src/+/cfb8556546fdcaa1419452903e8c061454d4af78

Thanks!

 
Actual_screenshot.png
264 KB View Download
Expected_screenshot.png
300 KB View Download
Status: Started (was: Assigned)
Summary: Should checkbox to label distance still be 16pt? (was: Regression : Unwanted extra space is seen between check box and 'Report Abuse' text label in remove extension overlay.)
Here are the View bounds rectangles. The checkbox <-> label distance is the same as the checkbox width (ie, 16pt), which is what DISTANCE_RELATED_LABEL_HORIZONTAL is set to right now. In the past we've discussed that distance being 12 or 8 instead, but I'm not sure we ever settled on a change. I'll follow up with bettes@.
boundsrects.png
11.1 KB View Download
bettes@ got back to me: this is meant to be 12pt instead of 16pt, so I'll fix that.
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/69ded0f7f1e33889316d12b8e3dedb66a42ab320

commit 69ded0f7f1e33889316d12b8e3dedb66a42ab320
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Thu Jan 04 12:25:08 2018

views: reduce checkbox and radiobutton spacing

In Harmony, the distance between these two elements is supposed
to be 3/4 of a layout unit instead of one layout unit, so:

1) Add DISTANCE_BUTTON_IMAGE_LABEL_PADDING to DistanceMetrics
2) Give it the same default value as DISTANCE_RELATED_CONTROL_HORIZONTAL
3) Add a Harmony value for the new DistanceMetric

Bug:  794480 
Change-Id: Ib7106612dd2c923386cdbf30219ff37180acdedd
Reviewed-on: https://chromium-review.googlesource.com/848533
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526963}
[modify] https://crrev.com/69ded0f7f1e33889316d12b8e3dedb66a42ab320/chrome/browser/ui/views/harmony/harmony_layout_provider.cc
[modify] https://crrev.com/69ded0f7f1e33889316d12b8e3dedb66a42ab320/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/69ded0f7f1e33889316d12b8e3dedb66a42ab320/ui/views/layout/layout_provider.cc
[modify] https://crrev.com/69ded0f7f1e33889316d12b8e3dedb66a42ab320/ui/views/layout/layout_provider.h

Status: Fixed (was: Started)
Mergedinto: 757673
Status: Duplicate (was: Fixed)
 Bug 757673  is the more general issue on this.

I don't think we should have added DISTANCE_BUTTON_IMAGE_LABEL_PADDING.  This is a rather implementation-specific concept rather than a semantic one; the semantic constant that already exists for this is DISTANCE_RELATED_LABEL_HORIZONTAL (note: not the same as RELATED_CONTROL), and its sibling DISTANCE_RELATED_LABEL_HORIZONTAL_LIST, which is the value to use in the context of a horizontal list of items (as on the bookmark bar).

I think we should have either changed the former of these, or both of them, to be 12.   Bug 757673  comments 7 and 8 have some more argumentation over this, though.  Another option is that instead of "space between an icon and a related label" we could have some kind of "minimum space on either side of an icon" constant, which could be used for the table cell padding Bret mentions as well.  But that has some ramifications if we really want to implement it that way (it implies we'd use this as a collapsible horizontal margin size around each icon).

Sign in to add a comment