Chromium Code Reviews
|
|
|
Created:
4 years, 11 months ago by aboxhall Modified:
4 years, 11 months ago CC:
aboxhall, apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, devtools-reviews_chromium.org, dmazzoni, je_julie, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, nektarios, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, yurys+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionShow human-readable names for accessibility properties
BUG=492407
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196197
Patch Set 1 #Patch Set 2 : Add AccessibilityStrings.js #
Total comments: 10
Patch Set 3 : dmazzoni review comments #
Total comments: 37
Patch Set 4 : paulirish review comments #Patch Set 5 : s/author/developer/ #Patch Set 6 : pfeldman review comments #
Messages
Total messages: 19 (3 generated)
aboxhall@chromium.org changed reviewers: + dmazzoni@chromium.org, paulirish@chromium.org, pfeldman@chromium.org
Dominic/Paul: PTAL for strings.
https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/AccessibilityStrings.js (right): https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:12: name : "Hidden", We don't actually show a full AX node in this case, right? If it's hidden, there is no AX node and we should just try to explain why. https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:17: name : "Hidden region root", I don't like this one but I'm not sure what it should be. Does this include nodes that have aria-hidden set on them and also nodes that aren't allowed to have children? It might be confusing if those two concepts are conflated. If they're not, maybe this could be something like "Root of hidden subtree", or "Ancestor that disallows children". https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:22: name : "Invalid", Invalid user entry? So it's not taken to mean that the AX data itself is invalid. https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:32: name : "Atomic", Is this grouped under "Live region properties"? If not it should say Atomic (Live Regions), or something like that. https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:152: name : "Owns", Note: with my latest change, we should reconsider how to surface this - maybe exposing a node's children and parents with explanations as to why (from layout tree, from DOM tree, etc.)
https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/AccessibilityStrings.js (right): https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:12: name : "Hidden", On 2015/05/26 23:15:03, dmazzoni wrote: > We don't actually show a full AX node in this case, right? If it's hidden, there > is no AX node and we should just try to explain why. Good point - this shouldn't actually ever get shown. Should I take it out? https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:17: name : "Hidden region root", On 2015/05/26 23:15:03, dmazzoni wrote: > I don't like this one but I'm not sure what it should be. > > Does this include nodes that have aria-hidden set on them and also nodes that > aren't allowed to have children? It might be confusing if those two concepts are > conflated. If they're not, maybe this could be something like "Root of hidden > subtree", or "Ancestor that disallows children". This is actually wrapped up in the hidden node explanation as well. It ends up as "aria-hidden is true on ancestor: <selector-esque string>". Should I take this out as well? https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:22: name : "Invalid", On 2015/05/26 23:15:04, dmazzoni wrote: > Invalid user entry? > > So it's not taken to mean that the AX data itself is invalid. Done. https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:32: name : "Atomic", On 2015/05/26 23:15:04, dmazzoni wrote: > Is this grouped under "Live region properties"? If not it should say Atomic > (Live Regions), or something like that. It's not currently grouped, no. Good suggestion - can remove if/when we do group live region properties. https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:152: name : "Owns", On 2015/05/26 23:15:04, dmazzoni wrote: > Note: with my latest change, we should reconsider how to surface this - maybe > exposing a node's children and parents with explanations as to why (from layout > tree, from DOM tree, etc.) Once we're showing the full tree, yes, we should definitely do something like that.
https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/AccessibilityStrings.js (right): https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:8: description : "If true, this element currently cannot be interacted with.", "Currently" always applies to these. The spec text has a slight advantage here: > "If true, this element is not editable or otherwise operable." https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:17: name : "Live", "Live region" since the rest are grouped under that full name? https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:18: description : "Whether and what type of live updates may be expected for this element.", s/type/priority ? Whereas atomic is the flavor of update, this is the priority of delivery, yeah? https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:23: description : "If this element may receive live updates, whether the entire live region should be presented to the user on changes.", add to the end? "… or just the changed nodes." https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:28: description : "If this element may receive live updates, what type of updates should trigger a notification.", Let's add the sort of updates? "…what type of updates (additions, removals, text, all)…" https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:78: description : "Whether this element is a required field in a form.", "Whether an input value is required for this element before its parent form may be submitted." https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:83: description : "For a range widget, the minimum possible value.", for this and the next, s/possible/allowed/ https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:153: description: "The role of this element.", Can we offer a tad more than this? My attempt: "The computed role of this element, indicating for assistive technology the presentation and interaction processing model" https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:158: description: "The value of this element.", "The value of this element, which may be a boolean, number, string, defined token, etc."
On Tue, May 26, 2015 at 4:31 PM, <aboxhall@chromium.org> wrote: > > > https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... > File Source/devtools/front_end/accessibility/AccessibilityStrings.js > (right): > > > https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/accessibility/AccessibilityStrings.js:12: name > : "Hidden", > On 2015/05/26 23:15:03, dmazzoni wrote: > >> We don't actually show a full AX node in this case, right? If it's >> > hidden, there > >> is no AX node and we should just try to explain why. >> > > Good point - this shouldn't actually ever get shown. Should I take it > out? Yeah, make it a notreached() instead! https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/accessibility/AccessibilityStrings.js:17: name > : "Hidden region root", > On 2015/05/26 23:15:03, dmazzoni wrote: > >> I don't like this one but I'm not sure what it should be. >> > > Does this include nodes that have aria-hidden set on them and also >> > nodes that > >> aren't allowed to have children? It might be confusing if those two >> > concepts are > >> conflated. If they're not, maybe this could be something like "Root of >> > hidden > >> subtree", or "Ancestor that disallows children". >> > > This is actually wrapped up in the hidden node explanation as well. It > ends up as "aria-hidden is true on ancestor: <selector-esque string>". > > Should I take this out as well? Yeah, take it out then. Awesome. https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/accessibility/AccessibilityStrings.js:22: name > : "Invalid", > On 2015/05/26 23:15:04, dmazzoni wrote: > >> Invalid user entry? >> > > So it's not taken to mean that the AX data itself is invalid. >> > > Done. > > > https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/accessibility/AccessibilityStrings.js:32: name > : "Atomic", > On 2015/05/26 23:15:04, dmazzoni wrote: > >> Is this grouped under "Live region properties"? If not it should say >> > Atomic > >> (Live Regions), or something like that. >> > > It's not currently grouped, no. Good suggestion - can remove if/when we > do group live region properties. > > > https://codereview.chromium.org/1156223002/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/accessibility/AccessibilityStrings.js:152: > name : "Owns", > On 2015/05/26 23:15:04, dmazzoni wrote: > >> Note: with my latest change, we should reconsider how to surface this >> > - maybe > >> exposing a node's children and parents with explanations as to why >> > (from layout > >> tree, from DOM tree, etc.) >> > > Once we're showing the full tree, yes, we should definitely do something > like that. > > https://codereview.chromium.org/1156223002/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/AccessibilityStrings.js (right): https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:8: description : "If true, this element currently cannot be interacted with.", On 2015/05/27 05:33:32, paulirish wrote: > "Currently" always applies to these. > > The spec text has a slight advantage here: > > "If true, this element is not editable or otherwise operable." I added "currently" to convey that it's a state rather than a property - i.e. it might become operable in the future. Happy to take it out if you are convinced it doesn't add anything. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:17: name : "Live", On 2015/05/27 05:33:32, paulirish wrote: > "Live region" since the rest are grouped under that full name? Done. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:18: description : "Whether and what type of live updates may be expected for this element.", On 2015/05/27 05:33:32, paulirish wrote: > s/type/priority ? > Whereas atomic is the flavor of update, this is the priority of delivery, yeah? Oh, good point. Done. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:23: description : "If this element may receive live updates, whether the entire live region should be presented to the user on changes.", On 2015/05/27 05:33:32, paulirish wrote: > add to the end? "… or just the changed nodes." Done. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:28: description : "If this element may receive live updates, what type of updates should trigger a notification.", On 2015/05/27 05:33:32, paulirish wrote: > Let's add the sort of updates? > "…what type of updates (additions, removals, text, all)…" Not sure about this. It will look something like: Relevant (live regions): `additions` ^ [If this element may receive live updates, what type of updates should trigger a notification.] Since one of the four options will be presented to the right of the label, I'm worried it might look a little redundant to have them in the tool tip as well. What do you think? https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:78: description : "Whether this element is a required field in a form.", On 2015/05/27 05:33:32, paulirish wrote: > "Whether an input value is required for this element before its parent form may > be submitted." I had something like that, and then I figured people know what a required field is - the critical information is that it refers to that concept rather than some other concept of "required". https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:83: description : "For a range widget, the minimum possible value.", On 2015/05/27 05:33:32, paulirish wrote: > for this and the next, s/possible/allowed/ Done. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:153: description: "The role of this element.", On 2015/05/27 05:33:32, paulirish wrote: > Can we offer a tad more than this? My attempt: > > "The computed role of this element, indicating for assistive technology the > presentation and interaction processing model" Yeah, I wrote these three in a hurry :) My attempt: "Indicates the purpose of this element, such as a user interface idiom for a widget, or structural role within a document." https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:158: description: "The value of this element.", On 2015/05/27 05:33:32, paulirish wrote: > "The value of this element, which may be a boolean, number, string, defined > token, etc." Again, when its actual value is right next to the label, I think enumerating the types it may take will be redundant. How about: "The value of this element; this may be user-provided or author-provided, depending on the element."
some nits and such. overall lgtm on wording. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/AccessibilityStrings.js (right): https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:8: description : "If true, this element currently cannot be interacted with.", Yeah not a big deal. let's keep 'currently' https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:28: description : "If this element may receive live updates, what type of updates should trigger a notification.", > Since one of the four options will be presented to the right of the label, I'm worried it might look a little redundant to have them in the tool tip as well. What do you think? Maybe, but as I'm personally quite new to this stuff, having the extra context really helps. Gives me something to google for like `live regions relevant removals` https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:78: description : "Whether this element is a required field in a form.", > I had something like that, and then I figured people know what a required field is - the critical information is that it refers to that concept rather than some other concept of "required". I added this because I think developers are still pretty unfamiliar with how a browser prevents submit if a required field is empty. (Since nearly everyone does clientside validation). Here we implicitly say "you're going to prevent the user from going forward unless this field is filled in. be SURE about that, b!" https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:153: description: "The role of this element.", > My attempt: "Indicates the purpose of this element, such as a user interface idiom for a widget, or structural role within a document." Perfekt. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:158: description: "The value of this element.", > Again, when its actual value is right next to the label, I think enumerating the types it may take will be redundant. How about: > "The value of this element; this may be user-provided or author-provided, depending on the element." Would rather see "developer" instead of "author". That said, I don't think that we're going to provide much value (hah) trying to define "value" for people. I'm OK with whatever string for this.
https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/AccessibilityStrings.js (right): https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:28: description : "If this element may receive live updates, what type of updates should trigger a notification.", On 2015/05/27 23:25:32, paulirish wrote: > > > Since one of the four options will be presented to the right of the label, I'm > worried it might look a little redundant to have them in the tool tip as well. > What do you think? > > > Maybe, but as I'm personally quite new to this stuff, having the extra context > really helps. Gives me something to google for like `live regions relevant > removals` > I really don't think this is the place to surface that type of information. Googling `live regions relevant` will get you the list of potential values; once we have in-place editing we can do autocompletions like in the CSS pane. Could also do a follow-up CL to add descriptions to token values. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:78: description : "Whether this element is a required field in a form.", On 2015/05/27 23:25:32, paulirish wrote: > > > I had something like that, and then I figured people know what a required > field is - the critical information is that it refers to that concept rather > than some other concept of "required". > > I added this because I think developers are still pretty unfamiliar with how a > browser prevents submit if a required field is empty. (Since nearly everyone > does clientside validation). > > Here we implicitly say "you're going to prevent the user from going forward > unless this field is filled in. be SURE about that, b!" They're not, though. This is just the semantic equivalent of putting a little star next to the field. Using the HTML5 `required` attribute will do that, but that's not necessary to trigger this property. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:158: description: "The value of this element.", On 2015/05/27 23:25:31, paulirish wrote: > > > Again, when its actual value is right next to the label, I think enumerating > the types it may take will be redundant. How about: > > "The value of this element; this may be user-provided or author-provided, > depending on the element." > > Would rather see "developer" instead of "author". That said, I don't think that > we're going to provide much value (hah) trying to define "value" for people. > I'm OK with whatever string for this. Totally agreed re: defining value (hence why I kinda phoned it in). Will change to 'developer'.
https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/AccessibilityStrings.js (right): https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:28: description : "If this element may receive live updates, what type of updates should trigger a notification.", ok https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:78: description : "Whether this element is a required field in a form.", > They're not, though. This is just the semantic equivalent of putting a little star next to the field. Using the HTML5 `required` attribute will do that, but that's not necessary to trigger this property. You're telling me that we can end up with a computed a11y required value of `true` and the form is still submittable? If that's the case, then the current text is great.
https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/AccessibilityStrings.js (right): https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:78: description : "Whether this element is a required field in a form.", On 2015/05/27 23:51:55, paulirish wrote: > > > They're not, though. This is just the semantic equivalent of putting a little > star next to the field. Using the HTML5 `required` attribute will do that, but > that's not necessary to trigger this property. > > You're telling me that we can end up with a computed a11y required value of > `true` and the form is still submittable? If that's the case, then the current > text is great. Afraid so! Welcome to the wonderful world of semantics.
pfeldman: PTAL?
https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/AccessibilityStrings.js (right): https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. TIL: no (c) as per http://dev.chromium.org/developers/coding-style. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:5: WebInspector.AXAttributes = { Pleasr add a prefix matching the file name https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/accessibilityNode.css (right): https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/accessibilityNode.css:12: /* border-bottom: solid 1px rgba(128, 128, 128, 0.3); */ We don't land commented out code https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/accessibilityNode.css:46: /* color: #006649; */ ditto
https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/AccessibilityStrings.js (right): https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/05/29 12:35:31, pfeldman wrote: > TIL: no (c) as per http://dev.chromium.org/developers/coding-style. Done. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/AccessibilityStrings.js:5: WebInspector.AXAttributes = { On 2015/05/29 12:35:31, pfeldman wrote: > Pleasr add a prefix matching the file name Done. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/accessibility/accessibilityNode.css (right): https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/accessibilityNode.css:12: /* border-bottom: solid 1px rgba(128, 128, 128, 0.3); */ On 2015/05/29 12:35:32, pfeldman wrote: > We don't land commented out code Done. https://codereview.chromium.org/1156223002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/accessibility/accessibilityNode.css:46: /* color: #006649; */ On 2015/05/29 12:35:32, pfeldman wrote: > ditto Done.
lgtm
The CQ bit was checked by aboxhall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from paulirish@chromium.org Link to the patchset: https://codereview.chromium.org/1156223002/#ps100001 (title: "pfeldman review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156223002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196197 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
