Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2469213002:
MacViews: TouchBar integration for toolkit-views dialogs. (Closed)
DescriptionMacViews: TouchBar integration for toolkit-views dialogs.
Native Mac NSAlert dialogs show the dialog buttons on the TouchBar, we
want similar behaviour for Chrome dialogs.
If a Widget's delegate implements views::DialogDelegate, add buttons to
the touchbar for the "OK" and "Cancel" buttons. Note the labels might
not necessarily be labeled "OK" and "Cancel".
Nearly all dialogs and bubbles work correctly with the TouchBar this
way. One exception is the Add Bookmark bubble -
LocationBarBubbleDelegateView::GetDialogButtons() returns
ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble.
SaveCardBubbleViews overrides again to return both buttons and works.
The translate bubble has "TODO: This should be using GetDialogButtons."
Screenshot: http://crbug.com/660126#c13
BUG=661581
Committed: https://crrev.com/c44c6a9a5b391a2081ed7259195ec1686482e2d0
Cr-Commit-Position: refs/heads/master@{#430209}
Patch Set 1 #Patch Set 2 : no touch unless dialog #Patch Set 3 : nit diff #Patch Set 4 : fix upstream, comppile #Patch Set 5 : I guess this is not needed either #Patch Set 6 : it works! #Patch Set 7 : Fix 10.10 SDK compile #Patch Set 8 : nit comments #
Total comments: 3
Patch Set 9 : Split files. #Patch Set 10 : close the client, not the delegate #
Total comments: 2
Patch Set 11 : update comment #
Total comments: 4
Patch Set 12 : : -> in #
Messages
Total messages: 51 (32 generated)
Description was changed from ========== MacViews: TouchBar integration for toolkit-views dialogs. moe props cl format Compiles. BUG= ========== to ========== MacViews: TouchBar integration for toolkit-views dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". BUG=660126 ==========
The CQ bit was checked by tapted@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...
Description was changed from ========== MacViews: TouchBar integration for toolkit-views dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". BUG=660126 ========== to ========== MacViews: TouchBar integration for toolkit-views dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". HTTP-auth and permission request bubbles work well. However, not all dialogs implement DialogDelegate properly. Examples: Edit bookmark and content settings bubbles show buttons, but they don't override DialogDelegate::Accept()/Cancel() as they should, so activating the TouchBar buttons does nothing. Add Bookmark bubble currently claims to have no buttons, so nothing shows up. These will be fixed in follow-ups. BUG=660126 ==========
tapted@chromium.org changed reviewers: + avi@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Avi, am I holding it right? Probably I should put the crazy SDK-availability stuff into ui/base/cocoa/fake_ns_touch_bar.h I think this is landable, and it's all behind a flag already anyway, so it will give us a chance to try things out safely. Screengrab using the touchbar simulator is at http://crbug.com/660126#c13 https://codereview.chromium.org/2469213002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2469213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1527: [button setKeyEquivalent:@"\n"]; I'd hoped this would give the blue color, but actually I don't think this is needed. -[NSButton setBezelColor] was introduced in 10.12.1 and is probably what NSAlert is using. The color values... I just picked from what NSAlert shows up as on the TouchBar simulator.
pinkerton@chromium.org changed reviewers: + pinkerton@chromium.org
Are we going to be perpetually following around feature developers and adding this delegate method to dialogs? How can we make it so that feature developers have to do the right thing at development time, not after we notice things are broken?
https://codereview.chromium.org/2469213002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2469213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1527: [button setKeyEquivalent:@"\n"]; On 2016/11/02 09:28:57, tapted wrote: > I'd hoped this would give the blue color, but actually I don't think this is > needed. -[NSButton setBezelColor] was introduced in 10.12.1 and is probably what > NSAlert is using. The color values... I just picked from what NSAlert shows up > as on the TouchBar simulator. Can we hopper up NSAlert and see what it's doing to achieve it?
On 2016/11/02 09:28:58, tapted wrote: > Probably I should put the crazy SDK-availability stuff into > ui/base/cocoa/fake_ns_touch_bar.h What's wrong with base/mac/sdk_forward_declarations.h?
I'll try firing up hopper today (although I'm build sheriff - maybe it will be quiet :). On 2016/11/02 14:13:59, pink wrote: > Are we going to be perpetually following around feature developers and adding > this delegate method to dialogs? How can we make it so that feature developers > have to do the right thing at development time, not after we notice things are > broken? Most dialogs already implement DialogDelegate, just not fully. I think it would be a simple matter of making DialogDelegate::Accept / Cancel pure virtual to fix the current set of problems, and encourage developers to read documentation we add to those methods about what they're used for. There's no reason for dialogs not to use Accept/Cancel - as part of their lifetime/logic. Beyond that, we can gather dialogs into a consolidated testing file that checks for things like an Accept/Cancel that do something. Or something similar to the TEST_VIEW macro that all dialogs should put in their test file that add test cases for touchbary things. On 2016/11/02 17:32:43, Avi wrote: > On 2016/11/02 09:28:58, tapted wrote: > > Probably I should put the crazy SDK-availability stuff into > > ui/base/cocoa/fake_ns_touch_bar.h > > What's wrong with base/mac/sdk_forward_declarations.h? These seem to fall into a different class of problems - it's not just adding methods to classes that exist via a category. The classes don't exist at all, so I need to pollute NSObject with categories (like [NSObject setView]), and that seems like a bad thing to expose in sdk_forward_declarations. (in fact I'm toying with the idea of making all the stuff I'm adding to BridgedContentView a category in a separate file, similar to how NSResponder adds a category to rope in the NSTouchProvider protocol - this way even BridgedContentView doesn't see the extra stuff I'm adding).
The CQ bit was checked by tapted@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...
Description was changed from ========== MacViews: TouchBar integration for toolkit-views dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". HTTP-auth and permission request bubbles work well. However, not all dialogs implement DialogDelegate properly. Examples: Edit bookmark and content settings bubbles show buttons, but they don't override DialogDelegate::Accept()/Cancel() as they should, so activating the TouchBar buttons does nothing. Add Bookmark bubble currently claims to have no buttons, so nothing shows up. These will be fixed in follow-ups. BUG=660126 ========== to ========== MacViews: TouchBar integration for toolkit-views dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". HTTP-auth and permission request bubbles work well. However, not all dialogs implement DialogDelegate properly. Examples: Edit bookmark and content settings bubbles show buttons, but they don't override DialogDelegate::Accept()/Cancel() as they should, so activating the TouchBar buttons does nothing. Add Bookmark bubble currently claims to have no buttons, so nothing shows up. These will be fixed in follow-ups. BUG=661581 ==========
PTAL - made touch_bar_forward_declarations.h and moved all the implementation into a category in bridged_content_view_touch_bar.mm -- bridged_content_view.mm doesn't change at all. code is the same except I removed the setKeyEquivalent and added some comments in makeTouchBar > On 2016/11/02 17:32:43, Avi wrote: > > On 2016/11/02 09:28:58, tapted wrote: > > > Probably I should put the crazy SDK-availability stuff into > > > ui/base/cocoa/fake_ns_touch_bar.h > > > > What's wrong with base/mac/sdk_forward_declarations.h? > > These seem to fall into a different class of problems - it's not just adding > methods to classes that exist via a category. The classes don't exist at all, so > I need to pollute NSObject with categories (like [NSObject setView]), and that > seems like a bad thing to expose in sdk_forward_declarations. > > (in fact I'm toying with the idea of making all the stuff I'm adding to > BridgedContentView a category in a separate file, similar to how NSResponder > adds a category to rope in the NSTouchProvider protocol - this way even > BridgedContentView doesn't see the extra stuff I'm adding). (Done.) https://codereview.chromium.org/2469213002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2469213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1527: [button setKeyEquivalent:@"\n"]; On 2016/11/02 14:50:41, Avi wrote: > On 2016/11/02 09:28:57, tapted wrote: > > I'd hoped this would give the blue color, but actually I don't think this is > > needed. -[NSButton setBezelColor] was introduced in 10.12.1 and is probably > what > > NSAlert is using. The color values... I just picked from what NSAlert shows up > > as on the TouchBar simulator. > > Can we hopper up NSAlert and see what it's doing to achieve it? Dumped my findings to http://crbug.com/661581 - it's using a lot of utility classes that aren't exposed in the public headers :/ But I removed this line anyway -- just setting the bezel color below gives the correct appearance.
The CQ bit was checked by tapted@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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== MacViews: TouchBar integration for toolkit-views dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". HTTP-auth and permission request bubbles work well. However, not all dialogs implement DialogDelegate properly. Examples: Edit bookmark and content settings bubbles show buttons, but they don't override DialogDelegate::Accept()/Cancel() as they should, so activating the TouchBar buttons does nothing. Add Bookmark bubble currently claims to have no buttons, so nothing shows up. These will be fixed in follow-ups. BUG=661581 ========== to ========== MacViews: TouchBar integration for toolkit-views dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". Nearly all dialogs and bubbles work correctly with the TouchBar this way. One exception is the Add Bookmark bubble - LocationBarBubbleDelegateView::GetDialogButtons() returns ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble. SaveCardBubbleViews overrides again to return both buttons and works. The translate bubble has "TODO: This should be using GetDialogButtons." BUG=661581 ==========
On 2016/11/02 20:47:27, tapted wrote: > On 2016/11/02 14:13:59, pink wrote: > > Are we going to be perpetually following around feature developers and adding > > this delegate method to dialogs? How can we make it so that feature developers > > have to do the right thing at development time, not after we notice things are > > broken? > > Most dialogs already implement DialogDelegate, just not fully. Actually the ones that "worked" were the ones doing the weird thing :/. In patchset 10 nearly everything now works correctly (hooray \o/). Fix was to call DialogClientView::AcceptWindow() rather than DialogDelegate::Accept() (the latter should probably be called OnAccept and be private - dialogs shouldn't close the window there but the ones that "worked" did :/). Bookmark bubble is still weird, but it should be easily fixed.
Rather cool, though I don't know the touchbar api or Views... https://codereview.chromium.org/2469213002/diff/180001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2469213002/diff/180001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view_touch_bar.mm:92: alpha:1.0]]; My comment still stands; _is_ this how NSAlert does it? If so, alter your comment, and if not, what does it do?
The CQ bit was checked by tapted@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...
Description was changed from ========== MacViews: TouchBar integration for toolkit-views dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". Nearly all dialogs and bubbles work correctly with the TouchBar this way. One exception is the Add Bookmark bubble - LocationBarBubbleDelegateView::GetDialogButtons() returns ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble. SaveCardBubbleViews overrides again to return both buttons and works. The translate bubble has "TODO: This should be using GetDialogButtons." BUG=661581 ========== to ========== MacViews: TouchBar integration for toolkit-views dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". Nearly all dialogs and bubbles work correctly with the TouchBar this way. One exception is the Add Bookmark bubble - LocationBarBubbleDelegateView::GetDialogButtons() returns ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble. SaveCardBubbleViews overrides again to return both buttons and works. The translate bubble has "TODO: This should be using GetDialogButtons." Screenshot: http://crbug.com/660126#c13 BUG=661581 ==========
Description was changed from ========== MacViews: TouchBar integration for toolkit-views dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". Nearly all dialogs and bubbles work correctly with the TouchBar this way. One exception is the Add Bookmark bubble - LocationBarBubbleDelegateView::GetDialogButtons() returns ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble. SaveCardBubbleViews overrides again to return both buttons and works. The translate bubble has "TODO: This should be using GetDialogButtons." Screenshot: http://crbug.com/660126#c13 BUG=661581 ========== to ========== MacViews: TouchBar integration for toolkit-views dialogs. Native Mac NSAlert dialogs show the dialog buttons on the TouchBar, we want similar behaviour for Chrome dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". Nearly all dialogs and bubbles work correctly with the TouchBar this way. One exception is the Add Bookmark bubble - LocationBarBubbleDelegateView::GetDialogButtons() returns ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble. SaveCardBubbleViews overrides again to return both buttons and works. The translate bubble has "TODO: This should be using GetDialogButtons." Screenshot: http://crbug.com/660126#c13 BUG=661581 ==========
tapted@chromium.org changed reviewers: + sky@chromium.org
+sky - this is all ObjC, but I want to make sure I'm using DialogDelegate/DialogClientView appropriately. E.g. the stuff at https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged... (i.e. AcceptWindow() / CancelWindow()) (http://crbug.com/660126 has some more context about this "TouchBar" thing - a touch screen that replaces the function keys on the latest MacBook pros). https://codereview.chromium.org/2469213002/diff/180001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2469213002/diff/180001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view_touch_bar.mm:92: alpha:1.0]]; On 2016/11/03 14:30:50, Avi wrote: > My comment still stands; _is_ this how NSAlert does it? If so, alter your > comment, and if not, what does it do? updated the comment: // NSAlert uses a private NSButton subclass (_NSTouchBarGroupButton) with // more bells and whistles. It doesn't use -setBezelColor: directly, but // this gives an appearance matching the default _NSTouchBarGroupButton. I can't think what else we'd need a default button to do on the touch bar apart from "look blue". I didn't find anything special on _NSTouchBarGroupButton. Maybe we find something later on, but this is simple so I think it's a good way to start.
Calling AcceptWindow/CancelWindow LGTM (I'm assuming the other reviewers are looking at the ObectiveC code).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view_touch_bar.mm:57: for (NSTouchBarItemIdentifier i : @[ kTouchBarCancelId, kTouchBarOKId ]) { :? I thought the objective c for-in was for( x in y)? https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view_touch_bar.mm:90: // this gives an appearance matching the default _NSTouchBarGroupButton. 👍
The CQ bit was checked by tapted@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...
Thanks all! https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view_touch_bar.mm:57: for (NSTouchBarItemIdentifier i : @[ kTouchBarCancelId, kTouchBarOKId ]) { On 2016/11/04 16:41:24, Avi wrote: > :? I thought the objective c for-in was for( x in y)? Changed to `in`. `:` wasn't intentional, but I investigated a ~year ago and the syntax actually results in identical assembly code (i.e. both using ObjC semantics/NSFastEnumeration). So I guess it's a good hint that this iterates over ObjC stuff rather than C++ iterators. (Of course, the other perspective is that the ObjC `in` could now be totally redundant and we could scrap it :).
The CQ bit was unchecked by tapted@chromium.org
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2469213002/#ps220001 (title: ": -> in")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view_touch_bar.mm:57: for (NSTouchBarItemIdentifier i : @[ kTouchBarCancelId, kTouchBarOKId ]) { On 2016/11/06 23:57:48, tapted wrote: > On 2016/11/04 16:41:24, Avi wrote: > > :? I thought the objective c for-in was for( x in y)? > > Changed to `in`. `:` wasn't intentional, but I investigated a ~year ago and the > syntax actually results in identical assembly code (i.e. both using ObjC > semantics/NSFastEnumeration). So I guess it's a good hint that this iterates > over ObjC stuff rather than C++ iterators. (Of course, the other perspective is > that the ObjC `in` could now be totally redundant and we could scrap it :). That is genuinely weird. Maybe they bridge it where begin()/end() on a fast-enumerable collection uses fast enumeration? I'm sure Nico will step in and provide a link to the Clang front-end where that happens.
lgtm lgtm https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view_touch_bar.mm (right): https://codereview.chromium.org/2469213002/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view_touch_bar.mm:57: for (NSTouchBarItemIdentifier i : @[ kTouchBarCancelId, kTouchBarOKId ]) { On 2016/11/06 23:57:48, tapted wrote: > On 2016/11/04 16:41:24, Avi wrote: > > :? I thought the objective c for-in was for( x in y)? > > Changed to `in`. `:` wasn't intentional, but I investigated a ~year ago and the > syntax actually results in identical assembly code (i.e. both using ObjC > semantics/NSFastEnumeration). So I guess it's a good hint that this iterates > over ObjC stuff rather than C++ iterators. (Of course, the other perspective is > that the ObjC `in` could now be totally redundant and we could scrap it :). That is genuinely weird. Maybe they bridge it where begin()/end() on a fast-enumerable collection uses fast enumeration? I'm sure Nico will step in and provide a link to the Clang front-end where that happens.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tapted@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 ========== MacViews: TouchBar integration for toolkit-views dialogs. Native Mac NSAlert dialogs show the dialog buttons on the TouchBar, we want similar behaviour for Chrome dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". Nearly all dialogs and bubbles work correctly with the TouchBar this way. One exception is the Add Bookmark bubble - LocationBarBubbleDelegateView::GetDialogButtons() returns ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble. SaveCardBubbleViews overrides again to return both buttons and works. The translate bubble has "TODO: This should be using GetDialogButtons." Screenshot: http://crbug.com/660126#c13 BUG=661581 ========== to ========== MacViews: TouchBar integration for toolkit-views dialogs. Native Mac NSAlert dialogs show the dialog buttons on the TouchBar, we want similar behaviour for Chrome dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". Nearly all dialogs and bubbles work correctly with the TouchBar this way. One exception is the Add Bookmark bubble - LocationBarBubbleDelegateView::GetDialogButtons() returns ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble. SaveCardBubbleViews overrides again to return both buttons and works. The translate bubble has "TODO: This should be using GetDialogButtons." Screenshot: http://crbug.com/660126#c13 BUG=661581 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: TouchBar integration for toolkit-views dialogs. Native Mac NSAlert dialogs show the dialog buttons on the TouchBar, we want similar behaviour for Chrome dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". Nearly all dialogs and bubbles work correctly with the TouchBar this way. One exception is the Add Bookmark bubble - LocationBarBubbleDelegateView::GetDialogButtons() returns ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble. SaveCardBubbleViews overrides again to return both buttons and works. The translate bubble has "TODO: This should be using GetDialogButtons." Screenshot: http://crbug.com/660126#c13 BUG=661581 ========== to ========== MacViews: TouchBar integration for toolkit-views dialogs. Native Mac NSAlert dialogs show the dialog buttons on the TouchBar, we want similar behaviour for Chrome dialogs. If a Widget's delegate implements views::DialogDelegate, add buttons to the touchbar for the "OK" and "Cancel" buttons. Note the labels might not necessarily be labeled "OK" and "Cancel". Nearly all dialogs and bubbles work correctly with the TouchBar this way. One exception is the Add Bookmark bubble - LocationBarBubbleDelegateView::GetDialogButtons() returns ui::DIALOG_BUTTON_NONE. That makes sense for the ZoomBubble. SaveCardBubbleViews overrides again to return both buttons and works. The translate bubble has "TODO: This should be using GetDialogButtons." Screenshot: http://crbug.com/660126#c13 BUG=661581 Committed: https://crrev.com/c44c6a9a5b391a2081ed7259195ec1686482e2d0 Cr-Commit-Position: refs/heads/master@{#430209} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c44c6a9a5b391a2081ed7259195ec1686482e2d0 Cr-Commit-Position: refs/heads/master@{#430209} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
