ui/views/layout/layout_provider.h - Issue 2932523003: Fix Harmony popover buttons being flush with their content.

Unified Diff: ui/views/layout/layout_provider.h

Issue 2932523003: Fix Harmony popover buttons being flush with their content. (Closed)

Patch Set: too much padding for bubbles Created 3 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« chrome/browser/ui/views/harmony/chrome_layout_provider.cc ('K') | « ui/views/bubble/bubble_dialog_delegate.cc ('k') | ui/views/layout/layout_provider.cc » ('j') | ui/views/window/dialog_client_view.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: ui/views/layout/layout_provider.h

diff --git a/ui/views/layout/layout_provider.h b/ui/views/layout/layout_provider.h

index 41da0c8b96fe2edc0259dacc4d155a424edeacf9..4c82945f2c83ab4901c9a14e40c6f164352815be 100644

--- a/ui/views/layout/layout_provider.h

+++ b/ui/views/layout/layout_provider.h

@@ -49,8 +49,11 @@ enum DistanceMetric {

// two types have not been interchanged.

VIEWS_DISTANCE_START = VIEWS_INSETS_MAX,

+ // If a bubble has buttons, this is the margin between them and the rest of

+ // the content.

+ DISTANCE_BUBBLE_BUTTON_TOP_MARGIN = VIEWS_DISTANCE_START,

Peter Kasting 2017/06/10 01:51:11 I'm not sure whether adding this is right. We see

Bret 2017/06/11 00:14:09 No, lots of bubbles have buttons (e.g. the bookmar

Peter Kasting 2017/06/12 22:47:14 OK, that all makes sense. But in that case it see

OK, that all makes sense. But in that case it seems like we should make the bubble and dialog insets all just match (as the dialog insets) for now. The dialog inset pre-Harmony is kButtonHorizontalMargin; if some bubbles can have buttons too, then they should also be using those margins. The only exception would be bubbles that lack any buttons, which I suppose could stay the old way, but it doesn't seem like a win to make them do so. If we were going to do this, then basically all the BUBBLE_ insets/distances would disappear, and we'd use the dialog ones everywhere. And that, I think, would still mean we could avoid adding this distance metric, and you could just use the .top() value of INSETS_DIALOG_BUTTON_ROW (or something like that) in the callers instead of this. The actual change to nuke the BUBBLE_ constants would probably happen separately, maybe as a precursor to this change. WDYT?

Bret 2017/06/13 21:00:41 I'd really like to get everything into a working s

On 2017/06/12 22:47:14, Peter Kasting wrote: > On 2017/06/11 00:14:09, Bret wrote: > > On 2017/06/10 01:51:11, Peter Kasting wrote: > > > I'm not sure whether adding this is right. > > > > > > We seem to have two sets of constants today: one for "dialogs" and one for > > > "bubbles". AFAICT the distinction between those two is that dialogs have a > > > button row and bubbles do not. That's certainly how I've been operating. > > > > > > But the premise of this patch is that we have "bubbles" with buttons. In > > which > > > case, my question is, why aren't these just dialogs, made to use the > > > DIALOG_BUTTON_ROW insets (and dialog content insets, etc.)? > > > > No, lots of bubbles have buttons (e.g. the bookmark bubble, crashed session > > bubble, translate bubble, and others). You might be right that they SHOULD be > > dialogs, but that's not really how the code works. > > > > Post-Harmony the distinction I've been using is that dialogs are modal and > > bubbles aren't, with the attendant consequences: bubbles have a close-X and > > close when you click away. Pre-Harmony there are a few extra differences: > > * Bubbles have arrows and dialogs don't. > > * Bubbles are anchored and not draggable, and dialogs are floating and > > draggable. > > * Dialogs have slightly larger margins. > > > > All that is baked into DialogDelegateView vs BubbleDialogDelegateView. This is > > why I mentioned in an earlier CL I thought we'd eventually merge the > > distinction, because conceptually these ought to be dictated by the same > > delegate class (with a switch for modality). But I think we should hold off on > > doing that refactor till Harmony is done. > > OK, that all makes sense. But in that case it seems like we should make the > bubble and dialog insets all just match (as the dialog insets) for now. The > dialog inset pre-Harmony is kButtonHorizontalMargin; if some bubbles can have > buttons too, then they should also be using those margins. The only exception > would be bubbles that lack any buttons, which I suppose could stay the old way, > but it doesn't seem like a win to make them do so. > > If we were going to do this, then basically all the BUBBLE_ insets/distances > would disappear, and we'd use the dialog ones everywhere. And that, I think, > would still mean we could avoid adding this distance metric, and you could just > use the .top() value of INSETS_DIALOG_BUTTON_ROW (or something like that) in the > callers instead of this. > > The actual change to nuke the BUBBLE_ constants would probably happen > separately, maybe as a precursor to this change. > > WDYT?

I'd really like to get everything into a working state before I do refactor-style changes. Plus removing the special case in DialogClientView::SetupLayout will make refactors easier. I think getting those two things is worth adding this constant, even if I have to remove it later. I think it will stick around, though. My goal is to collapse all the constants into a metric for the edges of the dialog, and then have this and a "between title and contents" metric. I do like having a mandate to make the bubble and dialog margins the same though, it will make my life a lot easier.

// Margin on the left and right of the contents of a bubble.

- DISTANCE_BUBBLE_CONTENTS_HORIZONTAL_MARGIN = VIEWS_DISTANCE_START,

+ DISTANCE_BUBBLE_CONTENTS_HORIZONTAL_MARGIN,

// Margin on the top and bottom of the contents of a bubble.

DISTANCE_BUBBLE_CONTENTS_VERTICAL_MARGIN,

// The default padding to add on each side of a button's label.