Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2742833002:
[Win10 FRE] Fix ARIA attributes to conform to accordion widget. (Closed)
Description[Win10 FRE] Fix ARIA attributes to conform to accordion widget.
This CL updates role and aria-* attributes to get screen readers to
work.
BUG=697142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2742833002
Cr-Commit-Position: refs/heads/master@{#456175}
Committed: https://chromium.googlesource.com/chromium/src/+/bdbe2f86eba4acf4ab9239e1277386089be1ab25
Patch Set 1 #
Total comments: 12
Patch Set 2 : Remove redundant attributes; fix spacing. #
Total comments: 2
Patch Set 3 : Fix typo. #
Messages
Total messages: 21 (8 generated)
Description was changed from ========== [Win10 FRE] Fix ARIA attributes to conform to accordion widget. This CL updates role and aria-* attributes to get screen readers to work. BUG=697142 ========== to ========== [Win10 FRE] Fix ARIA attributes to conform to accordion widget. This CL updates role and aria-* attributes to get screen readers to work. BUG=697142 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
huangs@chromium.org changed reviewers: + dmazzoni@chromium.org, tmartino@chromium.org, tommycli@chromium.org
PTAL. I won't be able to respond until late tonight. Thanks!
Also, I'm following some recommendations from: https://www.w3.org/TR/wai-aria-practices-1.1/#accordion
lgtm indeed you seem to be following the accordion recommendaitons. I'll leave dmazzoni to comment further on the accessibility though. He's the accessibility expert.
lgtm
Thanks. We have some code to automatically lint code like this, see third_party/accessibility-audit and search for the place we've already integrated it into webui. Any chance you could integrate that too? Not sure if this code is already using webui infrastructure. Basically if you can write any test that loads this HTML into a page, you can run the audit just by triggering some JS. Worth considering. https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... File chrome/browser/resources/welcome/win10/inline.html (right): https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:289: <div class="section-heading" tabindex="0" role="text"> What's the tabindex here for? It's unusual to make something keyboard focusable if it's not interactive. If the only purpose is so a screen reader can read it, that's unnecessary, there are other screen reader shortcuts to navigate to static text https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:295: <ol id="panel1" class="section-steps" role="list" aria-labelledby="tab1"> Role=list shouldn't be needed here, it's implicit with <ol> https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:296: <li tabindex="0"> No tabindex here. The link is already focusable https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:302: <div role="presentation">$i18nRaw{clickEdgeText}</div> All of the role=presentation in this change is safe and harmless but shouldn't really be needed. I'd leave it out unless there's a good reason The most common reason for role=presentation is if there's an extra "layer" between a container and its item, like: <div role="list"> <div role="presentation"> <!-- needed otherwise the listitem is lost --> <div role="listitem"> https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:319: <div role="heading"> aria-level=2 or something like that, just like a <h2>, <h3>, etc Any chance you could just use a native <h2>? Slightly preferred, but role=heading with aria-level is okay too https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:320: <a id="tab2" is="action-link" class="section-heading" on-tap="onToggle" role="button" area-controls="panel2" aria-expanded="false"> aria-controls (spelling)
nit: I think we use 80-char limits on HTML
Updated, PTAL. https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... File chrome/browser/resources/welcome/win10/inline.html (right): https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:289: <div class="section-heading" tabindex="0" role="text"> It was for my own convenience -- previously I didn't know MS Narrator uses <Space>, not <Enter> to activate. Removed. https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:295: <ol id="panel1" class="section-steps" role="list" aria-labelledby="tab1"> This was a workaround, but seems to be not needed any more. Removed. https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:296: <li tabindex="0"> Removed. https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:302: <div role="presentation">$i18nRaw{clickEdgeText}</div> These are added since otherwise MS Narrator, after reading the line, would proceed to highlight each word and read them one-by-one, which seems rather redundant. Also, $i18nRaw(...) does not inject pure text, but may contain tags. So left as-is, but please let me know if what I described is intended, or if there's are better solutions. Thanks! https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:319: <div role="heading"> That would require more complex CSS to restyle <h2>; I'd prefer to go with aria-level=2. https://codereview.chromium.org/2742833002/diff/1/chrome/browser/resources/we... chrome/browser/resources/welcome/win10/inline.html:320: <a id="tab2" is="action-link" class="section-heading" on-tap="onToggle" role="button" area-controls="panel2" aria-expanded="false"> Ah oops. Done.
lgtm https://codereview.chromium.org/2742833002/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline.html (right): https://codereview.chromium.org/2742833002/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.html:280: on-tap="onToggle" role="button" area-controls="panel1" area-controls -> aria-controls
Thanks. Committing! https://codereview.chromium.org/2742833002/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline.html (right): https://codereview.chromium.org/2742833002/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.html:280: on-tap="onToggle" role="button" area-controls="panel1" Hmm weird, I searched and failed to find this. Thanks!
The CQ bit was checked by huangs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tmartino@chromium.org, tommycli@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2742833002/#ps40001 (title: "Fix typo.")
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: 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 huangs@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": 40001, "attempt_start_ts": 1489181073127150,
"parent_rev": "921d5298e10e7010e3609f5608e060cfac761ef7", "commit_rev":
"bdbe2f86eba4acf4ab9239e1277386089be1ab25"}
Message was sent while issue was closed.
Description was changed from ========== [Win10 FRE] Fix ARIA attributes to conform to accordion widget. This CL updates role and aria-* attributes to get screen readers to work. BUG=697142 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Win10 FRE] Fix ARIA attributes to conform to accordion widget. This CL updates role and aria-* attributes to get screen readers to work. BUG=697142 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2742833002 Cr-Commit-Position: refs/heads/master@{#456175} Committed: https://chromium.googlesource.com/chromium/src/+/bdbe2f86eba4acf4ab9239e12773... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/bdbe2f86eba4acf4ab9239e12773... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
