Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2967443002:
[DevTools] Add ARIA semantics for AXBreadcrumbs (Closed)
|
Created:
3 years, 5 months ago by aboxhall Modified:
3 years, 5 months ago Reviewers:
lushnikov CC:
chromium-reviews, caseq+blink_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, pfeldman+blink_chromium.org, je_julie, dougt+watch_chromium.org, aleventhal+watch_chromium.org, dtseng+watch_chromium.org, devtools-reviews_chromium.org, blink-reviews, apavlov+blink_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman, dmazzoni, dmazzoni+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ARIA semantics for AXBreadcrumbs.
This implements the ARIA tree pattern: https://www.w3.org/TR/wai-aria-practices-1.1/#TreeView
BUG=560525
Review-Url: https://codereview.chromium.org/2967443002
Cr-Commit-Position: refs/heads/master@{#485171}
Committed: https://chromium.googlesource.com/chromium/src/+/f0da439177769bf6fe16eb727e8cdbd0b6f72e2c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add JSDoc for _children #Messages
Total messages: 23 (9 generated)
aboxhall@chromium.org changed reviewers: + lushnikov@chromium.org
This did require adding some hierarchy back in. I checked with my colleagues about the semantics and they agreed a tree made the most sense even though it's not really a traditional tree.
Description was changed from ========== Add ARIA semantics for AXBreadcrumbs BUG=560525 ========== to ========== Add ARIA semantics for AXBreadcrumbs BUG=560525 ==========
Ping?
I'm not sure I understand what we do here. Why would we want to get the tree-like structure back?
On 2017/07/05 22:06:29, lushnikov wrote: > I'm not sure I understand what we do here. Why would we want to get the > tree-like structure back? Assistive technology users need cues as to what type of UI they are dealing with. I spoke with members of my team (some of whom use assistive technology) and described the layout, and they unanimously agreed this should still use a 'tree' role. This role requires a tree structure to work correctly: https://www.w3.org/TR/wai-aria-practices-1.1/#TreeView
Thank you for the explanation! Shall we put it in the issue description? Quoting the spec: "A tree view widget presents a hierarchical list. Any item in the hierarchy may have child items, and items that have children may be expanded or collapsed to show or hide the children." It looks to me that this is not what we have here. For example, we cannot toggle the tree items which are not leafs. And once a node is selected, its siblings disappear. Don't you think it would be confusing? This inconsistency in behavior was a driving force behind the recent refactoring, which dropped the tree-like internal structure. As a result, the code is much cleaner now! Bringing the tree structure back feels awkward to me. We should either implement a full-blown tree behavior, or should not pretend to be one when we're actually not. WDYT?
On 2017/07/05 23:41:47, lushnikov wrote: > Thank you for the explanation! Shall we put it in the issue description? Sure! > Quoting the spec: > > "A tree view widget presents a hierarchical list. Any item in the hierarchy may > have child items, and items that have children may be expanded or collapsed to > show or hide the children." > > It looks to me that this is not what we have here. For example, we cannot toggle > the tree items which are not leafs. > And once a node is selected, its siblings disappear. Don't you think it would be > confusing? > > This inconsistency in behavior was a driving force behind the recent > refactoring, which dropped the tree-like internal structure. As a result, the > code is much cleaner now! Bringing the tree structure back feels awkward to me. > We should either implement a full-blown tree behavior, or should not pretend to > be one when we're actually not. > > WDYT? Like I said, I ran this past my colleagues, with a full description of the behaviour, and they all agreed it should be a tree.
Description was changed from ========== Add ARIA semantics for AXBreadcrumbs BUG=560525 ========== to ========== Add ARIA semantics for AXBreadcrumbs. This implements the ARIA tree pattern: https://www.w3.org/TR/wai-aria-practices-1.1/#TreeView BUG=560525 ==========
On 2017/07/06 01:06:37, aboxhall wrote: > On 2017/07/05 23:41:47, lushnikov wrote: > > Thank you for the explanation! Shall we put it in the issue description? > > Sure! > > > Quoting the spec: > > > > "A tree view widget presents a hierarchical list. Any item in the hierarchy > may > > have child items, and items that have children may be expanded or collapsed to > > show or hide the children." > > > > It looks to me that this is not what we have here. For example, we cannot > toggle > > the tree items which are not leafs. > > And once a node is selected, its siblings disappear. Don't you think it would > be > > confusing? > > > > This inconsistency in behavior was a driving force behind the recent > > refactoring, which dropped the tree-like internal structure. As a result, the > > code is much cleaner now! Bringing the tree structure back feels awkward to > me. > > We should either implement a full-blown tree behavior, or should not pretend > to > > be one when we're actually not. > > > > WDYT? > > Like I said, I ran this past my colleagues, with a full description of the > behaviour, and they all agreed it should be a tree. Regarding the partial tree, this is actually not unheard of in this type of tool. The Accessibility Inspector on Mac (https://developer.apple.com/library/content/documentation/Accessibility/Conce...) also uses a partial tree, and is marked up similarly regarding semantics.
What is the information that a tree is primarily trying to convey? The fact that there is a hierarchy in the listed elements, i.e. a parent - child relationship. When I cursor around I want to hear or see in Braille the level at which the current node is at. Root node is at level 0 and so on. Would it be better if you could expand / collapse every non-leaf node? Sure, but this is not the primary information that the tree is trying to convey. Could it be a problem if the siblings of a node disappear when a node is selected? Yes, but as a screen reader user I'll not be navigating around reading irrelevant nodes. So, the deviations from the spec should be noted but I don't think are essential when compared to the information I might be getting with a tree structure.
@nektarios: thanks for sharing your reasoning behind this. lgtm https://codereview.chromium.org/2967443002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/accessibility/AXBreadcrumbsPane.js (right): https://codereview.chromium.org/2967443002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/accessibility/AXBreadcrumbsPane.js:274: this._children = []; let's add jsdoc for this
Thanks! https://codereview.chromium.org/2967443002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/accessibility/AXBreadcrumbsPane.js (right): https://codereview.chromium.org/2967443002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/accessibility/AXBreadcrumbsPane.js:274: this._children = []; On 2017/07/06 15:49:09, lushnikov wrote: > let's add jsdoc for this Done.
The CQ bit was checked by aboxhall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2967443002/#ps20001 (title: "Add JSDoc for _children")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by aboxhall@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": 20001, "attempt_start_ts": 1499646412759120,
"parent_rev": "22df6a29480ecefff39ae4e9b06ffdf9a8b7d79b", "commit_rev":
"f0da439177769bf6fe16eb727e8cdbd0b6f72e2c"}
Message was sent while issue was closed.
Description was changed from ========== Add ARIA semantics for AXBreadcrumbs. This implements the ARIA tree pattern: https://www.w3.org/TR/wai-aria-practices-1.1/#TreeView BUG=560525 ========== to ========== Add ARIA semantics for AXBreadcrumbs. This implements the ARIA tree pattern: https://www.w3.org/TR/wai-aria-practices-1.1/#TreeView BUG=560525 Review-Url: https://codereview.chromium.org/2967443002 Cr-Commit-Position: refs/heads/master@{#485171} Committed: https://chromium.googlesource.com/chromium/src/+/f0da439177769bf6fe16eb727e8c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f0da439177769bf6fe16eb727e8c... |
