Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8511003:
Fix to correctly set accessible name of menu item when there is only one child and title is empty (Closed)
|
Created:
9 years, 1 month ago by hashimoto Modified:
9 years, 1 month ago CC:
chromium-reviews, tfarina, dhollowa Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix to correctly set accessible name of menu item when there is only one child and title is empty
BUG=chromium-os:22051
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109799
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add MenuItemView::IsFirstChildToTakeOver #
Total comments: 2
Patch Set 3 : Changed the method name from IsFirstChildToTakeOver from FirstChildTakesOver #
Total comments: 2
Patch Set 4 : Changed the method name from FirstChildTakesOver to IsContainer #Messages
Total messages: 18 (0 generated)
+sky for approval. I think this might be useful code if we have more than one menu with this pattern (no title or an icon for the title, and a single menu item containing status info). However, it seems brittle - what if another menu legitimately just one item for some other reason? This could mask another accessibility error. I'd be fine with it if we added a method UseFirstItemAsAccessibleMenuTitle() or something like that, and maybe it'd even make sense to automatically call that on all status bar menus. It also might be nice to add a DCHECK to ensure that a menu has an accessible name, so that anyone adding a status bar menu in the future will make sure it either has an accessible name or calls something like UseFirstItemAsAccessibleMenuTitle. http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... File views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... views/controls/menu/menu_item_view.cc:143: string16 item_text; If we do this, add a comment like "One case to deal with is a status bar menu with an icon for the title and a single item - in that case, use the text of the item as the title of the menu.
http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... File views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... views/controls/menu/menu_item_view.cc:144: if (child_count() == 1 && title_.empty()) { This seems weird. Why do we want the accessible title for a folder with no name to come from the first child?
http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... File views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... views/controls/menu/menu_item_view.cc:144: if (child_count() == 1 && title_.empty()) { On 2011/11/09 15:11:37, sky wrote: > This seems weird. Why do we want the accessible title for a folder with no name > to come from the first child? The motivating example is the status bar menus on Chrome OS - for example, the Battery menu has an icon for the title, and a single disabled item that says "Battery: 100%". I think it should be made explicit if we want to do this, e.g.: menu-> UseFirstItemAsAccessibleMenuTitle, otherwise it's brittle (and confusing).
On Wed, Nov 9, 2011 at 7:50 AM, <dmazzoni@chromium.org> wrote: > > http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... > File views/controls/menu/menu_item_view.cc (right): > > http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... > views/controls/menu/menu_item_view.cc:144: if (child_count() == 1 && > title_.empty()) { > On 2011/11/09 15:11:37, sky wrote: >> >> This seems weird. Why do we want the accessible title for a folder > > with no name >> >> to come from the first child? > > The motivating example is the status bar menus on Chrome OS - for > example, the Battery menu has an icon for the title, and a single > disabled item that says "Battery: 100%". I think it should be made > explicit if we want to do this, e.g.: menu-> > UseFirstItemAsAccessibleMenuTitle, otherwise it's brittle (and > confusing). > > http://codereview.chromium.org/8511003/ > Where does the code for this live? -Scott
Start with chrome/browser/chromeos/status/power_menu_button.* as one example. - Dominic On Wed, Nov 9, 2011 at 8:36 AM, Scott Violet <sky@chromium.org> wrote: > On Wed, Nov 9, 2011 at 7:50 AM, <dmazzoni@chromium.org> wrote: > > > > > http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... > > File views/controls/menu/menu_item_view.cc (right): > > > > > http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... > > views/controls/menu/menu_item_view.cc:144: if (child_count() == 1 && > > title_.empty()) { > > On 2011/11/09 15:11:37, sky wrote: > >> > >> This seems weird. Why do we want the accessible title for a folder > > > > with no name > >> > >> to come from the first child? > > > > The motivating example is the status bar menus on Chrome OS - for > > example, the Battery menu has an icon for the title, and a single > > disabled item that says "Battery: 100%". I think it should be made > > explicit if we want to do this, e.g.: menu-> > > UseFirstItemAsAccessibleMenuTitle, otherwise it's brittle (and > > confusing). > > > > http://codereview.chromium.org/8511003/ > > > > Where does the code for this live? > > -Scott >
I think powermenubutton should create a subclass that does this logic and not bake it into MenuItemView. -Scott On Wed, Nov 9, 2011 at 9:03 AM, Dominic Mazzoni <dmazzoni@chromium.org> wrote: > Start with chrome/browser/chromeos/status/power_menu_button.* as one > example. > - Dominic > > On Wed, Nov 9, 2011 at 8:36 AM, Scott Violet <sky@chromium.org> wrote: >> >> On Wed, Nov 9, 2011 at 7:50 AM, <dmazzoni@chromium.org> wrote: >> > >> > >> > http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... >> > File views/controls/menu/menu_item_view.cc (right): >> > >> > >> > http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... >> > views/controls/menu/menu_item_view.cc:144: if (child_count() == 1 && >> > title_.empty()) { >> > On 2011/11/09 15:11:37, sky wrote: >> >> >> >> This seems weird. Why do we want the accessible title for a folder >> > >> > with no name >> >> >> >> to come from the first child? >> > >> > The motivating example is the status bar menus on Chrome OS - for >> > example, the Battery menu has an icon for the title, and a single >> > disabled item that says "Battery: 100%". I think it should be made >> > explicit if we want to do this, e.g.: menu-> >> > UseFirstItemAsAccessibleMenuTitle, otherwise it's brittle (and >> > confusing). >> > >> > http://codereview.chromium.org/8511003/ >> > >> >> Where does the code for this live? >> >> -Scott > >
The case "child_count() == 1 && title_.empty()" is already treated in a special way by MenuItemView::Layout and MenuItemView::CalculatePreferredSize. I think having a mechanism to set accessibility name (with something like UseFirstItemAsAccessibleMenuTitle) for this case is necessary because it is better than nothing. On 2011/11/09 17:30:00, sky wrote: > I think powermenubutton should create a subclass that does this logic > and not bake it into MenuItemView. > > -Scott > > On Wed, Nov 9, 2011 at 9:03 AM, Dominic Mazzoni <mailto:dmazzoni@chromium.org> wrote: > > Start with chrome/browser/chromeos/status/power_menu_button.* as one > > example. > > - Dominic > > > > On Wed, Nov 9, 2011 at 8:36 AM, Scott Violet <mailto:sky@chromium.org> wrote: > >> > >> On Wed, Nov 9, 2011 at 7:50 AM, mailto: <dmazzoni@chromium.org> wrote: > >> > > >> > > >> > > http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... > >> > File views/controls/menu/menu_item_view.cc (right): > >> > > >> > > >> > > http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... > >> > views/controls/menu/menu_item_view.cc:144: if (child_count() == 1 && > >> > title_.empty()) { > >> > On 2011/11/09 15:11:37, sky wrote: > >> >> > >> >> This seems weird. Why do we want the accessible title for a folder > >> > > >> > with no name > >> >> > >> >> to come from the first child? > >> > > >> > The motivating example is the status bar menus on Chrome OS - for > >> > example, the Battery menu has an icon for the title, and a single > >> > disabled item that says "Battery: 100%". I think it should be made > >> > explicit if we want to do this, e.g.: menu-> > >> > UseFirstItemAsAccessibleMenuTitle, otherwise it's brittle (and > >> > confusing). > >> > > >> > http://codereview.chromium.org/8511003/ > >> > > >> > >> Where does the code for this live? > >> > >> -Scott > > > >
Good point, just add a description and LGTM
On 2011/11/09 14:43:20, Dominic Mazzoni wrote: > +sky for approval. > > I think this might be useful code if we have more than one menu with this > pattern (no title or an icon for the title, and a single menu item containing > status info). However, it seems brittle - what if another menu legitimately just > one item for some other reason? This could mask another accessibility error. This code is not for the case with a menu with one item because a menu item is a child of a MenuItemView's |submenu_|, not a child of a MenuItemView itself. > > I'd be fine with it if we added a method UseFirstItemAsAccessibleMenuTitle() or > something like that, and maybe it'd even make sense to automatically call that > on all status bar menus. I think setting accessibility name automatically without a method call is fine because there is no accessibility name without that. > > It also might be nice to add a DCHECK to ensure that a menu has an accessible > name, so that anyone adding a status bar menu in the future will make sure it > either has an accessible name or calls something like > UseFirstItemAsAccessibleMenuTitle. > > http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... > File views/controls/menu/menu_item_view.cc (right): > > http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... > views/controls/menu/menu_item_view.cc:143: string16 item_text; > If we do this, add a comment like "One case to deal with is a status bar menu > with an icon for the title and a single item - in that case, use the text of the > item as the title of the menu.
New patch set, added MenuItemView::IsFirstChildToTakeOver to make the objective of the code clear. http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... File views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/8511003/diff/1/views/controls/menu/menu_item_v... views/controls/menu/menu_item_view.cc:144: if (child_count() == 1 && title_.empty()) { On 2011/11/09 15:50:21, Dominic Mazzoni wrote: > On 2011/11/09 15:11:37, sky wrote: > > This seems weird. Why do we want the accessible title for a folder with no > name > > to come from the first child? > > The motivating example is the status bar menus on Chrome OS - for example, the > Battery menu has an icon for the title, and a single disabled item that says > "Battery: 100%". I think it should be made explicit if we want to do this, e.g.: > menu-> UseFirstItemAsAccessibleMenuTitle, otherwise it's brittle (and > confusing). I don't know examples other than ChromeOS status area menus , but there is already code treating the case "child_count() == 1 && title_.empty()" in a different way. Added IsFirstChildToTakeOver() to make it clear that this is the special case.
This looks great, refactoring IsFirstChildToTakeOver into a separate method makes it a lot more clear. Perhaps FirstChildTakesOver would be more grammatical? http://codereview.chromium.org/8511003/diff/5/views/controls/menu/menu_item_v... File views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/8511003/diff/5/views/controls/menu/menu_item_v... views/controls/menu/menu_item_view.h:397: // responsible to show content. responsible to show content -> responsible for showing content
New patch set. Thanks, with the new method name, "if (FirstChlidTakesOver())" looks more human readable. http://codereview.chromium.org/8511003/diff/5/views/controls/menu/menu_item_v... File views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/8511003/diff/5/views/controls/menu/menu_item_v... views/controls/menu/menu_item_view.h:397: // responsible to show content. On 2011/11/10 06:30:49, Dominic Mazzoni wrote: > responsible to show content -> responsible for showing content Done.
http://codereview.chromium.org/8511003/diff/4003/views/controls/menu/menu_ite... File views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/8511003/diff/4003/views/controls/menu/menu_ite... views/controls/menu/menu_item_view.h:399: bool FirstChildTakesOver() const; This name is confusing. How about IsContainer with a description of: // Returns true if this MenuItemView contains a single child // that is responsible for rendering the content.
Thanks for your suggestion. This is the new patch set with new method name. http://codereview.chromium.org/8511003/diff/4003/views/controls/menu/menu_ite... File views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/8511003/diff/4003/views/controls/menu/menu_ite... views/controls/menu/menu_item_view.h:399: bool FirstChildTakesOver() const; On 2011/11/10 15:58:48, sky wrote: > This name is confusing. How about IsContainer with a description of: > > // Returns true if this MenuItemView contains a single child > // that is responsible for rendering the content. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hashimoto@chromium.org/8511003/8001
Change committed as 109799 |
