Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
375873007:
Put alert dialog text in a scroll view. (Closed)
|
Created:
6 years, 5 months ago by meacer Modified:
6 years, 5 months ago CC:
chromium-reviews, tfarina, sadrul Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionPut alert dialog text in a scroll view.
This CL puts the dialog text inside a scroll view so that very long texts don't hide the dialog buttons by making the dialog oversized.
I also removed SetIcon method which isn't used anywhere.
Screenshots:
https://drive.google.com/file/d/0B9q2eN9gDoUIbU9Wb2F0OS1QTjA/edit?usp=sharing (alert)
https://drive.google.com/file/d/0B9q2eN9gDoUIYnh4ZWdFTEJBYVk/edit?usp=sharing (confirm)
https://drive.google.com/file/d/0B9q2eN9gDoUIQlhBS083ZDdGUWM/edit?usp=sharing (prompt)
BUG=391583
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282978
Patch Set 1 #
Total comments: 17
Patch Set 2 : msw comments #Patch Set 3 : Use native scroll bar #
Messages
Total messages: 19 (0 generated)
avi, jochen: Could one of you please take a look?
I'm not a Views person, so I can't say.
Better reviewers for this code are Scott and Mike. Removing Jochen as well.
lgtm with nits, but wait for Scott or another ui/views owner to review. https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... File ui/views/controls/message_box_view.cc (right): https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:15: #include "ui/views/controls/image_view.h" nit: remove this https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:207: // And an icon, if one has been set. nit: remove this part of the comment https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... File ui/views/controls/message_box_view.h (right): https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.h:14: class ImageSkia; nit: remove this. https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.h:20: class ImageView; nit: remove this
https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... File ui/views/controls/message_box_view.cc (right): https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:221: const int kMaxScrollViewHeight = 600; Why do you need to clip this? Wouldn't it be better to use work area? https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:228: scroll_view->SetVerticalScrollBar(new views::OverlayScrollBar(false)); Why are you going with overlay?
https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... File ui/views/controls/message_box_view.cc (right): https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:15: #include "ui/views/controls/image_view.h" On 2014/07/09 15:56:59, msw wrote: > nit: remove this Done. https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:207: // And an icon, if one has been set. On 2014/07/09 15:56:59, msw wrote: > nit: remove this part of the comment Done. https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:221: const int kMaxScrollViewHeight = 600; On 2014/07/09 16:20:15, sky wrote: > Why do you need to clip this? Wouldn't it be better to use work area? The clipping + overlay scroll bar gave a useful combination where both the dialog was constrained to a maximum height and the scroll bar was visible when needed. It looks like the scroll contents aren't visible at all when the view isn't clipped. I don't have much expertise with the views, so maybe I'm missing something? https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:228: scroll_view->SetVerticalScrollBar(new views::OverlayScrollBar(false)); On 2014/07/09 16:20:15, sky wrote: > Why are you going with overlay? Oddly, NativeScrollBar doesn't show for me at all, and Kennedy looked a bit too thick and I didn't see it used elsewhere so wanted to play safe. Happy to change it if you feel otherwise though. https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... File ui/views/controls/message_box_view.h (right): https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.h:14: class ImageSkia; On 2014/07/09 15:56:59, msw wrote: > nit: remove this. Done. https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.h:20: class ImageView; On 2014/07/09 15:56:59, msw wrote: > nit: remove this Done.
https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... File ui/views/controls/message_box_view.cc (right): https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:221: const int kMaxScrollViewHeight = 600; On 2014/07/09 18:06:56, Mustafa Emre Acer wrote: > On 2014/07/09 16:20:15, sky wrote: > > Why do you need to clip this? Wouldn't it be better to use work area? > > The clipping + overlay scroll bar gave a useful combination where both the > dialog was constrained to a maximum height and the scroll bar was visible when > needed. 'Clip' was a bad choice on my side. I'm suggesting you use the work area of the monitor you're on and not an arbitrary value. > It looks like the scroll contents aren't visible at all when the view isn't > clipped. I don't have much expertise with the views, so maybe I'm missing > something? ScrollView is a bit odd, you may need to explicitly set the bounds of message_contents. https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:228: scroll_view->SetVerticalScrollBar(new views::OverlayScrollBar(false)); On 2014/07/09 18:06:56, Mustafa Emre Acer wrote: > On 2014/07/09 16:20:15, sky wrote: > > Why are you going with overlay? > > Oddly, NativeScrollBar doesn't show for me at all, and Kennedy looked a bit too > thick and I didn't see it used elsewhere so wanted to play safe. Happy to change > it if you feel otherwise though. You should go with the default. The NativeScrollBar should only show if needed, although it may vary depending upon platform (NativeTheme).
https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... File ui/views/controls/message_box_view.cc (right): https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:221: const int kMaxScrollViewHeight = 600; On 2014/07/09 23:21:08, sky wrote: > On 2014/07/09 18:06:56, Mustafa Emre Acer wrote: > > On 2014/07/09 16:20:15, sky wrote: > > > Why do you need to clip this? Wouldn't it be better to use work area? > > > > The clipping + overlay scroll bar gave a useful combination where both the > > dialog was constrained to a maximum height and the scroll bar was visible when > > needed. > > 'Clip' was a bad choice on my side. I'm suggesting you use the work area of the > monitor you're on and not an arbitrary value. The message view is constrained by the browser window, which means I'll need to scale according to the browser window and not the whole screen, and will have to pass the browser window to MessageView. Does that sound reasonable? > > > It looks like the scroll contents aren't visible at all when the view isn't > > clipped. I don't have much expertise with the views, so maybe I'm missing > > something? > > ScrollView is a bit odd, you may need to explicitly set the bounds of > message_contents.
https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... File ui/views/controls/message_box_view.cc (right): https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:221: const int kMaxScrollViewHeight = 600; On 2014/07/10 00:27:53, Mustafa Emre Acer wrote: > On 2014/07/09 23:21:08, sky wrote: > > On 2014/07/09 18:06:56, Mustafa Emre Acer wrote: > > > On 2014/07/09 16:20:15, sky wrote: > > > > Why do you need to clip this? Wouldn't it be better to use work area? > > > > > > The clipping + overlay scroll bar gave a useful combination where both the > > > dialog was constrained to a maximum height and the scroll bar was visible > when > > > needed. > > > > 'Clip' was a bad choice on my side. I'm suggesting you use the work area of > the > > monitor you're on and not an arbitrary value. > > The message view is constrained by the browser window, which means I'll need to > scale according to the browser window and not the whole screen, and will have to > pass the browser window to MessageView. Does that sound reasonable? > > > > > > It looks like the scroll contents aren't visible at all when the view isn't > > > clipped. I don't have much expertise with the views, so maybe I'm missing > > > something? > > > > ScrollView is a bit odd, you may need to explicitly set the bounds of > > message_contents. > You may be able to override MessageBoxView::GetMinimumSize (and ditto for the relevant DialogDelegate?) to let the dialog be sized automatically to fit within the browser window (kind of like CollectedCookiesViews, which albeit isn't perfect...).
https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... File ui/views/controls/message_box_view.cc (right): https://codereview.chromium.org/375873007/diff/1/ui/views/controls/message_bo... ui/views/controls/message_box_view.cc:221: const int kMaxScrollViewHeight = 600; On 2014/07/10 00:34:58, msw wrote: > On 2014/07/10 00:27:53, Mustafa Emre Acer wrote: > > On 2014/07/09 23:21:08, sky wrote: > > > On 2014/07/09 18:06:56, Mustafa Emre Acer wrote: > > > > On 2014/07/09 16:20:15, sky wrote: > > > > > Why do you need to clip this? Wouldn't it be better to use work area? > > > > > > > > The clipping + overlay scroll bar gave a useful combination where both the > > > > dialog was constrained to a maximum height and the scroll bar was visible > > when > > > > needed. > > > > > > 'Clip' was a bad choice on my side. I'm suggesting you use the work area of > > the > > > monitor you're on and not an arbitrary value. > > > > The message view is constrained by the browser window, which means I'll need > to > > scale according to the browser window and not the whole screen, and will have > to > > pass the browser window to MessageView. Does that sound reasonable? > > > > > > > > > It looks like the scroll contents aren't visible at all when the view > isn't > > > > clipped. I don't have much expertise with the views, so maybe I'm missing > > > > something? > > > > > > ScrollView is a bit odd, you may need to explicitly set the bounds of > > > message_contents. > > > > You may be able to override MessageBoxView::GetMinimumSize (and ditto for the > relevant DialogDelegate?) to let the dialog be sized automatically to fit within > the browser window (kind of like CollectedCookiesViews, which albeit isn't > perfect...). I've tried a couple of approaches, and none seem to work other than using ClipHeightTo with a pixel value: - Overriding GetMinimumSize doesn't clamp the height to the browser window. - message_contents_->SetBoundsRect doesn't set a height for the scroll view. - NativeScrollBar doesn't show even when I set message_contents_->SetBoundsRect.
Thu, Jul 10, 2014 at 12:36 PM, <meacer@chromium.org> wrote: > > https://codereview.chromium.org/375873007/diff/1/ui/views/ > controls/message_box_view.cc > File ui/views/controls/message_box_view.cc (right): > > https://codereview.chromium.org/375873007/diff/1/ui/views/ > controls/message_box_view.cc#newcode221 > ui/views/controls/message_box_view.cc:221: const int > kMaxScrollViewHeight = 600; > On 2014/07/10 00:34:58, msw wrote: > >> On 2014/07/10 00:27:53, Mustafa Emre Acer wrote: >> > On 2014/07/09 23:21:08, sky wrote: >> > > On 2014/07/09 18:06:56, Mustafa Emre Acer wrote: >> > > > On 2014/07/09 16:20:15, sky wrote: >> > > > > Why do you need to clip this? Wouldn't it be better to use >> > work area? > >> > > > >> > > > The clipping + overlay scroll bar gave a useful combination >> > where both the > >> > > > dialog was constrained to a maximum height and the scroll bar >> > was visible > >> > when >> > > > needed. >> > > >> > > 'Clip' was a bad choice on my side. I'm suggesting you use the >> > work area of > >> > the >> > > monitor you're on and not an arbitrary value. >> > >> > The message view is constrained by the browser window, which means >> > I'll need > >> to >> > scale according to the browser window and not the whole screen, and >> > will have > >> to >> > pass the browser window to MessageView. Does that sound reasonable? >> > >> > > >> > > > It looks like the scroll contents aren't visible at all when the >> > view > >> isn't >> > > > clipped. I don't have much expertise with the views, so maybe >> > I'm missing > >> > > > something? >> > > >> > > ScrollView is a bit odd, you may need to explicitly set the bounds >> > of > >> > > message_contents. >> > >> > > You may be able to override MessageBoxView::GetMinimumSize (and ditto >> > for the > >> relevant DialogDelegate?) to let the dialog be sized automatically to >> > fit within > >> the browser window (kind of like CollectedCookiesViews, which albeit >> > isn't > >> perfect...). >> > > I've tried a couple of approaches, and none seem to work other than > using ClipHeightTo with a pixel value: > > - Overriding GetMinimumSize doesn't clamp the height to the browser > window. > - message_contents_->SetBoundsRect doesn't set a height for the scroll > view. > - NativeScrollBar doesn't show even when I set > message_contents_->SetBoundsRect. > Is this easy to reproduce with your patch? If so, tell me how and I'll poke at it. -Scott > > https://codereview.chromium.org/375873007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/10 20:05:19, sky wrote: > Thu, Jul 10, 2014 at 12:36 PM, <mailto:meacer@chromium.org> wrote: > > > > > https://codereview.chromium.org/375873007/diff/1/ui/views/ > > controls/message_box_view.cc > > File ui/views/controls/message_box_view.cc (right): > > > > https://codereview.chromium.org/375873007/diff/1/ui/views/ > > controls/message_box_view.cc#newcode221 > > ui/views/controls/message_box_view.cc:221: const int > > kMaxScrollViewHeight = 600; > > On 2014/07/10 00:34:58, msw wrote: > > > >> On 2014/07/10 00:27:53, Mustafa Emre Acer wrote: > >> > On 2014/07/09 23:21:08, sky wrote: > >> > > On 2014/07/09 18:06:56, Mustafa Emre Acer wrote: > >> > > > On 2014/07/09 16:20:15, sky wrote: > >> > > > > Why do you need to clip this? Wouldn't it be better to use > >> > > work area? > > > >> > > > > >> > > > The clipping + overlay scroll bar gave a useful combination > >> > > where both the > > > >> > > > dialog was constrained to a maximum height and the scroll bar > >> > > was visible > > > >> > when > >> > > > needed. > >> > > > >> > > 'Clip' was a bad choice on my side. I'm suggesting you use the > >> > > work area of > > > >> > the > >> > > monitor you're on and not an arbitrary value. > >> > > >> > The message view is constrained by the browser window, which means > >> > > I'll need > > > >> to > >> > scale according to the browser window and not the whole screen, and > >> > > will have > > > >> to > >> > pass the browser window to MessageView. Does that sound reasonable? > >> > > >> > > > >> > > > It looks like the scroll contents aren't visible at all when the > >> > > view > > > >> isn't > >> > > > clipped. I don't have much expertise with the views, so maybe > >> > > I'm missing > > > >> > > > something? > >> > > > >> > > ScrollView is a bit odd, you may need to explicitly set the bounds > >> > > of > > > >> > > message_contents. > >> > > >> > > > > You may be able to override MessageBoxView::GetMinimumSize (and ditto > >> > > for the > > > >> relevant DialogDelegate?) to let the dialog be sized automatically to > >> > > fit within > > > >> the browser window (kind of like CollectedCookiesViews, which albeit > >> > > isn't > > > >> perfect...). > >> > > > > I've tried a couple of approaches, and none seem to work other than > > using ClipHeightTo with a pixel value: > > > > - Overriding GetMinimumSize doesn't clamp the height to the browser > > window. > > - message_contents_->SetBoundsRect doesn't set a height for the scroll > > view. > > - NativeScrollBar doesn't show even when I set > > message_contents_->SetBoundsRect. > > > > Is this easy to reproduce with your patch? If so, tell me how and I'll poke > at it. > > -Scott Sure, just |alert|ing a long string with the patch should trigger the dialog with the scroll. s="aaaaa"; for(i=0;i<20;i++)s+=s; alert(s); > > > > > > https://codereview.chromium.org/375873007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I've convinced myself that the clip is ok, but the overlay is a special purpose scrollbar and not generally used. You shouldn't need that for this to work.
On 2014/07/11 22:27:13, sky wrote: > I've convinced myself that the clip is ok, but the overlay is a special purpose > scrollbar and not generally used. You shouldn't need that for this to work. Thanks for taking a look. Now using the default scroll bar (native).
LGTM
On 2014/07/14 15:30:28, sky wrote: > LGTM Thanks!
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/meacer@chromium.org/375873007/40001
Message was sent while issue was closed.
Change committed as 282978 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
