Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2189123002:
BubbleFrameView: add padding if platform dislikes close buttons (Closed)
|
Created:
4 years, 4 months ago by Elly Fong-Jones Modified:
4 years, 4 months ago CC:
chromium-reviews, tfarina, msw+watch_chromium.org, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBubbleFrameView: add padding if platform dislikes close buttons
On Mac, the close button is always hidden, which means that dialogs that expect
the height of the close button to be included in their insets have too little
top inset. This change causes dialogs to always include the close button size
in the top inset if the platform doesn't show close buttons.
BUG=622859
Committed: https://crrev.com/be7bb00e00a42a5d69c831758b4e24513d666f3a
Cr-Commit-Position: refs/heads/master@{#409322}
Patch Set 1 #
Total comments: 1
Patch Set 2 : just never show close button on Mac #
Total comments: 1
Patch Set 3 : !ShouldShowCloseButton in tests #Patch Set 4 : add test coverage! #
Total comments: 6
Patch Set 5 : OnThemeChanged -> ResetWindowControls #Patch Set 6 : final fixups #
Messages
Total messages: 43 (26 generated)
The CQ bit was checked by ellyjones@chromium.org to run a CQ dry run
Description was changed from ========== BubbleFrameView: add padding if platform dislikes close buttons On Mac, the close button is always hidden, which means that dialogs that expect the height of the close button to be included in their insets have too little top inset. This change causes dialogs to always include the close button size in the top inset if the platform doesn't show close buttons. BUG=622859 ========== to ========== BubbleFrameView: add padding if platform dislikes close buttons On Mac, the close button is always hidden, which means that dialogs that expect the height of the close button to be included in their insets have too little top inset. This change causes dialogs to always include the close button size in the top inset if the platform doesn't show close buttons. BUG=622859 ==========
ellyjones@chromium.org changed reviewers: + msw@chromium.org
msw: ptal? :)
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/2189123002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2189123002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:249: !GetWidget()->widget_delegate()->ShouldShowCloseButton() This is wrong, numerous cross-platform bubbles and dialogs will override WidgetDelegate::ShouldShowCloseButton to false: https://cs.chromium.org/search/?q=ShouldShowCloseButton.*override (this will pad unintended bubbles and dialogs like your reverted CL) Here's my 3-step plan to fix this issue: (1) Remove the OS_MACOSX exception in WidgetDelegate::ShouldShowCloseButton. (2) Leave |insets| as it was and change |close_height| to: const int close_height = GetWidget()->widget_delegate()->ShouldShowCloseButton() ? close_->height() : 0; insets += gfx::Insets(std::max(title_height, close_height), 0, 0, 0); (3) Change BubbleFrameView::ResetWindowControls to: #if defined(OS_MACOSX) // The close button is never shown for dialogs or bubbles on Mac. close_->SetVisible(false); #else close_->SetVisible(GetWidget()->widget_delegate()->ShouldShowCloseButton()); #endif This *may* work if I understand the intent correctly. If we *do* want to show the close button for some Mac dialogs and bubbles, we'll potentially want some other solution; let's talk more if that's the case, we could possible set some additional flag, like |show_padding_instead_of_the_close_button_on_this_mac_dialog| or |really_actually_show_the_close_button_on_this_mac_dialog|, or devise something better.
The CQ bit was checked by ellyjones@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...
This seems correct, at least for bubble dialogs... https://codereview.chromium.org/2189123002/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_delegate.cc (left): https://codereview.chromium.org/2189123002/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_delegate.cc:83: return false; Hmm, now it occurs to me that this might cause the close button to show on any Mac dialogs that don't use BubbleFrameView... I'm not sure if any Mac dialog windows use other frames... you might want to try asking around/auditing, taking a look at DialogDelegate::CreateNonClientFrameView/GetDialogWidgetInitParams and maybe adding DCHECK[s] there to ensure Mac only ever makes dialog frames with bubble frame views (or trying to hide other frames' close buttons...).
On 2016/07/28 19:18:47, msw wrote: > This seems correct, at least for bubble dialogs... > > https://codereview.chromium.org/2189123002/diff/20001/ui/views/widget/widget_... > File ui/views/widget/widget_delegate.cc (left): > > https://codereview.chromium.org/2189123002/diff/20001/ui/views/widget/widget_... > ui/views/widget/widget_delegate.cc:83: return false; > Hmm, now it occurs to me that this might cause the close button to show on any > Mac dialogs that don't use BubbleFrameView... I'm not sure if any Mac dialog > windows use other frames... you might want to try asking around/auditing, taking > a look at DialogDelegate::CreateNonClientFrameView/GetDialogWidgetInitParams and > maybe adding DCHECK[s] there to ensure Mac only ever makes dialog frames with > bubble frame views (or trying to hide other frames' close buttons...). Mac only uses Views for a whitelisted set of dialogs, so I can audit them all exhaustively. By default every Mac dialog is still Cocoa.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ellyjones@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: This issue passed the CQ dry run.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2189123002/#ps60001 (title: "add test coverage!")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2189123002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2189123002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:66: bool should_show_close_ = false; Make this private and add a setter. https://codereview.chromium.org/2189123002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:139: (void)frame.GetWidget(); I think there's a more chrome-standard way of doing this, but I don't recall the exact syntax, look for a minute?
https://codereview.chromium.org/2189123002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2189123002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:139: (void)frame.GetWidget(); On 2016/07/29 19:38:39, msw wrote: > I think there's a more chrome-standard way of doing this, but I don't recall the > exact syntax, look for a minute? Found it: https://cs.chromium.org/chromium/src/base/macros.h?rcl=0&l=60
The CQ bit was checked by ellyjones@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2189123002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2189123002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:66: bool should_show_close_ = false; On 2016/07/29 19:38:39, msw wrote: > Make this private and add a setter. Done. https://codereview.chromium.org/2189123002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:139: (void)frame.GetWidget(); On 2016/07/29 19:38:39, msw wrote: > I think there's a more chrome-standard way of doing this, but I don't recall the > exact syntax, look for a minute? Done. https://codereview.chromium.org/2189123002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:139: (void)frame.GetWidget(); On 2016/07/29 19:40:59, msw wrote: > On 2016/07/29 19:38:39, msw wrote: > > I think there's a more chrome-standard way of doing this, but I don't recall > the > > exact syntax, look for a minute? > > Found it: https://cs.chromium.org/chromium/src/base/macros.h?rcl=0&l=60 Done.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2189123002/#ps100001 (title: "final fixups")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ellyjones@chromium.org changed reviewers: + sky@chromium.org
sky: ptal for ui/views/widget owner review :)
LGTM
The CQ bit was checked by ellyjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== BubbleFrameView: add padding if platform dislikes close buttons On Mac, the close button is always hidden, which means that dialogs that expect the height of the close button to be included in their insets have too little top inset. This change causes dialogs to always include the close button size in the top inset if the platform doesn't show close buttons. BUG=622859 ========== to ========== BubbleFrameView: add padding if platform dislikes close buttons On Mac, the close button is always hidden, which means that dialogs that expect the height of the close button to be included in their insets have too little top inset. This change causes dialogs to always include the close button size in the top inset if the platform doesn't show close buttons. BUG=622859 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== BubbleFrameView: add padding if platform dislikes close buttons On Mac, the close button is always hidden, which means that dialogs that expect the height of the close button to be included in their insets have too little top inset. This change causes dialogs to always include the close button size in the top inset if the platform doesn't show close buttons. BUG=622859 ========== to ========== BubbleFrameView: add padding if platform dislikes close buttons On Mac, the close button is always hidden, which means that dialogs that expect the height of the close button to be included in their insets have too little top inset. This change causes dialogs to always include the close button size in the top inset if the platform doesn't show close buttons. BUG=622859 Committed: https://crrev.com/be7bb00e00a42a5d69c831758b4e24513d666f3a Cr-Commit-Position: refs/heads/master@{#409322} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/be7bb00e00a42a5d69c831758b4e24513d666f3a Cr-Commit-Position: refs/heads/master@{#409322} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
