Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2491773002:
Page Info (Views): remove images from accessibility order (Closed)
DescriptionPage Info (Views): remove images from accessibility order
Images are accompanied by descriptive text, so it is unnecessary for them to be in the accessibility order. (Currently, they are also read as "image", which is not useful.)
BUG=660239
TEST=Two platforms:
Mac:
1) Enable #mac-views-webui-dialogs in chrome://flags
2) Enable VoiceOver (Cmd-F5 by default)
3) Visit google.com and click on the lock icon to open the Page Info bubble.
4) Use Ctrl-Opt-(right arrow) to navigate the Page Info bubble. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should not.
Windows:
1) Enable Microsoft Narrator.
2) Visit google.com and click on the lock icon to open the Page Info bubble.
3) Navigate the Page Info bubble [1]. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should not.
[1] https://support.microsoft.com/en-us/help/14234/windows-hear-text-read-aloud-with-narrator
Committed: https://crrev.com/2b1f577315991631d7e9d02d514d7e5042862e28
Cr-Commit-Position: refs/heads/master@{#432122}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Move NonAccessibleImageView into a new class. #
Total comments: 17
Patch Set 3 : Address latest comments. #
Messages
Total messages: 30 (17 generated)
The CQ bit was checked by lgarron@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...
lgarron@chromium.org changed reviewers: + msw@chromium.org
msw@, could you review? I've created two separate implementations of NonAccessibleImageView because that was the simplest (it's a single-method override) and I don't know of a "right" way to share the code. I'd be happy to combine them if you advise on the best way.
Description was changed from ========== Page Info (Views): remove images from accessibility order Images are accompanied by descriptive text, so it is unnecessary for them to be int he accessibility order. (Currently, they are also read as "image", which is not useful.) BUG=660239 ========== to ========== Page Info (Views): remove images from accessibility order Images are accompanied by descriptive text, so it is unnecessary for them to be in the accessibility order. (Currently, they are also read as "image", which is not useful.) BUG=660239 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msw@chromium.org changed reviewers: + dmazzoni@chromium.org
Hey Dominic, please take a look at my question, thanks! https://codereview.chromium.org/2491773002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2491773002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_row.cc:33: class NonAccessibleImageView : public views::ImageView { Yeah, even though it's a fairly simple subclass, it makes sense to consolidate the two impls in a chrome/browser/ui/views/website_settings/non_accessible_image_view.[h|cc] or similar. https://codereview.chromium.org/2491773002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_row.cc:34: // Overridden from views::View. nit: s/View/ImageView/, views::ImageView itself has an override. https://codereview.chromium.org/2491773002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_row.cc:36: }; DISALLOW_COPY_AND_ASSIGN (please add an explicit private: visibility label). https://codereview.chromium.org/2491773002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_row.cc:39: node_data->role = ui::AX_ROLE_IGNORED; I wonder if SetFocusBehavior(NEVER) would suffice, but probably not... +dmazzoni@ for review; is this okay, do you see a better approach (eg. add a mutable AXRole member and setter to views::ImageView?).
https://codereview.chromium.org/2491773002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2491773002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_row.cc:39: node_data->role = ui::AX_ROLE_IGNORED; On 2016/11/10 03:04:14, msw wrote: > I wonder if SetFocusBehavior(NEVER) would suffice, but probably not... > +dmazzoni@ for review; is this okay, do you see a better approach (eg. add a > mutable AXRole member and setter to views::ImageView?). How about node_data->AddStateFlag(ui::AX_STATE_INVISIBLE)? I think making it invisible will already do the right thing on Windows and Chrome OS. If it doesn't work for MacViews, file a bug and assign it to patricialor@. Semantically, marking it as invisible is more clear. The right way to achieve that on Mac is an implementation detail - once we figure it out that can go into the MacViews code. If you want to avoid the subclass and there are other examples of ImageViews that have this need, I'd add something specific to ImageView, like SetDecorativeImageOnlyForAccessibility(true) and have that set a flag that affects ImageView::GetAccessibleNodeData.
https://codereview.chromium.org/2491773002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2491773002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/permission_selector_row.cc:39: node_data->role = ui::AX_ROLE_IGNORED; On 2016/11/10 at 18:13:59, dmazzoni wrote: > On 2016/11/10 03:04:14, msw wrote: > > I wonder if SetFocusBehavior(NEVER) would suffice, but probably not... > > +dmazzoni@ for review; is this okay, do you see a better approach (eg. add a > > mutable AXRole member and setter to views::ImageView?). > > How about node_data->AddStateFlag(ui::AX_STATE_INVISIBLE)? > > I think making it invisible will already do the right thing on Windows and Chrome OS. > > If it doesn't work for MacViews, file a bug and assign it to patricialor@. > Semantically, marking it as invisible is more clear. The right way to achieve > that on Mac is an implementation detail - once we figure it out that can go > into the MacViews code. > > If you want to avoid the subclass and there are other examples of ImageViews > that have this need, I'd add something specific to ImageView, like > SetDecorativeImageOnlyForAccessibility(true) and have that set a flag > that affects ImageView::GetAccessibleNodeData. The following does not work: icon->SetFocusBehavior(views::View::FocusBehavior::NEVER); I've filed https://crbug.com/664320 dmazzoni@, how do you feel about NonAccessibleImageView for now? I can file a followup bug to change it once we have a standard approach.
lgtm I'm fine with NonAccessibleImageView, but I think node_data->AddStateFlag(ui::AX_STATE_INVISIBLE) would be a more clear way of expressing that you want to hide the image. Since it's Views code, we can map that to whatever we need to natively (and I can own fixing that if it doesn't work on any platform).
msw@, could you review again? I've moved the non-accessible image view into a new class. ... except I just realized that the same problem is present in permission bubbles. Those are currently in the same folder, but we plan to separate them from the Page Info code. :-/
https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/non_accessible_image_view.cc (right): https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/non_accessible_image_view.cc:8: node_data->role = ui::AX_ROLE_IGNORED; Please address Dominic's feedback: node_data->AddStateFlag(ui::AX_STATE_INVISIBLE). https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/non_accessible_image_view.h (right): https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/non_accessible_image_view.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no (c) for new files; ditto in cc file https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/non_accessible_image_view.h:9: #include "ui/accessibility/ax_enums.h" nit: move to cc file https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/non_accessible_image_view.h:10: #include "ui/accessibility/ax_node_data.h" optional nit: rely on the transitive include/forward-decl from image_view.h https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:15: #include "ui/accessibility/ax_enums.h" nit: remove https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:33: nit: remove blank line, or add a matching one above the comment. https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:43: #include "ui/accessibility/ax_enums.h" nit: remove https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:44: #include "ui/accessibility/ax_node_data.h" nit: remove
https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/non_accessible_image_view.cc (right): https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/non_accessible_image_view.cc:8: node_data->role = ui::AX_ROLE_IGNORED; On 2016/11/11 at 15:55:01, msw wrote: > Please address Dominic's feedback: node_data->AddStateFlag(ui::AX_STATE_INVISIBLE). Done. node_data->AddStateFlag(ui::AX_STATE_INVISIBLE) appears to be sufficient, so I've switched to that. dmazzoni@, is it appropriate to set the state to invisible and leave the role as image? https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/non_accessible_image_view.h (right): https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/non_accessible_image_view.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/11/11 at 15:55:01, msw wrote: > nit: no (c) for new files; ditto in cc file Done. (Took me a while to figure out you meant to just take out the symbol "(c)", not the whole copyright line. I can't find a wiki page about creating files; how can I find out about details like this?) https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/non_accessible_image_view.h:9: #include "ui/accessibility/ax_enums.h" On 2016/11/11 at 15:55:01, msw wrote: > nit: move to cc file Done. https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/non_accessible_image_view.h:10: #include "ui/accessibility/ax_node_data.h" On 2016/11/11 at 15:55:01, msw wrote: > optional nit: rely on the transitive include/forward-decl from image_view.h Done. https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_selector_row.cc (right): https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:15: #include "ui/accessibility/ax_enums.h" On 2016/11/11 at 15:55:01, msw wrote: > nit: remove Removed. https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_selector_row.cc:33: On 2016/11/11 at 15:55:01, msw wrote: > nit: remove blank line, or add a matching one above the comment. Removed. https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:43: #include "ui/accessibility/ax_enums.h" On 2016/11/11 at 15:55:01, msw wrote: > nit: remove Removed. https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:44: #include "ui/accessibility/ax_node_data.h" On 2016/11/11 at 15:55:01, msw wrote: > nit: remove Removed.
lgtm https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/non_accessible_image_view.h (right): https://codereview.chromium.org/2491773002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/non_accessible_image_view.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/11/11 22:28:51, lgarron wrote: > On 2016/11/11 at 15:55:01, msw wrote: > > nit: no (c) for new files; ditto in cc file > > Done. > > (Took me a while to figure out you meant to just take out the symbol "(c)", not > the whole copyright line. I can't find a wiki page about creating files; how can > I find out about details like this?) Sorry for being unclear... our C++ style guide explains this: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
The CQ bit was checked by lgarron@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...
Description was changed from ========== Page Info (Views): remove images from accessibility order Images are accompanied by descriptive text, so it is unnecessary for them to be in the accessibility order. (Currently, they are also read as "image", which is not useful.) BUG=660239 ========== to ========== Page Info (Views): remove images from accessibility order Images are accompanied by descriptive text, so it is unnecessary for them to be in the accessibility order. (Currently, they are also read as "image", which is not useful.) BUG=660239 TEST=Two platforms: Mac: 1) Enable #secondary-ui-md in chrome://flags 2) Enable VoiceOver (Cmd-F5 by default) 3) Visit google.com and click on the lock icon to open the Page Info bubble. 4) Use Ctrl-Opt-(right arrow) to navigate the Page Info bubble. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should nod. Windows: 1) Enable Microsoft Narrator. 2) Visit google.com and click on the lock icon to open the Page Info bubble. 3) Navigate the Page Info bubble [1]. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should nod. [1] https://support.microsoft.com/en-us/help/14234/windows-hear-text-read-aloud-w... ==========
Description was changed from ========== Page Info (Views): remove images from accessibility order Images are accompanied by descriptive text, so it is unnecessary for them to be in the accessibility order. (Currently, they are also read as "image", which is not useful.) BUG=660239 TEST=Two platforms: Mac: 1) Enable #secondary-ui-md in chrome://flags 2) Enable VoiceOver (Cmd-F5 by default) 3) Visit google.com and click on the lock icon to open the Page Info bubble. 4) Use Ctrl-Opt-(right arrow) to navigate the Page Info bubble. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should nod. Windows: 1) Enable Microsoft Narrator. 2) Visit google.com and click on the lock icon to open the Page Info bubble. 3) Navigate the Page Info bubble [1]. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should nod. [1] https://support.microsoft.com/en-us/help/14234/windows-hear-text-read-aloud-w... ========== to ========== Page Info (Views): remove images from accessibility order Images are accompanied by descriptive text, so it is unnecessary for them to be in the accessibility order. (Currently, they are also read as "image", which is not useful.) BUG=660239 TEST=Two platforms: Mac: 1) Enable #mac-views-webui-dialogs in chrome://flags 2) Enable VoiceOver (Cmd-F5 by default) 3) Visit google.com and click on the lock icon to open the Page Info bubble. 4) Use Ctrl-Opt-(right arrow) to navigate the Page Info bubble. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should nod. Windows: 1) Enable Microsoft Narrator. 2) Visit google.com and click on the lock icon to open the Page Info bubble. 3) Navigate the Page Info bubble [1]. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should nod. [1] https://support.microsoft.com/en-us/help/14234/windows-hear-text-read-aloud-w... ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Page Info (Views): remove images from accessibility order Images are accompanied by descriptive text, so it is unnecessary for them to be in the accessibility order. (Currently, they are also read as "image", which is not useful.) BUG=660239 TEST=Two platforms: Mac: 1) Enable #mac-views-webui-dialogs in chrome://flags 2) Enable VoiceOver (Cmd-F5 by default) 3) Visit google.com and click on the lock icon to open the Page Info bubble. 4) Use Ctrl-Opt-(right arrow) to navigate the Page Info bubble. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should nod. Windows: 1) Enable Microsoft Narrator. 2) Visit google.com and click on the lock icon to open the Page Info bubble. 3) Navigate the Page Info bubble [1]. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should nod. [1] https://support.microsoft.com/en-us/help/14234/windows-hear-text-read-aloud-w... ========== to ========== Page Info (Views): remove images from accessibility order Images are accompanied by descriptive text, so it is unnecessary for them to be in the accessibility order. (Currently, they are also read as "image", which is not useful.) BUG=660239 TEST=Two platforms: Mac: 1) Enable #mac-views-webui-dialogs in chrome://flags 2) Enable VoiceOver (Cmd-F5 by default) 3) Visit google.com and click on the lock icon to open the Page Info bubble. 4) Use Ctrl-Opt-(right arrow) to navigate the Page Info bubble. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should not. Windows: 1) Enable Microsoft Narrator. 2) Visit google.com and click on the lock icon to open the Page Info bubble. 3) Navigate the Page Info bubble [1]. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should not. [1] https://support.microsoft.com/en-us/help/14234/windows-hear-text-read-aloud-w... ==========
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2491773002/#ps40001 (title: "Address latest comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Page Info (Views): remove images from accessibility order Images are accompanied by descriptive text, so it is unnecessary for them to be in the accessibility order. (Currently, they are also read as "image", which is not useful.) BUG=660239 TEST=Two platforms: Mac: 1) Enable #mac-views-webui-dialogs in chrome://flags 2) Enable VoiceOver (Cmd-F5 by default) 3) Visit google.com and click on the lock icon to open the Page Info bubble. 4) Use Ctrl-Opt-(right arrow) to navigate the Page Info bubble. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should not. Windows: 1) Enable Microsoft Narrator. 2) Visit google.com and click on the lock icon to open the Page Info bubble. 3) Navigate the Page Info bubble [1]. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should not. [1] https://support.microsoft.com/en-us/help/14234/windows-hear-text-read-aloud-w... ========== to ========== Page Info (Views): remove images from accessibility order Images are accompanied by descriptive text, so it is unnecessary for them to be in the accessibility order. (Currently, they are also read as "image", which is not useful.) BUG=660239 TEST=Two platforms: Mac: 1) Enable #mac-views-webui-dialogs in chrome://flags 2) Enable VoiceOver (Cmd-F5 by default) 3) Visit google.com and click on the lock icon to open the Page Info bubble. 4) Use Ctrl-Opt-(right arrow) to navigate the Page Info bubble. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should not. Windows: 1) Enable Microsoft Narrator. 2) Visit google.com and click on the lock icon to open the Page Info bubble. 3) Navigate the Page Info bubble [1]. Text, buttons, and links should be read, but the images next to "Cookies" and the various permission should not. [1] https://support.microsoft.com/en-us/help/14234/windows-hear-text-read-aloud-w... Committed: https://crrev.com/2b1f577315991631d7e9d02d514d7e5042862e28 Cr-Commit-Position: refs/heads/master@{#432122} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2b1f577315991631d7e9d02d514d7e5042862e28 Cr-Commit-Position: refs/heads/master@{#432122} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
