Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
433483002:
Fix padding of a11y keyboard indicator when shelf is vertically aligned. (Closed)
|
Created:
6 years, 4 months ago by kevers Modified:
6 years, 4 months ago CC:
chromium-reviews, sadrul, stevenjb+watch_chromium.org, ben+ash_chromium.org, oshima+watch_chromium.org, kalyank Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix padding of a11y keyboard indicator when shelf is vertically aligned.
BUG=384453
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287771
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix formatting. #
Total comments: 2
Patch Set 3 : Cleanup button sizing. #
Total comments: 2
Patch Set 4 : Remove padding on tray container #
Total comments: 1
Messages
Total messages: 19 (0 generated)
Please take a look at this CL.
On 2014/07/30 15:20:52, kevers wrote: > Please take a look at this CL. lgtm
https://codereview.chromium.org/433483002/diff/1/ash/system/chromeos/virtual_... File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/433483002/diff/1/ash/system/chromeos/virtual_... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:48: Shelf *shelf = Shelf::ForPrimaryDisplay(); Shelf* shelf = ... https://codereview.chromium.org/433483002/diff/1/ash/system/chromeos/virtual_... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:49: bool isHorizontal = shelf && (shelf->alignment() == SHELF_ALIGNMENT_BOTTOM || is_horizontal https://codereview.chromium.org/433483002/diff/1/ash/system/chromeos/virtual_... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:54: size.set_width(size.width() + padding); Why do we add this padding?
https://codereview.chromium.org/433483002/diff/1/ash/system/chromeos/virtual_... File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/433483002/diff/1/ash/system/chromeos/virtual_... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:48: Shelf *shelf = Shelf::ForPrimaryDisplay(); On 2014/07/31 21:18:52, sadrul wrote: > Shelf* shelf = ... Done. https://codereview.chromium.org/433483002/diff/1/ash/system/chromeos/virtual_... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:49: bool isHorizontal = shelf && (shelf->alignment() == SHELF_ALIGNMENT_BOTTOM || On 2014/07/31 21:18:52, sadrul wrote: > is_horizontal Done. https://codereview.chromium.org/433483002/diff/1/ash/system/chromeos/virtual_... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:54: size.set_width(size.width() + padding); On 2014/07/31 21:18:52, sadrul wrote: > Why do we add this padding? The a11y button is detached form the remaining controls on the right hand side (horizontally aligned) or bottom (vertically aligned) of the shelf. It is a separate control for displaying the virtual keyboard in locked mode independent of input focus, providing the ability to type hot-key combinations. The padding is for aesthetics to look consistent with other controls. When vertically aligned, the padding causes the button to be too wide for the shelf.
https://codereview.chromium.org/433483002/diff/20001/ash/system/chromeos/virt... File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/433483002/diff/20001/ash/system/chromeos/virt... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:68: views::ImageButton::ALIGN_MIDDLE); As discussed offline: you should be able to do: button_ = new views::ImageButton(this); button_->SetBorder(views::Border::CreateEmptyBorder(<top>, <left> ...)); button_->SetImage(....); ... And remove the VirtualKeyboardButton class. You would also want to SetBorder with the updated paddings in SetShelfAlignment below.
https://codereview.chromium.org/433483002/diff/20001/ash/system/chromeos/virt... File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/433483002/diff/20001/ash/system/chromeos/virt... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:68: views::ImageButton::ALIGN_MIDDLE); On 2014/08/01 15:29:17, sadrul wrote: > As discussed offline: you should be able to do: > > button_ = new views::ImageButton(this); > button_->SetBorder(views::Border::CreateEmptyBorder(<top>, <left> ...)); > button_->SetImage(....); > ... > > And remove the VirtualKeyboardButton class. > > You would also want to SetBorder with the updated paddings in SetShelfAlignment > below. Done.
https://codereview.chromium.org/433483002/diff/40001/ash/system/chromeos/virt... File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/433483002/diff/40001/ash/system/chromeos/virt... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:74: tray_container()->SetBorder(views::Border::CreateEmptyBorder(0, I think you want to SetBorder on button_, and not on tray_container()?
https://codereview.chromium.org/433483002/diff/40001/ash/system/chromeos/virt... File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/433483002/diff/40001/ash/system/chromeos/virt... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:74: tray_container()->SetBorder(views::Border::CreateEmptyBorder(0, On 2014/08/01 19:49:49, sadrul wrote: > I think you want to SetBorder on button_, and not on tray_container()? Done.
https://codereview.chromium.org/433483002/diff/60001/ash/system/chromeos/virt... File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/433483002/diff/60001/ash/system/chromeos/virt... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:78: right_padding)); I assume SetShelfAlignment() is called immediately after creation, and that's why you don't need to set this border in the constructor? Assuming this is right, LGTM. Otherwise, you would want to do this in the constructor too, right?
https://codereview.chromium.org/433483002/diff/60001/ash/system/chromeos/virt... File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/433483002/diff/60001/ash/system/chromeos/virt... ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:78: right_padding)); I assume SetShelfAlignment() is called immediately after creation, and that's why you don't need to set this border in the constructor? Assuming this is right, LGTM. Otherwise, you would want to do this in the constructor too, right?
On 2014/08/05 14:53:30, sadrul wrote: > https://codereview.chromium.org/433483002/diff/60001/ash/system/chromeos/virt... > File ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): > > https://codereview.chromium.org/433483002/diff/60001/ash/system/chromeos/virt... > ash/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:78: > right_padding)); > I assume SetShelfAlignment() is called immediately after creation, and that's > why you don't need to set this border in the constructor? Assuming this is > right, LGTM. Otherwise, you would want to do this in the constructor too, right? Yes, SetShelfAlignment is being called.
The CQ bit was checked by kevers@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kevers@chromium.org/433483002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42617)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42629)
The CQ bit was checked by kevers@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kevers@chromium.org/433483002/60001
Message was sent while issue was closed.
Change committed as 287771 |
