Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2943533002:
Use the result view font list for the base answers font. (Closed)
|
Created:
3 years, 6 months ago by Justin Donnelly Modified:
3 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, jdonnelly+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse the result view font list for the base answers font.
BUG=733681
Review-Url: https://codereview.chromium.org/2943533002
Cr-Commit-Position: refs/heads/master@{#481332}
Committed: https://chromium.googlesource.com/chromium/src/+/720ecfbbc6d951e02e1025b26eeecbc79d9dfedf
Patch Set 1 #
Total comments: 8
Patch Set 2 : Respond to comment. #
Total comments: 2
Patch Set 3 : Respond to comment #Messages
Total messages: 28 (15 generated)
The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run
Description was changed from ========== Use the result view font list for the base answers font. BUG=https://crbug.com/733681 ========== to ========== Use the result view font list for the base answers font. BUG=733681 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jdonnelly@chromium.org changed reviewers: + pkasting@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:744: // adjustments from BaseFont before it was provided to this class. Hmm. It seems like if the font passed in has been adjusted, we'd really want the adjustments to apply to non-base fonts too, thus preserving the relative differences between them. This basically preserves the adjustment for BaseFont, but leaves the other cases untouched. It seems like maybe all font cases in this function should be derived off |font_list_|, instead of ever using the resource bundle fonts directly? https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:745: if (GetTextStyle(text_type).font == ui::ResourceBundle::BaseFont) Nit: Use a temp for this subexpression that's repeated below?
The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:744: // adjustments from BaseFont before it was provided to this class. On 2017/06/15 22:50:28, Peter Kasting wrote: > Hmm. It seems like if the font passed in has been adjusted, we'd really want > the adjustments to apply to non-base fonts too, thus preserving the relative > differences between them. This basically preserves the adjustment for BaseFont, > but leaves the other cases untouched. It seems like maybe all font cases in > this function should be derived off |font_list_|, instead of ever using the > resource bundle fonts directly? Yeah, it feels like that's the right thing to do. But I think it's a solution in search of a problem. The current fonts work well. Presumably ui::ResourceBundle::LargeFont is designed in a way that it produces an appropriately sized large font for the current platform and settings, whereas BaseFont is being manipulated to specifically match the size of the omnibox field. https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:745: if (GetTextStyle(text_type).font == ui::ResourceBundle::BaseFont) On 2017/06/15 22:50:28, Peter Kasting wrote: > Nit: Use a temp for this subexpression that's repeated below? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:744: // adjustments from BaseFont before it was provided to this class. On 2017/06/16 14:28:23, Justin Donnelly wrote: > On 2017/06/15 22:50:28, Peter Kasting wrote: > > Hmm. It seems like if the font passed in has been adjusted, we'd really want > > the adjustments to apply to non-base fonts too, thus preserving the relative > > differences between them. This basically preserves the adjustment for > BaseFont, > > but leaves the other cases untouched. It seems like maybe all font cases in > > this function should be derived off |font_list_|, instead of ever using the > > resource bundle fonts directly? > > Yeah, it feels like that's the right thing to do. But I think it's a solution in > search of a problem. The current fonts work well. Presumably > ui::ResourceBundle::LargeFont is designed in a way that it produces an > appropriately sized large font for the current platform and settings, whereas > BaseFont is being manipulated to specifically match the size of the omnibox > field. Hrm, I would worry about this, especially for users with really large fonts. I suspect this is kinda broken today for those users. I'm not going to block you landing this, since it seems like a win over where we are today, but I would take the time to look into this a little more deeply. I think some combination of Derive calls would do what you want. You could ping tapted@ for help, he's looked at a lot of typography stuff lately. https://codereview.chromium.org/2943533002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2943533002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:746: if (font_style == ui::ResourceBundle::BaseFont) Nit: Or just: return (font_style == ui::ResourceBundle::BaseFont) ? font_list_ : ui::ResourceBundle::GetSharedInstance().GetFontList(font_style);
https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:744: // adjustments from BaseFont before it was provided to this class. On 2017/06/16 23:47:21, Peter Kasting wrote: > Hrm, I would worry about this, especially for users with really large fonts. I > suspect this is kinda broken today for those users. > > I'm not going to block you landing this, since it seems like a win over where we > are today, but I would take the time to look into this a little more deeply. I > think some combination of Derive calls would do what you want. You could ping > tapted@ for help, he's looked at a lot of typography stuff lately. I'm interested in how answers look with different font size settings and I tried doing so earlier. But the only setting in Windows that seems to exist ("Make text and other items larger or smaller") scales both the font and the UI (both Win 7 and Win 10). FWIW the result of adjusting that setting works well: https://screenshot.googleplex.com/5PeDxRXONhe.png Off the top of your head do you know of some other thing I can try to make the font larger? https://codereview.chromium.org/2943533002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2943533002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:746: if (font_style == ui::ResourceBundle::BaseFont) On 2017/06/16 23:47:21, Peter Kasting wrote: > Nit: Or just: > > return (font_style == ui::ResourceBundle::BaseFont) > ? font_list_ > : ui::ResourceBundle::GetSharedInstance().GetFontList(font_style); Done.
The CQ bit was checked by jdonnelly@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2943533002/#ps60001 (title: "Respond to comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:744: // adjustments from BaseFont before it was provided to this class. On 2017/06/21 21:13:49, Justin Donnelly wrote: > On 2017/06/16 23:47:21, Peter Kasting wrote: > > Hrm, I would worry about this, especially for users with really large fonts. > I > > suspect this is kinda broken today for those users. > > > > I'm not going to block you landing this, since it seems like a win over where > we > > are today, but I would take the time to look into this a little more deeply. > I > > think some combination of Derive calls would do what you want. You could ping > > tapted@ for help, he's looked at a lot of typography stuff lately. > > I'm interested in how answers look with different font size settings and I tried > doing so earlier. But the only setting in Windows that seems to exist ("Make > text and other items larger or smaller") scales both the font and the UI (both > Win 7 and Win 10). FWIW the result of adjusting that setting works well: > https://screenshot.googleplex.com/5PeDxRXONhe.png > > Off the top of your head do you know of some other thing I can try to make the > font larger? On Win 7, try Control Panel > Appearance and Personalization > Personalization, click the "Window color" at the very bottom, click "Advanced appearance settings", change "Item" to "Message box", adjust the font size, apply, and restart Chrome.
https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2943533002/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:744: // adjustments from BaseFont before it was provided to this class. On 2017/06/21 21:20:23, Peter Kasting wrote: > On 2017/06/21 21:13:49, Justin Donnelly wrote: > > On 2017/06/16 23:47:21, Peter Kasting wrote: > > > Hrm, I would worry about this, especially for users with really large fonts. > > > I > > > suspect this is kinda broken today for those users. > > > > > > I'm not going to block you landing this, since it seems like a win over > where > > we > > > are today, but I would take the time to look into this a little more deeply. > > > I > > > think some combination of Derive calls would do what you want. You could > ping > > > tapted@ for help, he's looked at a lot of typography stuff lately. > > > > I'm interested in how answers look with different font size settings and I > tried > > doing so earlier. But the only setting in Windows that seems to exist ("Make > > text and other items larger or smaller") scales both the font and the UI (both > > Win 7 and Win 10). FWIW the result of adjusting that setting works well: > > https://screenshot.googleplex.com/5PeDxRXONhe.png > > > > Off the top of your head do you know of some other thing I can try to make the > > font larger? > > On Win 7, try Control Panel > Appearance and Personalization > Personalization, > click the "Window color" at the very bottom, click "Advanced appearance > settings", change "Item" to "Message box", adjust the font size, apply, and > restart Chrome. Thanks! It's so obvious, why didn't I think of that? :P Ok, here's the result of maxing out the text size: https://screenshot.googleplex.com/dS4RFPJ69P1.png It looks to me like, insofar as the design calls for this to be large text, that the answer is doing the right thing. If anything's broken, I'd suggest it's the tabs and the omnibox which aren't growing to accommodate the user's desired font size.
On 2017/06/21 21:32:07, Justin Donnelly wrote: > Thanks! It's so obvious, why didn't I think of that? :P It used to be really easy to find this dialog in the XP days, but in 7 it got a lot harder :P > Ok, here's the result of maxing out the text size: > https://screenshot.googleplex.com/dS4RFPJ69P1.png > > It looks to me like, insofar as the design calls for this to be large text, that > the answer is doing the right thing. If anything's broken, I'd suggest it's the > tabs and the omnibox which aren't growing to accommodate the user's desired font > size. Things you might want to worry about: * Maybe the dictionary font you fixed in this case should have been larger in your testcase above than it will after your fix; is that OK? * Are there answers which put "normal" text right next to some kind of not-normal text, so the relative sizes between the two will change dramatically if the user changes their system font size? * If you shrink rather than enlarge the message box font, all the answer fonts will shrink except the "base font" ones, which will stay the same size. Is that OK?
On 2017/06/21 21:37:12, Peter Kasting wrote: > On 2017/06/21 21:32:07, Justin Donnelly wrote: > > Thanks! It's so obvious, why didn't I think of that? :P > > It used to be really easy to find this dialog in the XP days, but in 7 it got a > lot harder :P > > > Ok, here's the result of maxing out the text size: > > https://screenshot.googleplex.com/dS4RFPJ69P1.png > > > > It looks to me like, insofar as the design calls for this to be large text, > that > > the answer is doing the right thing. If anything's broken, I'd suggest it's > the > > tabs and the omnibox which aren't growing to accommodate the user's desired > font > > size. > > Things you might want to worry about: > * Maybe the dictionary font you fixed in this case should have been larger in > your testcase above than it will after your fix; is that OK? > * Are there answers which put "normal" text right next to some kind of > not-normal text, so the relative sizes between the two will change dramatically > if the user changes their system font size? > * If you shrink rather than enlarge the message box font, all the answer fonts > will shrink except the "base font" ones, which will stay the same size. Is that > OK? Those are all excellent questions that I don't have great answers to at the moment. I *can* verify that all of the current answer types are doing reasonable things at both very large and very small font sizes. Mainly because we don't currently have the capability to mix different font sizes within each RenderText and we're just using a baseline hack to make the text smaller in some cases. So within each line, nothing looks mismatched. I know the obvious thing to do would be to simply scale them all relative to the base font size as you've proposed. But that would have an adverse effect. Currently, the LargeFont answers are already pretty big. The scaling the omnibox applies to the default font size would make these even larger. The reality is that these mixed font sizes in answers are problematic. There's a very good chance that we'll get rid of them. (A design refresh is planned for Q3 and I know the designers don't like the giant font sizes.) If we decide we want to keep the ability to mix font sizes in the answers, I'll need to come up with a better way to do this in general.
Sounds like you have thought sufficiently about this and the right thing for now is not to change your CL :)
On 2017/06/21 21:55:21, Peter Kasting wrote: > Sounds like you have thought sufficiently about this and the right thing for now > is not to change your CL :) To be honest, I hadn't thought that much about it before you started bugging me. ;) So it's been a worthwhile discussion and now I know how to mess with fonts in Windows!
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498079633721670,
"parent_rev": "0c1f73a50ab7c31c987decaba4abb5e3cbe8e286", "commit_rev":
"720ecfbbc6d951e02e1025b26eeecbc79d9dfedf"}
Message was sent while issue was closed.
Description was changed from ========== Use the result view font list for the base answers font. BUG=733681 ========== to ========== Use the result view font list for the base answers font. BUG=733681 Review-Url: https://codereview.chromium.org/2943533002 Cr-Commit-Position: refs/heads/master@{#481332} Committed: https://chromium.googlesource.com/chromium/src/+/720ecfbbc6d951e02e1025b26eee... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/720ecfbbc6d951e02e1025b26eee... |
