New issue
Advanced search Search tips

Issue 633986 link

Starred by 2 users
Status: Fixed
Owner:
est...@chromium.org
Closed: Dec 2016
Cc:
sky@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

remove LabelButton::SetFontList

Project Member Reported by est...@chromium.org, Aug 3 2016 
see discussion here [1]. We'll need to create LabelButton::AdjustFontSize and update callers.

"""
I don't [think] that we should allow people to set arbitrary fonts. I can't think
of a reason to allow setting a different typeface, for example. This just leads
to mistakes where engineers see a mock with roboto (because it was created in
the context of chromeos) and think they need to apply roboto manually on Windows
or Mac or wherever. The set of allowable changes to fonts can be whittled down
to probably just size. I don't even think weight should be modifiable because it
will just lead to inconsistencies. Particularly in the case of "standard looking
button", i.e. MdTextButton, I think we should be more restrictive. ainslie@ et
al are busy trying to come up with design guidelines that apply to all secondary
UI, and I'm simultaneously trying to make it harder for engineers to diverge
from these guidelines.

In codesearch I see 15 calls to LabelButton::SetFontList, broken down as
follows:
1 seems completely pointless
6 just change the size
2 change the weight but will go away in MD land
3 calls set it to "Roboto-regular, 13px", which should actually just be +1 font
size
2 are unit tests
1 is inside MdTextButton
"""

[1] https://codereview.chromium.org/2190453002/
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 4 2016 

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

commit 3cd12bc7b5094336d2ab5b0a4e307225a458a9ad
Author: estade <estade@chromium.org>
Date: Thu Aug 04 15:08:49 2016

Remove a couple uses of LabelButton::SetFontList

other uses are still blocked on removal of pre-MD codepaths or other
code updates.

BUG= 633986 

Review-Url: https://codereview.chromium.org/2208773002
Cr-Commit-Position: refs/heads/master@{#409786}

[modify] https://crrev.com/3cd12bc7b5094336d2ab5b0a4e307225a458a9ad/chrome/browser/chromeos/profiles/multiprofiles_intro_dialog.cc
[modify] https://crrev.com/3cd12bc7b5094336d2ab5b0a4e307225a458a9ad/ui/views/touchui/touch_selection_menu_runner_views.cc

Comment 2 by pkasting@chromium.org, Aug 6 2016 

See also https://codereview.chromium.org/1020853018/ , which seems to contradict comment 0 here, because it was a way to just set font sizes that was abandoned in favor of using different font lists.

Comment 3 by est...@chromium.org, Aug 6 2016 

my reading of msw's comments there seem not to contradict my intent here. In particular I agree with

"""
I'm not adamantly opposed to allowing direct control of font sizes through RenderText
API, but I'd encourage an API that doesn't promote/risk "incorrect" usage
"""

Note that the SetFontList in the linked CL is a method on RenderText, and I'm arguing to remove a function of the same name from LabelButton, which is higher level.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 24 2016 

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

commit b5c0820644a3f5cfac8a60b91b2555c8d2123481
Author: estade <estade@chromium.org>
Date: Mon Oct 24 19:44:00 2016

A few more cleanups to overview mode code related to the pre-MD version.

BUG= 633986 

Review-Url: https://codereview.chromium.org/2441043003
Cr-Commit-Position: refs/heads/master@{#427122}

[modify] https://crrev.com/b5c0820644a3f5cfac8a60b91b2555c8d2123481/ash/common/wm/overview/scoped_transform_overview_window.cc
[modify] https://crrev.com/b5c0820644a3f5cfac8a60b91b2555c8d2123481/ash/common/wm/overview/window_selector_item.cc
[modify] https://crrev.com/b5c0820644a3f5cfac8a60b91b2555c8d2123481/ash/resources/ash_resources.grd
[delete] https://crrev.com/e865b1df520e0dfac85cf77958d80e220676d829/ash/resources/default_100_percent/common/window_overview_close_hover.png
[delete] https://crrev.com/e865b1df520e0dfac85cf77958d80e220676d829/ash/resources/default_100_percent/common/window_overview_close_normal.png
[delete] https://crrev.com/e865b1df520e0dfac85cf77958d80e220676d829/ash/resources/default_100_percent/common/window_overview_close_pressed.png
[delete] https://crrev.com/e865b1df520e0dfac85cf77958d80e220676d829/ash/resources/default_200_percent/common/window_overview_close_hover.png
[delete] https://crrev.com/e865b1df520e0dfac85cf77958d80e220676d829/ash/resources/default_200_percent/common/window_overview_close_normal.png
[delete] https://crrev.com/e865b1df520e0dfac85cf77958d80e220676d829/ash/resources/default_200_percent/common/window_overview_close_pressed.png

Comment 7 by est...@chromium.org, Dec 13 2016

Status: Fixed (was: Assigned)
It should be apparent enough that SetFontListDeprecated should not be used (and its only callsite will naturally be removed at some point in the hopefully near future).
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 5 2017 

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

commit fbb840ad09271f71f680f93a4484aaef68aaf952
Author: tapted <tapted@chromium.org>
Date: Wed Apr 05 01:10:14 2017

Remove LabelButton::SetFontListDeprecated().

The last remaining call site in profile_chooser_view.cc was removed in
r455113.

BUG= 633986 

Review-Url: https://codereview.chromium.org/2793283002
Cr-Commit-Position: refs/heads/master@{#461929}

[modify] https://crrev.com/fbb840ad09271f71f680f93a4484aaef68aaf952/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/fbb840ad09271f71f680f93a4484aaef68aaf952/ui/views/controls/button/label_button.h
►

Sign in to add a comment