Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2671203002:
DevTools: Added ARIA roles & states to treeoutline (Closed)
|
Created:
3 years, 10 months ago by hhillen Modified:
3 years, 10 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded ARIA roles & states to treeoutline
BUG=629190
Review-Url: https://codereview.chromium.org/2671203002
Cr-Commit-Position: refs/heads/master@{#451920}
Committed: https://chromium.googlesource.com/chromium/src/+/8529c47b368722b13a67f29bb1f0e1b772e1f0c3
Patch Set 1 #
Total comments: 9
Patch Set 2 : added unsetExpanded method, changed treeoutline arrow characters #Patch Set 3 : new expected result for interstitial-sidebar-expected.txt #Patch Set 4 : updated Layout tests with expected ARIA attributes #
Messages
Total messages: 30 (15 generated)
The CQ bit was checked by hans.hillen@gmail.com
The CQ bit was unchecked by hans.hillen@gmail.com
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 by full committers or 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.
hans.hillen@gmail.com changed reviewers: + aboxhall@chromium.org, einbinder@chromium.org - Hans.Hillen@gmail.com
https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js (right): https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeOutline.js:51: UI.ARIAUtils.setAccessibleName(this._element, Common.UIString('Page DOM')); @aboxhall let me know if you prefer a different accessible name here. https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:340: UI.ARIAUtils.markAsTreeitem(this._listItemNode); Screen readers are currently announcing "aa" for the expand / collapse triangles. This is caused by the 'content' declaration in ElementsTreeOutline.css (line 48): elements-disclosure li.parent::before { -webkit-user-select: none; -webkit-mask-image: url(Images/treeoutlineTriangles.png); -webkit-mask-size: 32px 24px; content: "aa"; color: transparent; text-shadow: none; margin-right: -3px; } Can we get rid of these characters? https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:713: this._listItemNode.removeAttribute('aria-expanded'); @einbinder Would you prefer a dedicated method in ARIAUtils for removing aria related attributes?
LGTM for semantics. https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/ARIAUtils.js (right): https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/ARIAUtils.js:61: Extra blank line.
LGTM for semantics.
lgtm https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:340: UI.ARIAUtils.markAsTreeitem(this._listItemNode); On 2017/02/05 at 21:41:11, hhillen wrote: > Screen readers are currently announcing "aa" for the expand / collapse triangles. This is caused by the 'content' declaration in ElementsTreeOutline.css (line 48): > > elements-disclosure li.parent::before { > -webkit-user-select: none; > -webkit-mask-image: url(Images/treeoutlineTriangles.png); > -webkit-mask-size: 32px 24px; > content: "aa"; > color: transparent; > text-shadow: none; > margin-right: -3px; > } > > Can we get rid of these characters? We should be able to replace them with content: '\00a0\00a0' https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:713: this._listItemNode.removeAttribute('aria-expanded'); On 2017/02/05 at 21:41:11, hhillen wrote: > @einbinder Would you prefer a dedicated method in ARIAUtils for removing aria related attributes? Yeah. It feels weird because expandable false is different from removing the attribute. Let's do a dedicated method for now. I'll concede it is a point in favor of just using raw aria attributes.
https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/ARIAUtils.js (right): https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/ARIAUtils.js:61: On 2017/02/06 at 05:06:26, aboxhall wrote: > Extra blank line. Done https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:340: UI.ARIAUtils.markAsTreeitem(this._listItemNode); On 2017/02/07 at 21:12:58, einbinder wrote: > On 2017/02/05 at 21:41:11, hhillen wrote: > > Screen readers are currently announcing "aa" for the expand / collapse triangles. This is caused by the 'content' declaration in ElementsTreeOutline.css (line 48): > > > > elements-disclosure li.parent::before { > > -webkit-user-select: none; > > -webkit-mask-image: url(Images/treeoutlineTriangles.png); > > -webkit-mask-size: 32px 24px; > > content: "aa"; > > color: transparent; > > text-shadow: none; > > margin-right: -3px; > > } > > > > Can we get rid of these characters? > > We should be able to replace them with content: '\00a0\00a0' Done https://codereview.chromium.org/2671203002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:713: this._listItemNode.removeAttribute('aria-expanded'); On 2017/02/07 at 21:12:57, einbinder wrote: > On 2017/02/05 at 21:41:11, hhillen wrote: > > @einbinder Would you prefer a dedicated method in ARIAUtils for removing aria related attributes? > > Yeah. It feels weird because expandable false is different from removing the attribute. Let's do a dedicated method for now. I'll concede it is a point in favor of just using raw aria attributes. Done
einbinder@chromium.org changed reviewers: + pfeldman@chromium.org
lgtm
The CQ bit was checked by hans.hillen@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from einbinder@chromium.org, aboxhall@chromium.org Link to the patchset: https://codereview.chromium.org/2671203002/#ps20001 (title: "added unsetExpanded method, changed treeoutline arrow characters")
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hans.hillen@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from einbinder@chromium.org, aboxhall@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2671203002/#ps40001 (title: "new expected result for interstitial-sidebar-expected.txt")
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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hans.hillen@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from einbinder@chromium.org, aboxhall@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2671203002/#ps60001 (title: "updated Layout tests with expected ARIA attributes")
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": 60001, "attempt_start_ts": 1487744406586440,
"parent_rev": "899bc281768913fa13729754b9725d1feadd3c9c", "commit_rev":
"8529c47b368722b13a67f29bb1f0e1b772e1f0c3"}
Message was sent while issue was closed.
Description was changed from ========== Added ARIA roles & states to treeoutline BUG=629190 ========== to ========== Added ARIA roles & states to treeoutline BUG=629190 Review-Url: https://codereview.chromium.org/2671203002 Cr-Commit-Position: refs/heads/master@{#451920} Committed: https://chromium.googlesource.com/chromium/src/+/8529c47b368722b13a67f29bb1f0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8529c47b368722b13a67f29bb1f0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
