|
|
Created:
7 years, 9 months ago by msw Modified:
7 years, 9 months ago CC:
chromium-reviews, tfarina, Mike Wittman, Daniel Erat Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove Aura's extra shadowed border in the new dialog style.
Set new Aura dialogs' corewm::ShadowType to SHADOW_TYPE_NONE.
Removes the extra frame around new style dialogs on CrOS.
( does the same on the experimental Windows ash_shell )
See http://crbug.com/166075#c37 for before/after screenshots.
This doesn't remove the black dialog border on Win.
( non-transparency is required to host native controls )
BUG=166075
TEST=CrOS --enable-new-dialog-style looks nicer.
R=sky@chromium.org,ben@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190030
Patch Set 1 : Simplify; use a TYPE_BUBBLE Widget for the new dialog style. #Patch Set 2 : Remove NativeWidgetAura's shadow via InitParams::remove_standard_frame. #Patch Set 3 : Remove NativeWidgetAura's shadow in DialogDelegate Widget construction. #Messages
Total messages: 18 (0 generated)
Hey Scott, please take a look; thanks!
It seems rather confusing that we have dialogs treated as bubbles. Is there some other way we could do this? Maybe add a DIALOG constant that is treated the same as a bubble?
On 2013/03/20 21:39:42, sky wrote: > It seems rather confusing that we have dialogs treated as bubbles. Is there some > other way we could do this? Maybe add a DIALOG constant that is treated the same > as a bubble? On 2013/03/20 21:39:42, sky wrote: > It seems rather confusing that we have dialogs treated as bubbles. Is there some > other way we could do this? Maybe add a DIALOG constant that is treated the same > as a bubble? I could add a TYPE_DIALOG = TYPE_BUBBLE, but the list already seems overly long: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi... Do you think it makes more sense to rename TYPE_BUBBLE to TYPE_DIALOG, or would you consider that to still conflate two logically different things? Note that I'm not incredibly familiar with the nuances between the window types that seem potentially redundant to me (especially considering other Widget::InitParams flags), eg. POPUP/MENU/TOOLTIP/BUBBLE. +cc Ben in case he has any thoughts.
On Wed, Mar 20, 2013 at 6:57 PM, <msw@chromium.org> wrote: > Note that I'm not incredibly familiar with the nuances between the window types that > seem potentially redundant to me (especially considering other Widget::InitParams > flags), eg. POPUP/MENU/TOOLTIP/BUBBLE. Popup and Bubble are specially confusing to me. Bubble for me is a window with (possible) an arrow and round borders. A popup would be a Window like a panel (with caption buttons), without all the bells and wishes? -- Thiago
indeed. we should not change type for a different presentation. instead we should separate presentation from type and use the shared presentation style for dialogs.
On 2013/03/20 22:03:54, Ben Goodger (Google) wrote: > indeed. we should not change type for a different presentation. > > instead we should separate presentation from type and use the shared > presentation style for dialogs. In this case, I'm changing the type of the new style dialogs from TYPE_WINDOW to TYPE_BUBBLE to eliminate Ash's extra TYPE_WINDOW border/decoration that looks like a bubble. This was causing a double-bubble border appearance, seen in http://crbug.com/166075#c37 Do you prefer that dialogs keep TYPE_WINDOW and I find some other way to eliminate that extra border? I tried params.remove_standard_frame = true; to no avail, but haven't investigated deeply; any tips?
I think windows should have the correct type. The type triggers behavioral and appearance traits. But the implementation of the appearance should be independent of behavior... i.e. a different NCFV rendering.
On 2013/03/20 23:46:26, Ben Goodger (Google) wrote: > I think windows should have the correct type. > > The type triggers behavioral and appearance traits. > > But the implementation of the appearance should be independent of behavior... > i.e. a different NCFV rendering. Definitely, I'll dig deeper to try getting the right appearance/behavior from TYPE_WINDOW. The bubble NCFV seems right, but some Ash shadowing decoration is (wrongly?) kicking in. Otherwise, I could nix the bubble NCFV if UX is okay with the differing Ash shadow type.
Please take another look; you may also consider Patch Set 2.
lgtm
It could even be that the NCFVs have different _behavior_ (not sure, haven't looked at them recently)... in which case you may want to factor the presentation into a views::Border/views::Background impl that can be shared. I'll defer to your best judgment here. -Ben On Wed, Mar 20, 2013 at 5:21 PM, <msw@chromium.org> wrote: > On 2013/03/20 23:46:26, Ben Goodger (Google) wrote: > >> I think windows should have the correct type. >> > > The type triggers behavioral and appearance traits. >> > > But the implementation of the appearance should be independent of >> behavior... >> i.e. a different NCFV rendering. >> > > Definitely, I'll dig deeper to try getting the right appearance/behavior > from > TYPE_WINDOW. > The bubble NCFV seems right, but some Ash shadowing decoration is > (wrongly?) > kicking in. > Otherwise, I could nix the bubble NCFV if UX is okay with the differing Ash > shadow type. > > https://codereview.chromium.**org/12391056/<https://codereview.chromium.org/1... >
On 2013/03/21 18:21:40, Ben Goodger (Google) wrote: > It could even be that the NCFVs have different _behavior_ (not sure, > haven't looked at them recently)... in which case you may want to factor > the presentation into a views::Border/views::Background impl that can be > shared. I'll defer to your best judgment here. > > -Ben That was my intent with BubbleBorder, but views::corewm::Shadow (+cc derat@) has similar Aura-specific functionality. My BubbleBorder w/ImagePainter rewrite ( http://crrev.com/12647016 ) yields something akin to Shadow; so some code sharing might be in order. I'm happy to discuss plans elsewhere; for now, I think this is the right change; landing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/12391056/47002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/12391056/47002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/12391056/47002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/12391056/47002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/12391056/47002
Message was sent while issue was closed.
Change committed as 190030 |