Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2506133003:
[ash-md] Allows ToggleButton to have a border and adds focus rectangle (Closed)
|
Created:
4 years, 1 month ago by varkha Modified:
4 years, 1 month ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, tfarina, shuchen+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, kalyank, stevenjb+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ash-md] Allows ToggleButton to have a border and adds focus rectangle
- Allows ToggleButton to accept a border set externally.
- Allows ToggleButton to have a tooltip that includes ThumbView.
- Adds TrayPopupUtils::CreateToggleButton to set up ToggleButtons for
use in system menu.
- Uses TrayPopupUtils::CreateToggleButton in IME, Bluetooth and Network
pages.
BUG=652492
TEST=manual
Bring focus using Tab to a toggle button in Network detailed page.
(focus rectangle appears).
Press Space (toggle switches)
Press Enter (toggle switches)
Tab (focus switches to the next row).
Committed: https://crrev.com/9bd7f8c886e2b6131646b78d7598d2115894b385
Cr-Commit-Position: refs/heads/master@{#433386}
Patch Set 1 #Patch Set 2 : [ash-md] Allows ToggleButton to have a border and adds focus rectangle (rebase) #
Total comments: 12
Patch Set 3 : [ash-md] Allows ToggleButton to have a border and adds focus rectangle (comments) #
Total comments: 14
Patch Set 4 : [ash-md] Allows ToggleButton to have a border and adds focus rectangle (comments) #
Total comments: 2
Patch Set 5 : [ash-md] Allows ToggleButton to have a border and adds focus rectangle (comments) #Patch Set 6 : [ash-md] Allows ToggleButton to have a border and adds focus rectangle (use GetContentBounds to cen… #
Total comments: 7
Patch Set 7 : [ash-md] Allows ToggleButton to have a border and adds focus rectangle (nits) #
Total comments: 2
Patch Set 8 : [ash-md] Allows ToggleButton to have a border and adds focus rectangle (nits) #
Total comments: 4
Patch Set 9 : [ash-md] Allows ToggleButton to have a border and adds focus rectangle (nits) #
Total comments: 10
Patch Set 10 : [ash-md] Allows ToggleButton to have a border and adds focus rectangle (nits) #Patch Set 11 : [ash-md] Allows ToggleButton to have a border and adds focus rectangle (rebase) #Messages
Total messages: 59 (29 generated)
varkha@chromium.org changed reviewers: + estade@chromium.org, tdanderson@chromium.org
estade@, tdanderson@, Please see if you like this better than https://codereview.chromium.org/2509943002/. I tried to simplify the sizing logic for the toggle button elements without making breaking changes. Thanks!
The CQ bit was checked by varkha@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_chromeos_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 varkha@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.
sorry for not being clear in the other patch. I would like to implement the simplest possible changes for M56 while we discuss the right path forward in the future (in terms of both design and engineering). I think the changes here may not be ones we want long-term, so I'd rather make the minimal change for now. The minimal change would be to simply draw a focus rect around the bounds of the toggle button. This doesn't match the spec, but it should be good enough for M56. Many other focus rects in the system menu also don't match the spec (the insets are slightly off, the slider's focus rect is around the whole control instead of the thumb, etc.).
https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:56: return parent_->GetTooltipText(p, tooltip); can't we just make this view invisible in terms of event targetting by returning false for CanProcessEventsWithinSubtree https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:104: ToggleButton* parent_; I don't think you need this, just call parent() (and probably don't even need to do that with the above recommendation) https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:117: focus_painter_(Painter::CreateDashedFocusPainter()) { I don't think we'll ever want a dashed focus painter for toggle buttons so I'd leave it null by default https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:146: void ToggleButton::SetFocusPainter(std::unique_ptr<Painter> focus_painter) { set_focus_painter https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:158: if (focus_painter_.get()) .get() shouldn't be necessary https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:164: (height() - kTrackHeight) / 2, kTrackWidth, please share the track rect calculations
The CQ bit was checked by varkha@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...
varkha@chromium.org changed reviewers: + sadrul@chromium.org
Thanks estade@! I think I've addressed the comments, PTAL. +sadrul@ for OWNERS in ui/views/. https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:56: return parent_->GetTooltipText(p, tooltip); On 2016/11/17 21:11:26, Evan Stade wrote: > can't we just make this view invisible in terms of event targetting by returning > false for CanProcessEventsWithinSubtree Yes, TIL. Done. https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:104: ToggleButton* parent_; On 2016/11/17 21:11:26, Evan Stade wrote: > I don't think you need this, just call parent() (and probably don't even need to > do that with the above recommendation) Done. https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:117: focus_painter_(Painter::CreateDashedFocusPainter()) { On 2016/11/17 21:11:26, Evan Stade wrote: > I don't think we'll ever want a dashed focus painter for toggle buttons so I'd > leave it null by default Done. https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:146: void ToggleButton::SetFocusPainter(std::unique_ptr<Painter> focus_painter) { On 2016/11/17 21:11:26, Evan Stade wrote: > set_focus_painter Done. https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:158: if (focus_painter_.get()) On 2016/11/17 21:11:26, Evan Stade wrote: > .get() shouldn't be necessary Done. https://codereview.chromium.org/2506133003/diff/20001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:164: (height() - kTrackHeight) / 2, kTrackWidth, On 2016/11/17 21:11:26, Evan Stade wrote: > please share the track rect calculations Done.
https://codereview.chromium.org/2506133003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_popup_utils.cc (right): https://codereview.chromium.org/2506133003/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_utils.cc:259: kFocusBorderColor, gfx::Insets(1))); insets should be gfx::Insets(0,0,1,1) (CreateSolidFocusPainter is broken like that) https://codereview.chromium.org/2506133003/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_utils.cc:260: toggle->SetFocusForPlatform(); seems like this should be part of the base ToggleButton impl https://codereview.chromium.org/2506133003/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_utils.cc:261: toggle->SetTooltipText(l10n_util::GetStringUTF16(accessible_name_id)); This is a behavioral change. Terry and I decided against tooltips for toggle buttons as they're redundant. You do want SetAccessibleName though. https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:160: gfx::RectF ToggleButton::GetTrackBounds() const { we can only lay out in DIP, so this should return a gfx::Rect https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:161: gfx::RectF track_bounds((gfx::SizeF(size()))); too many parens https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:209: gfx::ScopedCanvas scoped_canvas(canvas); Personally I think I'd prefer not indenting and using Save/Restore instead, but I don't feel strongly.
https://codereview.chromium.org/2506133003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_popup_utils.cc (right): https://codereview.chromium.org/2506133003/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_utils.cc:259: kFocusBorderColor, gfx::Insets(1))); On 2016/11/17 22:32:22, Evan Stade wrote: > insets should be gfx::Insets(0,0,1,1) (CreateSolidFocusPainter is broken like > that) This is probably the right thing to do but it needs to be done in sync with ImageButton as used in the system menu [1]. I can post a CL with that but they need to stay in sync. [1] - https://cs.chromium.org/chromium/src/ash/common/system/tray/system_menu_butto... https://codereview.chromium.org/2506133003/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_utils.cc:260: toggle->SetFocusForPlatform(); On 2016/11/17 22:32:22, Evan Stade wrote: > seems like this should be part of the base ToggleButton impl Done. https://codereview.chromium.org/2506133003/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_utils.cc:261: toggle->SetTooltipText(l10n_util::GetStringUTF16(accessible_name_id)); On 2016/11/17 22:32:22, Evan Stade wrote: > This is a behavioral change. Terry and I decided against tooltips for toggle > buttons as they're redundant. You do want SetAccessibleName though. Done. https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:160: gfx::RectF ToggleButton::GetTrackBounds() const { On 2016/11/17 22:32:22, Evan Stade wrote: > we can only lay out in DIP, so this should return a gfx::Rect Done. https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:161: gfx::RectF track_bounds((gfx::SizeF(size()))); On 2016/11/17 22:32:22, Evan Stade wrote: > too many parens I would also think so. Compiler disagreed though: ../../ui/views/controls/button/toggle_button.cc:162:26: error: parentheses were disambiguated as a function declaration [-Werror,-Wvexing-parse] gfx::RectF track_bounds(gfx::SizeF(size())); ^~~~~~~~~~~~~~~~~~~~ ../../ui/views/controls/button/toggle_button.cc:162:27: note: add a pair of parentheses to declare a variable gfx::RectF track_bounds(gfx::SizeF(size())); ^ ( ) This however is not necessary if we are OK with integer calculations. https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:209: gfx::ScopedCanvas scoped_canvas(canvas); On 2016/11/17 22:32:22, Evan Stade wrote: > Personally I think I'd prefer not indenting and using Save/Restore instead, but > I don't feel strongly. Done. We use both throughout the code but I've noticed that slider uses Save/Restore so I've switched to that.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
The CQ bit was unchecked by varkha@chromium.org
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/2506133003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:209: gfx::ScopedCanvas scoped_canvas(canvas); On 2016/11/17 22:51:54, varkha wrote: > On 2016/11/17 22:32:22, Evan Stade wrote: > > Personally I think I'd prefer not indenting and using Save/Restore instead, > but > > I don't feel strongly. > > Done. We use both throughout the code but I've noticed that slider uses > Save/Restore so I've switched to that. yes, ScopedCanvas is nice when you already have an appropriate scope https://codereview.chromium.org/2506133003/diff/60001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/60001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:161: gfx::Rect track_bounds(size()); I believe this should be GetContentsBounds() instead of size(), otherwise if you added a border that was 20px below and 0px above, it would center the button instead of leaving it in the same place. IOW, the track should be centered in the contents bounds, not the total bounds.
https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:209: gfx::ScopedCanvas scoped_canvas(canvas); On 2016/11/17 23:00:43, Evan Stade wrote: > On 2016/11/17 22:51:54, varkha wrote: > > On 2016/11/17 22:32:22, Evan Stade wrote: > > > Personally I think I'd prefer not indenting and using Save/Restore instead, > > but > > > I don't feel strongly. > > > > Done. We use both throughout the code but I've noticed that slider uses > > Save/Restore so I've switched to that. > > yes, ScopedCanvas is nice when you already have an appropriate scope Acknowledged. https://codereview.chromium.org/2506133003/diff/60001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/60001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:161: gfx::Rect track_bounds(size()); On 2016/11/17 23:00:43, Evan Stade wrote: > I believe this should be GetContentsBounds() instead of size(), otherwise if you > added a border that was 20px below and 0px above, it would center the button > instead of leaving it in the same place. IOW, the track should be centered in > the contents bounds, not the total bounds. Good catch in case we do asymmetric borders and very much with the spirit of this change! Done.
lgtm https://codereview.chromium.org/2506133003/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_constants.cc (right): https://codereview.chromium.org/2506133003/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_constants.cc:59: const int kTrayToggleButtonVerticalPadding = 13; can we calculate this? Should be more robust that way (to changes in ToggleButton, for example. e.g. (popup_item_min_height - toggle_button->GetPreferredSize().height()) / 2; https://codereview.chromium.org/2506133003/diff/100001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2506133003/diff/100001/ui/views/controls/butt... ui/views/controls/button/toggle_button.h:32: void OnFocus() override; nit: put these in the same block as the other overrides
https://codereview.chromium.org/2506133003/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_constants.cc (right): https://codereview.chromium.org/2506133003/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_constants.cc:59: const int kTrayToggleButtonVerticalPadding = 13; On 2016/11/17 23:24:36, Evan Stade wrote: > can we calculate this? Should be more robust that way (to changes in > ToggleButton, for example. > > e.g. (popup_item_min_height - toggle_button->GetPreferredSize().height()) / 2; > I'd like to but I am not sure how to do that without exposing GetPreferredSize to code in ash/.../system/tray. Would querying toggle->size() work upon creation? https://codereview.chromium.org/2506133003/diff/100001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2506133003/diff/100001/ui/views/controls/butt... ui/views/controls/button/toggle_button.h:32: void OnFocus() override; On 2016/11/17 23:24:36, Evan Stade wrote: > nit: put these in the same block as the other overrides Done. Also split View overrides from the CustomButton and reordered.
https://codereview.chromium.org/2506133003/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_constants.cc (right): https://codereview.chromium.org/2506133003/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_constants.cc:59: const int kTrayToggleButtonVerticalPadding = 13; On 2016/11/17 23:40:51, varkha wrote: > On 2016/11/17 23:24:36, Evan Stade wrote: > > can we calculate this? Should be more robust that way (to changes in > > ToggleButton, for example. > > > > e.g. (popup_item_min_height - toggle_button->GetPreferredSize().height()) / 2; > > > > I'd like to but I am not sure how to do that without exposing GetPreferredSize > to code in ash/.../system/tray. Would querying toggle->size() work upon > creation? See if this change works - I think 68 is the dimension that is in the mocks and I made GetVisibleSize visible from ToggleButton.
https://codereview.chromium.org/2506133003/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_constants.cc (right): https://codereview.chromium.org/2506133003/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_constants.cc:59: const int kTrayToggleButtonVerticalPadding = 13; On 2016/11/18 00:00:34, varkha wrote: > On 2016/11/17 23:40:51, varkha wrote: > > On 2016/11/17 23:24:36, Evan Stade wrote: > > > can we calculate this? Should be more robust that way (to changes in > > > ToggleButton, for example. > > > > > > e.g. (popup_item_min_height - toggle_button->GetPreferredSize().height()) / > 2; > > > > > > > I'd like to but I am not sure how to do that without exposing GetPreferredSize > > to code in ash/.../system/tray. Would querying toggle->size() work upon > > creation? > > See if this change works - I think 68 is the dimension that is in the mocks and > I made GetVisibleSize visible from ToggleButton. I was really only talking about the height, as the width padding really is a magic number (unless we can reuse the vertical padding number we calculate --- they seem very close). https://codereview.chromium.org/2506133003/diff/120001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2506133003/diff/120001/ui/views/controls/butt... ui/views/controls/button/toggle_button.h:32: gfx::Size GetVisibleSize() const; I'd prefer exposing GetPreferredSize(). That should be enough as you will only use it before setting a border and it should be public anyway (it is on View).
https://codereview.chromium.org/2506133003/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_constants.cc (right): https://codereview.chromium.org/2506133003/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_constants.cc:59: const int kTrayToggleButtonVerticalPadding = 13; On 2016/11/18 00:20:21, Evan Stade wrote: > On 2016/11/18 00:00:34, varkha wrote: > > On 2016/11/17 23:40:51, varkha wrote: > > > On 2016/11/17 23:24:36, Evan Stade wrote: > > > > can we calculate this? Should be more robust that way (to changes in > > > > ToggleButton, for example. > > > > > > > > e.g. (popup_item_min_height - toggle_button->GetPreferredSize().height()) > / > > 2; > > > > > > > > > > I'd like to but I am not sure how to do that without exposing > GetPreferredSize > > > to code in ash/.../system/tray. Would querying toggle->size() work upon > > > creation? > > > > See if this change works - I think 68 is the dimension that is in the mocks > and > > I made GetVisibleSize visible from ToggleButton. > > I was really only talking about the height, as the width padding really is a > magic number (unless we can reuse the vertical padding number we calculate --- > they seem very close). I think it is easier to have the horizontal size based on the full size (same as kMenuButtonSize) rather than insets. Yes, the insets end up similar and only different (I think) because we need to reserve space for shadow and because (I am guessing here) you wanted the heights and insets to be whole numbers for accurate layout. https://codereview.chromium.org/2506133003/diff/120001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2506133003/diff/120001/ui/views/controls/butt... ui/views/controls/button/toggle_button.h:32: gfx::Size GetVisibleSize() const; On 2016/11/18 00:20:21, Evan Stade wrote: > I'd prefer exposing GetPreferredSize(). That should be enough as you will only > use it before setting a border and it should be public anyway (it is on View). Done.
The CQ bit was checked by varkha@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...
LGTM with a nit and a question. https://codereview.chromium.org/2506133003/diff/140001/ash/common/system/tray... File ash/common/system/tray/tray_constants.h (right): https://codereview.chromium.org/2506133003/diff/140001/ash/common/system/tray... ash/common/system/tray/tray_constants.h:65: // The width of ToggleButtons. nit: mention this includes padding. https://codereview.chromium.org/2506133003/diff/140001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/140001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:56: // Make the thumb behave as part of the parent for tooltip event handling. I thought that we were not going to show tooltips on toggles since their meaning was clear because of surrounding text? And in this CL it looks like you aren't setting tooltips because you've only called SetAccessibleName() in CreateToggleButton(). Regardless, I suggest changing the comment to just "Make the thumb behave as part of the parent for event handling."
sadrul@, PTAL, hoping to land this before M-56 branches out. https://codereview.chromium.org/2506133003/diff/140001/ash/common/system/tray... File ash/common/system/tray/tray_constants.h (right): https://codereview.chromium.org/2506133003/diff/140001/ash/common/system/tray... ash/common/system/tray/tray_constants.h:65: // The width of ToggleButtons. On 2016/11/18 01:46:50, tdanderson wrote: > nit: mention this includes padding. Done. https://codereview.chromium.org/2506133003/diff/140001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/140001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:56: // Make the thumb behave as part of the parent for tooltip event handling. On 2016/11/18 01:46:50, tdanderson wrote: > I thought that we were not going to show tooltips on toggles since their meaning > was clear because of surrounding text? And in this CL it looks like you aren't > setting tooltips because you've only called SetAccessibleName() in > CreateToggleButton(). > > Regardless, I suggest changing the comment to just "Make the thumb behave as > part of the parent for event handling." Done.
The CQ bit was checked by varkha@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/2506133003/diff/160001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:56: // Make the thumb behave as part of the parent for event handling. other events like clicks and so forth already trickle up by default when unhandled, so isn't this unnecessary without tooltips?
https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:56: // Make the thumb behave as part of the parent for event handling. On 2016/11/18 14:29:06, Evan Stade wrote: > other events like clicks and so forth already trickle up by default when > unhandled, so isn't this unnecessary without tooltips? I guess it's good to keep this anyway as some other user of ToggleButton may want a tooltip, even if our current uses don't need one.
https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:144: void ToggleButton::set_focus_painter(std::unique_ptr<Painter> focus_painter) { Not sure if std::move is trivial enough to allow this, or whether this should be SetFocusPainter() (although I notice we call View::SetBorder() instead of View::set_border(), so perhaps this should be SetFocusPainter() too) https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:196: canvas->Save(); gfx::ScopedCanvas But why do you need to do this? View::Paint already does this save/restore before calling OnPaint() https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:210: views::Painter::PaintFocusPainter(this, canvas, focus_painter_.get()); Omit views::
https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:144: void ToggleButton::set_focus_painter(std::unique_ptr<Painter> focus_painter) { On 2016/11/18 16:37:30, sadrul wrote: > Not sure if std::move is trivial enough to allow this, or whether this should be > SetFocusPainter() (although I notice we call View::SetBorder() instead of > View::set_border(), so perhaps this should be SetFocusPainter() too) Done. https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:196: canvas->Save(); On 2016/11/18 16:37:30, sadrul wrote: > gfx::ScopedCanvas > > But why do you need to do this? View::Paint already does this save/restore > before calling OnPaint() This is to restore scale before painting the focus rect. I'll update the rest, hope this one is right. https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:210: views::Painter::PaintFocusPainter(this, canvas, focus_painter_.get()); On 2016/11/18 16:37:30, sadrul wrote: > Omit views:: Done.
lgtm
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org, tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2506133003/#ps180001 (title: "[ash-md] Allows ToggleButton to have a border and adds focus rectangle (nits)")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:144: void ToggleButton::set_focus_painter(std::unique_ptr<Painter> focus_painter) { On 2016/11/18 16:55:35, varkha wrote: > On 2016/11/18 16:37:30, sadrul wrote: > > Not sure if std::move is trivial enough to allow this, or whether this should > be > > SetFocusPainter() (although I notice we call View::SetBorder() instead of > > View::set_border(), so perhaps this should be SetFocusPainter() too) > > Done. I requested set_focus_painter --- it seems that std::move is sufficiently trivial from a conceptual perspective (regardless of what may be going on under the hood, the naming convention to me signals whether something above and beyond assignment happens). I hate that we use different naming conventions for set_background and SetBorder. Always trips me up.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, sadrul@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2506133003/#ps200001 (title: "[ash-md] Allows ToggleButton to have a border and adds focus rectangle (rebase)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:144: void ToggleButton::set_focus_painter(std::unique_ptr<Painter> focus_painter) { On 2016/11/18 17:10:42, Evan Stade wrote: > On 2016/11/18 16:55:35, varkha wrote: > > On 2016/11/18 16:37:30, sadrul wrote: > > > Not sure if std::move is trivial enough to allow this, or whether this > should > > be > > > SetFocusPainter() (although I notice we call View::SetBorder() instead of > > > View::set_border(), so perhaps this should be SetFocusPainter() too) > > > > Done. > > I requested set_focus_painter --- it seems that std::move is sufficiently > trivial from a conceptual perspective (regardless of what may be going on under > the hood, the naming convention to me signals whether something above and beyond > assignment happens). > > I hate that we use different naming conventions for set_background and > SetBorder. Always trips me up. set_background takes a raw-pointer, vs. SetBorder takes a unique_ptr<> It used to be called set_border() too, and take a raw pointer. But it was then changed to SetBorder(unique_ptr<>) later (https://codereview.chromium.org/145033006). No one just converted set_background() to be consistent with that yet.
On 2016/11/18 17:22:46, sadrul wrote: > https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... > File ui/views/controls/button/toggle_button.cc (right): > > https://codereview.chromium.org/2506133003/diff/160001/ui/views/controls/butt... > ui/views/controls/button/toggle_button.cc:144: void > ToggleButton::set_focus_painter(std::unique_ptr<Painter> focus_painter) { > On 2016/11/18 17:10:42, Evan Stade wrote: > > On 2016/11/18 16:55:35, varkha wrote: > > > On 2016/11/18 16:37:30, sadrul wrote: > > > > Not sure if std::move is trivial enough to allow this, or whether this > > should > > > be > > > > SetFocusPainter() (although I notice we call View::SetBorder() instead of > > > > View::set_border(), so perhaps this should be SetFocusPainter() too) > > > > > > Done. > > > > I requested set_focus_painter --- it seems that std::move is sufficiently > > trivial from a conceptual perspective (regardless of what may be going on > under > > the hood, the naming convention to me signals whether something above and > beyond > > assignment happens). > > > > I hate that we use different naming conventions for set_background and > > SetBorder. Always trips me up. > > set_background takes a raw-pointer, vs. SetBorder takes a unique_ptr<> > > It used to be called set_border() too, and take a raw pointer. But it was then > changed to SetBorder(unique_ptr<>) later > (https://codereview.chromium.org/145033006). No one just converted > set_background() to be consistent with that yet. and yet when I write set_background(views::Background::CreateFoo()); vs. SetBorder(views::CreateFooBorder()); it's completely unimportant (and unapparent) to me whether it's a smart pointer or a raw pointer. We try to make smart pointers masquerade as raw pointers as much as possible (e.g. supporting if (my_pointer)) so I'm not sure why this is a significant distinction in this case.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_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 varkha@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.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Allows ToggleButton to have a border and adds focus rectangle - Allows ToggleButton to accept a border set externally. - Allows ToggleButton to have a tooltip that includes ThumbView. - Adds TrayPopupUtils::CreateToggleButton to set up ToggleButtons for use in system menu. - Uses TrayPopupUtils::CreateToggleButton in IME, Bluetooth and Network pages. BUG=652492 TEST=manual Bring focus using Tab to a toggle button in Network detailed page. (focus rectangle appears). Press Space (toggle switches) Press Enter (toggle switches) Tab (focus switches to the next row). ========== to ========== [ash-md] Allows ToggleButton to have a border and adds focus rectangle - Allows ToggleButton to accept a border set externally. - Allows ToggleButton to have a tooltip that includes ThumbView. - Adds TrayPopupUtils::CreateToggleButton to set up ToggleButtons for use in system menu. - Uses TrayPopupUtils::CreateToggleButton in IME, Bluetooth and Network pages. BUG=652492 TEST=manual Bring focus using Tab to a toggle button in Network detailed page. (focus rectangle appears). Press Space (toggle switches) Press Enter (toggle switches) Tab (focus switches to the next row). Committed: https://crrev.com/9bd7f8c886e2b6131646b78d7598d2115894b385 Cr-Commit-Position: refs/heads/master@{#433386} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/9bd7f8c886e2b6131646b78d7598d2115894b385 Cr-Commit-Position: refs/heads/master@{#433386} |
