Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(248)

Issue 2765493002: Fixing accessibility bug with extensions preferences (Closed)

Created:
3 years, 9 months ago by nklein
Modified:
3 years, 8 months ago
Reviewers:
Finnur
CC:
arv+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -2 lines) Patch
M AUTHORS View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/extensions/extension_options_overlay.html View 1 2 4 5 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/resources/extensions/extensions.js View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/extensions/extension_settings_browsertest.js View 1 2 4 5 3 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 47 (20 generated)
nklein
This is my first patch. It is unclear to me whether I am supposed to ...
3 years, 9 months ago (2017-03-20 18:08:16 UTC) #4
Finnur
Thanks for contributing! Here's a good read on what your next steps need to be. ...
3 years, 9 months ago (2017-03-22 10:37:25 UTC) #13
nklein
Okay... I had missed the 'Legal' section before on the contributing-code page. I have now ...
3 years, 9 months ago (2017-03-22 13:54:54 UTC) #14
nklein
On 2017/03/22 at 10:37:25, finnur wrote: > Thanks for contributing! Here's a good read on ...
3 years, 9 months ago (2017-03-22 18:04:33 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2765493002/20001
3 years, 9 months ago (2017-03-23 15:37:15 UTC) #19
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the ...
3 years, 9 months ago (2017-03-23 15:37:16 UTC) #21
Finnur
Sorry to create confusion. But, yeah, I was thinking more down the line, if you ...
3 years, 9 months ago (2017-03-23 15:39:27 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2765493002/20001
3 years, 9 months ago (2017-03-23 15:40:02 UTC) #24
commit-bot: I haz the power
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)
3 years, 9 months ago (2017-03-23 16:36:45 UTC) #26
nklein
Hmm... so... it seems that when I was running unit tests on my Mac I ...
3 years, 9 months ago (2017-03-23 16:52:50 UTC) #27
nklein
Okay... I have fixed two of the four failing tests. SettingsCommandsExtensionSettingsWebUITest.extensionSettingsUri SettingsCommandsExtensionSettingsWebUITest.testChromeSendHandler These two are ...
3 years, 9 months ago (2017-03-24 13:10:42 UTC) #28
nklein
Okay... I have fixed the other two tests, also. I had thought about giving alt-text ...
3 years, 9 months ago (2017-03-24 13:24:43 UTC) #29
nklein
bash-3.2$ ./out/Default/browser_tests --gtest_filter=*ExtensionSettingsWebUITest.* 2>&1 | egrep 'RUN|OK|SUCCESS' [ RUN ] ExtensionSettingsWebUITest.testEmptyExtensionList [ OK ] ExtensionSettingsWebUITest.testEmptyExtensionList ...
3 years, 9 months ago (2017-03-24 13:31:48 UTC) #30
Finnur
https://codereview.chromium.org/2765493002/diff/40001/chrome/browser/resources/extensions/extension_options_overlay.html File chrome/browser/resources/extensions/extension_options_overlay.html (right): https://codereview.chromium.org/2765493002/diff/40001/chrome/browser/resources/extensions/extension_options_overlay.html#newcode10 chrome/browser/resources/extensions/extension_options_overlay.html:10: aria-hidden="true"></img> I don't know... I'm a little uncomfortable with ...
3 years, 9 months ago (2017-03-24 14:25:42 UTC) #31
nklein
My thinking was that this icon is redundant with the headline that immediately follows it. ...
3 years, 9 months ago (2017-03-24 15:13:44 UTC) #32
nklein
Okay... added alt text. And added bits to make the alt text include the name ...
3 years, 9 months ago (2017-03-24 16:39:17 UTC) #33
Finnur
If that's the case, that the alt text is not useful for the visually impaired, ...
3 years, 9 months ago (2017-03-27 10:36:35 UTC) #34
nklein
Hmm. It will surprise me if the accessibility check is okay with the alt text ...
3 years, 9 months ago (2017-03-27 12:44:05 UTC) #35
nklein
Okay, so, option #2 there does pass the accessibility tests in Chrome. On VoiceOver on ...
3 years, 9 months ago (2017-03-27 16:22:19 UTC) #36
nklein
I will make a video tonight of navigating options #2, #3, and #4 with VoiceOver ...
3 years, 9 months ago (2017-03-27 16:30:30 UTC) #37
nklein
On 2017/03/27 at 10:36:35, finnur wrote: > If that's the case, that the alt text ...
3 years, 8 months ago (2017-03-28 15:54:21 UTC) #38
Finnur
First of all, thank you for your patience on this. I have been a little ...
3 years, 8 months ago (2017-03-29 15:04:17 UTC) #39
nklein
No problem. I'm sure you have plenty to do outside of my little patch. :) ...
3 years, 8 months ago (2017-03-29 20:04:58 UTC) #40
nklein
Okay. The patch is now back to the ARIA-HIDDEN version and rebased onto the current ...
3 years, 8 months ago (2017-03-30 00:45:57 UTC) #41
Finnur
LGTM.
3 years, 8 months ago (2017-03-30 11:01:36 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2765493002/100001
3 years, 8 months ago (2017-03-30 11:02:22 UTC) #44
commit-bot: I haz the power
3 years, 8 months ago (2017-03-30 12:44:12 UTC) #47
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c773631198a95466aa396f0b978c...

Powered by Google App Engine
This is Rietveld 408576698