Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1306173002:
Set font size to 14 in location bar for material design (Closed)
|
Created:
5 years, 4 months ago by tdanderson Modified:
5 years, 3 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet font size to 14 in location bar for material design
Move the constant specifying the desired
font size for the toolbar view into
ThemeProperties and specify a size of
14 for the two variants of material
design. Also decreased the constant used
for minimum vertical padding in the
location bar in order to give the text
sufficient space.
BUG=522590
TEST=manual
Committed: https://crrev.com/f722fd76578be9b3ef4bd079e7cae1b65754d30b
Cr-Commit-Position: refs/heads/master@{#349670}
Patch Set 1 #
Total comments: 3
Patch Set 2 : remove constant #Patch Set 3 : smaller change #Patch Set 4 : use GetLayoutConstant() #Patch Set 5 : padding #
Total comments: 2
Messages
Total messages: 39 (10 generated)
tdanderson@chromium.org changed reviewers: + jonross@chromium.org
Jon, mind taking an informal first glance?
https://codereview.chromium.org/1306173002/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/1306173002/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:226: location_height - bubble_vertical_padding)); Do we intend to have the bubble font also refer to different heights in MD?
On 2015/08/21 14:44:14, jonross wrote: > https://codereview.chromium.org/1306173002/diff/1/chrome/browser/ui/views/loc... > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > https://codereview.chromium.org/1306173002/diff/1/chrome/browser/ui/views/loc... > chrome/browser/ui/views/location_bar/location_bar_view.cc:226: location_height - > bubble_vertical_padding)); > Do we intend to have the bubble font also refer to different heights in MD? The spec says the bubble font should be 11pt, and this will be addressed by a different CL (and a different bug).
On 2015/08/21 14:46:27, tdanderson wrote: > On 2015/08/21 14:44:14, jonross wrote: > > > https://codereview.chromium.org/1306173002/diff/1/chrome/browser/ui/views/loc... > > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > > > > https://codereview.chromium.org/1306173002/diff/1/chrome/browser/ui/views/loc... > > chrome/browser/ui/views/location_bar/location_bar_view.cc:226: location_height > - > > bubble_vertical_padding)); > > Do we intend to have the bubble font also refer to different heights in MD? > > The spec says the bubble font should be 11pt, and this will be > addressed by a different CL (and a different bug). Ah, in that case: LGTM
tdanderson@chromium.org changed reviewers: + pkasting@chromium.org
Peter, can you please take a look? Have a look at comment #7 in crbug.com/522590 for some additional context.
It worries me that your description says the internal padding is used for bubbles, but not for the main font. The bubble size and the fonts inside the bubble all need to track the right way with the main font. Using internal padding for one and not the other is almost certainly not right. https://codereview.chromium.org/1306173002/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (left): https://codereview.chromium.org/1306173002/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:204: const int desired_font_size = browser_defaults::kOmniboxFontPixelSize; Please remove the declaration and definition of this constant.
The hard constraints of the MD spec are: * The location bar must have a height of 28 * The bubbles/icons must have internal padding of 6 * The font size must be 14 The issue is that ResourceBundle::BaseFont reports a font height of 17 when we only have an available height of 28 - 2*6 = 16, so the font size is forced to shrink from 14 to 12. (I'm assuming that the font height computation of 17 is correct.) IIUC the reason fonts may not fit in the location bar is because its height depends on the height of the back button, which can vary based on custom themes. But since MD explicitly specifies the location bar height as 28, would there even be any situations where we'd need to shrink the font? https://codereview.chromium.org/1306173002/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (left): https://codereview.chromium.org/1306173002/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:204: const int desired_font_size = browser_defaults::kOmniboxFontPixelSize; On 2015/08/21 17:14:35, Peter Kasting wrote: > Please remove the declaration and definition of this constant. Done.
On 2015/08/21 18:17:24, tdanderson wrote: > The hard constraints of the MD spec are: > > * The location bar must have a height of 28 > * The bubbles/icons must have internal padding of 6 > * The font size must be 14 We can't enforce most of these. You can set defaults, but the sizes of various elements will change on different systems, e.g. due to visual accessibility needs (some users require very large fonts). You need to design your system around the requirement that these be relaxable, and instead of assuming a size, describe what should happen as we need to increase font size (possibly by a lot). This is a bit tangential to my concern, though. My worry is the inconsistency of trying to use the internal height for bubbles but not for the main font. The internal height doesn't include any "extra padding" and should be the full available height for both bubbles and font. (Bubbles are then separately given extra padding elsewhere). > IIUC the reason fonts may not fit in the location bar is because its height > depends on the height of the back button, which can vary based on custom themes. Nope. The location bar height doesn't necessarily need to match the back button. Actually the reverse is true: you should start with a description of how the location bar should change as the font size changes, and then work from there to describe what should happen to the other toolbar elements in those cases.
On 2015/08/21 18:25:26, Peter Kasting wrote: > On 2015/08/21 18:17:24, tdanderson wrote: > > The hard constraints of the MD spec are: > > > > * The location bar must have a height of 28 > > * The bubbles/icons must have internal padding of 6 > > * The font size must be 14 > > We can't enforce most of these. You can set defaults, but the sizes of various > elements will change on different systems, e.g. due to visual accessibility > needs (some users require very large fonts). You need to design your system > around the requirement that these be relaxable, and instead of assuming a size, > describe what should happen as we need to increase font size (possibly by a > lot). > > This is a bit tangential to my concern, though. My worry is the inconsistency > of trying to use the internal height for bubbles but not for the main font. The > internal height doesn't include any "extra padding" and should be the full > available height for both bubbles and font. (Bubbles are then separately given > extra padding elsewhere). > > > IIUC the reason fonts may not fit in the location bar is because its height > > depends on the height of the back button, which can vary based on custom > themes. > > Nope. The location bar height doesn't necessarily need to match the back > button. Actually the reverse is true: you should start with a description of > how the location bar should change as the font size changes, and then work from > there to describe what should happen to the other toolbar elements in those > cases. You're right, I see now that I am going about this in the wrong way, and that the hard constraints I listed aren't necessarily hard. In the next patch set I've only moved the font size constant and have not made any changes to layout/padding for the time being. Can you please take a look?
LGTM
tdanderson@chromium.org changed reviewers: + thakis@chromium.org
Nico, can you please take a look?
rs-lgtm
The CQ bit was checked by tdanderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jonross@chromium.org Link to the patchset: https://codereview.chromium.org/1306173002/#ps40001 (title: "smaller change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306173002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306173002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tdanderson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306173002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306173002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/08/24 15:48:51, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Peter, LocationBarView is being instantiated from SimpleWebViewDialog, and the DefaultThemeProvider is being returned when the location bar calls GetThemeProvider(). The default theme provider gives a |desired_font_size| of -1, and as a result CaptivePortalWindowTest.* is failing. Is there a fix/workaround you can suggest for this (would it be reasonable for DefaultThemeProvider::GetDisplayProperty() to return ThemeProperties::GetDefaultDisplayProperty()?), or will we have to move the MD constants out of ThemeProperties?
On 2015/08/24 19:16:02, tdanderson wrote: > On 2015/08/24 15:48:51, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > Peter, > > LocationBarView is being instantiated from > SimpleWebViewDialog, and the DefaultThemeProvider is > being returned when the location bar calls > GetThemeProvider(). The default theme provider > gives a |desired_font_size| of -1, and as a result > CaptivePortalWindowTest.* is failing. > > Is there a fix/workaround you can suggest for this > (would it be reasonable for > DefaultThemeProvider::GetDisplayProperty() to > return ThemeProperties::GetDefaultDisplayProperty()?), > or will we have to move the MD constants out of > ThemeProperties? Please disregard my suggestion about calling GetDefaultDisplayProperty(), I see that will not work.
On 2015/08/24 19:16:02, tdanderson wrote: > On 2015/08/24 15:48:51, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > Peter, > > LocationBarView is being instantiated from > SimpleWebViewDialog, and the DefaultThemeProvider is > being returned when the location bar calls > GetThemeProvider(). The default theme provider > gives a |desired_font_size| of -1, and as a result > CaptivePortalWindowTest.* is failing. > > Is there a fix/workaround you can suggest for this > (would it be reasonable for > DefaultThemeProvider::GetDisplayProperty() to > return ThemeProperties::GetDefaultDisplayProperty()?), > or will we have to move the MD constants out of > ThemeProperties? I don't know. It seems a bit strange, fundamentally, that we have a dialog that has non-web elements (like a location bar) but doesn't have the correct specific theme provider. That seems like a bug to me; I'd focus on how SimpleWebViewDialog can return the correct theme provider. But maybe someone who understands theming better would know a reason that's not true? Not sure whom to ask.
On 2015/08/24 20:27:49, Peter Kasting wrote: > On 2015/08/24 19:16:02, tdanderson wrote: > > On 2015/08/24 15:48:51, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > Peter, > > > > LocationBarView is being instantiated from > > SimpleWebViewDialog, and the DefaultThemeProvider is > > being returned when the location bar calls > > GetThemeProvider(). The default theme provider > > gives a |desired_font_size| of -1, and as a result > > CaptivePortalWindowTest.* is failing. > > > > Is there a fix/workaround you can suggest for this > > (would it be reasonable for > > DefaultThemeProvider::GetDisplayProperty() to > > return ThemeProperties::GetDefaultDisplayProperty()?), > > or will we have to move the MD constants out of > > ThemeProperties? > > I don't know. It seems a bit strange, fundamentally, that we have a dialog that > has non-web elements (like a location bar) but doesn't have the correct specific > theme provider. That seems like a bug to me; I'd focus on how > SimpleWebViewDialog can return the correct theme provider. But maybe someone > who understands theming better would know a reason that's not true? Not sure > whom to ask. Currently, it is the responsibility of the top-most Widget to provide the ThemeProvider to all of its Views. Dialogs and Popups are considered to be top-most Widgets. The browser's chrome instantiates ThemeService as its ThemeProvider, and sets it on its Widget. Currently all of the dialogs/popups created by top chrome UI elements do not directly set this ThemeProvider on their Widget. Instead there is a lot of passing around Views from top chrome, and using them to get the appropriate ThemeProvider. OmniboxResultView: location_bar_view->GetThemeProvider() https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I think that this is an incorrect workaround, and that these Dialogs/Popups should be properly providing a ThemeProvider to their Views.
On 2015/08/24 20:36:30, jonross wrote: > On 2015/08/24 20:27:49, Peter Kasting wrote: > > On 2015/08/24 19:16:02, tdanderson wrote: > > > On 2015/08/24 15:48:51, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > > > Peter, > > > > > > LocationBarView is being instantiated from > > > SimpleWebViewDialog, and the DefaultThemeProvider is > > > being returned when the location bar calls > > > GetThemeProvider(). The default theme provider > > > gives a |desired_font_size| of -1, and as a result > > > CaptivePortalWindowTest.* is failing. > > > > > > Is there a fix/workaround you can suggest for this > > > (would it be reasonable for > > > DefaultThemeProvider::GetDisplayProperty() to > > > return ThemeProperties::GetDefaultDisplayProperty()?), > > > or will we have to move the MD constants out of > > > ThemeProperties? > > > > I don't know. It seems a bit strange, fundamentally, that we have a dialog > that > > has non-web elements (like a location bar) but doesn't have the correct > specific > > theme provider. That seems like a bug to me; I'd focus on how > > SimpleWebViewDialog can return the correct theme provider. But maybe someone > > who understands theming better would know a reason that's not true? Not sure > > whom to ask. > > Currently, it is the responsibility of the top-most Widget to provide the > ThemeProvider to all of its Views. > > Dialogs and Popups are considered to be top-most Widgets. > > The browser's chrome instantiates ThemeService as its ThemeProvider, and sets it > on its Widget. > > Currently all of the dialogs/popups created by top chrome UI elements do not > directly set this ThemeProvider on their Widget. Instead there is a lot of > passing around Views from top chrome, and using them to get the appropriate > ThemeProvider. > OmniboxResultView: location_bar_view->GetThemeProvider() > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > I think that this is an incorrect workaround, and that these Dialogs/Popups > should be properly providing a ThemeProvider to their Views. Yeah, I've never liked the hack where various omnibox elements have to ask for the location bar's theme provider. If we can solve both these issues at once with a change to the core dialog code or something, that SGTM.
On 2015/08/24 20:38:03, Peter Kasting wrote: > On 2015/08/24 20:36:30, jonross wrote: > > On 2015/08/24 20:27:49, Peter Kasting wrote: > > > On 2015/08/24 19:16:02, tdanderson wrote: > > > > On 2015/08/24 15:48:51, commit-bot: I haz the power wrote: > > > > > Try jobs failed on following builders: > > > > > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux > (JOB_FAILED, > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > > > > > Peter, > > > > > > > > LocationBarView is being instantiated from > > > > SimpleWebViewDialog, and the DefaultThemeProvider is > > > > being returned when the location bar calls > > > > GetThemeProvider(). The default theme provider > > > > gives a |desired_font_size| of -1, and as a result > > > > CaptivePortalWindowTest.* is failing. > > > > > > > > Is there a fix/workaround you can suggest for this > > > > (would it be reasonable for > > > > DefaultThemeProvider::GetDisplayProperty() to > > > > return ThemeProperties::GetDefaultDisplayProperty()?), > > > > or will we have to move the MD constants out of > > > > ThemeProperties? > > > > > > I don't know. It seems a bit strange, fundamentally, that we have a dialog > > that > > > has non-web elements (like a location bar) but doesn't have the correct > > specific > > > theme provider. That seems like a bug to me; I'd focus on how > > > SimpleWebViewDialog can return the correct theme provider. But maybe > someone > > > who understands theming better would know a reason that's not true? Not > sure > > > whom to ask. > > > > Currently, it is the responsibility of the top-most Widget to provide the > > ThemeProvider to all of its Views. > > > > Dialogs and Popups are considered to be top-most Widgets. > > > > The browser's chrome instantiates ThemeService as its ThemeProvider, and sets > it > > on its Widget. > > > > Currently all of the dialogs/popups created by top chrome UI elements do not > > directly set this ThemeProvider on their Widget. Instead there is a lot of > > passing around Views from top chrome, and using them to get the appropriate > > ThemeProvider. > > OmniboxResultView: location_bar_view->GetThemeProvider() > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > I think that this is an incorrect workaround, and that these Dialogs/Popups > > should be properly providing a ThemeProvider to their Views. > > Yeah, I've never liked the hack where various omnibox elements have to ask for > the location bar's theme provider. If we can solve both these issues at once > with a change to the core dialog code or something, that SGTM. Thanks, Jon or I will look into fixing the general problem in more detail (and let's put this CL on hold for the time being).
On 2015/08/24 21:32:00, tdanderson wrote: > On 2015/08/24 20:38:03, Peter Kasting wrote: > > On 2015/08/24 20:36:30, jonross wrote: > > > On 2015/08/24 20:27:49, Peter Kasting wrote: > > > > On 2015/08/24 19:16:02, tdanderson wrote: > > > > > On 2015/08/24 15:48:51, commit-bot: I haz the power wrote: > > > > > > Try jobs failed on following builders: > > > > > > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux > > (JOB_FAILED, > > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > > > > > > > Peter, > > > > > > > > > > LocationBarView is being instantiated from > > > > > SimpleWebViewDialog, and the DefaultThemeProvider is > > > > > being returned when the location bar calls > > > > > GetThemeProvider(). The default theme provider > > > > > gives a |desired_font_size| of -1, and as a result > > > > > CaptivePortalWindowTest.* is failing. > > > > > > > > > > Is there a fix/workaround you can suggest for this > > > > > (would it be reasonable for > > > > > DefaultThemeProvider::GetDisplayProperty() to > > > > > return ThemeProperties::GetDefaultDisplayProperty()?), > > > > > or will we have to move the MD constants out of > > > > > ThemeProperties? > > > > > > > > I don't know. It seems a bit strange, fundamentally, that we have a > dialog > > > that > > > > has non-web elements (like a location bar) but doesn't have the correct > > > specific > > > > theme provider. That seems like a bug to me; I'd focus on how > > > > SimpleWebViewDialog can return the correct theme provider. But maybe > > someone > > > > who understands theming better would know a reason that's not true? Not > > sure > > > > whom to ask. > > > > > > Currently, it is the responsibility of the top-most Widget to provide the > > > ThemeProvider to all of its Views. > > > > > > Dialogs and Popups are considered to be top-most Widgets. > > > > > > The browser's chrome instantiates ThemeService as its ThemeProvider, and > sets > > it > > > on its Widget. > > > > > > Currently all of the dialogs/popups created by top chrome UI elements do not > > > directly set this ThemeProvider on their Widget. Instead there is a lot of > > > passing around Views from top chrome, and using them to get the appropriate > > > ThemeProvider. > > > OmniboxResultView: location_bar_view->GetThemeProvider() > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > I think that this is an incorrect workaround, and that these Dialogs/Popups > > > should be properly providing a ThemeProvider to their Views. > > > > Yeah, I've never liked the hack where various omnibox elements have to ask for > > the location bar's theme provider. If we can solve both these issues at once > > with a change to the core dialog code or something, that SGTM. > > Thanks, Jon or I will look into fixing the general problem in > more detail (and let's put this CL on hold for the time being). The test failure this change is encountering is fixed by: https://codereview.chromium.org/1310183009/ We can land this once that lands.
Peter, please take a look.
LGTM https://chromiumcodereview.appspot.com/1306173002/diff/80001/chrome/browser/u... File chrome/browser/ui/views/layout_constants.cc (right): https://chromiumcodereview.appspot.com/1306173002/diff/80001/chrome/browser/u... chrome/browser/ui/views/layout_constants.cc:16: const int kLocationBarVerticalPadding[] = {2, 2, 2}; You didn't make this change in old versions of this CL. Was this intentional? Might want to mention it in the CL description if so.
https://chromiumcodereview.appspot.com/1306173002/diff/80001/chrome/browser/u... File chrome/browser/ui/views/layout_constants.cc (right): https://chromiumcodereview.appspot.com/1306173002/diff/80001/chrome/browser/u... chrome/browser/ui/views/layout_constants.cc:16: const int kLocationBarVerticalPadding[] = {2, 2, 2}; On 2015/09/16 23:22:11, Peter Kasting wrote: > You didn't make this change in old versions of this CL. Was this intentional? > Might want to mention it in the CL description if so. Yes, this is an intentional change. Changed the CL description as you suggested.
The CQ bit was checked by tdanderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jonross@chromium.org, thakis@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1306173002/#ps80001 (title: "padding")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306173002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306173002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f722fd76578be9b3ef4bd079e7cae1b65754d30b Cr-Commit-Position: refs/heads/master@{#349670} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
