Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2318873002:
[MD-Settings-A11y] cr-expand-button should communicate that it expanded. (Closed)
|
Created:
4 years, 3 months ago by hcarmona Modified:
4 years, 2 months ago Reviewers:
michaelpg CC:
chromium-reviews, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-elements_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD-Settings-A11y] cr-expand-button should communicate that it expanded.
Default in paper-icon-button is to aria-pressed, changing to
aria-expanded because it makes more sense for expand buttons to expand.
BUG=626875
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/8e69885a473fcaf0f6c22664a3ae5fc2b4d38425
Cr-Commit-Position: refs/heads/master@{#421346}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix OOBE a11y test failure #Patch Set 3 : rebase #
Messages
Total messages: 23 (16 generated)
Description was changed from ========== [MD-Settings-A11y] cr-expand-button should communicate that it expanded. Default in paper-icon-button is to aria-pressed, changing to aria-expanded because it makes more sense for expand buttons to expand. BUG=626875 ========== to ========== [MD-Settings-A11y] cr-expand-button should communicate that it expanded. Default in paper-icon-button is to aria-pressed, changing to aria-expanded because it makes more sense for expand buttons to expand. BUG=626875 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@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: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
hcarmona@chromium.org changed reviewers: + michaelpg@chromium.org
PTAL this will make it more clear for a11y that the button expands a section.
https://codereview.chromium.org/2318873002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html (right): https://codereview.chromium.org/2318873002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:11: icon="[[iconName_(expanded)]]" aria-active-attribute="aria-expanded"> shouldn't this reference *what* it expands? > If the element with the aria-expanded attribute controls the expansion of another grouping container that is not 'owned by' the element, the author SHOULD reference the container by using the aria-controls attribute. https://www.w3.org/TR/wai-aria/states_and_properties#aria-expanded How do we refer to an element that isn't in our shadow root? :-\
The CQ bit was checked by hcarmona@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: This issue passed the CQ dry run.
The CQ bit was checked by hcarmona@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...
Sorry for the long delay in responding to this. https://codereview.chromium.org/2318873002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html (right): https://codereview.chromium.org/2318873002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:11: icon="[[iconName_(expanded)]]" aria-active-attribute="aria-expanded"> On 2016/09/07 22:38:42, michaelpg wrote: > shouldn't this reference *what* it expands? > > > If the element with the aria-expanded attribute controls the expansion of > another grouping container that is not 'owned by' the element, the author SHOULD > reference the container by using the aria-controls attribute. > > https://www.w3.org/TR/wai-aria/states_and_properties#aria-expanded > > How do we refer to an element that isn't in our shadow root? :-\ Only one screen reader supports this property and not all aria properties work well with shadow dom. (this is one of the ones) that doesn't work well. :-/ I've filed http://crbug.com/650334 to track adding the aria-controls attribute after speaking with dmazzoni@ about this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/09/26 18:22:51, Hector Carmona wrote: > Sorry for the long delay in responding to this. > > https://codereview.chromium.org/2318873002/diff/1/ui/webui/resources/cr_eleme... > File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html > (right): > > https://codereview.chromium.org/2318873002/diff/1/ui/webui/resources/cr_eleme... > ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:11: > icon="[[iconName_(expanded)]]" aria-active-attribute="aria-expanded"> > On 2016/09/07 22:38:42, michaelpg wrote: > > shouldn't this reference *what* it expands? > > > > > If the element with the aria-expanded attribute controls the expansion of > > another grouping container that is not 'owned by' the element, the author > SHOULD > > reference the container by using the aria-controls attribute. > > > > https://www.w3.org/TR/wai-aria/states_and_properties#aria-expanded > > > > How do we refer to an element that isn't in our shadow root? :-\ > > Only one screen reader supports this property and not all aria > properties work well with shadow dom. (this is one of the ones) that > doesn't work well. :-/ > > I've filed http://crbug.com/650334 to track adding the aria-controls > attribute after speaking with dmazzoni@ about this. ok, lgtm
The CQ bit was checked by hcarmona@chromium.org
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 ========== [MD-Settings-A11y] cr-expand-button should communicate that it expanded. Default in paper-icon-button is to aria-pressed, changing to aria-expanded because it makes more sense for expand buttons to expand. BUG=626875 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD-Settings-A11y] cr-expand-button should communicate that it expanded. Default in paper-icon-button is to aria-pressed, changing to aria-expanded because it makes more sense for expand buttons to expand. BUG=626875 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/8e69885a473fcaf0f6c22664a3ae5fc2b4d38425 Cr-Commit-Position: refs/heads/master@{#421346} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8e69885a473fcaf0f6c22664a3ae5fc2b4d38425 Cr-Commit-Position: refs/heads/master@{#421346} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
