|
|
DescriptionFixing accessibility bug with extensions preferences
As reported in bug 702971:
https://bugs.chromium.org/p/chromium/issues/detail?id=702971
The 'Keyboard shortcuts' and extension 'options' panels on the
extensions page are not accessible. The code was trying to use the
'ARIA-HIDDEN' attribute to keep extension options hidden until it was
time to expose them. However, there was a small bug in the exposing
code that resulted in the 'ARIA-HIDDEN' being set to 'TRUE' for all
panels (or, in some cases, 'FALSE' for all panels.
This is my first patch. If someone could request a try job for me on this, I would appreciate it.
BUG=702971
TEST=Using a screenreader, navigate extension options and keyboard
shortcuts options for extensions. Before this patch, the screenreader
says nothing about what control it is on. After this patch, the
screenreader reads the panel text as appropriate.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2765493002
Cr-Commit-Position: refs/heads/master@{#460734}
Committed: https://chromium.googlesource.com/chromium/src/+/c773631198a95466aa396f0b978c033626133bb8
Patch Set 1 #Patch Set 2 : Added name to AUTHORS file #Patch Set 3 : Fixed unit tests for aria-hidden and fixed accessibility of extension-options-overlay-icon #
Total comments: 1
Patch Set 4 : Added alt text for extension-options-overlay-icon #Patch Set 5 : Fixed name of one internal test step #Patch Set 6 : Reverted back to aria-hidden version. #
Messages
Total messages: 47 (20 generated)
Description was changed from ========== Fixing accessibility bug with extensions preferences As reported in bug 702971: https://bugs.chromium.org/p/chromium/issues/detail?id=702971 The 'Keyboard shortcuts' and extension 'options' panels on the extensions page are not accessible. The code was trying to use the 'ARIA-HIDDEN' attribute to keep extension options hidden until it was time to expose them. However, there was a small bug in the exposing code that resulted in the 'ARIA-HIDDEN' being set to 'TRUE' for all panels (or, in some cases, 'FALSE' for all panels. BUG=702971 TEST=Using a screenreader, navigate extension options and keyboard shortcuts options for extensions. Before this patch, the screenreader says nothing about what control it is on. After this patch, the screenreader reads the panel text as appropriate. ========== to ========== Fixing accessibility bug with extensions preferences As reported in bug 702971: https://bugs.chromium.org/p/chromium/issues/detail?id=702971 The 'Keyboard shortcuts' and extension 'options' panels on the extensions page are not accessible. The code was trying to use the 'ARIA-HIDDEN' attribute to keep extension options hidden until it was time to expose them. However, there was a small bug in the exposing code that resulted in the 'ARIA-HIDDEN' being set to 'TRUE' for all panels (or, in some cases, 'FALSE' for all panels. BUG=702971 TEST=Using a screenreader, navigate extension options and keyboard shortcuts options for extensions. Before this patch, the screenreader says nothing about what control it is on. After this patch, the screenreader reads the panel text as appropriate. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fixing accessibility bug with extensions preferences As reported in bug 702971: https://bugs.chromium.org/p/chromium/issues/detail?id=702971 The 'Keyboard shortcuts' and extension 'options' panels on the extensions page are not accessible. The code was trying to use the 'ARIA-HIDDEN' attribute to keep extension options hidden until it was time to expose them. However, there was a small bug in the exposing code that resulted in the 'ARIA-HIDDEN' being set to 'TRUE' for all panels (or, in some cases, 'FALSE' for all panels. BUG=702971 TEST=Using a screenreader, navigate extension options and keyboard shortcuts options for extensions. Before this patch, the screenreader says nothing about what control it is on. After this patch, the screenreader reads the panel text as appropriate. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fixing accessibility bug with extensions preferences As reported in bug 702971: https://bugs.chromium.org/p/chromium/issues/detail?id=702971 The 'Keyboard shortcuts' and extension 'options' panels on the extensions page are not accessible. The code was trying to use the 'ARIA-HIDDEN' attribute to keep extension options hidden until it was time to expose them. However, there was a small bug in the exposing code that resulted in the 'ARIA-HIDDEN' being set to 'TRUE' for all panels (or, in some cases, 'FALSE' for all panels. This is my first patch. If someone could request a try job for me on this, I would appreciate it. BUG=702971 TEST=Using a screenreader, navigate extension options and keyboard shortcuts options for extensions. Before this patch, the screenreader says nothing about what control it is on. After this patch, the screenreader reads the panel text as appropriate. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
patrickwonders@gmail.com changed reviewers: + finnur@chromium.org
This is my first patch. It is unclear to me whether I am supposed to be hitting the 'PUBLISH' button here. I'm not sure if there is anything more that I need to do to get this into the review queue. Thanks, Patrck
The CQ bit was checked by patrickwonders@gmail.com
The CQ bit was unchecked by patrickwonders@gmail.com
The CQ bit was checked by patrickwonders@gmail.com to run a CQ dry run
The CQ bit was unchecked by patrickwonders@gmail.com
The CQ bit was checked by finnur@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: The author patrickwonders@gmail.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
Thanks for contributing! Here's a good read on what your next steps need to be. https://www.chromium.org/getting-involved/become-a-committer This is also a good reference: https://www.chromium.org/developers/contributing-code I don't see anything wrong with the patch, per se, so I think we can wrap this up pretty quickly, once you've signed the docs in the email you should have gotten.
Okay... I had missed the 'Legal' section before on the contributing-code page. I have now added my name to the AUTHORS file and signed the CLA. Thanks, Patrick
On 2017/03/22 at 10:37:25, finnur wrote: > Thanks for contributing! Here's a good read on what your next steps need to be. > https://www.chromium.org/getting-involved/become-a-committer You said this says what my next steps need to be. I'm scratching my head. This is about getting write access to the repository which it says I need to do about 10 to 20 non-trivial patches with reviews from at least three different owners before I ask for committer access. Even 'try' access recommends that I have a few patches under my belt already. So, if there is more that I should do now beyond the CLA, let me know. Thanks, Patrick
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
The CQ bit was unchecked by finnur@chromium.org
The CQ bit was checked by finnur@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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Sorry to create confusion. But, yeah, I was thinking more down the line, if you want to take the next step (once you have the non-trivial patches mentioned). LGTM
The CQ bit was checked by finnur@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Hmm... so... it seems that when I was running unit tests on my Mac I was either not actually running the tests correctly, or it looked to me like it was succeeding when it wasn't. I'm not sure why so many platforms passed when the unit tests should be just as broken everywhere. I am checking with `assertFalse( ... )` when I should be checking `assertEquals( 'false', ... )` because the ARIA-HIDDEN property has a string value. Hmm. Will fix. On Thu, Mar 23, 2017 at 11:36 AM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Try jobs failed on following builders: > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/ > builders/win_chromium_x64_rel_ng/builds/390160) > > https://codereview.chromium.org/2765493002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Okay... I have fixed two of the four failing tests. SettingsCommandsExtensionSettingsWebUITest.extensionSettingsUri SettingsCommandsExtensionSettingsWebUITest.testChromeSendHandler These two are still failing though. If these worked before it was because the whole page was ARIA-HIDDEN=true and so the accessibility check didn't care that the #extension-options-overlay-icon didn't have ALT text. But, I'm looking into fixing that, too: OptionsDialogExtensionSettingsWebUITest.testAccessibility InstallGoodExtensionSettingsWebUITest.showOptions ...Patrick
Okay... I have fixed the other two tests, also. I had thought about giving alt-text to the extension-options-overlay-icon, but I opted for ARIA-HIDDEN="true" on it because it is just an icon to indicate the extension options when the header for the extension options immediately follows it.
bash-3.2$ ./out/Default/browser_tests --gtest_filter=*ExtensionSettingsWebUITest.* 2>&1 | egrep 'RUN|OK|SUCCESS' [ RUN ] ExtensionSettingsWebUITest.testEmptyExtensionList [ OK ] ExtensionSettingsWebUITest.testEmptyExtensionList (5425 ms) [ RUN ] ExtensionSettingsWebUITest.testChromeSendHandled [ OK ] ExtensionSettingsWebUITest.testChromeSendHandled (1396 ms) [ RUN ] BasicExtensionSettingsWebUITest.testDeveloperModeManyExtensions [ OK ] BasicExtensionSettingsWebUITest.testDeveloperModeManyExtensions (2537 ms) [ RUN ] BasicExtensionSettingsWebUITest.testDisable [ OK ] BasicExtensionSettingsWebUITest.testDisable (2469 ms) [ RUN ] BasicExtensionSettingsWebUITest.testEnable [ OK ] BasicExtensionSettingsWebUITest.testEnable (2461 ms) [ RUN ] BasicExtensionSettingsWebUITest.testUninstall [ OK ] BasicExtensionSettingsWebUITest.testUninstall (2294 ms) [ RUN ] BasicExtensionSettingsWebUITest.testNonEmptyExtensionList [ OK ] BasicExtensionSettingsWebUITest.testNonEmptyExtensionList (2462 ms) [ RUN ] AutoScrollExtensionSettingsWebUITest.testAutoScroll [ OK ] AutoScrollExtensionSettingsWebUITest.testAutoScroll (2599 ms) [ RUN ] SettingsCommandsExtensionSettingsWebUITest.testChromeSendHandler [ OK ] SettingsCommandsExtensionSettingsWebUITest.testChromeSendHandler (1545 ms) [ RUN ] SettingsCommandsExtensionSettingsWebUITest.extensionSettingsUri [ OK ] SettingsCommandsExtensionSettingsWebUITest.extensionSettingsUri (1650 ms) [ RUN ] InstallGoodExtensionSettingsWebUITest.testAccessibility [ OK ] InstallGoodExtensionSettingsWebUITest.testAccessibility (1674 ms) [ RUN ] InstallGoodExtensionSettingsWebUITest.showOptions [ OK ] InstallGoodExtensionSettingsWebUITest.showOptions (1618 ms) [ RUN ] ManagedExtensionSettingsWebUITest.testAccessibility [ OK ] ManagedExtensionSettingsWebUITest.testAccessibility (1644 ms) [ RUN ] OptionsDialogExtensionSettingsWebUITest.testAccessibility [ OK ] OptionsDialogExtensionSettingsWebUITest.testAccessibility (1690 ms) SUCCESS: all tests passed.
https://codereview.chromium.org/2765493002/diff/40001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_options_overlay.html (right): https://codereview.chromium.org/2765493002/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extension_options_overlay.html:10: aria-hidden="true"></img> I don't know... I'm a little uncomfortable with this... As I understand it, the accessibility check is to ensure everything that should have an alt-text has it. Why is this better than actually providing the alt-text? The alt-text is useful for accessibility once the image is shown, is it not?
My thinking was that this icon is redundant with the headline that immediately follows it. So, it is okay if the screen reader just ignores the icon. The way I have it, the image will be shown but the screen reader will ignore the icon. If I add alt text, the screen reader will read the alt text. I considered adding alt="extension settings icon" but it seemed like that wouldn't add anything to visually-impaired user's experience. I can certainly add alt-text rather than hiding the icon from the screen-reader. I defer to your judgement. -- Patrick <pat@nklein.com> > On Mar 24, 2017, at 9:25 AM, finnur@chromium.org wrote: > > > https://codereview.chromium.org/2765493002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/extensions/extension_options_overlay.html > (right): > > https://codereview.chromium.org/2765493002/diff/40001/chrome/browser/resource... > chrome/browser/resources/extensions/extension_options_overlay.html:10: > aria-hidden="true"></img> > I don't know... I'm a little uncomfortable with this... As I understand > it, the accessibility check is to ensure everything that should have an > alt-text has it. Why is this better than actually providing the > alt-text? The alt-text is useful for accessibility once the image is > shown, is it not? > > https://codereview.chromium.org/2765493002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Okay... added alt text. And added bits to make the alt text include the name of the extensions the icon is for. And, added unit tests to verify this information. Uploading now.
If that's the case, that the alt text is not useful for the visually impaired, then I think it is better to specify alt="" than to add some string that doesn't add anything.
Hmm. It will surprise me if the accessibility check is okay with the alt text being empty. That seems worse to me than having no ALT text. The possibilities are: 1. No ALT attribute, 2. ALT="" 3. ALT="Foobar icon" (assuming the extension is called "Foobar") 4. ARIA-HIDDEN="true" In case #1, a screen reader would let the user navigate to the image and announce "image". Not very useful at all. And, verifiably does not pass the accessibility tests in Chrome. In case #2, this will (depending on their screen reader settings) either be exactly like #1 or a screen reader would let the user navigate to the image and not say a thing. This seems like it would be the most confusing possible choice. There would be no indication for the visually impaired person that they were on an image as opposed to something else. I am not very good with using screen readers so there may be ways in some of them to still find out you are on an image, but not anything to tell whether that image is important. I would be surprised if this passes the accessibility tests in Chrome, but won't get to try it for 12 hours or so, In case #3, a screen reader would let the user navigate to the icon and say "Foobar icon". The next thing they navigate to would say "Heading, level 1, Foobar". This case seems fine to me, but a little redundant. This is my current patch. In case #4, the screen reader would entirely ignore the icon. It wouldn't let the user navigate to it. They would have all of the options page except for the extension's icon. This was my initial fix for the accessibility test failing. I would strongly recommend #3 or #4. Is there an accessibility "OWNER" that we should get to weigh in? -- Patrick <pat@nklein.com> > On Mar 27, 2017, at 5:36 AM, finnur@chromium.org wrote: > > If that's the case, that the alt text is not useful for the visually impaired, > then I think it is better to specify alt="" than to add some string that doesn't > add anything. > > https://codereview.chromium.org/2765493002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Okay, so, option #2 there does pass the accessibility tests in Chrome. On VoiceOver on the Mac, having no alt-text or having empty alt-text results in the VoiceOver reading the URL of the image followed by the word "image". For contrast, on the list of extensions, the icon for each extension is included using CSS to add an inlined background image with a "data:" URL. Because the image is added with CSS rather than with an IMG tag, the screen reader doesn't think of it as content at all, so the screen reader cannot navigate to it and doesn't say anything about it. ...Patrick
I will make a video tonight of navigating options #2, #3, and #4 with VoiceOver on the Mac. That should give a better idea of how it would feel to a visually-impaired user than my descriptions above.
On 2017/03/27 at 10:36:35, finnur wrote: > If that's the case, that the alt text is not useful for the visually impaired, then I think it is better to specify alt="" than to add some string that doesn't add anything. Here is a video showing the options #2, #3, and #4 above. http://nklein.com/wp-content/uploads/2017/03/ChromeExtensionAccessibility.mp4 Let me know which you think is best. ...Patrick
First of all, thank you for your patience on this. I have been a little busy and distracted lately and have not been providing feedback in a timely manner. I apologize for that and will try to fix that going forward. Also, thank you for the video. It really puts things into perspective. I agree with you that your original implementation was better and is consistent with the list of extensions, where you can't navigate to the image using the screen reader. So, looks like we're back to aria-hidden.
No problem. I'm sure you have plenty to do outside of my little patch. :) I will get things back to the ARIA-HIDDEN version, probably tonight. -- Patrick <pat@nklein.com> > On Mar 29, 2017, at 8:04 AM, finnur@chromium.org wrote: > > First of all, thank you for your patience on this. I have been a little busy and > distracted lately and have not been providing feedback in a timely manner. I > apologize for that and will try to fix that going forward. > > Also, thank you for the video. It really puts things into perspective. I agree > with you that your original implementation was better and is consistent with the > list of extensions, where you can't navigate to the image using the screen > reader. So, looks like we're back to aria-hidden. > > https://codereview.chromium.org/2765493002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Okay. The patch is now back to the ARIA-HIDDEN version and rebased onto the current master branch.
LGTM.
The CQ bit was checked by finnur@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490871710273070, "parent_rev": "95504aea9818ec4e593358d0373f9c41531a3b67", "commit_rev": "c773631198a95466aa396f0b978c033626133bb8"}
Message was sent while issue was closed.
Description was changed from ========== Fixing accessibility bug with extensions preferences As reported in bug 702971: https://bugs.chromium.org/p/chromium/issues/detail?id=702971 The 'Keyboard shortcuts' and extension 'options' panels on the extensions page are not accessible. The code was trying to use the 'ARIA-HIDDEN' attribute to keep extension options hidden until it was time to expose them. However, there was a small bug in the exposing code that resulted in the 'ARIA-HIDDEN' being set to 'TRUE' for all panels (or, in some cases, 'FALSE' for all panels. This is my first patch. If someone could request a try job for me on this, I would appreciate it. BUG=702971 TEST=Using a screenreader, navigate extension options and keyboard shortcuts options for extensions. Before this patch, the screenreader says nothing about what control it is on. After this patch, the screenreader reads the panel text as appropriate. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fixing accessibility bug with extensions preferences As reported in bug 702971: https://bugs.chromium.org/p/chromium/issues/detail?id=702971 The 'Keyboard shortcuts' and extension 'options' panels on the extensions page are not accessible. The code was trying to use the 'ARIA-HIDDEN' attribute to keep extension options hidden until it was time to expose them. However, there was a small bug in the exposing code that resulted in the 'ARIA-HIDDEN' being set to 'TRUE' for all panels (or, in some cases, 'FALSE' for all panels. This is my first patch. If someone could request a try job for me on this, I would appreciate it. BUG=702971 TEST=Using a screenreader, navigate extension options and keyboard shortcuts options for extensions. Before this patch, the screenreader says nothing about what control it is on. After this patch, the screenreader reads the panel text as appropriate. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2765493002 Cr-Commit-Position: refs/heads/master@{#460734} Committed: https://chromium.googlesource.com/chromium/src/+/c773631198a95466aa396f0b978c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c773631198a95466aa396f0b978c... |