views: implement dialog width snapping

ui/views/bubble/bubble_frame_view.cc

Index: ui/views/bubble/bubble_frame_view.cc
diff --git a/ui/views/bubble/bubble_frame_view.cc b/ui/views/bubble/bubble_frame_view.cc
index 7ba6184978fd9c1610e9d93b43daf94618ea74f4..83e4bf0ab205110d3c4085899a76d5f891d34ef0 100644
--- a/ui/views/bubble/bubble_frame_view.cc
+++ b/ui/views/bubble/bubble_frame_view.cc
@@ -531,6 +531,9 @@ gfx::Size BubbleFrameView::GetSizeForClientSize(
   if (footnote_container_)
     size.Enlarge(0, footnote_container_->GetHeightForWidth(size.width()));
 
+  size.set_width(
+      ViewsDelegate::GetInstance()->GetSnappedDialogWidth(size.width()));

[tapted, 2017/03/29 23:16:16]

I like the idea of hooking in to BubbleFrameView::GetSizeForClientSize() :) ..
(I'm pretty sure that..) the title margins and insets above need to be
incorporated into the size calculation, and trying to feed through the correct
widths to/from where the snapping gets done might get tricky. E.g. the
GetPreferredSnappedWidth(..) virtual method might also need to take arguments
for insets/margins so they can be subtracted/added from/to the harmony dialog
constants.

But Peter is right to be concerned that this may affect things that are not
dialogs - I think some tooltips and IME windows also use BubbleFrameView.
Extension "PageAction" bubbles and other WebUI dialogs might also need to be
considered (we shouldn't be snapping those -- webui may get messed up if we give
it a width it doesn't expect).


So a method on views::DialogDelegate seems necessary. Sadly, I think tooltips in
CrOS/Ash use views::InfoBubble which is a BubbleDialogDelegateView, so just
having [Bubble]DialogDelegate[View] as an indicator that "I am a dialog and
should be snapped" probably will not work either.


One possible heuristic..... is if the dialog has dialog buttons, then it is an
"actual" dialog and should be snapped. But there are exceptions - e.g. site info
bubble. So dialogs would need a way to override it. (I think some other
non-webUI extension bubbles also have a way to report "no buttons", as does the
add bookmark bubble [for bad reasons])


Also note there are also dialogs which use views::DialogDelegateView rather than
views::BubbleDialogDelegateView (e.g. AppInfo's BaseDialogContainer, which
should be snapped).



So. What to do.

I'm trying to think of a way that "forces" new dialogs to be snapped without
having to think about it, and doesn't require us to update some ~180 subclasses
of DialogDelegate with their snapping preferences. 

Note that DialogDelegate::GetContentsView() is "just" a views::View -- it
doesn't have to be dialog-specific (but it is _often_ a DialogDelegateView or
BubbleDialogDelegateView). It's the vanilla views::View from GetContentsView()
that determines GetPreferredSize() -- not the dialog delegate -- and I don't
think we can add "snapping" concepts to views::View (e.g. views::Label shouldn't
have a "GetUnsnappedPreferredSize()" to support a DialogDelegate that uses a
views::Label for its GetContentsView())


I'm currently leaning towards something like *keeping*
ViewsDelegate::GetSnappedDialogWidth(..), but also adding `virtual bool
DialogDelegate::ShouldSnapFrameWidth() const;`. To get started faster, the
default implementation in DialogDelegate could be `return GetDialogButtons() !=
ui::DIALOG_BUTTON_NONE;`. Origin info bubble and add bookmark bubble would need
to override ShouldSnapWidth.

Here, in BubbleFrameView::GetSizeForClientSize(), we check:

DialogDelegate* as_dialog = GetWidget()->widget_delegate()->AsDialogDelegate();
if (as_dialog && as_dialog->ShouldSnapFrameWidth()) {
  size.set_width(..GetSnappedDialogWidth());
}



(disclaimer: pkasting or sky may hate this idea)


(curveball: I see there's ui::mojom::kResizeBehavior on views::WidgetDelegate --
perhaps this could include a bit-flag for "snapping", so we wouldn't also need
ShouldSnapFrameWidth()...)


A possible alternative may be to overhaul
BubbleFrameView::GetSizeForClientSize(). Basically, add
DialogDelegate::GetFrameSizeForClientSize(..) [or
WidgetDelegate::GetFrameSizeForClientSize], but it would need extra arguments
from the FrameView to pass through: e.g. (some or all of) title widths, insets,
margins, footnote view size, ... I think this will end up a lot more complex
than DialogDelegate::ShouldSnapFrameWidth() +
ViewsDelegate::GetSnappedDialogWidth()

[Peter Kasting, 2017/03/30 00:18:23]

Yes, we need to know how big the title wants to be, and anything else (buttons
on the bottom?) that wouldn't be included in the main size.
What cases were you thinking of?  Naively it seems like we don't need to do
anything special here.
Are you sure we shouldn't be snapping those?  It seems like we should be
snapping all those things.

If we should snap All The Things, I think all the rest of your comment goes away
because we can avoid solving the problem.

[tapted, 2017/03/30 01:57:13]

Most dialogs are happy using the default panel inset. Some can't use it. E.g.
site info/OIB has a line that goes across the entire width (even in Harmony
mocks - http://go/tghpy ) -- it sets margins to 0 and increases its preferred
width so the line can be included. It does this by convincing the dialog
delegate to pass zero-insets to the BubbleFrameView constructor.
I guess that's a question for a designer :). Tooltips might look weird if they
were not snapping to the text string + margin. WebUI stuff might come from
extensions - it's hard to say how they will be affected, since we have no
control over their layout (e.g. it might be "absolute").
I'm certainly open to trying it :). It's possibly only an issue on ChromeOS, and
we can worry about it later. Or there are enough isolated cases that it's better
to address them specifically once we know they are negatively affected.

[Peter Kasting, 2017/03/30 04:07:02]

As long as GetInsets() returns the real desired insets, though, we can handle
this without any funky plumbing, right?

Like here, for example, the returned size includes the specified insets, and we
snap after those are applied.  If those are 0, that still works just fine.

I'm not thinking of a case where we have to plumb something special.  Maybe I'm
missing it; feel free to share pseudocode for the sort of problematic case and
solution you're thinking of?
Right, if tooltips are affected by this, that's probably bad.  Are they?  I
didn't think they were on Windows at least, I'm not as familiar with, say, CrOS.
I'm fine with that, since if we break something, we should be only breaking it
for Harmony, and hopefully with most of this (e.g. tooltips) we'll notice pretty
quickly if it does the wrong thing.

[tapted, 2017/03/30 06:11:49]

We may have had different ideas in our heads about where this CL was headed ;).
I'm mostly thinking about the logic that's sitting above this comment, in
BubbleFrameView::GetSizeForClientSize(..). 

e.g. One line uses `title_->GetPreferredSize()`, and that influences the width.

So snapping will need to occur after the call to GetSizeForClientSize() -- this
could be in BubbleDialogDelegateView::SizeToContents()  or
BubbleDialogDelegateView::GetBubbleBounds(). 

However (unless we do a bit more juggling) it can't be in
DialogClientView::GetPreferredSize() (or ClientView::GetPreferredSize(), which
uses BDDV::GetContentsView() and is how dialogs currently specify their width)
since that happens before the call to GetSizeForClientSize().
I put a screenshot and code fragment at http://crbug.com/635173#c14 for the
types of "tooltip" that could be affected by this. (this one was actually not
ChromeOS-only - turns out views::InfoBubble is used elsewhere.)

But ash::ShelfTooltipManager::ShelfTooltipBubble is also definitely a
BubbleDialogDelegateView, and is the other one I had on the brain.

[Peter Kasting, 2017/03/31 22:09:58]

Do all dialogs use a BubbleFrameView?  They probably do, right?

So maybe we don't need to snap things in DialogClientView::GetPreferredSize() at
all (as the other CL does), and we can _just_ snap in the location here.

Why do you say "snapping will need to occur after the call to
GetSizeForClientSize()"?  Why can't it occur at the very end of that function,
as it does in this patch?  We've taken title, content, insets etc. into account
by this point, so it seems like the right place to do this.
Thanks, that's super helpful.  Replied on the bug.

[tapted, 2017/04/03 09:53:48]

Most do.. nearly all child classes of DialogDelegate[View] and
BubbleDialogDelegateView in the dialog audit
https://docs.google.com/spreadsheets/d/1xDrxVl--AysmTowWQ7x5XeNVjpomZzQwGX-lh...
 (see ParentClass column).

Dialogs can override DialogDelegate::ShouldUseCustomFrame() to avoid the
BubbleFrameView or override DialogDelegate::CreateNonClientFrameView -- some
dialogs do this, but they might be corner cases -- things like task manager,
hung renderer dialog, ..

Dialogs can also override WidgetDelegate::CreateClientView to avoid getting a
DialogClientView (e.g. app info dialog does this).
This was wrt the comment at https://codereview.chromium.org/2750063002/#msg12
(i.e. around exploring GetPreferredSize() overrides in DialogclientView). I am
fine with snapping here :). I think it can work, so long as we have a way to cut
out a lot of obscure things that also use BubbleFrameView, but shouldn't be
snapped

[Elly Fong-Jones, 2017/04/05 18:17:53]

Wrapping up this thread:

I ended up adding DialogDelegate::ShouldSnapFrameWidth() and restricting the
snapping to BubbleFrameViews that have a DialogDelegate installed as their
WidgetDelegate and for which ShouldSnapFrameWidth() returns true.

+
   return size;
 }