Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968713003:
Harmonize the find in page dialog. (Closed)
DescriptionHarmonize the find in page dialog.
This patch changes the insets and control spacing and the match count text
We space the controls in the find bar using margins to ensure that the
distances are computed to the vector image glyphs and not to the border.
Next steps would be to tackle the rest of the items in this dialog for harmony
BUG=651643
TBR=blundell
Review-Url: https://codereview.chromium.org/2968713003
Cr-Commit-Position: refs/heads/master@{#487316}
Committed: https://chromium.googlesource.com/chromium/src/+/a5f3fa6e02a96926c01d20fbf38be2257c251d49
Patch Set 1 #
Total comments: 7
Patch Set 2 : Set margins on the controls in the find bar. Experimental #
Total comments: 13
Patch Set 3 : Remove margins and just use INSETS_DIALOG_CONTROL for the dialog insets and DISTANCE_RELATED_CONTRO… #Patch Set 4 : removed spaces #Patch Set 5 : Added distance metrics for toast button/label/text. Using these in the find bar. #
Total comments: 12
Patch Set 6 : Fix insets and the rest of the review comments #Patch Set 7 : Change the match text to ordinal/number of matches #Patch Set 8 : Call through to ChromeLayoutProvider from HarmonyLayoutProvider::GetDistanceMetric() and add consta… #Patch Set 9 : Remove NOTREACHED() #Patch Set 10 : Remove unreachable code #Patch Set 11 : Fix dumb error in insets code #
Total comments: 4
Patch Set 12 : Localize strings #Patch Set 13 : Add an empty border around the separator with the same left and right values as the image button bo… #
Total comments: 2
Patch Set 14 : Revert empty border and fix test failures due to match text format change #Patch Set 15 : Use margins to space the controls #Patch Set 16 : Fix GlobalPasteBoardClearMatches browser test to account for the changed format of match count #
Total comments: 10
Patch Set 17 : Move INSETS_TOAST to chrome_layout_provider.cc/.h #Patch Set 18 : Address review comments. Use insets for margin calculation #Patch Set 19 : format fixes #
Total comments: 8
Patch Set 20 : Address review comments #Patch Set 21 : Address review comments #Patch Set 22 : Fix format #
Messages
Total messages: 83 (45 generated)
ananta@chromium.org changed reviewers: + pkasting@chromium.org
The CQ bit was checked by ananta@chromium.org
The CQ bit was unchecked by ananta@chromium.org
The CQ bit was checked by ananta@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...
https://codereview.chromium.org/2968713003/diff/1/chrome/browser/ui/views/fin... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:161: 0, related_control_horizontal, 0, related_control_horizontal)); It seems like this will result in 2x "related control spacing" around the separator, since we'll get this border + the standard boxlayout spacing below. https://codereview.chromium.org/2968713003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:170: views::INSETS_BUBBLE_CONTENTS)), The bubble insets aren't correct for a toast like this. Per the spec ( https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... ), we'd want to use 8 DIP on the top and bottom, whereas this uses 16. You could address this by adding yet more inset types for "toasts with text" and "toasts with buttons", but I was going to accomplish this differently: use the newly-added margin functionality on BoxLayout, and give each subcomponent of the find bar vertical margins. 8 DIP on the buttons, 12 DIP on the text. (These constants would, for now, be done by adding layout provider distance metrics for "button/text vertical margin in a toast.) Then if it made sense I'd find a way to subsequently refactor the button/text creation to set these margins automatically, so that if you e.g. created a label whose context was "toast", it would auto-set that margin on the label, and we wouldn't need any code in the find bar at all. https://codereview.chromium.org/2968713003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:171: related_control_horizontal); This is sort of tricky, because on the current spec at least ( https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... ) we want the "related control" spacing between the 16 DIP inner parts of the buttons, but each button is really 4 DIP bigger on every side. You should probably confirm with bettes@ that we want to end up with 8 DIP between these 24-DIP-per-side touchable buttons (or do we want to go ahead with 16 from the outer edges of them? That'd be easier but I think it'd look really bad.) Then we should find a way to make it so the layout provider spaces these buttons based on the inner region and not the outer one, without having to manually compute spacing offsets between each child. The buttons currently use a border that's INSETS_VECTOR_IMAGE_BUTTON in size to outdent from 16 to 24. Maybe kylixrd@ has an idea of how you could tell the layout provider "space these things 16 DIP apart not including their borders"?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2968713003/diff/1/chrome/browser/ui/views/fin... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:161: 0, related_control_horizontal, 0, related_control_horizontal)); On 2017/07/06 06:29:16, Peter Kasting wrote: > It seems like this will result in 2x "related control spacing" around the > separator, since we'll get this border + the standard boxlayout spacing below. Thanks. With the margins as suggested incorporated, we can do away with the border. https://codereview.chromium.org/2968713003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:170: views::INSETS_BUBBLE_CONTENTS)), On 2017/07/06 06:29:16, Peter Kasting wrote: > The bubble insets aren't correct for a toast like this. Per the spec ( > https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... > ), we'd want to use 8 DIP on the top and bottom, whereas this uses 16. > > You could address this by adding yet more inset types for "toasts with text" and > "toasts with buttons", but I was going to accomplish this differently: use the > newly-added margin functionality on BoxLayout, and give each subcomponent of the > find bar vertical margins. 8 DIP on the buttons, 12 DIP on the text. (These > constants would, for now, be done by adding layout provider distance metrics for > "button/text vertical margin in a toast.) Then if it made sense I'd find a way > to subsequently refactor the button/text creation to set these margins > automatically, so that if you e.g. created a label whose context was "toast", it > would auto-set that margin on the label, and we wouldn't need any code in the > find bar at all. Thanks. I added some code to set the margins explicitly on the controls. https://codereview.chromium.org/2968713003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:171: related_control_horizontal); On 2017/07/06 06:29:16, Peter Kasting wrote: > This is sort of tricky, because on the current spec at least ( > https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... > ) we want the "related control" spacing between the 16 DIP inner parts of the > buttons, but each button is really 4 DIP bigger on every side. You should > probably confirm with bettes@ that we want to end up with 8 DIP between these > 24-DIP-per-side touchable buttons (or do we want to go ahead with 16 from the > outer edges of them? That'd be easier but I think it'd look really bad.) > > Then we should find a way to make it so the layout provider spaces these buttons > based on the inner region and not the outer one, without having to manually > compute spacing offsets between each child. The buttons currently use a border > that's INSETS_VECTOR_IMAGE_BUTTON in size to outdent from 16 to 24. Maybe > kylixrd@ has an idea of how you could tell the layout provider "space these > things 16 DIP apart not including their borders"? The next patch uses an inset of 0, 16. Each control has its own margin specified. This seemed to look reasonable to me.
Obviously, you'll want to implement these toast label and control distances in the layout provider rather than hardcoded locally. Responding just to the actual numbers used. https://codereview.chromium.org/2968713003/diff/1/chrome/browser/ui/views/fin... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:171: related_control_horizontal); On 2017/07/07 22:51:02, ananta wrote: > On 2017/07/06 06:29:16, Peter Kasting wrote: > > This is sort of tricky, because on the current spec at least ( > > > https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... > > ) we want the "related control" spacing between the 16 DIP inner parts of the > > buttons, but each button is really 4 DIP bigger on every side. You should > > probably confirm with bettes@ that we want to end up with 8 DIP between these > > 24-DIP-per-side touchable buttons (or do we want to go ahead with 16 from the > > outer edges of them? That'd be easier but I think it'd look really bad.) > > > > Then we should find a way to make it so the layout provider spaces these > buttons > > based on the inner region and not the outer one, without having to manually > > compute spacing offsets between each child. The buttons currently use a > border > > that's INSETS_VECTOR_IMAGE_BUTTON in size to outdent from 16 to 24. Maybe > > kylixrd@ has an idea of how you could tell the layout provider "space these > > things 16 DIP apart not including their borders"? > > The next patch uses an inset of 0, 16. Each control has its own margin > specified. This seemed to look reasonable to me. The buttons get really far apart doing this, because of the issue I described here. Basically, you implemented what I said about "16 from the outer edges of them? That'd be easier but I think it'd look really bad", and it definitely looks like the buttons are too spaced-out to me. Maybe we could make this outer touchable expansion region be margin rather than border and have the layout manager collapse margins here, which I think would do what you want, but it's not clear to me that it makes logical sense for that to be margin rather than border, since it's really "inside" the button and not outside. https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:159: find_text_->SetProperty(views::kMarginsKey, new gfx::Insets(12, 0)); I think you want 8 here since this is a textfield control and not a label. https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:160: match_count_text_->SetProperty(views::kMarginsKey, Whereas this is a label, so you'd want 12. https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:162: separator_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 16, 8, 16)); Don't add horizontal margins here.
Description was changed from ========== Harmonize the find in page dialog. This patch changes the insets and control spacing. We use the DISTANCE_RELATED_CONTROL_HORIZONTAL metric for the spacing for the find buttons and INSETS_BUBBLE_CONTENTS for the insets for the find bubble. Next steps would be to replace the text from 1 of n to 1/n and then the rest of the items BUG=651643 ========== to ========== Harmonize the find in page dialog. This patch changes the insets and control spacing. We use the DISTANCE_RELATED_CONTROL_HORIZONTAL metric for the spacing for the controls and INSETS_DIALOG_CONTENTS for the dialog insets. Next steps would be to replace the text from 1 of n to 1/n and then the rest of the items BUG=651643 ==========
https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:159: find_text_->SetProperty(views::kMarginsKey, new gfx::Insets(12, 0)); On 2017/07/10 20:13:08, Peter Kasting wrote: > I think you want 8 here since this is a textfield control and not a label. The mocks are a touch confusing. They show the insets from the top to be 16 at places and 12 at the others. Looked at this with kylixrd. We thought it would be better to just specify the insets as 16 through out for horizontal and vertical and then revisit this with the UI folks. https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:160: match_count_text_->SetProperty(views::kMarginsKey, On 2017/07/10 20:13:08, Peter Kasting wrote: > Whereas this is a label, so you'd want 12. Not needed anymore. https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:162: separator_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 16, 8, 16)); On 2017/07/10 20:13:08, Peter Kasting wrote: > Don't add horizontal margins here. We need the horizontal margins for the separator to ensure that it has the correct space on its left and right.
On 2017/07/12 19:19:48, ananta wrote: > https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/find_bar_view.cc (right): > > https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/find_bar_view.cc:159: > find_text_->SetProperty(views::kMarginsKey, new gfx::Insets(12, 0)); > On 2017/07/10 20:13:08, Peter Kasting wrote: > > I think you want 8 here since this is a textfield control and not a label. > > The mocks are a touch confusing. They show the insets from the top to be > 16 at places and 12 at the others. Looked at this with kylixrd. We thought it > would be better to just specify the insets as 16 through out for horizontal and > vertical and then revisit this with the UI folks. > > https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/find_bar_view.cc:160: > match_count_text_->SetProperty(views::kMarginsKey, > On 2017/07/10 20:13:08, Peter Kasting wrote: > > Whereas this is a label, so you'd want 12. > > Not needed anymore. > > https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/find_bar_view.cc:162: > separator_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 16, 8, 16)); > On 2017/07/10 20:13:08, Peter Kasting wrote: > > Don't add horizontal margins here. > > We need the horizontal margins for the separator to ensure that it has the > correct space on its left and right. I added snaps of how this looks with and without harmony enabled to the bug.
The CQ bit was checked by ananta@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...
https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:159: find_text_->SetProperty(views::kMarginsKey, new gfx::Insets(12, 0)); On 2017/07/12 19:19:48, ananta wrote: > On 2017/07/10 20:13:08, Peter Kasting wrote: > > I think you want 8 here since this is a textfield control and not a label. > > The mocks are a touch confusing. They show the insets from the top to be > 16 at places and 12 at the others. Looked at this with kylixrd. We thought it > would be better to just specify the insets as 16 through out for horizontal and > vertical and then revisit this with the UI folks. I'm not sure what's confusing. Use 12 pt margins above labels and 8 pt above everything else, including all controls (such as textfields). What did I miss? https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:162: separator_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 16, 8, 16)); On 2017/07/12 19:19:48, ananta wrote: > On 2017/07/10 20:13:08, Peter Kasting wrote: > > Don't add horizontal margins here. > > We need the horizontal margins for the separator to ensure that it has the > correct space on its left and right. Your separator has 2x too much space with these (as visible on your screenshot). It should be correct without them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:159: find_text_->SetProperty(views::kMarginsKey, new gfx::Insets(12, 0)); On 2017/07/12 20:33:01, Peter Kasting wrote: > On 2017/07/12 19:19:48, ananta wrote: > > On 2017/07/10 20:13:08, Peter Kasting wrote: > > > I think you want 8 here since this is a textfield control and not a label. > > > > The mocks are a touch confusing. They show the insets from the top to be > > 16 at places and 12 at the others. Looked at this with kylixrd. We thought it > > would be better to just specify the insets as 16 through out for horizontal > and > > vertical and then revisit this with the UI folks. > > I'm not sure what's confusing. Use 12 pt margins above labels and 8 pt above > everything else, including all controls (such as textfields). What did I miss? It looks like 12 pt is not a harmony unit?. Some areas arond the text field show a 16 px margin from the top. Other places show 12 from the top to the border of the text control and 4 px to be the width of the border. Based on that it seemed like 16 px for all controls in the bar could be acceptable? https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:162: separator_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 16, 8, 16)); On 2017/07/12 20:33:01, Peter Kasting wrote: > On 2017/07/12 19:19:48, ananta wrote: > > On 2017/07/10 20:13:08, Peter Kasting wrote: > > > Don't add horizontal margins here. > > > > We need the horizontal margins for the separator to ensure that it has the > > correct space on its left and right. > > Your separator has 2x too much space with these (as visible on your screenshot). > It should be correct without them. If we don't specify the margin on both sides of the separator it shows up too close to the match count field in the non harmony case. From the mock it appears to be equidistant from the button on the right and the text match field on the left. It seems like the distance between controls in the box layout is not spacing the separator correctly?
https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:159: find_text_->SetProperty(views::kMarginsKey, new gfx::Insets(12, 0)); On 2017/07/13 04:04:34, ananta wrote: > On 2017/07/12 20:33:01, Peter Kasting wrote: > > On 2017/07/12 19:19:48, ananta wrote: > > > On 2017/07/10 20:13:08, Peter Kasting wrote: > > > > I think you want 8 here since this is a textfield control and not a label. > > > > > > The mocks are a touch confusing. They show the insets from the top to be > > > 16 at places and 12 at the others. Looked at this with kylixrd. We thought > it > > > would be better to just specify the insets as 16 through out for horizontal > > and > > > vertical and then revisit this with the UI folks. > > > > I'm not sure what's confusing. Use 12 pt margins above labels and 8 pt above > > everything else, including all controls (such as textfields). What did I > miss? > > It looks like 12 pt is not a harmony unit? Not everything in Harmony is 16s. > Some areas arond the text field show > a 16 px margin from the top. Text fields != labels. Labels always use 12 pt vertical margins in toasts, other controls use 8, and the total margin is set (automatically by the layout manager) based on the largest height + margin. > Other places show 12 from the top to the border of > the text control and 4 px to be the width of the border. I don't know what you're referring to here. The source of the 8 and 12 values is the "toasts with" images on https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... . https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:162: separator_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 16, 8, 16)); On 2017/07/13 04:04:34, ananta wrote: > On 2017/07/12 20:33:01, Peter Kasting wrote: > > On 2017/07/12 19:19:48, ananta wrote: > > > On 2017/07/10 20:13:08, Peter Kasting wrote: > > > > Don't add horizontal margins here. > > > > > > We need the horizontal margins for the separator to ensure that it has the > > > correct space on its left and right. > > > > Your separator has 2x too much space with these (as visible on your > screenshot). > > It should be correct without them. > > If we don't specify the margin on both sides of the separator it shows up too > close to the match count field in the non harmony case. As long as non-Harmony isn't completely broken, we don't care how it looks. > It seems like the distance between controls in the box layout is not spacing the > separator correctly? I think you're seeing the problems of built-in padding on the objects to either side (some small amount in the label, a larger amount in the button equivalent to the vector image button inset, which is why I keep talking about trying to lay out ignoring that value).
https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:162: separator_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 16, 8, 16)); On 2017/07/13 04:11:24, Peter Kasting wrote: > On 2017/07/13 04:04:34, ananta wrote: > > On 2017/07/12 20:33:01, Peter Kasting wrote: > > > On 2017/07/12 19:19:48, ananta wrote: > > > > On 2017/07/10 20:13:08, Peter Kasting wrote: > > > > > Don't add horizontal margins here. > > > > > > > > We need the horizontal margins for the separator to ensure that it has the > > > > correct space on its left and right. > > > > > > Your separator has 2x too much space with these (as visible on your > > screenshot). > > > It should be correct without them. > > > > If we don't specify the margin on both sides of the separator it shows up too > > close to the match count field in the non harmony case. > > As long as non-Harmony isn't completely broken, we don't care how it looks. > > > It seems like the distance between controls in the box layout is not spacing > the > > separator correctly? > > I think you're seeing the problems of built-in padding on the objects to either > side (some small amount in the label, a larger amount in the button equivalent > to the vector image button inset, which is why I keep talking about trying to > lay out ignoring that value). ok. thanks. I updated the patchset with distance metrics added as per your original suggestion. I also added an image for the find bar with harmony on with these values.
https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:161: find_next_button_->SetProperty(views::kMarginsKey, new gfx::Insets(0, For insets, vertical comes first, then horizontal (lots of places) https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:174: provider->GetInsetsMetric( This shouldn't have dialog contents insets, as that puts vertical insets atop the dialog. It should just have insets of 16 horizontal and 0 vertical. https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right): https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/chrome_layout_provider.h:41: DISTANCE_TOAST_BUTTON_VERTICAL, Combine BUTTON and TEXT (which I think really meant TEXTFIELD) into a single CONTROL value. Nit: Alphabetize https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/harmony_layout_provider.cc (right): https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_layout_provider.cc:74: case DISTANCE_TOAST_BUTTON_VERTICAL: I don't think you need to override any of these, since they all match your base values.
https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:161: find_next_button_->SetProperty(views::kMarginsKey, new gfx::Insets(0, On 2017/07/13 04:52:47, Peter Kasting wrote: > For insets, vertical comes first, then horizontal (lots of places) Yeah. I looked at it after I sent out the patch. Sorry about that. https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:174: provider->GetInsetsMetric( On 2017/07/13 04:52:47, Peter Kasting wrote: > This shouldn't have dialog contents insets, as that puts vertical insets atop > the dialog. > > It should just have insets of 16 horizontal and 0 vertical. Thanks. I used views::INSETS_DIALOG_CONTENTS).left(). Could not find a constant matching 16 there. https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right): https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/chrome_layout_provider.h:41: DISTANCE_TOAST_BUTTON_VERTICAL, On 2017/07/13 04:52:47, Peter Kasting wrote: > Combine BUTTON and TEXT (which I think really meant TEXTFIELD) into a single > CONTROL value. > > Nit: Alphabetize Done. https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/harmony_layout_provider.cc (right): https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_layout_provider.cc:74: case DISTANCE_TOAST_BUTTON_VERTICAL: On 2017/07/13 04:52:47, Peter Kasting wrote: > I don't think you need to override any of these, since they all match your base > values. Not doing this caused the NOTREACHED() in the function to fire for harmony
https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:174: provider->GetInsetsMetric( On 2017/07/13 05:22:10, ananta wrote: > On 2017/07/13 04:52:47, Peter Kasting wrote: > > This shouldn't have dialog contents insets, as that puts vertical insets atop > > the dialog. > > > > It should just have insets of 16 horizontal and 0 vertical. > > Thanks. I used views::INSETS_DIALOG_CONTENTS).left(). Could not find a constant > matching 16 there. I would probably just add an INSETS_TOAST in the layout provider that is (0, 16). https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/harmony_layout_provider.cc (right): https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_layout_provider.cc:74: case DISTANCE_TOAST_BUTTON_VERTICAL: On 2017/07/13 05:22:11, ananta wrote: > On 2017/07/13 04:52:47, Peter Kasting wrote: > > I don't think you need to override any of these, since they all match your > base > > values. > > Not doing this caused the NOTREACHED() in the function to fire for harmony Hmm. I'd say, remove the NOTREACHED and return 0, and instead add a default case to the loop that calls the ChromeLayoutProvider, as we did with the insets function above.
Description was changed from ========== Harmonize the find in page dialog. This patch changes the insets and control spacing. We use the DISTANCE_RELATED_CONTROL_HORIZONTAL metric for the spacing for the controls and INSETS_DIALOG_CONTENTS for the dialog insets. Next steps would be to replace the text from 1 of n to 1/n and then the rest of the items BUG=651643 ========== to ========== Harmonize the find in page dialog. This patch changes the insets and control spacing. We use the DISTANCE_RELATED_CONTROL_HORIZONTAL metric for the spacing for the controls and INSETS_DIALOG_CONTENTS for the dialog insets. Next steps would be to tackle the rest of the items in this dialog for harmony BUG=651643 ==========
Description was changed from ========== Harmonize the find in page dialog. This patch changes the insets and control spacing. We use the DISTANCE_RELATED_CONTROL_HORIZONTAL metric for the spacing for the controls and INSETS_DIALOG_CONTENTS for the dialog insets. Next steps would be to tackle the rest of the items in this dialog for harmony BUG=651643 ========== to ========== Harmonize the find in page dialog. This patch changes the insets and control spacing and the match count text We use the DISTANCE_RELATED_CONTROL_HORIZONTAL metric for the spacing for the controls and INSETS_DIALOG_CONTENTS for the dialog insets. Next steps would be to tackle the rest of the items in this dialog for harmony BUG=651643 ==========
I updated the match text to match the spec, i.e. ordinal/matches. It works well on my Windows 10 box. https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:174: provider->GetInsetsMetric( On 2017/07/13 05:31:37, Peter Kasting wrote: > On 2017/07/13 05:22:10, ananta wrote: > > On 2017/07/13 04:52:47, Peter Kasting wrote: > > > This shouldn't have dialog contents insets, as that puts vertical insets > atop > > > the dialog. > > > > > > It should just have insets of 16 horizontal and 0 vertical. > > > > Thanks. I used views::INSETS_DIALOG_CONTENTS).left(). Could not find a > constant > > matching 16 there. > > I would probably just add an INSETS_TOAST in the layout provider that is (0, > 16). Done. https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/harmony_layout_provider.cc (right): https://codereview.chromium.org/2968713003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_layout_provider.cc:74: case DISTANCE_TOAST_BUTTON_VERTICAL: On 2017/07/13 05:31:37, Peter Kasting wrote: > On 2017/07/13 05:22:11, ananta wrote: > > On 2017/07/13 04:52:47, Peter Kasting wrote: > > > I don't think you need to override any of these, since they all match your > > base > > > values. > > > > Not doing this caused the NOTREACHED() in the function to fire for harmony > > Hmm. I'd say, remove the NOTREACHED and return 0, and instead add a default > case to the loop that calls the ChromeLayoutProvider, as we did with the insets > function above. Thanks. Done
Patchset #11 (id:200001) has been deleted
So, the remaining issue is finding a way to not include the vector image padding around the buttons when spacing them horizontally; otherwise they're too far apart (and too far from the separator). Things should mostly be good otherwise. https://codereview.chromium.org/2968713003/diff/100002/chrome/browser/ui/view... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/100002/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:231: + L"/" + base::FormatNumber(result.number_of_matches())); This isn't localizable. You need to change IDS_FIND_IN_PAGE_COUNT to be "$1/$2" instead. https://codereview.chromium.org/2968713003/diff/100002/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right): https://codereview.chromium.org/2968713003/diff/100002/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_layout_provider.h:36: // Vertical spacing for labels in a toast. Nit: spacing -> margin
Will look into the vector image padding in a followup patch https://codereview.chromium.org/2968713003/diff/100002/chrome/browser/ui/view... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/100002/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:231: + L"/" + base::FormatNumber(result.number_of_matches())); On 2017/07/13 06:05:35, Peter Kasting wrote: > This isn't localizable. You need to change IDS_FIND_IN_PAGE_COUNT to be "$1/$2" > instead. Thanks for catching this. Dunno how I assumed that numbers don't need to be localized. https://codereview.chromium.org/2968713003/diff/100002/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right): https://codereview.chromium.org/2968713003/diff/100002/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_layout_provider.h:36: // Vertical spacing for labels in a toast. On 2017/07/13 06:05:35, Peter Kasting wrote: > Nit: spacing -> margin Done.
On 2017/07/13 06:27:43, ananta wrote: > Will look into the vector image padding in a followup patch I'd normally say that's fine, but I think without looking at it this is a pre-Harmony change people will notice and complain about. It might be worth trying to investigate how to do that as a separate, dependent patch set; then land both once that one is ready. That will keep the other reviewable by keeping it separate.
On 2017/07/13 06:32:27, Peter Kasting wrote: > On 2017/07/13 06:27:43, ananta wrote: > > Will look into the vector image padding in a followup patch > > I'd normally say that's fine, but I think without looking at it this is a > pre-Harmony change people will notice and complain about. > > It might be worth trying to investigate how to do that as a separate, dependent > patch set; then land both once that one is ready. That will keep the other > reviewable by keeping it separate. Sure thing. Please check the find_with_match_count.png which is non harmony and see if it is too noticeable. I thought it looked ok. But my eyes are no good in identifying these issues.
On 2017/07/13 06:36:21, ananta wrote: > On 2017/07/13 06:32:27, Peter Kasting wrote: > > On 2017/07/13 06:27:43, ananta wrote: > > > Will look into the vector image padding in a followup patch > > > > I'd normally say that's fine, but I think without looking at it this is a > > pre-Harmony change people will notice and complain about. > > > > It might be worth trying to investigate how to do that as a separate, > dependent > > patch set; then land both once that one is ready. That will keep the other > > reviewable by keeping it separate. > > Sure thing. Please check the find_with_match_count.png which is non harmony and > see if it is too noticeable. > I thought it looked ok. But my eyes are no good in identifying these issues. I noticed it because of that screenshot :) The main effect is the unbalanced appearance around the separator. That's the part people will notice the most.
On 2017/07/13 06:43:43, Peter Kasting wrote: > On 2017/07/13 06:36:21, ananta wrote: > > On 2017/07/13 06:32:27, Peter Kasting wrote: > > > On 2017/07/13 06:27:43, ananta wrote: > > > > Will look into the vector image padding in a followup patch > > > > > > I'd normally say that's fine, but I think without looking at it this is a > > > pre-Harmony change people will notice and complain about. > > > > > > It might be worth trying to investigate how to do that as a separate, > > dependent > > > patch set; then land both once that one is ready. That will keep the other > > > reviewable by keeping it separate. > > > > Sure thing. Please check the find_with_match_count.png which is non harmony > and > > see if it is too noticeable. > > I thought it looked ok. But my eyes are no good in identifying these issues. > > I noticed it because of that screenshot :) > > The main effect is the unbalanced appearance around the separator. That's the > part people will notice the most. Thanks. Trying another approach which is to use the INSETS_VECTOR_IMAGE_BUTTON insets left() and right() values for an empty border around the separator. This seems to look better. I will attach the images to the bug.
The CQ bit was checked by ananta@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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/2968713003/diff/250001/chrome/browser/ui/view... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:165: 0, image_button_insets.right())); This won't make things equidistant (as is visible in your screenshot), since this border is being applied to the button on the right but not the text on the left, so to counter that you'd want to add it only on the left side. But don't do that. That doesn't fix the buttons being too far apart and making the dialog too wide. We really just need to be able to space buttons in a way that ignores this offset. If need be, we can hack this for this dialog, basically by putting 0 default spacing between items, and then manually adding padding columns to the layout manager. It's not an automated solution, but it would serve.
https://codereview.chromium.org/2968713003/diff/250001/chrome/browser/ui/view... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/250001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:165: 0, image_button_insets.right())); On 2017/07/14 00:40:12, Peter Kasting wrote: > This won't make things equidistant (as is visible in your screenshot), since > this border is being applied to the button on the right but not the text on the > left, so to counter that you'd want to add it only on the left side. > > But don't do that. That doesn't fix the buttons being too far apart and making > the dialog too wide. We really just need to be able to space buttons in a way > that ignores this offset. > > If need be, we can hack this for this dialog, basically by putting 0 default > spacing between items, and then manually adding padding columns to the layout > manager. It's not an automated solution, but it would serve. ok. thanks. The patch came out wrong though. I was going to add the border to the text and ended up adding it to the separator.
The CQ bit was checked by ananta@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...
On 2017/07/14 01:04:29, ananta wrote: > https://codereview.chromium.org/2968713003/diff/250001/chrome/browser/ui/view... > File chrome/browser/ui/views/find_bar_view.cc (right): > > https://codereview.chromium.org/2968713003/diff/250001/chrome/browser/ui/view... > chrome/browser/ui/views/find_bar_view.cc:165: 0, image_button_insets.right())); > On 2017/07/14 00:40:12, Peter Kasting wrote: > > This won't make things equidistant (as is visible in your screenshot), since > > this border is being applied to the button on the right but not the text on > the > > left, so to counter that you'd want to add it only on the left side. > > > > But don't do that. That doesn't fix the buttons being too far apart and > making > > the dialog too wide. We really just need to be able to space buttons in a way > > that ignores this offset. > > > > If need be, we can hack this for this dialog, basically by putting 0 default > > spacing between items, and then manually adding padding columns to the layout > > manager. It's not an automated solution, but it would serve. > > ok. thanks. The patch came out wrong though. I was going to add the border to > the text and ended up adding it to the separator. The old code hacked around this by setting a space of 4 between children. I will look into a possibility of spacing these controls by ignoring the empty border around the buttons. Wondering if we can add a spacing specifically for toasts like these in the short term
On 2017/07/14 01:48:11, ananta wrote: > On 2017/07/14 01:04:29, ananta wrote: > > > https://codereview.chromium.org/2968713003/diff/250001/chrome/browser/ui/view... > > File chrome/browser/ui/views/find_bar_view.cc (right): > > > > > https://codereview.chromium.org/2968713003/diff/250001/chrome/browser/ui/view... > > chrome/browser/ui/views/find_bar_view.cc:165: 0, > image_button_insets.right())); > > On 2017/07/14 00:40:12, Peter Kasting wrote: > > > This won't make things equidistant (as is visible in your screenshot), since > > > this border is being applied to the button on the right but not the text on > > the > > > left, so to counter that you'd want to add it only on the left side. > > > > > > But don't do that. That doesn't fix the buttons being too far apart and > > making > > > the dialog too wide. We really just need to be able to space buttons in a > way > > > that ignores this offset. > > > > > > If need be, we can hack this for this dialog, basically by putting 0 default > > > spacing between items, and then manually adding padding columns to the > layout > > > manager. It's not an automated solution, but it would serve. > > > > ok. thanks. The patch came out wrong though. I was going to add the border to > > the text and ended up adding it to the separator. > > The old code hacked around this by setting a space of 4 between children. That's not really what accounted for this. The old code didn't enforce uniform spacing between children at all. But it did have the separator left and right spacing differ by 4, which would account for the thing on one side having 4 px extra padding and the other not. > Wondering if we can add a spacing > specifically for toasts like these in the short term No, because you need several values: the spacing between two non-vectoriconbuttons, the spacing between a non-VIB and a VIB, and the spacing between two VIBs. Then you have to manually determine what things are what anyway, so this doesn't help you. You may as well just compute the spacing manually between every child pair. Note that this even affects the padding at the right edge of the find bar: we set it to 16 in this patch, but that 16 is to the 16 px "core" of the close button, not to the surrounding border. The padding to the border should be 12.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Harmonize the find in page dialog. This patch changes the insets and control spacing and the match count text We use the DISTANCE_RELATED_CONTROL_HORIZONTAL metric for the spacing for the controls and INSETS_DIALOG_CONTENTS for the dialog insets. Next steps would be to tackle the rest of the items in this dialog for harmony BUG=651643 ========== to ========== Harmonize the find in page dialog. This patch changes the insets and control spacing and the match count text We space the controls in the find bar using margins to ensure that the distances are computed to the vector image glyphs and not to the border. Next steps would be to tackle the rest of the items in this dialog for harmony BUG=651643 ==========
On 2017/07/14 01:56:39, Peter Kasting wrote: > On 2017/07/14 01:48:11, ananta wrote: > > On 2017/07/14 01:04:29, ananta wrote: > > > > > > https://codereview.chromium.org/2968713003/diff/250001/chrome/browser/ui/view... > > > File chrome/browser/ui/views/find_bar_view.cc (right): > > > > > > > > > https://codereview.chromium.org/2968713003/diff/250001/chrome/browser/ui/view... > > > chrome/browser/ui/views/find_bar_view.cc:165: 0, > > image_button_insets.right())); > > > On 2017/07/14 00:40:12, Peter Kasting wrote: > > > > This won't make things equidistant (as is visible in your screenshot), > since > > > > this border is being applied to the button on the right but not the text > on > > > the > > > > left, so to counter that you'd want to add it only on the left side. > > > > > > > > But don't do that. That doesn't fix the buttons being too far apart and > > > making > > > > the dialog too wide. We really just need to be able to space buttons in a > > way > > > > that ignores this offset. > > > > > > > > If need be, we can hack this for this dialog, basically by putting 0 > default > > > > spacing between items, and then manually adding padding columns to the > > layout > > > > manager. It's not an automated solution, but it would serve. > > > > > > ok. thanks. The patch came out wrong though. I was going to add the border > to > > > the text and ended up adding it to the separator. > > > > The old code hacked around this by setting a space of 4 between children. > > That's not really what accounted for this. The old code didn't enforce uniform > spacing between children at all. But it did have the separator left and right > spacing differ by 4, which would account for the thing on one side having 4 px > extra padding and the other not. > > > Wondering if we can add a spacing > > specifically for toasts like these in the short term > > No, because you need several values: the spacing between two > non-vectoriconbuttons, the spacing between a non-VIB and a VIB, and the spacing > between two VIBs. Then you have to manually determine what things are what > anyway, so this doesn't help you. You may as well just compute the spacing > manually between every child pair. > > Note that this even affects the padding at the right edge of the find bar: we > set it to 16 in this patch, but that 16 is to the 16 px "core" of the close > button, not to the surrounding border. The padding to the border should be 12. Thanks. I updated the code to explicitly specify margins and specify 0 as the control spacing for the box layout. I uploaded the images to the bug with and without harmony. PTAL
The CQ bit was checked by ananta@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...
The CQ bit was checked by ananta@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...
https://codereview.chromium.org/2968713003/diff/310001/chrome/browser/ui/view... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:164: // To workaround this we space the controls using the desired margins. Nit: Maybe this would be clearer?: Normally we could space objects horizontally by simply passing a constant value to BoxLayout for between-child spacing. But for the vector image buttons, we want the spacing to apply between the inner "glyph" portions of the buttons, ignoring the surrounding borders. BoxLayout has no way to dynamically adjust for this, so instead of using between-child spacing, we place views directly adjacent, with horizontal margins on each view that will add up to the right spacing amounts. https://codereview.chromium.org/2968713003/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:166: int horizontal_margin = A couple of these assume things like the vector image button or toast insets being the same on both sides. If we keep these all as Insets, we can avoid this assumption: const gfx::Insets horizontal_margin( 0, provider->GetDistanceMetric( views::DISTANCE_RELATED_CONTROL_HORIZONTAL) / 2); const gfx::Insets vector_button = provider->GetInsetsMetric(views::INSETS_VECTOR_IMAGE_BUTTON); const gfx::Insets vector_button_horizontal_margin( 0, horizontal_margin.left() - vector_button.left(), 0, horizontal_margin.right() - vector_button.right()); const gfx::Insets toast_control_vertical_margin( provider->GetDistanceMetric(DISTANCE_TOAST_CONTROL_VERTICAL), 0); const gfx::Insets toast_label_vertical_margin( provider->GetDistanceMetric(DISTANCE_TOAST_LABEL_VERTICAL), 0); find_previous_button_->SetProperty( views::kMarginsKey, new gfx::Insets(toast_control_vertical_margin + vector_button_horizontal_margin)); ... https://codereview.chromium.org/2968713003/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:167: provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_HORIZONTAL) / This should be UNRELATED rather than RELATED. This will have no visible effect in Harmony and should be closer to the current behavior pre-Harmony; plus it's more semantically correct. https://codereview.chromium.org/2968713003/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:195: new gfx::Insets(toast_label_vertical_margin, horizontal_margin)); I think you should use the control margin rather than the label margin for this one. A separator is more like a vertical divider that should have button height, rather than a | symbol or something. https://codereview.chromium.org/2968713003/diff/310001/ui/views/layout/layout... File ui/views/layout/layout_provider.h (right): https://codereview.chromium.org/2968713003/diff/310001/ui/views/layout/layout... ui/views/layout/layout_provider.h:38: INSETS_TOAST, This should be in chrome_layout_provider.h and not here, since it is not used by ui/views code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2968713003/diff/310001/chrome/browser/ui/view... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:164: // To workaround this we space the controls using the desired margins. On 2017/07/15 02:48:08, Peter Kasting wrote: > Nit: Maybe this would be clearer?: > > Normally we could space objects horizontally by simply passing a constant value > to BoxLayout for between-child spacing. But for the vector image buttons, we > want the spacing to apply between the inner "glyph" portions of the buttons, > ignoring the surrounding borders. BoxLayout has no way to dynamically adjust > for this, so instead of using between-child spacing, we place views directly > adjacent, with horizontal margins on each view that will add up to the right > spacing amounts. Done. https://codereview.chromium.org/2968713003/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:166: int horizontal_margin = On 2017/07/15 02:48:08, Peter Kasting wrote: > A couple of these assume things like the vector image button or toast insets > being the same on both sides. If we keep these all as Insets, we can avoid this > assumption: > > const gfx::Insets horizontal_margin( > 0, provider->GetDistanceMetric( > views::DISTANCE_RELATED_CONTROL_HORIZONTAL) / 2); > const gfx::Insets vector_button = > provider->GetInsetsMetric(views::INSETS_VECTOR_IMAGE_BUTTON); > const gfx::Insets vector_button_horizontal_margin( > 0, horizontal_margin.left() - vector_button.left(), > 0, horizontal_margin.right() - vector_button.right()); > const gfx::Insets toast_control_vertical_margin( > provider->GetDistanceMetric(DISTANCE_TOAST_CONTROL_VERTICAL), 0); > const gfx::Insets toast_label_vertical_margin( > provider->GetDistanceMetric(DISTANCE_TOAST_LABEL_VERTICAL), 0); > > find_previous_button_->SetProperty( > views::kMarginsKey, > new gfx::Insets(toast_control_vertical_margin + > vector_button_horizontal_margin)); > ... Thanks. done. https://codereview.chromium.org/2968713003/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:167: provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_HORIZONTAL) / On 2017/07/15 02:48:08, Peter Kasting wrote: > This should be UNRELATED rather than RELATED. This will have no visible effect > in Harmony and should be closer to the current behavior pre-Harmony; plus it's > more semantically correct. Done. https://codereview.chromium.org/2968713003/diff/310001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:195: new gfx::Insets(toast_label_vertical_margin, horizontal_margin)); On 2017/07/15 02:48:08, Peter Kasting wrote: > I think you should use the control margin rather than the label margin for this > one. A separator is more like a vertical divider that should have button > height, rather than a | symbol or something. Done. https://codereview.chromium.org/2968713003/diff/310001/ui/views/layout/layout... File ui/views/layout/layout_provider.h (right): https://codereview.chromium.org/2968713003/diff/310001/ui/views/layout/layout... ui/views/layout/layout_provider.h:38: INSETS_TOAST, On 2017/07/15 02:48:08, Peter Kasting wrote: > This should be in chrome_layout_provider.h and not here, since it is not used by > ui/views code. This was already moved in the patchset i uploaded before leaving
The CQ bit was checked by ananta@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...
LGTM https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:168: Nit: Why did you put blank lines between everything here? Seems like they could be grouped a bit. https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_layout_provider.cc (right): https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_layout_provider.cc:66: DCHECK_LT(metric, views::VIEWS_INSETS_MAX); No need for this line, the base class checks it https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right): https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_layout_provider.h:46: enum InsetsMetric { This has to be ChromeInsetsMetric, or you have a name conflict. Nit: This should go above ChromeDistanceMetric to match the base class order https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_layout_provider.h:62: gfx::Insets GetInsetsMetric(int metric) const override; Nit: Please match the base class order
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/find_bar_view.cc:168: On 2017/07/15 07:38:36, Peter Kasting wrote: > Nit: Why did you put blank lines between everything here? Seems like they could > be grouped a bit. Thanks. Done https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_layout_provider.cc (right): https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_layout_provider.cc:66: DCHECK_LT(metric, views::VIEWS_INSETS_MAX); On 2017/07/15 07:38:36, Peter Kasting wrote: > No need for this line, the base class checks it Done. https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right): https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_layout_provider.h:46: enum InsetsMetric { On 2017/07/15 07:38:36, Peter Kasting wrote: > This has to be ChromeInsetsMetric, or you have a name conflict. > > Nit: This should go above ChromeDistanceMetric to match the base class order Done. https://codereview.chromium.org/2968713003/diff/370001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_layout_provider.h:62: gfx::Insets GetInsetsMetric(int metric) const override; On 2017/07/15 07:38:36, Peter Kasting wrote: > Nit: Please match the base class order Done.
The CQ bit was checked by ananta@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/2968713003/#ps390001 (title: "Address review comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ananta@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/2968713003/#ps430001 (title: "Fix format")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ananta@chromium.org changed reviewers: + blundell@chromium.org
+blundell for components owners
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Harmonize the find in page dialog. This patch changes the insets and control spacing and the match count text We space the controls in the find bar using margins to ensure that the distances are computed to the vector image glyphs and not to the border. Next steps would be to tackle the rest of the items in this dialog for harmony BUG=651643 ========== to ========== Harmonize the find in page dialog. This patch changes the insets and control spacing and the match count text We space the controls in the find bar using margins to ensure that the distances are computed to the vector image glyphs and not to the border. Next steps would be to tackle the rest of the items in this dialog for harmony BUG=651643 TBR=blundell ==========
TBR'ing blundell. Will address comments in a followup
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 430001, "attempt_start_ts": 1500330336493540,
"parent_rev": "211fb8b5e6d4a167d28f28774070e10b845951bf", "commit_rev":
"a5f3fa6e02a96926c01d20fbf38be2257c251d49"}
Message was sent while issue was closed.
Description was changed from ========== Harmonize the find in page dialog. This patch changes the insets and control spacing and the match count text We space the controls in the find bar using margins to ensure that the distances are computed to the vector image glyphs and not to the border. Next steps would be to tackle the rest of the items in this dialog for harmony BUG=651643 TBR=blundell ========== to ========== Harmonize the find in page dialog. This patch changes the insets and control spacing and the match count text We space the controls in the find bar using margins to ensure that the distances are computed to the vector image glyphs and not to the border. Next steps would be to tackle the rest of the items in this dialog for harmony BUG=651643 TBR=blundell Review-Url: https://codereview.chromium.org/2968713003 Cr-Commit-Position: refs/heads/master@{#487316} Committed: https://chromium.googlesource.com/chromium/src/+/a5f3fa6e02a96926c01d20fbf38b... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:430001) as https://chromium.googlesource.com/chromium/src/+/a5f3fa6e02a96926c01d20fbf38b...
Message was sent while issue was closed.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-07a-layout.png){kind=link}
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-07c-layout.png){kind=link}