Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2343603003:
[Chrome OS MD] Implement accessibility detailed view for the MD Ash system menu (Closed)
|
Created:
4 years, 3 months ago by yiyix Modified:
4 years, 3 months ago CC:
chromium-reviews, sadrul, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, kalyank Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement accessibility detailed view for the MD Ash system menu
Introduced a new layout for accessibility detailed view. Each row now
contains a vectorized icon which is shown on left, a description which
is shown in the middle and a check mark is shown on the right. The row
on the bottom which contains "setting" option and "learn more" option
is removed and it will be included in the title row in the future cls.
This CL also updates the description of ChromeVox from "ChromeVox (Spoken
feedback)" to "ChromeVox (spoken feedback)"
TEST=tray_accessibility_browsertest
BUG=632102
Committed: https://crrev.com/3fd37d926efd712c690acee0bafbf07fd27d519e
Cr-Commit-Position: refs/heads/master@{#420720}
Patch Set 1 : fix #
Total comments: 22
Patch Set 2 : address comments #
Total comments: 30
Patch Set 3 : address comments #
Total comments: 7
Patch Set 4 : merge #
Total comments: 2
Patch Set 5 : address comments #Patch Set 6 : merge #Patch Set 7 : ' #Messages
Total messages: 65 (44 generated)
The CQ bit was checked by yiyix@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 yiyix@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.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== 1st attempt BUG= ========== to ========== Implement accessibility detailed view for the material design Ash system menu BUG=632102 ==========
Description was changed from ========== Implement accessibility detailed view for the material design Ash system menu BUG=632102 ========== to ========== Implement accessibility detailed view for the MD Ash system menu Introduced a new layout for accessibility detailed view. Each row now contains a vectorized icon which is shown on left, a description which is shown in the middle and a check mark is shown on the right. The row on the bottom which contains "setting" option and "learn more" option is removed and it will be included in the title row in the future cls. TEST=tray_accessibility_browsertest BUG=632102 ==========
yiyix@chromium.org changed reviewers: + tdanderson@chromium.org
Description was changed from ========== Implement accessibility detailed view for the MD Ash system menu Introduced a new layout for accessibility detailed view. Each row now contains a vectorized icon which is shown on left, a description which is shown in the middle and a check mark is shown on the right. The row on the bottom which contains "setting" option and "learn more" option is removed and it will be included in the title row in the future cls. TEST=tray_accessibility_browsertest BUG=632102 ========== to ========== Implement accessibility detailed view for the MD Ash system menu Introduced a new layout for accessibility detailed view. Each row now contains a vectorized icon which is shown on left, a description which is shown in the middle and a check mark is shown on the right. The row on the bottom which contains "setting" option and "learn more" option is removed and it will be included in the title row in the future cls. This CL also updates the description of ChromeVox from "ChromeVox (Spoken feedback)" to "ChromeVox (spoken feedback)" TEST=tray_accessibility_browsertest BUG=632102 ==========
@tdanderson, could you please take a first look? Thank you.
Yi, please see my comments below. https://chromiumcodereview.appspot.com/2343603003/diff/60001/ash/common/syste... File ash/common/system/tray/tray_constants.h (right): https://chromiumcodereview.appspot.com/2343603003/diff/60001/ash/common/syste... ash/common/system/tray/tray_constants.h:104: extern const int kIconPadding; Perhaps introduce a constant for the size of the buttons in the MD system menu (say, kMenuButtonSize = 48) and then deriving the padding value using kMenuButtonSize and the size of the icon. Note also that you're repeating some constants introduced in common_palette_tool. Now would be a good opportunity to have common_palette_tool and tray_accessibility both use the same set of constants. https://chromiumcodereview.appspot.com/2343603003/diff/60001/ash/common/syste... File ash/common/system/tray_accessibility.cc (right): https://chromiumcodereview.appspot.com/2343603003/diff/60001/ash/common/syste... ash/common/system/tray_accessibility.cc:160: AppendHelpEntries(); This initializes two members that are now non-MD-only. Can you make a comment above these members in the .h? Also, you don't want to compare against these members in HandleButtonPressed() if you're in MD, so an early return should be added there too. https://chromiumcodereview.appspot.com/2343603003/diff/60001/ash/common/syste... ash/common/system/tray_accessibility.cc:179: if (ash::MaterialDesignController::IsSystemTrayMenuMaterial()) { This feels like a lot of repeated code. Can you instead just check for MD or non-MD in AddScrollListItem() rather than introducing an AddScrollListItemMD() ? https://chromiumcodereview.appspot.com/2343603003/diff/60001/ash/common/syste... ash/common/system/tray_accessibility.cc:188: kMenuIconSize, kMenuIconColor); Since the icon size is specified in the .icon files themselves, there is no need to pass in kMenuIconSize. https://chromiumcodereview.appspot.com/2343603003/diff/60001/ash/common/syste... ash/common/system/tray_accessibility.cc:316: image, text, highlight, kMenuIconSize + kMenuIconMargin + kMenuIconMargin, Prefer using kMenuIconSize + 2 * kMenuIconMargin in this situation. https://chromiumcodereview.appspot.com/2343603003/diff/60001/ash/common/syste... ash/common/system/tray_accessibility.cc:320: container->AddRightIcon(check, kMenuIconSize); I don't think AddRightIcon() actually needs to have the icon size passed in as a parameter, it can just derive the size from the ImageSkia. https://chromiumcodereview.appspot.com/2343603003/diff/60001/chrome/browser/c... File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right): https://chromiumcodereview.appspot.com/2343603003/diff/60001/chrome/browser/c... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:873: if (!ash::MaterialDesignController::IsSystemTrayMenuMaterial()) { nit (possibly personal preference): in an if-else situation, the code reads easier if you avoid ! in the boolean expression. So: if (IsSystemTrayMenuMaterial()) { ... } else { ... } here and at line 891. https://chromiumcodereview.appspot.com/2343603003/diff/60001/chrome/browser/c... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:895: EXPECT_FALSE(IsHelpShownOnDetailMenu()); EXPECT_TRUE? https://chromiumcodereview.appspot.com/2343603003/diff/60001/ui/gfx/vector_ic... File ui/gfx/vector_icons/check_circle.icon (right): https://chromiumcodereview.appspot.com/2343603003/diff/60001/ui/gfx/vector_ic... ui/gfx/vector_icons/check_circle.icon:5: CANVAS_DIMENSIONS, 40, The appearance of the checkmark icon used by the palette tool will be improved by this change. However note that the throbber is also using VectorIconId::CHECK_CIRCLE, rendering it at 18dp. See the comment "This magic number matches the default diamater plus padding inherent in the checkmark SVG." - since you're now using a different SVG with a different pixel grid, you'll need to make sure that the checkmark in the throbber still looks good. (You can do this by building and running the views_examples_with_content_exe target; it looks like there is a throbber example you can use.) The solution may be to tweak the value of 18 used by the throbber, or it may be that we'll need to keep ui/gfx/vector_icons/check_circle.icon as-is and have a separate ash/resources/vector_icons/system_menu_check_circle.icon used by the system menu and palette tool.
I took a look at contrast: https://chromiumcodereview.appspot.com/2343603003/diff/60001/ash/resources/ve... File ash/resources/vector_icons/system_menu_accessibility_contrast.icon (right): https://chromiumcodereview.appspot.com/2343603003/diff/60001/ash/resources/ve... ash/resources/vector_icons/system_menu_accessibility_contrast.icon:6: MOVE_TO, 0, 0, Looks like fill-inverting path is actually hiding at the bottom of the file, so just remove it and you won't have to add your own (ditto for .1x version).
varkha@chromium.org changed reviewers: + varkha@chromium.org
https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray_... File ash/common/system/tray_accessibility.cc (right): https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:329: WmShell::Get()->RecordUserMetricsAction( While here, can you cache WmShell::Get() or even better make it so that there is only one call to RecordUserMetricsAction() for an ID that depends on which view is clicked.
@tdanderson and @varkha, could you please take another look? https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray/... File ash/common/system/tray/tray_constants.h (right): https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray/... ash/common/system/tray/tray_constants.h:104: extern const int kIconPadding; On 2016/09/15 16:13:26, tdanderson wrote: > Perhaps introduce a constant for the size of the buttons in the MD system menu > (say, kMenuButtonSize = 48) and then deriving the padding value using > kMenuButtonSize and the size of the icon. > > Note also that you're repeating some constants introduced in > common_palette_tool. Now would be a good opportunity to have common_palette_tool > and tray_accessibility both use the same set of constants. I like the idea of using 48, as the design changes in the future, we don't need to update this constant. Do you mean that I have introduced kMenuIconMargin again in this file because the variable kExtraMarginFromLeftEdge = 4 is also introduced in common_palette_tool? I can make common_palette_tool to use this one instead. I think this variable will be used in other detailed view rows as well. I also found that kIconSize is introduced in both common_palette_tool and palette_tray and which again defined as kMenuIconSize in tray_constants.i can unify that variable too. https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray_... File ash/common/system/tray_accessibility.cc (right): https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:160: AppendHelpEntries(); On 2016/09/15 16:13:26, tdanderson wrote: > This initializes two members that are now non-MD-only. Can you make a comment > above these members in the .h? > > Also, you don't want to compare against these members in HandleButtonPressed() > if you're in MD, so an early return should be added there too. Good call. https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:179: if (ash::MaterialDesignController::IsSystemTrayMenuMaterial()) { On 2016/09/15 16:13:26, tdanderson wrote: > This feels like a lot of repeated code. Can you instead just check for MD or > non-MD in AddScrollListItem() rather than introducing an AddScrollListItemMD() ? Done. https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:188: kMenuIconSize, kMenuIconColor); On 2016/09/15 16:13:26, tdanderson wrote: > Since the icon size is specified in the .icon files themselves, there is no need > to pass in kMenuIconSize. Done. https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:316: image, text, highlight, kMenuIconSize + kMenuIconMargin + kMenuIconMargin, On 2016/09/15 16:13:26, tdanderson wrote: > Prefer using kMenuIconSize + 2 * kMenuIconMargin in this situation. Done. https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:320: container->AddRightIcon(check, kMenuIconSize); On 2016/09/15 16:13:26, tdanderson wrote: > I don't think AddRightIcon() actually needs to have the icon size passed in as a > parameter, it can just derive the size from the ImageSkia. Done. https://codereview.chromium.org/2343603003/diff/60001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:329: WmShell::Get()->RecordUserMetricsAction( On 2016/09/16 17:46:13, varkha wrote: > While here, can you cache WmShell::Get() or even better make it so that there is > only one call to RecordUserMetricsAction() for an ID that depends on which view > is clicked. Done. https://codereview.chromium.org/2343603003/diff/60001/ash/resources/vector_ic... File ash/resources/vector_icons/system_menu_accessibility_contrast.icon (right): https://codereview.chromium.org/2343603003/diff/60001/ash/resources/vector_ic... ash/resources/vector_icons/system_menu_accessibility_contrast.icon:6: MOVE_TO, 0, 0, On 2016/09/15 20:17:59, tdanderson wrote: > Looks like fill-inverting path is actually hiding at the bottom of the file, so > just remove it and you won't have to add your own (ditto for .1x version). Done. https://codereview.chromium.org/2343603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right): https://codereview.chromium.org/2343603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:873: if (!ash::MaterialDesignController::IsSystemTrayMenuMaterial()) { On 2016/09/15 16:13:26, tdanderson wrote: > nit (possibly personal preference): in an if-else situation, the code reads > easier if you avoid ! in the boolean expression. So: > > if (IsSystemTrayMenuMaterial()) { > ... > } else { > ... > } > > here and at line 891. Done. https://codereview.chromium.org/2343603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:895: EXPECT_FALSE(IsHelpShownOnDetailMenu()); On 2016/09/15 16:13:26, tdanderson wrote: > EXPECT_TRUE? Done. https://codereview.chromium.org/2343603003/diff/60001/ui/gfx/vector_icons/che... File ui/gfx/vector_icons/check_circle.icon (right): https://codereview.chromium.org/2343603003/diff/60001/ui/gfx/vector_icons/che... ui/gfx/vector_icons/check_circle.icon:5: CANVAS_DIMENSIONS, 40, On 2016/09/15 16:13:26, tdanderson wrote: > The appearance of the checkmark icon used by the palette tool will be improved > by this change. > > However note that the throbber is also using VectorIconId::CHECK_CIRCLE, > rendering it at 18dp. See the comment "This magic number matches the default > diamater plus padding inherent in the checkmark SVG." - since you're now using a > different SVG with a different pixel grid, you'll need to make sure that the > checkmark in the throbber still looks good. (You can do this by building and > running the views_examples_with_content_exe target; it looks like there is a > throbber example you can use.) > > The solution may be to tweak the value of 18 used by the throbber, or it may be > that we'll need to keep ui/gfx/vector_icons/check_circle.icon as-is and have a > separate ash/resources/vector_icons/system_menu_check_circle.icon used by the > system menu and palette tool. Done.
Looking good! Please take a look at my latest round of comments. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... File ash/common/system/chromeos/palette/common_palette_tool.cc (left): https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/chromeos/palette/common_palette_tool.cc:35: const int kMarginBetweenIconAndText = 18; Awesome!!! https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... File ash/common/system/chromeos/palette/common_palette_tool.cc (right): https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/chromeos/palette/common_palette_tool.cc:97: const int margin_from_edge = (kMenuButtonSize - kMenuIconSize) / 2; nit: Consider renaming to |interior_button_padding| or similar to make it more clear what the variable represents. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... File ash/common/system/tray/tray_constants.h (right): https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray/tray_constants.h:100: // The margin between the menu and the icon in system menu. I would suggest wording as "The margin between the edge of the menu and the edge of the button" or similar since the spacing from the menu edge to the icon itself would actually be 4 + 14. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray/tray_constants.h:103: // The space reserved to display menu icons in detailed view page. Suggested wording: "The size of buttons in the system menu." https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... File ash/common/system/tray_accessibility.cc (right): https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray_accessibility.cc:29: #include "ui/gfx/vector_icon_types.h" I don't think you need this #include (#including paint_vector_icon should be sufficient?) https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray_accessibility.cc:158: if (!ash::MaterialDesignController::IsSystemTrayMenuMaterial()) nit: ash:: not needed (here and elsewhere in this CL). Originally I suggested adding ash:: before MaterialDesignController to avoid confusing it with the top chrome MDC, but that is in the process of being killed off anyway. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray_accessibility.cc:276: UserMetricsAction user_action; Nice. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray_accessibility.cc:308: return; An early return at the top of the function would be preferable. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray_accessibility.cc:315: // Since this row is not initialized in MD, this function should be called for I suggest removing this comment, since in the very near future you/someone will be using this for the MD "info" button. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray_accessibility.cc:317: if (!MaterialDesignController::UseMaterialDesignSystemIcons()) { To avoid indenting the whole function, it would be better to use an early return instead. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... File ash/common/system/tray_accessibility.h (right): https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray_accessibility.h:58: // Create the detailed view of accessibility tray. Note that the help option Generally a comment directly above a constructor should be about something specific to that constructor. Consider removing the first sentence and, optionally, adding class-level documentation (above line 55). As for the second sentence, I think a better place for it would be above AppendHelpEntries() on line 73. You could just add "Only used for non-MD" to the comment that's already there. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray_accessibility.h:76: // Helper function to create entries in the detailed accessibility view. nit: Maybe mention that the |icon| parameter is only used for MD. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ui/gfx/vector_ic... File ui/gfx/vector_icons/check_circle.1x.icon (right): https://chromiumcodereview.appspot.com/2343603003/diff/80001/ui/gfx/vector_ic... ui/gfx/vector_icons/check_circle.1x.icon:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: update year. https://chromiumcodereview.appspot.com/2343603003/diff/80001/ui/views/example... File ui/views/examples/throbber_example.cc (right): https://chromiumcodereview.appspot.com/2343603003/diff/80001/ui/views/example... ui/views/examples/throbber_example.cc:24: // View implementation. nit: Just "// View:" https://chromiumcodereview.appspot.com/2343603003/diff/80001/ui/views/example... ui/views/examples/throbber_example.cc:46: is_checked_ = true; You can avoid duplicated code with: throbber->SetChecked(!is_checked_); is_checked_ = !is_checked_;
Could you please take another look? Thanks https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/common_palette_tool.cc (left): https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/common_palette_tool.cc:35: const int kMarginBetweenIconAndText = 18; On 2016/09/19 23:32:27, tdanderson wrote: > Awesome!!! Thanks~! https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/common_palette_tool.cc (right): https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/common_palette_tool.cc:97: const int margin_from_edge = (kMenuButtonSize - kMenuIconSize) / 2; On 2016/09/19 23:32:27, tdanderson wrote: > nit: Consider renaming to |interior_button_padding| or similar to make it more > clear what the variable represents. Done. https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray/... File ash/common/system/tray/tray_constants.h (right): https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray/... ash/common/system/tray/tray_constants.h:100: // The margin between the menu and the icon in system menu. On 2016/09/19 23:32:27, tdanderson wrote: > I would suggest wording as "The margin between the edge of the menu and the edge > of the button" or similar since the spacing from the menu edge to the icon > itself would actually be 4 + 14. Done. https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray/... ash/common/system/tray/tray_constants.h:103: // The space reserved to display menu icons in detailed view page. On 2016/09/19 23:32:27, tdanderson wrote: > Suggested wording: "The size of buttons in the system menu." Done. https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray_... File ash/common/system/tray_accessibility.cc (right): https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:29: #include "ui/gfx/vector_icon_types.h" On 2016/09/19 23:32:27, tdanderson wrote: > I don't think you need this #include (#including paint_vector_icon should be > sufficient?) you are right! https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:158: if (!ash::MaterialDesignController::IsSystemTrayMenuMaterial()) On 2016/09/19 23:32:27, tdanderson wrote: > nit: ash:: not needed (here and elsewhere in this CL). Originally I suggested > adding ash:: before MaterialDesignController to avoid confusing it with the top > chrome MDC, but that is in the process of being killed off anyway. I know MaterialDesignController exists in UI space and ash space. I thought it is a good hobbit of adding it, so it can be differentiated from the UI one. Does it mean that the ui one is enabled by default? https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:276: UserMetricsAction user_action; On 2016/09/19 23:32:27, tdanderson wrote: > Nice. Thanks :) https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:315: // Since this row is not initialized in MD, this function should be called for On 2016/09/19 23:32:27, tdanderson wrote: > I suggest removing this comment, since in the very near future you/someone will > be using this for the MD "info" button. Done. https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray_... ash/common/system/tray_accessibility.cc:317: if (!MaterialDesignController::UseMaterialDesignSystemIcons()) { On 2016/09/19 23:32:27, tdanderson wrote: > To avoid indenting the whole function, it would be better to use an early return > instead. Done. https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray_... File ash/common/system/tray_accessibility.h (right): https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray_... ash/common/system/tray_accessibility.h:58: // Create the detailed view of accessibility tray. Note that the help option On 2016/09/19 23:32:27, tdanderson wrote: > Generally a comment directly above a constructor should be about something > specific to that constructor. > > Consider removing the first sentence and, optionally, adding class-level > documentation (above line 55). > > As for the second sentence, I think a better place for it would be above > AppendHelpEntries() on line 73. You could just add "Only used for non-MD" to the > comment that's already there. I put the first sentence as a class description to the AccessibilityDetailedView; and add the second sentence to AppendHelpEntries. Thanks. https://codereview.chromium.org/2343603003/diff/80001/ash/common/system/tray_... ash/common/system/tray_accessibility.h:76: // Helper function to create entries in the detailed accessibility view. On 2016/09/19 23:32:27, tdanderson wrote: > nit: Maybe mention that the |icon| parameter is only used for MD. Done. https://codereview.chromium.org/2343603003/diff/80001/ui/gfx/vector_icons/che... File ui/gfx/vector_icons/check_circle.1x.icon (right): https://codereview.chromium.org/2343603003/diff/80001/ui/gfx/vector_icons/che... ui/gfx/vector_icons/check_circle.1x.icon:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/09/19 23:32:27, tdanderson wrote: > nit: update year. I updated the year in both check_circle files. https://codereview.chromium.org/2343603003/diff/80001/ui/views/examples/throb... File ui/views/examples/throbber_example.cc (right): https://codereview.chromium.org/2343603003/diff/80001/ui/views/examples/throb... ui/views/examples/throbber_example.cc:24: // View implementation. On 2016/09/19 23:32:27, tdanderson wrote: > nit: Just "// View:" Done. https://codereview.chromium.org/2343603003/diff/80001/ui/views/examples/throb... ui/views/examples/throbber_example.cc:46: is_checked_ = true; On 2016/09/19 23:32:27, tdanderson wrote: > You can avoid duplicated code with: > > throbber->SetChecked(!is_checked_); > is_checked_ = !is_checked_; Thanks, I thought about it, then i don't know how to combine start() and stop() in one line, so i give up.... sorry, i should combine as many line as possible.
lgtm with nits https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... File ash/common/system/tray_accessibility.cc (right): https://chromiumcodereview.appspot.com/2343603003/diff/80001/ash/common/syste... ash/common/system/tray_accessibility.cc:158: if (!ash::MaterialDesignController::IsSystemTrayMenuMaterial()) On 2016/09/21 17:10:48, yiyix wrote: > On 2016/09/19 23:32:27, tdanderson wrote: > > nit: ash:: not needed (here and elsewhere in this CL). Originally I suggested > > adding ash:: before MaterialDesignController to avoid confusing it with the > top > > chrome MDC, but that is in the process of being killed off anyway. > > I know MaterialDesignController exists in UI space and ash space. I thought it > is a good hobbit of adding it, so it can be differentiated from the UI one. Does > it mean that the ui one is enabled by default? As discussed in person, top-chrome MDC calls are in the process of being removed, and in retrospect telling the difference between the two was probably only an issue for the handful of people who worked on both projects. https://chromiumcodereview.appspot.com/2343603003/diff/100001/ash/common/syst... File ash/common/system/tray/tray_constants.h (right): https://chromiumcodereview.appspot.com/2343603003/diff/100001/ash/common/syst... ash/common/system/tray/tray_constants.h:104: extern const int kMenuButtonSize; kMenuButtonSize was added by another CL, make sure you pull, rebase, and fix merge conflicts before putting this in the CQ. https://chromiumcodereview.appspot.com/2343603003/diff/100001/ash/common/syst... File ash/common/system/tray_accessibility.cc (right): https://chromiumcodereview.appspot.com/2343603003/diff/100001/ash/common/syst... ash/common/system/tray_accessibility.cc:169: // Generate entries in Accessibility detailed view menu for MD and non-MD, nit: I don't think this comment adds much information, so I'd suggest removing it. https://chromiumcodereview.appspot.com/2343603003/diff/100001/ash/common/syst... ash/common/system/tray_accessibility.cc:314: if (MaterialDesignController::UseMaterialDesignSystemIcons()) { nit: {} not needed https://chromiumcodereview.appspot.com/2343603003/diff/100001/ash/common/syst... File ash/common/system/tray_accessibility.h (right): https://chromiumcodereview.appspot.com/2343603003/diff/100001/ash/common/syst... ash/common/system/tray_accessibility.h:71: // Add help entries only used for non-MD. Note that the help entries row will nit: "Add help entries. Only used for non-MD."
yiyix@chromium.org changed reviewers: + sadrul@chromium.org, xiyuan@chromium.org
@xiyuan, could you do an owner review on file chrome/browser/chromeos/system/tray_accessibility_browsertest.cc @sadrul, could you do an owner review on changes in ui/? Thank you all, Yi Xu https://codereview.chromium.org/2343603003/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_constants.h (right): https://codereview.chromium.org/2343603003/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_constants.h:104: extern const int kMenuButtonSize; On 2016/09/21 17:59:32, tdanderson wrote: > kMenuButtonSize was added by another CL, make sure you pull, rebase, and fix > merge conflicts before putting this in the CQ. Sure, i will rebase the code before send to other people for review. Thank you for the call out. https://codereview.chromium.org/2343603003/diff/100001/ash/common/system/tray... File ash/common/system/tray_accessibility.cc (right): https://codereview.chromium.org/2343603003/diff/100001/ash/common/system/tray... ash/common/system/tray_accessibility.cc:169: // Generate entries in Accessibility detailed view menu for MD and non-MD, On 2016/09/21 17:59:32, tdanderson wrote: > nit: I don't think this comment adds much information, so I'd suggest removing > it. Done. https://codereview.chromium.org/2343603003/diff/100001/ash/common/system/tray... File ash/common/system/tray_accessibility.h (right): https://codereview.chromium.org/2343603003/diff/100001/ash/common/system/tray... ash/common/system/tray_accessibility.h:71: // Add help entries only used for non-MD. Note that the help entries row will On 2016/09/21 17:59:33, tdanderson wrote: > nit: "Add help entries. Only used for non-MD." Done.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
c/b/chromeos/system/tray_accessibility_browsertest.cc lgtm
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yiyix@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 https://codereview.chromium.org/2343603003/diff/120001/ui/views/examples/thro... File ui/views/examples/throbber_example.cc (right): https://codereview.chromium.org/2343603003/diff/120001/ui/views/examples/thro... ui/views/examples/throbber_example.cc:38: if (GetEventHandlerForPoint(event.location()) == throbber_) { Early return when possible, i.e: if (!GetEventHandlerForPoint(..)) return false; if (is_checked_) ... ... return true;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyix@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...
https://codereview.chromium.org/2343603003/diff/120001/ui/views/examples/thro... File ui/views/examples/throbber_example.cc (right): https://codereview.chromium.org/2343603003/diff/120001/ui/views/examples/thro... ui/views/examples/throbber_example.cc:38: if (GetEventHandlerForPoint(event.location()) == throbber_) { On 2016/09/22 02:06:42, sadrul wrote: > Early return when possible, i.e: > > if (!GetEventHandlerForPoint(..)) > return false; > if (is_checked_) > ... > ... > return true; Done.
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 yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, sadrul@chromium.org, xiyuan@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2343603003/#ps160001 (title: "merge")
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_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, sadrul@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2343603003/#ps180001 (title: "merge")
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
Failed to apply patch for ash/resources/vector_icons/system_menu_accessibility_contrast.icon: While running git apply --index -3 -p1; error: patch failed: ash/resources/vector_icons/system_menu_update.icon:5 Falling back to three-way merge... Applied patch to 'ash/resources/vector_icons/system_menu_accessibility_contrast.icon' with conflicts. U ash/resources/vector_icons/system_menu_accessibility_contrast.icon Patch: NR ash/resources/vector_icons/system_menu_update.icon->ash/resources/vector_icons/system_menu_accessibility_contrast.icon Index: ash/resources/vector_icons/system_menu_accessibility_contrast.icon diff --git a/ash/resources/vector_icons/system_menu_update.icon b/ash/resources/vector_icons/system_menu_accessibility_contrast.icon similarity index 58% copy from ash/resources/vector_icons/system_menu_update.icon copy to ash/resources/vector_icons/system_menu_accessibility_contrast.icon index b1641c9fe05e47d8645296d7dfc00f70e4c3fe4b..94bdd469f5af9f2c09e665ca713d2fc2f0db7e47 100644 --- a/ash/resources/vector_icons/system_menu_update.icon +++ b/ash/resources/vector_icons/system_menu_accessibility_contrast.icon @@ -5,17 +5,13 @@ CANVAS_DIMENSIONS, 40, MOVE_TO, 20, 36, R_CUBIC_TO, 8.84f, 0, 16, -7.16f, 16, -16, -CUBIC_TO, 36, 11.16f, 28.84f, 4, 20, 4, -CUBIC_TO, 11.16f, 4, 4, 11.16f, 4, 20, +CUBIC_TO_SHORTHAND, 28.84f, 4, 20, 4, +CUBIC_TO_SHORTHAND, 4, 11.16f, 4, 20, R_CUBIC_TO, 0, 8.84f, 7.16f, 16, 16, 16, CLOSE, -R_MOVE_TO, -3, -9, -R_H_LINE_TO, 6, -R_V_LINE_TO, -6, -R_H_LINE_TO, 4, -R_LINE_TO, -7, -8, -R_LINE_TO, -7, 8, -R_H_LINE_TO, 4, -R_V_LINE_TO, 6, +R_MOVE_TO, 0, -28.23f, +R_CUBIC_TO, 6.76f, 0, 12.24f, 5.48f, 12.24f, 12.24f, +CUBIC_TO_SHORTHAND, 26.76f, 32.24f, 20, 32.24f, +V_LINE_TO, 7.77f, CLOSE, END
The CQ bit was checked by yiyix@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, sadrul@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2343603003/#ps240001 (title: "'")
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 ========== Implement accessibility detailed view for the MD Ash system menu Introduced a new layout for accessibility detailed view. Each row now contains a vectorized icon which is shown on left, a description which is shown in the middle and a check mark is shown on the right. The row on the bottom which contains "setting" option and "learn more" option is removed and it will be included in the title row in the future cls. This CL also updates the description of ChromeVox from "ChromeVox (Spoken feedback)" to "ChromeVox (spoken feedback)" TEST=tray_accessibility_browsertest BUG=632102 ========== to ========== Implement accessibility detailed view for the MD Ash system menu Introduced a new layout for accessibility detailed view. Each row now contains a vectorized icon which is shown on left, a description which is shown in the middle and a check mark is shown on the right. The row on the bottom which contains "setting" option and "learn more" option is removed and it will be included in the title row in the future cls. This CL also updates the description of ChromeVox from "ChromeVox (Spoken feedback)" to "ChromeVox (spoken feedback)" TEST=tray_accessibility_browsertest BUG=632102 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Implement accessibility detailed view for the MD Ash system menu Introduced a new layout for accessibility detailed view. Each row now contains a vectorized icon which is shown on left, a description which is shown in the middle and a check mark is shown on the right. The row on the bottom which contains "setting" option and "learn more" option is removed and it will be included in the title row in the future cls. This CL also updates the description of ChromeVox from "ChromeVox (Spoken feedback)" to "ChromeVox (spoken feedback)" TEST=tray_accessibility_browsertest BUG=632102 ========== to ========== Implement accessibility detailed view for the MD Ash system menu Introduced a new layout for accessibility detailed view. Each row now contains a vectorized icon which is shown on left, a description which is shown in the middle and a check mark is shown on the right. The row on the bottom which contains "setting" option and "learn more" option is removed and it will be included in the title row in the future cls. This CL also updates the description of ChromeVox from "ChromeVox (Spoken feedback)" to "ChromeVox (spoken feedback)" TEST=tray_accessibility_browsertest BUG=632102 Committed: https://crrev.com/3fd37d926efd712c690acee0bafbf07fd27d519e Cr-Commit-Position: refs/heads/master@{#420720} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3fd37d926efd712c690acee0bafbf07fd27d519e Cr-Commit-Position: refs/heads/master@{#420720} |
