Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2469663002:
[ash-md] Add on-screen keyboard toggle row in IME menu view. (Closed)
|
Created:
4 years, 1 month ago by Azure Wei Modified:
4 years, 1 month ago Reviewers:
tdanderson CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, kalyank Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ash-md] Add on-screen keyboard toggle row in IME menu view.
1. Add vector icons ime_menu_on_screen_keyboard.*.icon for on-screen keyboard.
2. Add new class MaterialKeyboardStatusRowView in ImeListView to show the keyboard row: a on-screen keyboard, a label and a toggle button.
3. Makes s shown only for MD and sticky when scrolling.
4. Updates the ImeInfoView as simple Label+Label+Image with TriView. And updates the id size.
* Still needs follow-up CL to make MaterialKeyboardStatusRowView has shadow and shown in opt-in IME menu.
BUG=657146, 642385, 652677
TEST=Verified on local build.
Committed: https://crrev.com/974c67a70351cf6b243833b309996046fdcab1f5
Cr-Commit-Position: refs/heads/master@{#430186}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Use TriView. #Patch Set 3 : Use TriView. #
Total comments: 16
Patch Set 4 : Not use ImageButton and LabelButton. #Patch Set 5 : Usage Label and ImageView. #
Total comments: 35
Patch Set 6 : Addressed comments. #
Total comments: 12
Patch Set 7 : Addressed comments. #Patch Set 8 : Update tooltip text of toggle. #Patch Set 9 : Remove use of SizeToFit(). #Patch Set 10 : Remove use of SetMaximumWidth(). #
Messages
Total messages: 34 (16 generated)
Description was changed from ========== Add on-screen keyboard toggle row in IME menu view. BUG=657146 TEST=Verified on local build. ========== to ========== [ash-md] Add on-screen keyboard toggle row in IME menu view. 1. Add vector icons ime_menu_on_screen_keyboard.*.icon for on-screen keyboard. 2. Add new class MaterialKeyboardStatusRowView in ImeListView to show the keyboard row: a on-screen keyboard, a label and a toggle button. 3. Makes s shown only for MD and sticky when scrolling. * Still needs follow-up CL to make MaterialKeyboardStatusRowView has shadow and shown in opt-in IME menu. BUG=657146 TEST=Verified on local build. ==========
azurewei@chromium.org changed reviewers: + tdanderson@chromium.org
Please review this CL. Thanks!
Please see my comments below. https://codereview.chromium.org/2469663002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2469663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_list_view.cc:176: SystemMenuButton* on_screen_keyboard_ = new SystemMenuButton( I checked with Sebastien, and the toggle button is the only thing that should be clickable on this row. It's also the only thing that should have a ripple (which it already has baked-in). The keyboard icon should just be an image view instead of a SystemMenuButton. https://codereview.chromium.org/2469663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_list_view.cc:179: IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD); In terms of accessibility, the a11y string should be on the toggle button itself. I filed crbug.com/661319 to track that work; I'll leave it to you if you want to include that change as part of this CL or defer to a follow-up CL. https://codereview.chromium.org/2469663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_list_view.cc:242: SetLayoutManager(layout); Instead of adding this layout code and introducing the separate class KeyboardButtonView, this seems like a good candidate for the TriView class. TriView was designed to handle the layout of rows in the system menu. You should be able to do something like the following: TriView* tri_view = TrayPopupUtils::CreateDefaultRowView(); AddChildView(tri_view); SetLayoutManager(new views::FillLayout); tri_view->AddView(TriView::Container::START, icon); tri_view->AddView(TriView::Container::CENTER, label); tri_view->AddView(TriView::Container::END, toggle); See tray_item_more.cc in https://chromiumcodereview.appspot.com/2468533002/ as a concrete example of where and how TriView is used. https://codereview.chromium.org/2469663002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): https://codereview.chromium.org/2469663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_list_view.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Regarding the shadow, Evan is currently working on the shadow for the other detailed view title rows, so I filed crbug.com/661330 and assigned it to him. Once he gets the other shadows implemented I assume it will be a small amount of work for him to apply it to the keyboard row.
https://codereview.chromium.org/2469663002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2469663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_list_view.cc:176: SystemMenuButton* on_screen_keyboard_ = new SystemMenuButton( On 2016/11/01 21:42:11, tdanderson wrote: > I checked with Sebastien, and the toggle button is the only thing that should be > clickable on this row. It's also the only thing that should have a ripple (which > it already has baked-in). The keyboard icon should just be an image view instead > of a SystemMenuButton. Updated this as ImageButton. https://codereview.chromium.org/2469663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_list_view.cc:179: IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD); On 2016/11/01 21:42:11, tdanderson wrote: > In terms of accessibility, the a11y string should be on the toggle button > itself. I filed crbug.com/661319 to track that work; I'll leave it to you if you > want to include that change as part of this CL or defer to a follow-up CL. Sets the toggle button's a11y text here. https://codereview.chromium.org/2469663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_list_view.cc:242: SetLayoutManager(layout); On 2016/11/01 21:42:11, tdanderson wrote: > Instead of adding this layout code and introducing the separate class > KeyboardButtonView, this seems like a good candidate for the TriView class. > TriView was designed to handle the layout of rows in the system menu. You should > be able to do something like the following: > > TriView* tri_view = TrayPopupUtils::CreateDefaultRowView(); > AddChildView(tri_view); > SetLayoutManager(new views::FillLayout); > tri_view->AddView(TriView::Container::START, icon); > tri_view->AddView(TriView::Container::CENTER, label); > tri_view->AddView(TriView::Container::END, toggle); > > See tray_item_more.cc in https://chromiumcodereview.appspot.com/2468533002/ as a > concrete example of where and how TriView is used. Thanks for the detailed advice of using TriView. I've updated this as well as the ImeListItemView. Please take another look.
Hi, thanks for making the change to use TriView. I have another set of comments and questions below: https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:100: TriView* tri_view = TrayPopupUtils::CreateDefaultRowView(); Thanks for making use of TriView here, too. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:105: views::LabelButton* id_button = new views::LabelButton(nullptr, id); optional as follow on work in another CL: consider using just a label instead of a LabelButton. The entire row is one click target, so I don't think it needs to be a button. Similarly, |check_button| can probably just be replaced by an image. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:199: gfx::Insets(vertical_padding, horizontal_padding))); For lines 192-198, I don't think this needs to be an ImageButton since it is not clickable. I also think you should be able to re-use some of the layout code in TrayPopupUtils instead of doing the computations yourself. Can you try the following (similar to what is done in TrayItemMore) and see if it works / looks correct? ImageView* image = TrayPopupUtils::CreateMainImageView(); image->SetImage(CreateVectorIcon(...)); tri_view->AddView(TriView::Container::START, image); https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:202: image_button->SetFocusForPlatform(); The image button shouldn't need to have system focus. The only thing that should have focus is the toggle button, which you have done on line 216. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:208: views::Label* label = new views::Label( I think you will also need to use the TrayPopupItemStyle and a call to style.SetupLabel(label) similar to what is done on line 118 in order for this label to have the correct size and font style. In the mocks it looks like it should have the same style as the labels used in the individual rows. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:218: IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD)); This lg for the time being, thanks for adding it. There may need to be a small tweak to this in the near future though depending on the outcome of the discussion in issue crbug.com/652677. If so I will either let you know by filing a separate bug or make the change myself. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:251: MaterialDesignController::IsSystemTrayMenuMaterial()) { Should you also include a check that |material_keyboard_status_view_| hasn't already been created? (i.e., && !material_keyboard_status_view_| ? https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:383: !material_keyboard_statuts_view_->is_toggled()) { Isn't there also a call you need to make when material_keyboard_status_view_->is_toggled() is true?
Description was changed from ========== [ash-md] Add on-screen keyboard toggle row in IME menu view. 1. Add vector icons ime_menu_on_screen_keyboard.*.icon for on-screen keyboard. 2. Add new class MaterialKeyboardStatusRowView in ImeListView to show the keyboard row: a on-screen keyboard, a label and a toggle button. 3. Makes s shown only for MD and sticky when scrolling. * Still needs follow-up CL to make MaterialKeyboardStatusRowView has shadow and shown in opt-in IME menu. BUG=657146 TEST=Verified on local build. ========== to ========== [ash-md] Add on-screen keyboard toggle row in IME menu view. 1. Add vector icons ime_menu_on_screen_keyboard.*.icon for on-screen keyboard. 2. Add new class MaterialKeyboardStatusRowView in ImeListView to show the keyboard row: a on-screen keyboard, a label and a toggle button. 3. Makes s shown only for MD and sticky when scrolling. 4. Updates the ImeInfoView as simple Label+Label+Image with TriView. And updates the id size. * Still needs follow-up CL to make MaterialKeyboardStatusRowView has shadow and shown in opt-in IME menu. BUG=657146, 642385, 652677 TEST=Verified on local build. ==========
Thanks for the review. The comments have been addressed. PTAL. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:105: views::LabelButton* id_button = new views::LabelButton(nullptr, id); On 2016/11/02 18:44:36, tdanderson wrote: > optional as follow on work in another CL: consider using just a label instead of > a LabelButton. The entire row is one click target, so I don't think it needs to > be a button. > > Similarly, |check_button| can probably just be replaced by an image. Done. Updated |id_button | as a Label and |check_button| as a ImageView. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:199: gfx::Insets(vertical_padding, horizontal_padding))); On 2016/11/02 18:44:36, tdanderson wrote: > For lines 192-198, I don't think this needs to be an ImageButton since it is not > clickable. I also think you should be able to re-use some of the layout code in > TrayPopupUtils instead of doing the computations yourself. Can you try the > following (similar to what is done in TrayItemMore) and see if it works / looks > correct? > > ImageView* image = TrayPopupUtils::CreateMainImageView(); > image->SetImage(CreateVectorIcon(...)); > tri_view->AddView(TriView::Container::START, image); Done. Updated as using CreateMainImageView() and it works well. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:202: image_button->SetFocusForPlatform(); On 2016/11/02 18:44:36, tdanderson wrote: > The image button shouldn't need to have system focus. The only thing that should > have focus is the toggle button, which you have done on line 216. Done. Removed this line. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:208: views::Label* label = new views::Label( On 2016/11/02 18:44:36, tdanderson wrote: > I think you will also need to use the TrayPopupItemStyle and a call to > style.SetupLabel(label) similar to what is done on line 118 in order for this > label to have the correct size and font style. In the mocks it looks like it > should have the same style as the labels used in the individual rows. Done. Used TrayPopupItemStyle to update the label's style. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:218: IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD)); On 2016/11/02 18:44:36, tdanderson wrote: > This lg for the time being, thanks for adding it. There may need to be a small > tweak to this in the near future though depending on the outcome of the > discussion in issue crbug.com/652677. If so I will either let you know by filing > a separate bug or make the change myself. Sorry, this should be 'Enable/Disable on-screen keyboard'. Updated. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:251: MaterialDesignController::IsSystemTrayMenuMaterial()) { On 2016/11/02 18:44:36, tdanderson wrote: > Should you also include a check that |material_keyboard_status_view_| hasn't > already been created? (i.e., && !material_keyboard_status_view_| ? Done. Added the check for |material_keyboard_status_view_| in AppendMaterialKeyboardStatus(). https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:383: !material_keyboard_statuts_view_->is_toggled()) { On 2016/11/02 18:44:36, tdanderson wrote: > Isn't there also a call you need to make when > material_keyboard_status_view_->is_toggled() is true? Removed the check for material_keyboard_status_view_->is_toggled(). Actually the toggled should always be on. Otherwise the toggle won't be shown.
Please see my comments below. https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2469663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:383: !material_keyboard_statuts_view_->is_toggled()) { On 2016/11/03 02:06:44, Azure Wei wrote: > On 2016/11/02 18:44:36, tdanderson wrote: > > Isn't there also a call you need to make when > > material_keyboard_status_view_->is_toggled() is true? > > Removed the check for material_keyboard_status_view_->is_toggled(). > Actually the toggled should always be on. Otherwise the toggle won't be shown. I went into non-MD mode on canary, plugged in an external keyboard, and entered touchview (maximize mode). When I go into the IME detailed view I see a "disable on-screen keyboard" button, and when I tap it it turns into an "enable on-screen keyboard" button. So the toggle row needs to be shown regardless of whether the toggle state is on or off. What you have in patch set 5 on lines ~255 and ImeListView::ButtonPressed() looks to be doing the correct thing. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:41: const int kKeyboardRowkVerticalInset = 4; nit: 'kKeyboardRowVerticalInset' instead of 'kKeyboardRowkVerticalInset' (no extra k) https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:43: // const int kFocusBorderInset = 1; Don't forget to delete this line if it's no longer needed. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:44: const int kMaxReduceFontSize = -10; nit: I would instead call this something with 'min' in it instead of 'max' for clarity. Consider "kMinFontSizeDelta" and add a brief explanation. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:112: int size_deta = -1; nit: 'size_delta' instead of 'size_deta' https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:113: while (id_label->GetPreferredSize().width() > kMenuIconSize) { nit: please include a comment to explain what you're doing here, e.g., "Shrink the font size until it fits within the bounds" https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:117: if (size_deta < kMaxReduceFontSize) Instead of this break, can you instead just put it in the while, i.e., while (width > kMenuIconSize && size_delta >= kMaxReduceFontSize) { --size_delta; } ? https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:158: : views::View(), listener_(listener), toggle_(nullptr) { I don't know if you need the call up to views::View(). https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:173: ? l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_DISABLE_KEYBOARD) This is fine to land, but FYI we will eventually want to change this to "on-screen keyboard, on switch" and "on-screen keyboard, off switch" (see discussion about toggle button a11y text in crbug.com/652677). https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:210: TrayPopupItemStyle style( Sorry, I should have mentioned something in a previous review cycle. Take a look at the class-level documentation of TrayPoupupItemStyle. The correct way to use it is to make |label| a member variable, then move lines 210-212 into a private helper function UpdateStyle(). Then have this class override OnNativeThemeChanged(), which should call UpdateStyle(). Same comment for lines 124-126. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:219: toggle_->SetTooltipText( I think you want to call into SetKeyboardToggleText() here instead. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:247: keyboard_status_ = nullptr; Can you clarify why you are setting these to null in the destructor? https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:279: AppendKeyboardStatus(); Can you add a DCHECK at the top of AppendKeyboardStatus() to check !IsSystemTrayMenuMaterial()? https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:288: material_keyboard_statuts_view_ = nullptr; optional nit: When I first saw this it seemed as though you were nulling out these two members but not actually freeing the memory anywhere. But once I followed Reset() I see that it calls RemoveAllChildViews(true) which will free the memory. Consider adding a comment here saying something like "Note that the children are removed from the view hierarchy and deleted in Reset()." https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:359: void ImeListView::AppendMaterialKeyboardStatus() { Can you add a DCHECK at the top of this function to check that IsSystemTrayMenuMaterial() ? https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.h:42: class MaterialKeyboardStatusRowView; I don't think this should be necessary? https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.h:71: views::View* keyboard_status_; Can you please add a comment above this to say "Not used in material design"? https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.h:72: MaterialKeyboardStatusRowView* material_keyboard_statuts_view_; Similarly please add a comment above this member to say "Only used in material design"
Please review the latest patch set. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:41: const int kKeyboardRowkVerticalInset = 4; On 2016/11/03 21:17:06, tdanderson wrote: > nit: 'kKeyboardRowVerticalInset' instead of 'kKeyboardRowkVerticalInset' (no > extra k) Done. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:43: // const int kFocusBorderInset = 1; On 2016/11/03 21:17:06, tdanderson wrote: > Don't forget to delete this line if it's no longer needed. Done. Sorry about the silly mistake. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:44: const int kMaxReduceFontSize = -10; On 2016/11/03 21:17:05, tdanderson wrote: > nit: I would instead call this something with 'min' in it instead of 'max' for > clarity. Consider "kMinFontSizeDelta" and add a brief explanation. Done. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:112: int size_deta = -1; On 2016/11/03 21:17:05, tdanderson wrote: > nit: 'size_delta' instead of 'size_deta' Done. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:113: while (id_label->GetPreferredSize().width() > kMenuIconSize) { On 2016/11/03 21:17:05, tdanderson wrote: > nit: please include a comment to explain what you're doing here, e.g., "Shrink > the font size until it fits within the bounds" Done. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:117: if (size_deta < kMaxReduceFontSize) On 2016/11/03 21:17:06, tdanderson wrote: > Instead of this break, can you instead just put it in the while, i.e., > > while (width > kMenuIconSize && size_delta >= kMaxReduceFontSize) { > --size_delta; > } > > ? Done. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:158: : views::View(), listener_(listener), toggle_(nullptr) { On 2016/11/03 21:17:06, tdanderson wrote: > I don't know if you need the call up to views::View(). Should be unnecessary. Removed. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:173: ? l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_DISABLE_KEYBOARD) On 2016/11/03 21:17:06, tdanderson wrote: > This is fine to land, but FYI we will eventually want to change this to > "on-screen keyboard, on switch" and "on-screen keyboard, off switch" (see > discussion about toggle button a11y text in crbug.com/652677). The 'on/off switch' will be automatically appended to ToggleButton items, right? I've added a TODO here. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:210: TrayPopupItemStyle style( On 2016/11/03 21:17:05, tdanderson wrote: > Sorry, I should have mentioned something in a previous review cycle. Take a look > at the class-level documentation of TrayPoupupItemStyle. The correct way to use > it is to make |label| a member variable, then move lines 210-212 into a private > helper function UpdateStyle(). Then have this class override > OnNativeThemeChanged(), which should call UpdateStyle(). > > Same comment for lines 124-126. Updated this and lines 124-126 by using UpdateStyle(). https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:219: toggle_->SetTooltipText( On 2016/11/03 21:17:05, tdanderson wrote: > I think you want to call into SetKeyboardToggleText() here instead. Done. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:247: keyboard_status_ = nullptr; On 2016/11/03 21:17:06, tdanderson wrote: > Can you clarify why you are setting these to null in the destructor? Nulling the two items are only needed in ResetImeListView(). Reverted these unnecessary ops. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:279: AppendKeyboardStatus(); On 2016/11/03 21:17:05, tdanderson wrote: > Can you add a DCHECK at the top of AppendKeyboardStatus() to check > !IsSystemTrayMenuMaterial()? Done. The previous change should be bug. Shouldn't add check for MD before adding scroll separator. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:288: material_keyboard_statuts_view_ = nullptr; On 2016/11/03 21:17:05, tdanderson wrote: > optional nit: When I first saw this it seemed as though you were nulling out > these two members but not actually freeing the memory anywhere. But once I > followed Reset() I see that it calls RemoveAllChildViews(true) which will free > the memory. Consider adding a comment here saying something like "Note that the > children are removed from the view hierarchy and deleted in Reset()." Done. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:359: void ImeListView::AppendMaterialKeyboardStatus() { On 2016/11/03 21:17:05, tdanderson wrote: > Can you add a DCHECK at the top of this function to check that > IsSystemTrayMenuMaterial() ? Done. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.h:42: class MaterialKeyboardStatusRowView; On 2016/11/03 21:17:06, tdanderson wrote: > I don't think this should be necessary? Removed. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.h:71: views::View* keyboard_status_; On 2016/11/03 21:17:06, tdanderson wrote: > Can you please add a comment above this to say "Not used in material design"? Done. https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.h:72: MaterialKeyboardStatusRowView* material_keyboard_statuts_view_; On 2016/11/03 21:17:06, tdanderson wrote: > Similarly please add a comment above this member to say "Only used in material > design" Done.
Thanks for all the changes! LGTM with a last few comments below: https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2469663002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:173: ? l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_DISABLE_KEYBOARD) On 2016/11/04 01:28:34, Azure Wei wrote: > On 2016/11/03 21:17:06, tdanderson wrote: > > This is fine to land, but FYI we will eventually want to change this to > > "on-screen keyboard, on switch" and "on-screen keyboard, off switch" (see > > discussion about toggle button a11y text in crbug.com/652677). > > The 'on/off switch' will be automatically appended to ToggleButton items, right? > I've added a TODO here. Yes, that's right. The change actually just landed today (https://codereview.chromium.org/2472303002). Thanks for adding the TODO. https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.cc:114: // or "...". So we shrink the font size until it fits within the bounds. Nice comment. https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.cc:119: // In case we don't run into endless loop Since you changed the loop, I think you can remove this comment now. https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.cc:186: // be updated as 'on/off switch'. (crbug.com/652677). nit: When you make this change, you should be able to remove the helper function SetKeyboardToggleText() and just set the text once during Init(). It won't need to change when the toggle changes. (you can also remove line 423) https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.cc:187: toggle_->SetTooltipText( We've decided that we're not going to have tooltip text on toggle buttons, but of course we still need to have accessible text. So here instead of calling SetTooltipText(), please call SetAccessibleName(). This sets the spoken feedback but no tooltip. https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.cc:241: void UpdateStyle() { Thanks for changing this. I'm 99% sure this is the right thing to do as compared to what you had in the last patch set, but did you happen to double-check that the font style/size still looks correct? https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.cc:301: DCHECK(!MaterialDesignController::IsSystemTrayMenuMaterial()); I think you will want to move this to the top of AppendKeyboardStatus(), and line 298 should be changed back to be: if (show_keyboard_toggle && !MDC::IsSystemTrayMenuMaterial()) Otherwise with the code as-is you could actually append both rows. https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.h:65: // Appends the meterial on-screen keyboard status to the top area of the nit: "material"
https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.cc:119: // In case we don't run into endless loop On 2016/11/04 19:01:04, tdanderson wrote: > Since you changed the loop, I think you can remove this comment now. Done. https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.cc:187: toggle_->SetTooltipText( On 2016/11/04 19:01:04, tdanderson wrote: > We've decided that we're not going to have tooltip text on toggle buttons, but > of course we still need to have accessible text. So here instead of calling > SetTooltipText(), please call SetAccessibleName(). This sets the spoken feedback > but no tooltip. Done. https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.cc:241: void UpdateStyle() { On 2016/11/04 19:01:05, tdanderson wrote: > Thanks for changing this. I'm 99% sure this is the right thing to do as compared > to what you had in the last patch set, but did you happen to double-check that > the font style/size still looks correct? Added calling of UpdateStyle() in Init() to make sure this will be run. https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.cc:301: DCHECK(!MaterialDesignController::IsSystemTrayMenuMaterial()); On 2016/11/04 19:01:05, tdanderson wrote: > I think you will want to move this to the top of AppendKeyboardStatus(), and > line 298 should be changed back to be: > > if (show_keyboard_toggle && !MDC::IsSystemTrayMenuMaterial()) > > Otherwise with the code as-is you could actually append both rows. Sorry, I've got wrong about the scroll separator. There should be no separator if no keyboard toggle. https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): https://codereview.chromium.org/2469663002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/ime_menu/ime_list_view.h:65: // Appends the meterial on-screen keyboard status to the top area of the On 2016/11/04 19:01:05, tdanderson wrote: > nit: "material" Done.
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2469663002/#ps120001 (title: "Addressed comments.")
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: linux_chromium_chromeos_ozone_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 azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2469663002/#ps140001 (title: "Update tooltip text of toggle.")
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: linux_chromium_chromeos_ozone_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 azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2469663002/#ps160001 (title: "Remove use of SizeToFit().")
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: linux_chromium_chromeos_ozone_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 azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2469663002/#ps180001 (title: "Remove use of SetMaximumWidth().")
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 ========== [ash-md] Add on-screen keyboard toggle row in IME menu view. 1. Add vector icons ime_menu_on_screen_keyboard.*.icon for on-screen keyboard. 2. Add new class MaterialKeyboardStatusRowView in ImeListView to show the keyboard row: a on-screen keyboard, a label and a toggle button. 3. Makes s shown only for MD and sticky when scrolling. 4. Updates the ImeInfoView as simple Label+Label+Image with TriView. And updates the id size. * Still needs follow-up CL to make MaterialKeyboardStatusRowView has shadow and shown in opt-in IME menu. BUG=657146, 642385, 652677 TEST=Verified on local build. ========== to ========== [ash-md] Add on-screen keyboard toggle row in IME menu view. 1. Add vector icons ime_menu_on_screen_keyboard.*.icon for on-screen keyboard. 2. Add new class MaterialKeyboardStatusRowView in ImeListView to show the keyboard row: a on-screen keyboard, a label and a toggle button. 3. Makes s shown only for MD and sticky when scrolling. 4. Updates the ImeInfoView as simple Label+Label+Image with TriView. And updates the id size. * Still needs follow-up CL to make MaterialKeyboardStatusRowView has shadow and shown in opt-in IME menu. BUG=657146, 642385, 652677 TEST=Verified on local build. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Add on-screen keyboard toggle row in IME menu view. 1. Add vector icons ime_menu_on_screen_keyboard.*.icon for on-screen keyboard. 2. Add new class MaterialKeyboardStatusRowView in ImeListView to show the keyboard row: a on-screen keyboard, a label and a toggle button. 3. Makes s shown only for MD and sticky when scrolling. 4. Updates the ImeInfoView as simple Label+Label+Image with TriView. And updates the id size. * Still needs follow-up CL to make MaterialKeyboardStatusRowView has shadow and shown in opt-in IME menu. BUG=657146, 642385, 652677 TEST=Verified on local build. ========== to ========== [ash-md] Add on-screen keyboard toggle row in IME menu view. 1. Add vector icons ime_menu_on_screen_keyboard.*.icon for on-screen keyboard. 2. Add new class MaterialKeyboardStatusRowView in ImeListView to show the keyboard row: a on-screen keyboard, a label and a toggle button. 3. Makes s shown only for MD and sticky when scrolling. 4. Updates the ImeInfoView as simple Label+Label+Image with TriView. And updates the id size. * Still needs follow-up CL to make MaterialKeyboardStatusRowView has shadow and shown in opt-in IME menu. BUG=657146, 642385, 652677 TEST=Verified on local build. Committed: https://crrev.com/974c67a70351cf6b243833b309996046fdcab1f5 Cr-Commit-Position: refs/heads/master@{#430186} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/974c67a70351cf6b243833b309996046fdcab1f5 Cr-Commit-Position: refs/heads/master@{#430186} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
